Cleanup operator class #113
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D48619001 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D48619001 |
larryliu0820
force-pushed
the
export-D48619001
branch
from
95ac0e1 to
4f8ccab
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48619001 |
larryliu0820
force-pushed
the
export-D48619001
branch
from
4f8ccab to
c985be0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48619001 |
larryliu0820
force-pushed
the
export-D48619001
branch
from
c985be0 to
eb134e9
Compare
Summary: Pull Request resolved: pytorch/executorch#113 This work is to improve memory efficiency on operator/kernel registration. We deprecated `Operator` in favor of `Kernel` quite a while ago in the specialized kernels project. From then on `Operator` contains a fixed number ( 8 ) of kernels and the registry contains a fixed number (250) of `Operator`s. This is a waste of memory in two aspects: 1. We register prim ops unconditionally as `Operators` but they only have 1 kernel. This is a 8 times of waste. 2. We should be able to leverage selective build information on how many kernels are being registered for this build. This number can be passed in by a preprocessing flag. To address this issue, this diff changes the 2-layer structure of Operator and Kernel, only store kernels in the registry. The downside is that we may experience a slight regression on static init time `register_kernels()` call because now it needs to do a linear search and make sure there's no duplicate kernels in the registry. This is O(N^2). Reviewed By: JacobSzwejbka Differential Revision: D48619001 fbshipit-source-id: 42cadc31aa266085519425d079db09dbe181141e
|
This pull request was exported from Phabricator. Differential Revision: D48619001 |
larryliu0820
force-pushed
the
export-D48619001
branch
from
eb134e9 to
223e745
Compare
|
This pull request has been merged in 792388f. |
Gasoonjia
pushed a commit
that referenced
this pull request
Jul 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This work is to improve memory efficiency on operator/kernel registration.
We deprecated
Operatorin favor ofKernelquite a while ago in the specialized kernels project. From then onOperatorcontains a fixed number ( 8 ) of kernels and the registry contains a fixed number (250) ofOperators. This is a waste of memory in two aspects:Operatorsbut they only have 1 kernel. This is a 8 times of waste.To address this issue, this diff changes the 2-layer structure of Operator and Kernel, only store kernels in the registry.
The downside is that we may experience a slight regression on static init time
register_kernels()call because now it needs to do a linear search and make sure there's no duplicate kernels in the registry. This is O(N^2).Reviewed By: JacobSzwejbka
Differential Revision: D48619001