[PyTorch] Store Tensor explicitly in IValue #48824

swolchok · 2020-12-04T05:10:56Z

Stack from ghstack:

[PyTorch] List::operator[] can return const ref for Tensor & string #50083 [PyTorch] List::operator[] can return const ref for Tensor & string
[PyTorch] IValue::toTensor can now return const Tensor& #48868 [PyTorch] IValue::toTensor can now return const Tensor&
[PyTorch] Store Tensor explicitly in IValue #48824 [PyTorch] Store Tensor explicitly in IValue
[PyTorch] Additional IValue tests #49718 [PyTorch] Additional IValue tests

Enables following diff, which will make toTensor() return
const Tensor& and allow callers to avoid refcounting overhead.

Differential Revision: D25324617

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) ghstack-source-id: 117841198 Pull Request resolved: #48824

dr-ci · 2020-12-04T05:25:48Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

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.

This comment has been revised 94 times.

smessmer · 2020-12-04T18:46:28Z

aten/src/ATen/core/ivalue.h

  /// @private [doxygen private]
  ~IValue() {
    if (is_intrusive_ptr) {
      c10::raw::intrusive_ptr::decref(payload.as_intrusive_ptr);
+    } else if (isTensor()) {


Do we gain something by putting the isTensor check first? I would assume that Tensor objects are much more common than non-Tensor intrusive_ptr.

smessmer · 2020-12-04T18:47:38Z

aten/src/ATen/core/ivalue.h

+      return *this;
+    }
+
+    // Tear down our state.


can we deduplicate this logic with the logic in the constructor by moving them into a destroy() method?

Sure, but that's more work for the inliner to get right and these code paths are critical. I can try it.

smessmer · 2020-12-04T18:48:22Z

aten/src/ATen/core/ivalue.h

+      c10::raw::intrusive_ptr::decref(payload.as_intrusive_ptr);
+    }
+
+    if (rhs.isTensor()) {


and deduplicate this logic with the one from the move constructor?

smessmer · 2020-12-04T18:50:19Z

aten/src/ATen/core/ivalue.h

+    if (isTensor() && rhs.isTensor()) {
+      std::swap(payload.as_tensor, rhs.payload.as_tensor);
+    } else if (isTensor()) {
+      at::Tensor t = std::move(payload.as_tensor);


ugh this is more involved than I had hoped. I guess it's UB to just relocate the Tensor without destructing and constructing again?

IIUC, you have to construct it in rhs.payload using placement new or it's UB. Skiping the destructor call is legit, and I'll probably try that.

smessmer · 2020-12-04T18:52:37Z

aten/src/ATen/core/ivalue.h

+      memcpy(&payload, &rhs.payload, sizeof(payload));
+      new (&rhs.payload.as_tensor) at::Tensor(std::move(t));
+    } else if (rhs.isTensor()) {
+      rhs.swap(*this);


this is potentially slow because it needs to do the isTensor checks again (depending on how smart the compiler is with inlining this and proving that the extra branches are never executed). Not sure if relevant in practice, but if you want to optimize it, you could just move lines 332 to 335 into their own subfunction swapWithTensor(lhs, rhs) or something like that and call it from both the isTensor() and rhs.isTensor() case.

smessmer · 2020-12-04T18:57:01Z

aten/src/ATen/core/ivalue.h

    struct {
      DeviceType type;
      DeviceIndex index;
    } as_device;
+
+    Payload() : as_int(0) {}
+    ~Payload() {}


Any reason you're user-defining the destructor? = default should do the trick and would not make the destructor user defined, or just keep it omitted as before.

Unions with non-POD types in them are a pain. The destructor cannot be defined by default -- do you run ~Tensor() or not? So, we have to define it to do nothing.

smessmer · 2020-12-04T18:58:30Z

aten/src/ATen/core/ivalue.h

  };

-  IValue(Payload p, Tag t, bool i) : payload(p), tag(t), is_intrusive_ptr(i) {}
+  IValue(const Payload& p, Tag t, bool i) : tag(t), is_intrusive_ptr(i) {


even the largest Payload should be 64bit only and Payload has trivial copy/move constructors, so I would assume passing by value is better. Is passing by reference here related to the Itanium ABI thing you posted about?

Payload has trivial copy/move constructors

Not with Tensor in it -- do you run the Tensor copy/move constructors or not? It's not copyable.

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Pull Request resolved: #48824 Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. ghstack-source-id: 117906329 Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/)

… IValue" Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

swolchok · 2020-12-15T22:37:56Z

@smessmer could you take another look? I've had to change the approach to improve performance.

… explicitly in IValue" Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

… IValue" Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Pull Request resolved: #48824 Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. ghstack-source-id: 118955313 Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/)

…IValue" Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

facebook-github-bot · 2021-01-06T16:41:18Z

This pull request has been merged in 1b31e13.

Summary: Pull Request resolved: pytorch#48824 Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. ghstack-source-id: 119327370 Test Plan: ivalue_test Internal benchmark to ensure perf parity. Some interesting steps during the debugging process: - First version was about a 5% regression - Directly implementing move construction instead of using swap lowered the regression to 2-3% - Directly implementing move assign was maybe an 0.5% improvement - Adding C10_ALWAYS_INLINE on move assign got our regression to negligible - Fixing toTensor() to actually be correct regressed us again, but omitting the explicit dtor call as exhaustively spelled out in a comment fixed it. Reviewed By: bwasti Differential Revision: D25324617 fbshipit-source-id: 7518c1c67f6f2661f151b43310aaddf4fb6e511a

[PyTorch] Store Tensor explicitly in IValue

b48bbb4

Enables following diff, which will make toTensor() return `const Tensor&` and allow callers to avoid refcounting overhead. Differential Revision: [D25324617](https://our.internmc.facebook.com/intern/diff/D25324617/) [ghstack-poisoned]

facebook-github-bot added the cla signed label Dec 4, 2020

smessmer approved these changes Dec 4, 2020

View reviewed changes

swolchok mentioned this pull request Dec 4, 2020

[PyTorch] IValue::toTensor can now return const Tensor& #48868

Closed

swolchok added 3 commits December 4, 2020 15:01

swolchok requested a review from smessmer December 15, 2020 22:37

swolchok added 4 commits December 16, 2020 09:43

swolchok added 2 commits December 18, 2020 21:39

swolchok mentioned this pull request Dec 22, 2020

[PyTorch] Additional IValue tests #49718

Closed

swolchok added 2 commits December 22, 2020 13:45

swolchok mentioned this pull request Jan 5, 2021

[PyTorch] List::operator[] can return const ref for Tensor & string #50083

Closed

facebook-github-bot closed this in 1b31e13 Jan 6, 2021

facebook-github-bot added the Merged label Jan 6, 2021

facebook-github-bot deleted the gh/swolchok/30/head branch January 10, 2021 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PyTorch] Store Tensor explicitly in IValue #48824

[PyTorch] Store Tensor explicitly in IValue #48824

Uh oh!

swolchok commented Dec 4, 2020 •

edited

Loading

Uh oh!

dr-ci bot commented Dec 4, 2020 •

edited by facebook-github-bot

Loading

Uh oh!

smessmer Dec 4, 2020

Uh oh!

smessmer Dec 4, 2020

Uh oh!

swolchok Dec 4, 2020

Uh oh!

smessmer Dec 4, 2020

Uh oh!

smessmer Dec 4, 2020

Uh oh!

swolchok Dec 4, 2020

Uh oh!

smessmer Dec 4, 2020

Uh oh!

smessmer Dec 4, 2020

Uh oh!

swolchok Dec 4, 2020

Uh oh!

smessmer Dec 4, 2020

Uh oh!

swolchok Dec 4, 2020

Uh oh!

swolchok commented Dec 15, 2020

Uh oh!

facebook-github-bot commented Jan 6, 2021

Uh oh!

Uh oh!

[PyTorch] Store Tensor explicitly in IValue #48824

[PyTorch] Store Tensor explicitly in IValue #48824

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dr-ci bot commented Dec 4, 2020 • edited by facebook-github-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swolchok commented Dec 15, 2020

Uh oh!

facebook-github-bot commented Jan 6, 2021

Uh oh!

Uh oh!

swolchok commented Dec 4, 2020 •

edited

Loading

dr-ci bot commented Dec 4, 2020 •

edited by facebook-github-bot

Loading