Use ComplexHolder pointer in c10:Scalar #53247
Closed
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
Summary: Test Plan: USE_CUDA=0 BUILD_TEST=0 python setup.py install Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
Contributor
💊 CI failures summary and remediationsAs of commit 95fee44 (more details on the Dr. CI page):
🕵️ 11 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
Contributor
Author
|
Confirmed the Scalar size is reduced to 16 bytes with this PR. However, the following micro-benchmark shows ~1% regression. Will dig into. from torch.utils.benchmark import Timer
counts = Timer(
stmt="""auto out = t.add(a, b);""",
setup="auto t = torch::ones({1}); auto a = torch::Scalar(3); auto b = torch::Scalar(1);",
language="cpp",
).collect_callgrind(number=1_000)
print(counts)Use ComplexHolder in Scalar (4bd9eeb) : 29,024,819 |
smessmer
approved these changes
Mar 4, 2021
| } | ||
| } | ||
|
|
||
| Scalar(Scalar&& rhs) noexcept : tag(rhs.tag), v(rhs.v) {} |
Contributor
There was a problem hiding this comment.
this is likely a bug causing a double free. After this, you now have two Scalar objects that both are going to destroy the same complex. Could be fixed by setting the *this scalar to not store a complex anymore similar to what you're doing in the move assignment operator
Contributor
There was a problem hiding this comment.
as a side note: Having all four copy/move constructors and assignment operators implemented explicitly is some code duplication with the chance of bugs like this one. There's a nice pattern where you can implement all four of those in terms of just one implementation of swap, and then you have all the refcounting or destroying of state in just one function.
Contributor
Author
There was a problem hiding this comment.
@smessmer : Nice catch! The RHS complex will be cleaned up in move assignment but forget to to the same cleanup in move constructor.
Summary: Test Plan: USE_CUDA=0 BUILD_TEST=0 python setup.py install Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D26810622](https://our.internmc.facebook.com/intern/diff/D26810622) [ghstack-poisoned]
smessmer
reviewed
Mar 5, 2021
| return *this; | ||
| } | ||
|
|
||
| C10_ALWAYS_INLINE Scalar& operator=(Scalar&& rhs) & noexcept { |
Contributor
There was a problem hiding this comment.
any specific reason for using C10_ALWAYS_INLINE here? In almost all cases, the compiler knows better and forcing them to inline is a bad idea. Without this, for example, the compiler can make different decisions like say inlining it in server code but not on mobile.
Contributor
Author
There was a problem hiding this comment.
@smessmer : Got it. Originally when I start to write these move/copy methods, I followed the corresponding implementation in IValue and didn't reove C10_ALWAYS_INLINE:
pytorch/aten/src/ATen/core/ivalue.h
Line 197 in 56f8379
Side note: during benchmark, sometimes I found manually inline (i.e. copy the method impl) or some (seemingly unrelated) method code change can result in tiny (0.02%- 0.04%) instruction count noise . (is it due to inline decisions at certain callsites?) So a small potential small benefit of C10_ALWAYS_INLINE is to make the result more deterministic? Not a big advantage , though :)
Contributor
Author
|
Superseded by the pass-by reference solution (#53583) due to overhead of non-trivial constructors. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
Summary:
Test Plan:
USE_CUDA=0 BUILD_TEST=0 python setup.py install
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D26810622