Add Slot type to abstract the raw pointers being used for slots. #18226
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
the
oncall: jit
This was referenced Mar 22, 2019
This was referenced Mar 26, 2019
zdevito
requested review from
Krovatkin and
dzhulgakov
and removed request for
dzhulgakov
Krovatkin
approved these changes
Mar 27, 2019
| return slot_ == rhs.slot_; | ||
| } | ||
| private: | ||
| at::IValue* slot_; |
There was a problem hiding this comment.
an enclosed IValue always outlives its Slot?
There was a problem hiding this comment.
The IValue* lives as long as the module is alive.
| } | ||
| private: | ||
| at::IValue* slot_; | ||
| friend struct std::hash<Slot>; |
There was a problem hiding this comment.
I don't know if return std::hash<at::IValue*>{}(&(*s.slot_)); is any better than declaring a friend.
There was a problem hiding this comment.
: This particular implementation will only live until a later diff in this stack merges Slot and NamedIValue, so I will keep it as is for now.
| IValue* slot() const { | ||
| return ivalue.get(); | ||
| Slot slot() const { | ||
| return Slot(ivalue_.get()); |
There was a problem hiding this comment.
Oh, I see now, the lifetimes of IValue and Slot are the same unless someone creates the Slot outside of a NamedValue. Would it make sense to make Slot c-tor private, so only NamedValues can create Slots?
|
This pull request has been merged in aa20591. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 3, 2019
Summary: Pull Request resolved: #18314 ghimport-source-id: 8cecb76 Stack from [ghstack](https://github.com/ezyang/ghstack): * **#18314 Add ability to specialize class types to ArgumentSpec** * #18226 Add Slot type to abstract the raw pointers being used for slots. Differential Revision: D14574395 fbshipit-source-id: cc3af6e56e9ae52990f4a1ad56ecceaa2d493577
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 3, 2019
Summary: Pull Request resolved: #18378 ghimport-source-id: 55c29bb Stack from [ghstack](https://github.com/ezyang/ghstack): * #18379 Enforce single parent for script submodules * **#18378 Unify namespace of script::Module** * #18314 Add ability to specialize class types to ArgumentSpec * #18226 Add Slot type to abstract the raw pointers being used for slots. This removes individual OrderedDicts in favor of a single unified namespace for all things in a script::Module. This removes a whole class of bugs where both a method and an parameter could get the same name, for instance. Since we no longer have to expose OrderedDict::Item objects, a lot of downstream code can be simplified. We no longer now double-store names (both in the key of the dictionary, and in the object itself). Differential Revision: D14603723 fbshipit-source-id: b5f7551b3074679623edd6ea70269830353b4d4c
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 4, 2019
Summary: Pull Request resolved: #18379 ghimport-source-id: 9895ecc Stack from [ghstack](https://github.com/ezyang/ghstack): * **#18379 Enforce single parent for script submodules** * #18378 Unify namespace of script::Module * #18314 Add ability to specialize class types to ArgumentSpec * #18226 Add Slot type to abstract the raw pointers being used for slots. The assumption that a ScriptModule has a single parent is present in our serialization format, and likely a few other places. It is not enforced on creation of script module hierarchies though, meaning that problems associated with (e.g. replicating a module twice in the output format) will not be caught until much later in the development cycle. This patch enforces the property when a submodule is registered. It also removes NamedModule since it is no longer necessary in this regime. This will also allow the easy discover of a modules fully-qualified name without needing to traverse the Module hierarchy. Differential Revision: D14603722 fbshipit-source-id: 63ab5d0cccf7d66c7833e0adf9023024ca9607cb
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 5, 2019
Summary: Pull Request resolved: #18467 ghimport-source-id: d51bdd6 Stack from [ghstack](https://github.com/ezyang/ghstack): * #18469 Create Object that represents a Module * #18468 slots with explicit value/setValue make more sense in future patches * **#18467 Make Object hold its ClassType** * #18379 Enforce single parent for script submodules * #18378 Unify namespace of script::Module * #18314 Add ability to specialize class types to ArgumentSpec * #18226 Add Slot type to abstract the raw pointers being used for slots. Currently it holds a symbol whose unqualified name is the name of the class. This will get confusing when there are multiple possible registries, and it makes getting the class type from the object difficult. The pointer to the class is only 4 more bytes so this patch just puts it in the object. Reviewed By: suo Differential Revision: D14613510 fbshipit-source-id: b35175ba4be83d2522deaa6dad5070d6ec691fed
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 5, 2019
…18468) Summary: Pull Request resolved: #18468 ghimport-source-id: d4b41c5 Stack from [ghstack](https://github.com/ezyang/ghstack): * #18469 Create Object that represents a Module * **#18468 slots with explicit value/setValue make more sense in future patches** * #18467 Make Object hold its ClassType * #18379 Enforce single parent for script submodules * #18378 Unify namespace of script::Module * #18314 Add ability to specialize class types to ArgumentSpec * #18226 Add Slot type to abstract the raw pointers being used for slots. Reviewed By: suo Differential Revision: D14613509 fbshipit-source-id: 9f2208d0efd01465c78cebdc3e8365a9e0adf9ff
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 6, 2019
Summary: Pull Request resolved: #18469 ghimport-source-id: 73cb8b5 Stack from [ghstack](https://github.com/ezyang/ghstack): * **#18469 Create Object that represents a Module** * #18468 slots with explicit value/setValue make more sense in future patches * #18467 Make Object hold its ClassType * #18379 Enforce single parent for script submodules * #18378 Unify namespace of script::Module * #18314 Add ability to specialize class types to ArgumentSpec * #18226 Add Slot type to abstract the raw pointers being used for slots. This changes the underlying storage for script::Module to hold a ivalue::Object which has slots for all the parameters and attributes. NamedIValue and Slot are now merged together into one class Slot that stores the tuple (ivalue::Object, offset) and can be used to read the name, type, or value of the slot and also to set the value. This cleans up a bunch of client uses. This PR does not actually use the module object in any generated code. A future PR will switch how code is generated to treat modules as first class. Differential Revision: D14613508 fbshipit-source-id: d853a7559f58d244de2ef54a781427fcd1060ed0
zhangguanheng66
pushed a commit
to zhangguanheng66/pytorch
that referenced
this pull request
May 6, 2019
Summary: Pull Request resolved: pytorch#18467 ghimport-source-id: d51bdd6 Stack from [ghstack](https://github.com/ezyang/ghstack): * pytorch#18469 Create Object that represents a Module * pytorch#18468 slots with explicit value/setValue make more sense in future patches * **pytorch#18467 Make Object hold its ClassType** * pytorch#18379 Enforce single parent for script submodules * pytorch#18378 Unify namespace of script::Module * pytorch#18314 Add ability to specialize class types to ArgumentSpec * pytorch#18226 Add Slot type to abstract the raw pointers being used for slots. Currently it holds a symbol whose unqualified name is the name of the class. This will get confusing when there are multiple possible registries, and it makes getting the class type from the object difficult. The pointer to the class is only 4 more bytes so this patch just puts it in the object. Reviewed By: suo Differential Revision: D14613510 fbshipit-source-id: b35175ba4be83d2522deaa6dad5070d6ec691fed
zhangguanheng66
pushed a commit
to zhangguanheng66/pytorch
that referenced
this pull request
May 6, 2019
…ytorch#18468) Summary: Pull Request resolved: pytorch#18468 ghimport-source-id: d4b41c5 Stack from [ghstack](https://github.com/ezyang/ghstack): * pytorch#18469 Create Object that represents a Module * **pytorch#18468 slots with explicit value/setValue make more sense in future patches** * pytorch#18467 Make Object hold its ClassType * pytorch#18379 Enforce single parent for script submodules * pytorch#18378 Unify namespace of script::Module * pytorch#18314 Add ability to specialize class types to ArgumentSpec * pytorch#18226 Add Slot type to abstract the raw pointers being used for slots. Reviewed By: suo Differential Revision: D14613509 fbshipit-source-id: 9f2208d0efd01465c78cebdc3e8365a9e0adf9ff
zhangguanheng66
pushed a commit
to zhangguanheng66/pytorch
that referenced
this pull request
May 6, 2019
Summary: Pull Request resolved: pytorch#18469 ghimport-source-id: 73cb8b5 Stack from [ghstack](https://github.com/ezyang/ghstack): * **pytorch#18469 Create Object that represents a Module** * pytorch#18468 slots with explicit value/setValue make more sense in future patches * pytorch#18467 Make Object hold its ClassType * pytorch#18379 Enforce single parent for script submodules * pytorch#18378 Unify namespace of script::Module * pytorch#18314 Add ability to specialize class types to ArgumentSpec * pytorch#18226 Add Slot type to abstract the raw pointers being used for slots. This changes the underlying storage for script::Module to hold a ivalue::Object which has slots for all the parameters and attributes. NamedIValue and Slot are now merged together into one class Slot that stores the tuple (ivalue::Object, offset) and can be used to read the name, type, or value of the slot and also to set the value. This cleans up a bunch of client uses. This PR does not actually use the module object in any generated code. A future PR will switch how code is generated to treat modules as first class. Differential Revision: D14613508 fbshipit-source-id: d853a7559f58d244de2ef54a781427fcd1060ed0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
If modules become first-class IValues, then the slots will no longer be raw pointers but (IValue, index) pairs. This commit inserts the Slot abstraction so that this change can be made in later patches.
Stack from ghstack:
Differential Revision: D14542022