Make TensorName::unifyFromRight in-place for efficiency #29307
Conversation
In our name inference functions we currently create an extra TensorNames every time we unify names. This isn't completely necessary. To do this, I made the following changes: - TensorName now has two states, initialized and uninitialized - Renamed unifyFromRight to unifyFromRightInplace. Test Plan: - `pytest test/test_namedtensor.py -v` [ghstack-poisoned]
In our name inference functions we currently create an extra TensorNames every time we unify names. This isn't completely necessary. To do this, I made the following changes: - TensorName now has two states, initialized and uninitialized - Renamed unifyFromRight to unifyFromRightInplace. Test Plan: - `pytest test/test_namedtensor.py -v` ghstack-source-id: ac8f2d5 Pull Request resolved: #29307
aten/src/ATen/TensorNames.h
Outdated
| // to A, `tensor` would have duplicate names [A, A]. Therefore we need to check | ||
| // tensor.names [A, None] for the existence of A. | ||
| struct CAFFE2_API TensorName { | ||
| TensorName() |
There was a problem hiding this comment.
where is this constructor needed?
There was a problem hiding this comment.
There was a problem hiding this comment.
std::vector<TensorName>::resize resizes the container. If it is resized to a larger size, then the container needs to insert new elements. The value of the new elements is either passed in via resize, or they are default constructed (the TensorName constructor gets called).
Now that I think about it more, there is a good way around this problem that I will go and implement to remove the initialized_ flag. Instead of resizing the container, I can prepend to it names from the other list of names.
There was a problem hiding this comment.
I've removed the initialized_ flag
In our name inference functions we currently create an extra TensorNames every time we unify names. This isn't completely necessary. To do this, I made the following changes: - TensorName now has two states, initialized and uninitialized - Renamed unifyFromRight to unifyFromRightInplace. Test Plan: - `pytest test/test_namedtensor.py -v` [ghstack-poisoned]
In our name inference functions we currently create an extra TensorNames every time we unify names. This isn't completely necessary. To do this, I made the following changes: - TensorName now has two states, initialized and uninitialized - Renamed unifyFromRight to unifyFromRightInplace. Test Plan: - `pytest test/test_namedtensor.py -v` [ghstack-poisoned]
aten/src/ATen/TensorNames.cpp
Outdated
| int64_t size_difference = longer_size - shorter_size; | ||
| bool this_is_longer = names_.size() == longer_size; | ||
|
|
||
| if (this_is_longer) { |
There was a problem hiding this comment.
nit: I might prefer something like size_diff = std::abs(names.size() - other.names.size()); and if (names.size() > other.names.size()) if that also works, instead of tracking min/max sizes and a bool.
There was a problem hiding this comment.
Sure, that sounds reasonable
nairbv
left a comment
nairbv
left a comment
There was a problem hiding this comment.
one suggestions but this looks cleaner without the initialized tracking.
In our name inference functions we currently create an extra TensorNames every time we unify names. This isn't completely necessary. To do this, I made the following changes: - TensorName now has two states, initialized and uninitialized - Renamed unifyFromRight to unifyFromRightInplace. Test Plan: - `pytest test/test_namedtensor.py -v` [ghstack-poisoned]
Summary: Pull Request resolved: pytorch/pytorch#29307 In our name inference functions we currently create an extra TensorNames every time we unify names. This isn't completely necessary. To do this, I made the following changes: - TensorName now has two states, initialized and uninitialized - Renamed unifyFromRight to unifyFromRightInplace. Test Plan: - `pytest test/test_namedtensor.py -v` Differential Revision: D18453388 Pulled By: zou3519 fbshipit-source-id: 96c3c6fd9478d57e92e1cf770c864aeac6d29dd2
Stack from ghstack:
In our name inference functions we currently create an extra TensorNames
every time we unify names. This isn't completely necessary.
To do this, I made the following changes:
Test Plan:
pytest test/test_namedtensor.py -vDifferential Revision: D18453388