functionalization: avoid some unnecessary view_copy calls by bdhirsh · Pull Request #75819 · pytorch/pytorch

bdhirsh · 2022-04-14T19:13:29Z

When we're performing a sync on a FuntionalTensorWrapper, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base.

This came up in the context of mobile using functionalize() and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872

cc @ZolotukhinM

We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply.

Stack from ghstack:

integrate functionalization <> LTC torchscript backend #75527 [prototype] integrate functionalization <> LTC torchscript backend
functionalization: introduce a "zero()" aten op #75913 functionalization: introduce a "zero()" aten op
functionalization: avoid some unnecessary view_copy calls #75819 functionalization: avoid some unnecessary view_copy calls
fix unfold for meta tensors #75717 fix unfold for meta tensors
teach ivalue about List[Optional[Tensor]], fix fallbacks #75716 teach ivalue about List[Optional[Tensor]], fix fallbacks
fix out= op handling for functionalization #75818 fix out= op handling for functionalization
split out functionalization codegen to use view_copy operators #75302 split out functionalization codegen to use view_copy operators

Differential Revision: D35705380

[ghstack-poisoned]

facebook-github-bot · 2022-04-14T19:13:36Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/75819
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

💊 CI failures summary and remediations

As of commit f96d020 (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).

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

Click here to manually regenerate this comment.

@ZolotukhinM

When we're performing a sync on a `FuntionalTensorWrapper`, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base. This came up in the context of mobile using `functionalize()` and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872 cc @ZolotukhinM We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply. [ghstack-poisoned]

@ZolotukhinM

When we're performing a sync on a `FuntionalTensorWrapper`, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base. This came up in the context of mobile using `functionalize()` and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872 cc @ZolotukhinM We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply. [ghstack-poisoned]

and exporting it to fbcode (not actually landing tis) [ghstack-poisoned]

and exporting it to fbcode (not actually landing tis) ghstack-source-id: b486c57 Pull Request resolved: #75835

… (not actually landing)

and exporting it to fbcode (not actually landing tis) ghstack-source-id: b486c57 Pull Request resolved: #75835

ezyang

Nice! Having a test would be even better!

@ZolotukhinM

When we're performing a sync on a `FuntionalTensorWrapper`, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base. This came up in the context of mobile using `functionalize()` and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872 cc @ZolotukhinM We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply. [ghstack-poisoned]

@ZolotukhinM

When we're performing a sync on a `FuntionalTensorWrapper`, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base. This came up in the context of mobile using `functionalize()` and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872 cc @ZolotukhinM We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply. [ghstack-poisoned]

@ZolotukhinM

When we're performing a sync on a `FuntionalTensorWrapper`, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base. This came up in the context of mobile using `functionalize()` and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872 cc @ZolotukhinM We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply. [ghstack-poisoned]

bdhirsh · 2022-04-17T01:02:12Z

@bdhirsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-04-18T20:04:04Z

@pytorchbot merge this

(Initiating merge automatically since Phabricator Diff has merged)

pytorchmergebot · 2022-04-18T20:05:29Z

Merge failed due to Command git -C /home/runner/work/pytorch/pytorch push origin master returned non-zero exit code 1
To https://github.com/pytorch/pytorch
! [remote rejected] master -> master (cannot lock ref 'refs/heads/master': is at 7be1b29 but expected 4c7b4b5)
error: failed to push some refs to 'https://github.com/pytorch/pytorch'

Raised by https://github.com/pytorch/pytorch/actions/runs/2185815423

bdhirsh · 2022-04-18T20:36:17Z

@pytorchmergebot merge this please

github-actions · 2022-04-18T20:47:19Z

Hey @bdhirsh.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Pull Request resolved: #75819 When we're performing a sync on a `FuntionalTensorWrapper`, right now we first apply any updates to the base, and then we unconditionally regenerate the current tensor from the base. This came up in the context of mobile using `functionalize()` and seeing an extra copy, so I have tests in a corresponding functorch PR: https://github.com/pytorch/functorch/pull/678/files#diff-449582f4f3e1ae76a73397c4b8ca62cc9dd5235dd831187f143180b498dee332R2872 cc ZolotukhinM We can avoid some unnecessary copies going to the backend if we skip the regenerating when we know that there weren't any updates to apply. Test Plan: Imported from OSS Reviewed By: zhxchen17 Differential Revision: D35705380 Pulled By: bdhirsh fbshipit-source-id: acd6811b4ca45e3b9d277496b391994fc7240927

…iews of same base" welp, I realized my "perf improvement" from #75819 was wrong. This originally came up because `functionalize()` was sometimes emit an unnecessary `view_copy`, but there's a more correct fix that I can make inside of functorch, which I have here: pytorch/functorch#795 [ghstack-poisoned]