Skip to content
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.

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

Allow more inserts before reIndexTopology #102312

Closed
wants to merge 2 commits into from

Conversation

zdevito
Copy link
Contributor

@zdevito zdevito commented May 25, 2023

Stack from ghstack (oldest at bottom):

Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

Differential Revision: D46206617

Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

[ghstack-poisoned]
@pytorch-bot pytorch-bot bot added the release notes: jit release notes category label May 25, 2023
@pytorch-bot
Copy link

pytorch-bot bot commented May 25, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 91ce1e3:
💚 Looks good so far! There are no failures yet. 💚

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

zdevito added a commit that referenced this pull request May 25, 2023
Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

ghstack-source-id: 80b872aa1bf2410ba6418d38aa66ac9906a16496
Pull Request resolved: #102312
@zdevito
Copy link
Contributor Author

zdevito commented May 25, 2023

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

@zdevito zdevito requested a review from eellison May 25, 2023 22:58
@zdevito zdevito added the ciflow/trunk Trigger trunk jobs on your pull request label May 25, 2023
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

Nice !! This works only with the setInsertPoint api and not with n1->insertBefore(n2)... The former is the common idiom and i dont know how to fix the second anyway.

Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

Differential Revision: [D46206617](https://our.internmc.facebook.com/intern/diff/D46206617)

[ghstack-poisoned]
zdevito added a commit that referenced this pull request May 31, 2023
Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

ghstack-source-id: 73164b5b99b888dabcc4344ab10feea18ccb3986
Pull Request resolved: #102312
@zdevito
Copy link
Contributor Author

zdevito commented May 31, 2023

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

@zdevito
Copy link
Contributor Author

zdevito commented May 31, 2023

@pytorchbot land

@pytorch-bot
Copy link

pytorch-bot bot commented May 31, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: argument command: invalid choice: 'land' (choose from 'merge', 'revert', 'rebase', 'label', 'drci')

usage: @pytorchbot [-h] {merge,revert,rebase,label,drci} ...

Try @pytorchbot --help for more info.

@zdevito
Copy link
Contributor Author

zdevito commented May 31, 2023

@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

shaoyf42 pushed a commit to shaoyf42/pytorch that referenced this pull request Jun 1, 2023
Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

Differential Revision: [D46206617](https://our.internmc.facebook.com/intern/diff/D46206617)
Pull Request resolved: pytorch#102312
Approved by: https://github.com/eellison
alimoezzi pushed a commit to alimoezzi/pytorch that referenced this pull request Jun 3, 2023
Summary:
Currently if you are inserting into JIT IR at the same point in the middle of the graph,
it only allows for 40 inserts before it has to reindex. Reindexing is N**2 behavior, which can
lead to slow load times. This changes it so that it keeps track of how many insertions happen
at single point (like when a function is being inlined) to predict how many future insertions will happen
there. It then adjusts how it assigns topology to make sure there is enough room for those predicted insertions.
In practice this will allow around 2M inserts at a single point before it reindexes.

Test Plan: test_jit.py

Differential Revision: [D46206617](https://our.internmc.facebook.com/intern/diff/D46206617)
Pull Request resolved: pytorch#102312
Approved by: https://github.com/eellison
@facebook-github-bot facebook-github-bot deleted the gh/zdevito/241/head branch June 8, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: jit release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants