[Pytorch] Specialize guts of c10::optional for 32-bit scalars by swolchok · Pull Request #47015 · pytorch/pytorch

swolchok · 2020-10-28T20:39:17Z

Stack from ghstack:

[Pytorch] Specialize guts of c10::optional for 32-bit scalars #47015 [Pytorch] Specialize guts of c10::optional for 32-bit scalars
[Pytorch] Add basic c10::optional tests #47014 [Pytorch] Add basic c10::optional tests
[pytorch] Remove c10::nullopt_t::init #47013 [pytorch] Remove c10::nullopt_t::init

c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use std::optional (or implement that functionality ourselves). We can't use std::optional because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step.

Differential Revision: D24552280

c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) ghstack-source-id: 115395876 Pull Request resolved: #47015

dr-ci · 2020-10-28T20:48:11Z

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 26 times.

swolchok · 2020-10-28T21:01:34Z

I tried to replicate the benchmark on #46598 so as to provide an apples-to-apple comparison, but I couldn't run the benchmark with the most recent version of that branch checked out.

…r 32-bit scalars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

Pull Request resolved: #47015 c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. ghstack-source-id: 115425175 Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/)

…tional for 32-bit scalars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

Pull Request resolved: #47015 c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. ghstack-source-id: 115447317 Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/)

smessmer · 2020-10-29T12:56:15Z

c10/util/Optional.h

