Skip to content

[AOTAutograd] Micro-optimize runtime_wrapper #128188

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
wants to merge 15 commits into from
Closed

[AOTAutograd] Micro-optimize runtime_wrapper #128188

wants to merge 15 commits into from

Conversation

peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented Jun 7, 2024 •
edited
Loading

Stack from ghstack (oldest at bottom):

This moves a bunch of runtime inspection of the output_info for alias handling into the construction of fixed output handlers that are created during compilation and captured by the runtime wrapper.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang

@peterbell10
Update
7b05cc9 
[ghstack-poisoned]
This was referenced Jun 7, 2024
Closed
Closed
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jun 7, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/128188

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit d774dbd with merge base a21d436 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@peterbell10 peterbell10 mentioned this pull request Jun 7, 2024
Closed
This was referenced Jun 7, 2024
Closed
Closed
@peterbell10
Update
99c4836 
[ghstack-poisoned]
@peterbell10
Update
f2936bb 
[ghstack-poisoned]
@peterbell10
Update on "[AOTAutograd] Micro-optimize runtime_wrapper"
72a1461 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

[ghstack-poisoned]
@peterbell10
Update on "[AOTAutograd] Micro-optimize runtime_wrapper"
2cbabf3 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

[ghstack-poisoned]
@peterbell10 peterbell10 marked this pull request as ready for review June 7, 2024 17:17
@peterbell10 peterbell10 requested review from bdhirsh and lezcano June 7, 2024 17:17
peterbell10 added a commit to peterbell10/pytorch that referenced this pull request Jun 9, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
6c351ee 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: 59b865c
Pull Request resolved: pytorch#128188
peterbell10 added a commit to peterbell10/pytorch that referenced this pull request Jun 9, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
0efd5bf 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: b5b6f51
Pull Request resolved: pytorch#128188
@peterbell10
Update
1e3d946 
[ghstack-poisoned]
@peterbell10
Lint
971dd30 
[ghstack-poisoned]
@peterbell10
Update on "[AOTAutograd] Micro-optimize runtime_wrapper"
09c3649 
This moves a bunch of runtime inspection of the `output_info` for alias handling into the construction of fixed output handlers that are called at runtime.

[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Jun 9, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
ac6c7ee 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: 19c599c
Pull Request resolved: #128188
lezcano
lezcano reviewed Jun 10, 2024
View reviewed changes
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let Brian look into these.

Could you also add a benchmark at ./benchmarks/dynamo/microbenchmarks/microbench.py and do a before / after with how much speed-up this PR is expected to bring?

@peterbell10
Update
c8d87bb 
[ghstack-poisoned]
@peterbell10
Copy link
Collaborator Author

I'm seeing a 20% speedup in return (aten.alias(x),)

peterbell10 added a commit that referenced this pull request Jun 10, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
f915923 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: aceeb13
Pull Request resolved: #128188
@peterbell10
Rebase
6ad91c6 
[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Jun 20, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
3e74c70 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: c5507f1
Pull Request resolved: #128188
@peterbell10
Update
b7013ae 
[ghstack-poisoned]
@peterbell10
Update
cdf4044 
[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Jun 28, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
d8e5f6b 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: d2aa014
Pull Request resolved: #128188
@peterbell10
Update
3d38aac 
[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Jun 28, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
c947f9f 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: 57ce0b4
Pull Request resolved: #128188
@peterbell10
Lint
49be6f6 
[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request Jul 2, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
347aeda 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: 5ef3111
Pull Request resolved: #128188
@peterbell10
Copy link
Collaborator Author

I've run some benchmarks and the cold start times are the same to within .1% in a variety of torchbench models, so I don't think there is any issue here.

@peterbell10
Copy link
Collaborator Author

@pytorchbot merge -r

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 4, 2024
@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@peterbell10
Update
d774dbd 
[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/peterbell10/740/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/128188)

pytorchmergebot pushed a commit that referenced this pull request Jul 4, 2024
@peterbell10
[AOTAutograd] Micro-optimize runtime_wrapper
0e404a8 
1. `config.__getattr__` takes ~.5us so move outside of the wrapper
2. use builtins.zip instead of `strict_zip` since we assert the lengths anyway
3. also in `strict_zip` we don't need to find the min length

ghstack-source-id: d75cc1a
Pull Request resolved: #128188
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@peterbell10 peterbell10 added the topic: not user facing topic category label Jul 4, 2024
@peterbell10
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@github-actions github-actions bot deleted the gh/peterbell10/740/head branch August 4, 2024 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lezcano lezcano lezcano left review comments

@jamesjwu jamesjwu jamesjwu left review comments

@bdhirsh bdhirsh bdhirsh approved these changes

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo open source topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@peterbell10 @jamesjwu @pytorchmergebot @lezcano @bdhirsh @pytorchbot