Make TensorOptions contain optional fields, optimize struct size #12103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

ezyang wants to merge 13 commits into master from export-D10052523

Contributor

ezyang commented Sep 26, 2018 •

edited

Loading

Stack:
:black_circle: #12103 Make TensorOptions contain optional fields, optimize struct size 💛

This defers lookup of defaults to the site where we read
out of TensorOptions. THIS IS A BC-BREAKING BEHAVIOR CHANGE,
but we expect the bulk of uses of OptionsGuard don't allocate TensorOptions
inside the OptionsGuard region, and then use it outside of the region
(the situation where behavior could change.)

I also optimize the size of TensorOptions by rearranging fields, so that we
always fit in two 64-bit words.

Differential Revision: D10052523

ezyang added 4 commits

September 26, 2018 10:45


          V1: Initial commit

f40f187

Differential Revision: D10052523
Differential Version: 58936952


          V2: Stop using optional

ba015ee

Differential Revision: D10052523
Differential Version: 58951676


          V3: (no description)

739b00f

Differential Revision: D10052523
Differential Version: 58957413


          V4: Fix some usage sites.

9c089c1

Differential Revision: D10052523
Differential Version: 58961023

ezyang requested review from apaszke, colesbury, ebetica, gchanan, goldsborough, soumith and zdevito as code owners

September 26, 2018 21:19

ezyang added 2 commits

September 26, 2018 14:55


          V5: Fix a bug

0eac4d0

Differential Revision: D10052523
Differential Version: 58965097


          V6: Fix some more compile bugs

f9b439b

Differential Revision: D10052523
Differential Version: 59013668

Contributor Author

ezyang commented Sep 27, 2018

CC also @gchanan, you asked for this!

goldsborough approved these changes

View reviewed changes

Contributor

goldsborough left a comment

I think this behavior could use some tests in test/cpp/api/tensor_options.cpp

aten/src/ATen/core/OptionsGuard.cpp Outdated

    
              #else

              TensorOptions DefaultTensorOptions::options_(/*use_thread_local_default_options=*/false);

              static DefaultTensorOptions options_;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/OptionsGuard.h Outdated

    
                /// to Windows and other compilers.

                /// TODO: The inability to use optional in headers is no longer true

                CAFFE2_API static TensorOptions& get();

                DefaultTensorOptions() {}

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/OptionsGuard.h Outdated

    
                DefaultTensorOptions(DefaultTensorOptions&&) = default;

                DefaultTensorOptions& operator=(DefaultTensorOptions&&) = default;

                ScalarType dtype()    const { return dtype_; }

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/OptionsGuard.h Outdated

    
                bool requires_grad()  const { return requires_grad_; }

                bool is_variable()    const { return is_variable_; }

                DefaultTensorOptions& apply(const TensorOptions& options) {

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/OptionsGuard.h Outdated

    
                ScalarType dtype()    const { return dtype_; }

                Device device()       const { return device_; }

                Layout layout()       const { return layout_; }

                bool requires_grad()  const { return requires_grad_; }

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/TensorOptions.h Outdated

    
                }

                Device device() const noexcept;

                optional<Device> device_opt() const noexcept { return device_; }

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/TensorOptions.h Outdated

    
                }

                ScalarType dtype() const noexcept;

                optional<ScalarType> dtype_opt() const noexcept { return dtype_; }

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/TensorOptions.h Outdated

    
                }

                Device device() const noexcept;

                optional<Device> device_opt() const noexcept { return device_; }

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

goldsborough reviewed

View reviewed changes

aten/src/ATen/core/OptionsGuard.h Outdated

    
                CAFFE2_API static TensorOptions& get();

                DefaultTensorOptions() {}

                DefaultTensorOptions(const DefaultTensorOptions&) = default;

                DefaultTensorOptions& operator=(const DefaultTensorOptions&) = default;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

goldsborough reviewed

View reviewed changes

aten/src/ATen/core/OptionsGuard.h Outdated

    
               private:

                static TensorOptions options_;

              };

              // Return the global, immutable default tensor options

This comment was marked as off-topic.

Sign in to view

goldsborough reviewed

View reviewed changes

aten/src/ATen/core/TensorOptions.h Outdated

    
                Layout layout() const noexcept {

                  return layout_;

                }

                Layout layout() const noexcept;

This comment was marked as off-topic.

Sign in to view

goldsborough reviewed

View reviewed changes

aten/src/ATen/core/TensorOptions.h

    
                optional<bool> is_variable_opt() const noexcept { return is_variable_; }

                // Resolves the ATen backend specified by the current construction axes.

                Backend backend() const noexcept {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

goldsborough reviewed

View reviewed changes

aten/src/ATen/core/TensorOptions.h Outdated

    
                Layout layout_{Layout::Strided};

                bool requires_grad_{false};

                bool is_variable_{false};

                optional<ScalarType> dtype_;

This comment was marked as off-topic.

Sign in to view

ezyang added 2 commits

October 3, 2018 10:34


          V7: CR comments

22dfb9e

Differential Revision: D10052523
Differential Version: 59581525


          V8: Optimize for performance

cda65b3

Differential Revision: D10052523
Differential Version: 59740131

ezyang changed the title ~~Make TensorOptions contain optional fields.~~ Make TensorOptions contain optional fields, optimize struct size

ezyang added 3 commits

October 4, 2018 11:27


          V9: Add messages to static assert

Differential Revision: D10052523
Differential Version: 59742335


          V10: Missing const

1d8a354

Differential Revision: D10052523
Differential Version: 59742581


          V11: Some more compilefixes

dbe4114

Differential Revision: D10052523
Differential Version: 59745290

Contributor Author

ezyang commented Oct 4, 2018

@goldsborough I think all CR comments are addressed. I noticed that use of optional increased size of TensorOptions, so I rewrote it with a bitfield.

ezyang added 2 commits

October 4, 2018 15:11


          V12: Fix compile error

e573b97

Differential Revision: D10052523
Differential Version: 59777715


          V13: fix mising has

Differential Revision: D10052523
Differential Version: 59813029

facebook-github-bot closed this in

1e70500

zdevito pushed a commit to zdevito/ATen that referenced this pull request


          Make TensorOptions contain optional fields, optimize struct size (#12…

36bbc4d

…103)

Summary:
Pull Request resolved: pytorch/pytorch#12103

This defers lookup of defaults to the site where we read
out of TensorOptions. THIS IS A BC-BREAKING BEHAVIOR CHANGE,
but we expect the bulk of uses of OptionsGuard don't allocate TensorOptions
inside the OptionsGuard region, and then use it outside of the region
(the situation where behavior could change.)

I also optimize the size of TensorOptions by rearranging fields, so that we
always fit in two 64-bit words.

Reviewed By: goldsborough

Differential Revision: D10052523

fbshipit-source-id: f454a15b4dbf8cd17bc902ab7d2016f2f689ed13

soumith deleted the export-D10052523 branch

February 21, 2019 12:11

ezyang added the merged label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

apaszke Awaiting requested review from apaszke

colesbury Awaiting requested review from colesbury

ebetica Awaiting requested review from ebetica

gchanan Awaiting requested review from gchanan

soumith Awaiting requested review from soumith

zdevito Awaiting requested review from zdevito

1 more reviewer

goldsborough goldsborough approved these changes

Labels

None yet