Small fixes to improve TensorIterator overhead for the common case of inputs and outputs of the same type #27457
Conversation
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
pytorchbot
added
module: internals
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17784532 |
ngimel
force-pushed
the
export-D17784532
branch
from
59120d7 to
7310549
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17784532 |
Collaborator
Author
colesbury
reviewed
Oct 7, 2019
ngimel
force-pushed
the
export-D17784532
branch
from
7310549 to
c83fa03
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17784532 |
resistor
reviewed
Oct 8, 2019
Contributor
There was a problem hiding this comment.
Should this check be sunk into assert_no_partial_overlap?
Collaborator
Author
There was a problem hiding this comment.
I already added a short-circuit in get_overlap_status, but it was behind .contiguous so was slower, now I pulled it up there and removed from here, seems to be ok.
… inputs and outputs of the same type (pytorch#27457) Summary: Pull Request resolved: pytorch#27457 1) Short-circuits computing common type and type promotion logic for the common case of operands and result of the same type 2) Improves performance of checking memory overlap by returning MemoryOverlap::FULL if tensors are the same, skips the call from TensorIterator when tensors are the same 3) Changes the default size of DimVector from 5 to 6, thus allowing it not to be resized for a common case of binary operation. `strides` DimVector is forced to have at least 2*num_tensors elements, which for an operation with 2 inputs and one output is 6 4) If `offset` is 0 (common non-broadcasting case), don't fill `strides` vector with 0-s, because all the values will be subsequently written to. These changes combined improve the overhead from 1.02 us to .74 us for a simple in-place operation. Test Plan: should be covered by existing tests Differential Revision: D17784532 fbshipit-source-id: 5e523e1892cfe4b7aee0d90cc8e820409c309e9f
ngimel
force-pushed
the
export-D17784532
branch
from
c83fa03 to
3960835
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D17784532 |
VitalyFedyunin
approved these changes
Oct 14, 2019
| struct CAFFE2_API TensorIterator { | ||
| using DimMask = std::bitset<64>; | ||
| using PtrVector = SmallVector<char*, 4>; | ||
| using StrideVector = SmallVector<int64_t, 6>; |
Contributor
There was a problem hiding this comment.
super nit: maybe move this magical numbers into constant with good name
super super nit: make Ptr vector same size as StrideVector
Contributor
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zdevito
pushed a commit
to zdevito/ATen
that referenced
this pull request
Oct 16, 2019
… inputs and outputs of the same type (#27457) Summary: 1) Short-circuits computing common type and type promotion logic for the common case of operands and result of the same type 2) Improves performance of checking memory overlap by returning MemoryOverlap::FULL if tensors are the same, skips the call from TensorIterator when tensors are the same 3) Changes the default size of DimVector from 5 to 6, thus allowing it not to be resized for a common case of binary operation. `strides` DimVector is forced to have at least 2*num_tensors elements, which for an operation with 2 inputs and one output is 6 4) If `offset` is 0 (common non-broadcasting case), don't fill `strides` vector with 0-s, because all the values will be subsequently written to. These changes combined improve the overhead from 1.02 us to .74 us for a simple in-place operation. Pull Request resolved: pytorch/pytorch#27457 Test Plan: should be covered by existing tests Differential Revision: D17784532 Pulled By: ngimel fbshipit-source-id: e6a8ee58be5de14461bdbc2e2b0b6d16a96c309f
Contributor
thiagocrepaldi
pushed a commit
to thiagocrepaldi/pytorch
that referenced
this pull request
Feb 4, 2020
… inputs and outputs of the same type (pytorch#27457) Summary: 1) Short-circuits computing common type and type promotion logic for the common case of operands and result of the same type 2) Improves performance of checking memory overlap by returning MemoryOverlap::FULL if tensors are the same, skips the call from TensorIterator when tensors are the same 3) Changes the default size of DimVector from 5 to 6, thus allowing it not to be resized for a common case of binary operation. `strides` DimVector is forced to have at least 2*num_tensors elements, which for an operation with 2 inputs and one output is 6 4) If `offset` is 0 (common non-broadcasting case), don't fill `strides` vector with 0-s, because all the values will be subsequently written to. These changes combined improve the overhead from 1.02 us to .74 us for a simple in-place operation. Pull Request resolved: pytorch#27457 Test Plan: should be covered by existing tests Differential Revision: D17784532 Pulled By: ngimel fbshipit-source-id: e6a8ee58be5de14461bdbc2e2b0b6d16a96c309f
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.
Summary:
from TensorIterator when tensors are the same
stridesDimVector is forced to have at least 2*num_tensors elements, which for an operation with 2 inputs and one output is 6
offsetis 0 (common non-broadcasting case), don't fillstridesvector with 0-s, because all the values will be subsequently written to.These changes combined improve the overhead from 1.02 us to .74 us for a simple in-place operation.
Test Plan: should be covered by existing tests
Differential Revision: D17784532