[PyTorch] Introduce packed SizesAndStrides abstraction#47507
Closed
swolchok wants to merge 14 commits intogh/swolchok/9/basefrom
Closed
[PyTorch] Introduce packed SizesAndStrides abstraction#47507swolchok wants to merge 14 commits intogh/swolchok/9/basefrom
swolchok wants to merge 14 commits intogh/swolchok/9/basefrom
Conversation
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit e3b0199 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
…ked SizesAndStrides abstraction" This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
ezyang
reviewed
Nov 10, 2020
| for (int64_t dim = 0; dim < public_dims; dim++) { | ||
| auto actual_dim = actualDim(dim, /*wrap_dim=*/false); | ||
| sizes_.push_back(value_sizes.at(actual_dim)); | ||
| sizes_and_strides_.size_at_unchecked(dim) = value_sizes.at(actual_dim); |
Contributor
There was a problem hiding this comment.
@zou3519 Previously, this code did not initialize strides at all. Is it the case that BatchedTensor doesn't have valid strides at all?
Contributor
There was a problem hiding this comment.
Yes, BatchedTensor doesn't have valid strides until #47621 goes in.
ezyang
reviewed
Nov 10, 2020
| int64_t BatchedTensorImpl::actualDim(int64_t dim, bool wrap_dim) const { | ||
| if (wrap_dim) { | ||
| const auto ndim = sizes_.size(); | ||
| const auto ndim = sizes_and_strides_.size(); |
Contributor
There was a problem hiding this comment.
No action needed here: UGHH this should have been a call to dim() but that's virtual so it is a pessimization over this version reeeee
ezyang
reviewed
Nov 10, 2020
| bool allow_tensor_metadata_change) const override { | ||
| auto impl = c10::make_intrusive<OpaqueTensorImpl<OpaqueHandle>>( | ||
| key_set(), dtype(), device(), opaque_handle_, sizes_); | ||
| key_set(), dtype(), device(), opaque_handle_, IntArrayRef{sizes_and_strides_.sizes_data(), sizes_and_strides_.size()}); |
Contributor
There was a problem hiding this comment.
Probably would be handy if there was a way to directly get at a sizes IntArrayRef, instead of having to construct it by hand here.
ezyang
reviewed
Nov 10, 2020
| } | ||
|
|
||
| if ((!size.equals(sizes_)) || (sparse_dim != sparse_dim_) || (dense_dim != dense_dim_)) { | ||
| const bool size_equals_sizes = size.size() == sizes_and_strides_.size() && memcmp(size.data(), sizes_and_strides_.sizes_data(), sizes_and_strides_.size() * sizeof(*size.data())) == 0; |
Contributor
There was a problem hiding this comment.
Surely there is some helper method we could add here :)
ezyang
reviewed
Nov 10, 2020
| TORCH_CHECK(allow_tensor_metadata_change(), "raw_resize_ ", err_msg_tensor_metadata_change_not_allowed); | ||
| sizes_ = size.vec(); | ||
| sizes_and_strides_.resize(size.size()); | ||
| std::copy(size.begin(), size.end(), sizes_and_strides_.sizes_begin()); |
Contributor
There was a problem hiding this comment.
I'd probably also vote for a helper method for abstracting over std::copy calls
Contributor
Author
There was a problem hiding this comment.
adding set_sizes, but it feels a little dangerous to me to cover up a resize like that.
ezyang
reviewed
Nov 10, 2020
| if (sizes_[d] != 1) { | ||
| if (strides_[d] == z) { | ||
| z *= sizes_[d]; | ||
| const auto size_d = sizes_and_strides_.size_at_unchecked(d); |
Contributor
There was a problem hiding this comment.
Blegh, size(d) here should have been OK but it's also virtual sighhh
ezyang
reviewed
Nov 10, 2020
ezyang
approved these changes
Nov 10, 2020
Contributor
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
This looks good. I'd probably err on the side of more helper methods than less, and commented a few places.
smessmer
approved these changes
Nov 10, 2020
Contributor
|
The open source ASAN failure looks pretty sus, and may be related to how we handle empty sizes/strides. Hopefully you can repro it in fbcode; the oss ASAN repro process is pretty involved (unfortunately...) |
Contributor
|
I patched this stack onto #47428 and ran the following A/B comparison: Baseline#47507 & #47508I deliberately chose one with the "good" (fast) way to get metadata and one with the "bad" (slow) way. (As an aside, we should bridge that gap.) As you can see, this stack adds an instruction or two (presumably because we now have to unpack?) and 0.1-0.35 ns. Not the end of the world, but I'm not sure if it makes sense to take on the complexity if it won't help perf. Though of course this is a microbenchmark, and shrinking TensorImpl will have positives which are not captured here. |
…ed SizesAndStrides abstraction" This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
swolchok
added a commit
that referenced
this pull request
Nov 10, 2020
Pull Request resolved: #47507 This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. ghstack-source-id: 116352504 Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)!
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
swolchok
added a commit
that referenced
this pull request
Nov 17, 2020
Pull Request resolved: #47507 This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. ghstack-source-id: 116928200 Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)!
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
…ce packed SizesAndStrides abstraction" This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
bhosmer
approved these changes
Dec 1, 2020
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
…ides abstraction" This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
…] Introduce packed SizesAndStrides abstraction" This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. Differential Revision: [D24762557](https://our.internmc.facebook.com/intern/diff/D24762557/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D24762557/)! [ghstack-poisoned]
Contributor
|
This pull request has been merged in 882ddb2. |
hwangdeyu
pushed a commit
to hwangdeyu/pytorch
that referenced
this pull request
Jan 14, 2021
Summary: Pull Request resolved: pytorch#47507 This introduces a new SizesAndStrides class as a helper for TensorImpl, in preparation for changing its representation. ghstack-source-id: 119313559 Test Plan: Added new automated tests as well. Run framework overhead benchmarks. Results seem to be neutral-ish. Reviewed By: ezyang Differential Revision: D24762557 fbshipit-source-id: 6cc0ede52d0a126549fb51eecef92af41c3e1a98
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
This introduces a new SizesAndStrides class as a helper for
TensorImpl, in preparation for changing its representation.
Differential Revision: D24762557
NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!