[PyTorch] RFC: optimize c10::optional<ArrayRef<T>> by swolchok · Pull Request #59333 · pytorch/pytorch

swolchok · 2021-06-02T19:11:52Z

Stack from ghstack:

[PyTorch] RFC: optimize c10::optional<ArrayRef<T>> #59333 [PyTorch] RFC: optimize c10::optional<ArrayRef>

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers.

Differential Revision: D28843027

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

facebook-github-bot · 2021-06-02T19:11:55Z

💊 CI failures summary and remediations

As of commit 2d02790 (more details on the Dr. CI page):

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build (1/1)

Step: "Build" (full log | diagnosis details | 🔁 rerun)

Jun 04 20:05:58 rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory

Jun 04 20:05:58 ++++ extract_trap_cmd
Jun 04 20:05:58 ++++ printf '%s\n' ''
Jun 04 20:05:58 +++ printf '%s\n' cleanup
Jun 04 20:05:58 ++ trap -- '
Jun 04 20:05:58 cleanup' EXIT
Jun 04 20:05:58 ++ [[ pytorch-linux-xenial-cuda11.1-cudnn8-py3-gcc7-build != *pytorch-win-* ]]
Jun 04 20:05:58 ++ which sccache
Jun 04 20:05:58 ++ sccache --stop-server
Jun 04 20:05:58 ++ true
Jun 04 20:05:58 ++ rm /var/lib/jenkins/sccache_error.log
Jun 04 20:05:58 rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory
Jun 04 20:05:58 ++ true
Jun 04 20:05:58 ++ [[ -n '' ]]
Jun 04 20:05:58 ++ [[ pytorch-linux-xenial-cuda11.1-cudnn8-py3-gcc7-build == *rocm* ]]
Jun 04 20:05:58 ++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log
Jun 04 20:05:58 ++ SCCACHE_IDLE_TIMEOUT=1200
Jun 04 20:05:58 ++ RUST_LOG=sccache::server=error
Jun 04 20:05:58 ++ sccache --start-server
Jun 04 20:05:58 sccache: Starting the server...
Jun 04 20:05:58 ++ sccache --zero-stats
Jun 04 20:05:58 Compile requests                      0

1 job timed out:

pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

Pull Request resolved: #59333 Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. ghstack-source-id: 130402821 Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/)

swolchok · 2021-06-02T19:42:56Z

If and when this is committed, we should leave a breadcrumb somewhere indicating that this is a thing that will regress when we upgrade to std::optional, and we'll need to port this optimization over.

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

Pull Request resolved: #59333 Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. ghstack-source-id: 130427769 Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28843027/)!

ezyang · 2021-06-03T14:50:28Z

Easy breadcrumb is add a static assert on c10::optional<ArrayRef<T>> size with a comment saying what to do when the static assert fails.

ezyang · 2021-06-03T15:04:19Z

  size_type Length;

+  void debugCheckNullptrInvariant() {
+    TORCH_INTERNAL_ASSERT(


DEBUG_ONLY assert?

ezyang · 2021-06-03T15:04:50Z

+    "c10::optional<IntArrayRef> should be trivially copyable");
+static_assert(
+    sizeof(c10::optional<c10::IntArrayRef>) == sizeof(c10::IntArrayRef),
+    "c10::optional<IntArrayRef> should be size-optimized");


This is assert is good, just shouldn't be in this file (because it's likely to get deleted when we go to std::optional)

if people are going to delete this file without looking at it, what's going to stop them from deleting the contents of Optional.h or Optional_test.cpp without looking? (and which one of those two did you want this moved to)

Blugh, IDK, some random test file in ATen/test maybe. Or we can just trust in code review, I guess.

ezyang · 2021-06-03T15:05:06Z

+};
+
+// HACK: Optimization for ArrayRef<T>. We take advantage of an unused
+// bit pattern in ArrayRef (inspirted by Arthur O'Dwyer's


nit: inspired

ezyang

okey dokey

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

…o on "[PyTorch] RFC: optimize c10::optional<ArrayRef<T>>" Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

Pull Request resolved: #59333 Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. ghstack-source-id: 130553181 Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28843027/)!

…ize c10::optional<ArrayRef<T>>" Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

…nal<ArrayRef<T>>" Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

Pull Request resolved: #59333 Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. ghstack-source-id: 130623937 Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28843027/)!

Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) [ghstack-poisoned]

Pull Request resolved: #59333 Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. ghstack-source-id: 130631329 Differential Revision: [D28843027](https://our.internmc.facebook.com/intern/diff/D28843027/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28843027/)!

facebook-github-bot · 2021-06-07T18:36:40Z

This pull request has been merged in 1798ff0.

Summary: Pull Request resolved: pytorch#59333 Code comment should explain this in sufficient detail. In brief, making it 16 bytes should get it to be passed in registers. ghstack-source-id: 130631329 Test Plan: Updated optional_test and added static_assert in Optional.cpp. Reviewed By: ezyang Differential Revision: D28843027 fbshipit-source-id: 3029f05e03a9f04ca7337962e7770cdeb9a608d9

facebook-github-bot added the cla signed label Jun 2, 2021

swolchok requested review from ezyang and samestep June 2, 2021 19:39

ezyang reviewed Jun 3, 2021

View reviewed changes

ezyang approved these changes Jun 3, 2021

View reviewed changes

facebook-github-bot closed this in 1798ff0 Jun 7, 2021

facebook-github-bot added the Merged label Jun 7, 2021

facebook-github-bot deleted the gh/swolchok/252/head branch June 11, 2021 14:17

ejguan mentioned this pull request Jun 15, 2021

Compilation issue with CUDA 92 #60016

Closed

dagitses mentioned this pull request Sep 20, 2021

delegate to the proper comparison operator #62889

Closed

dagitses mentioned this pull request Nov 15, 2022

Migrate PyTorch to C++17 #85969

Closed

swolchok mentioned this pull request Oct 10, 2025

Refactor out headeronly ArrayRef #164991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PyTorch] RFC: optimize c10::optional<ArrayRef<T>>#59333

[PyTorch] RFC: optimize c10::optional<ArrayRef<T>>#59333
swolchok wants to merge 12 commits intogh/swolchok/252/basefrom
gh/swolchok/252/head

swolchok commented Jun 2, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Jun 2, 2021 •

edited

Loading

Uh oh!

swolchok commented Jun 2, 2021

Uh oh!

ezyang commented Jun 3, 2021

Uh oh!

ezyang Jun 3, 2021

Uh oh!

ezyang Jun 3, 2021

Uh oh!

swolchok Jun 3, 2021

Uh oh!

ezyang Jun 4, 2021

Uh oh!

ezyang Jun 3, 2021

Uh oh!

ezyang left a comment

Uh oh!

facebook-github-bot commented Jun 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

swolchok commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build (1/1)

Uh oh!

swolchok commented Jun 2, 2021

Uh oh!

ezyang commented Jun 3, 2021

Uh oh!

ezyang Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

swolchok Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swolchok commented Jun 2, 2021 •

edited

Loading

facebook-github-bot commented Jun 2, 2021 •

edited

Loading