+      std::is_nothrow_move_assignable<T>::value &&
+          std::is_nothrow_move_constructible<T>::value) {
+    if (init_ && !rhs.init_) {
+      clear();


clear() itself has a branch checking init_ again. The compiler might be able to optimize it away and branch prediction can probably handle it quite well too, but it might be safer to have a separate private method for clearing when we already know that init_==true.

Same above in the copy constructor and for the constexpr version.

smessmer · 2020-10-29T13:05:02Z

c10/util/Optional.h

-    constexpr_optional_base<typename std::remove_const<
-        T>::type>, // use base with trivial destructor
-    optional_base<typename std::remove_const<T>::type>>::type;
+    std::is_scalar<T>::value && sizeof(T) <= 4,


c10::ScalarType and c10::Layout are enums, so they'll hit the optimization here, but we'd also like to hit the optimization for c10::Device, which is a 32bit struct that can for most purposes be treated like a Scalar.

Should we use checks for trivial movability/copyability here instead of std::is_scalar? That might hit c10::Device too. Also not sure if we need the 32bit sizeof constraint. If a type is trivially movable/copyable, then the optimization could make sense even if the type is larger.

We might need to rewrite the optimization a bit though. What it currently does is it unconditionally calls the copy/move constructor, possibly on uninitialized elements. With scalars, that is likely within the standard. I'm not sure if doing that for trivially copy/movable types is within the standard or UB. Do you happen to know?

it unconditionally calls the copy/move constructor, possibly on uninitialized elements. With scalars, that is likely within the standard. I'm not sure if doing that for trivially copy/movable types is within the standard or UB. Do you happen to know?

We're not using a union, so all we require is that we can copy and move default-constructed objects of type T. If a type can't do that, it's broken. I'm not sure what's possibly uninitialized by the optional implementation; initialization is left up to T's default constructor.

We're not using a union

Ah, and that's a problem because T may not have a default constructor. I'll see what I can do.

I feel like, for all the types we care about this optimization applying to, we have default constructors

…ars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

Pull Request resolved: #47015 c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. ghstack-source-id: 115514082 Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/)

ezyang · 2020-10-30T16:11:21Z

This was a very interesting and informative PR to review, learned something. Thanks!

c10/util/Optional.h

smessmer · 2020-11-02T13:17:48Z

c10/util/Optional.h

 class optional : private OptionalBase<T> {
  template <class U> // re-declaration for nvcc on Windows.
  using OptionalBase = typename std::conditional<
-      std::is_trivially_destructible<U>::value, // if possible


We could use this PR to fix the duplication here. But not mandatory, up to you.

…ars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

Pull Request resolved: #47015 c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. ghstack-source-id: 115769681 Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/)

…tional for 32-bit scalars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

… on "[Pytorch] Specialize guts of c10::optional for 32-bit scalars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

Pull Request resolved: #47015 c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. ghstack-source-id: 115804292 Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/)

…pecialize guts of c10::optional for 32-bit scalars" c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/) [ghstack-poisoned]

Pull Request resolved: #47015 c10::optional has non-trivial copy and move operations always. This change specializes it for 32-bit scalars so that it has trivial copy and move operations in that case. Ideally, we would instead rely on P0602 "variant and optional should propagate copy/move triviality" and use `std::optional` (or implement that functionality ourselves). We can't use `std::optional` because we are stuck with C++14. Implementing the full P0602 ourselves would add even more complexity. We could do it, but this should be a helpful first step. ghstack-source-id: 115886743 Differential Revision: [D24552280](https://our.internmc.facebook.com/intern/diff/D24552280/)

codecov · 2020-11-05T03:42:00Z

Codecov Report

Merging #47015 into gh/swolchok/4/base will decrease coverage by 0.01%.
The diff coverage is 86.79%.

@@                  Coverage Diff                   @@
##           gh/swolchok/4/base   #47015      +/-   ##
======================================================
- Coverage               60.82%   60.81%   -0.02%     
======================================================
  Files                    2749     2750       +1     
  Lines                  254038   254009      -29     
======================================================
- Hits                   154531   154481      -50     
- Misses                  99507    99528      +21

facebook-github-bot · 2020-11-05T05:14:58Z

This pull request has been merged in df5b469.

This was referenced Oct 28, 2020

[pytorch] Remove c10::nullopt_t::init #47013

Closed

[Pytorch] Add basic c10::optional tests #47014

Closed

swolchok mentioned this pull request Oct 28, 2020

Pass optional by reference #46598

Closed

dzhulgakov requested review from ezyang and smessmer October 28, 2020 21:15

smessmer approved these changes Oct 29, 2020

View reviewed changes

facebook-github-bot added the cla signed label Oct 30, 2020

smessmer reviewed Nov 2, 2020

View reviewed changes

c10/util/Optional.h Outdated Show resolved Hide resolved

smessmer reviewed Nov 2, 2020

View reviewed changes

facebook-github-bot closed this in df5b469 Nov 5, 2020

facebook-github-bot added the Merged label Nov 5, 2020

facebook-github-bot deleted the gh/swolchok/4/head branch November 8, 2020 15:18

walterddr mentioned this pull request Nov 20, 2020

Compilation issue pulling latest pytorch nightly meta-pytorch/csprng#84

Closed

swolchok mentioned this pull request May 27, 2021

C++17 for PyTorch #56055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pytorch] Specialize guts of c10::optional for 32-bit scalars#47015

[Pytorch] Specialize guts of c10::optional for 32-bit scalars#47015
swolchok wants to merge 8 commits intogh/swolchok/4/basefrom
gh/swolchok/4/head

swolchok commented Oct 28, 2020 •

edited

Loading

Uh oh!

dr-ci bot commented Oct 28, 2020 •

edited

Loading

Uh oh!

swolchok commented Oct 28, 2020

Uh oh!

smessmer Oct 29, 2020

Uh oh!

smessmer Oct 29, 2020

Uh oh!

swolchok Oct 29, 2020

Uh oh!

swolchok Oct 29, 2020

Uh oh!

ezyang Oct 30, 2020

Uh oh!

ezyang commented Oct 30, 2020

Uh oh!

Uh oh!

smessmer Nov 2, 2020

Uh oh!

codecov bot commented Nov 5, 2020 •

edited

Loading

Uh oh!

facebook-github-bot commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

swolchok commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

swolchok commented Oct 28, 2020

Uh oh!

smessmer Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

smessmer Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

swolchok Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

swolchok Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang commented Oct 30, 2020

Uh oh!

Uh oh!

smessmer Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

swolchok commented Oct 28, 2020 •

edited

Loading

dr-ci bot commented Oct 28, 2020 •

edited

Loading

codecov bot commented Nov 5, 2020 •

edited

Loading