Some improvements to IValue #11238
Some improvements to IValue #11238
Conversation
Differential Revision: D9644700 Differential Version: 56870160
smessmer
requested review from
apaszke,
colesbury,
ezyang,
gchanan,
soumith and
zdevito
as code owners
torch/csrc/jit/ivalue.h
Outdated
| : payload(rhs.payload) | ||
| , tag(rhs.tag) | ||
| , is_intrusive_ptr(rhs.is_intrusive_ptr) { | ||
| rhs.clearToNone(); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
torch/csrc/jit/ivalue.h
Outdated
| : payload(rhs.payload) | ||
| , tag(rhs.tag) | ||
| , is_intrusive_ptr(rhs.is_intrusive_ptr) { | ||
| rhs.clearToNone(); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@zdevito The old constructor default-initializes all members and then overwrites them in the swap. The new version doesn't default-initialize them but directly initializes them with the correct values, which is (in theory) a bit faster. |
|
but you're actually right that the old implementation also set rhs to None because it was swapped with the new instance. Where I actually changed that behavior is in the move assignment - that didn't set rhs to None before but now does. |
Differential Revision: D9644700 Differential Version: 56900331
|
@pytorchbot retest this please |
Differential Revision: D9644700 Differential Version: 56982494
Differential Revision: D9644700 Differential Version: 57101652
Differential Revision: D9644700 Differential Version: 57135224
Differential Revision: D9644700 Differential Version: 57214023
Summary: Pull Request resolved: pytorch#11238 - when moving an IValue, free the old value instead of keeping it allocated - making classes final - moving std::string - making ConstantList const Reviewed By: ezyang Differential Revision: D9644700 fbshipit-source-id: ab7228368e4f00f664ba54e1242b0307d91c5e7e
Stack:
:white_circle: #11167 Narrowing Blob 💛
:black_circle: #11238 Some improvements to IValue 💛
:white_circle: #11258 Improve Tensor() constructor 💛
:white_circle: #11260 Fix intrusive_ptr move/copy for different NullType's 💛
:white_circle: #11294 Remove manual refcounting from Tensor class 💛
:white_circle: #11352 Remove intrusive_ptr::reclaim() in Storage 💛
:white_circle: #11353 Simplify union payload copying 💛
:white_circle: #11355 Simplify IValue::toTensor() 💛
:white_circle: #11402 Simplify Blob move constructor/assignment 💛
:white_circle: #11414 IValue can store Blob 💛
:white_circle: #11481 Blob doesn't allow access to destroyCall anymore 💛
Differential Revision: D9644700