-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[JIT] Load original SourceRanges on import #22180
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
Conversation
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
zdevito
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool -- see inline comments for some suggestions. Need to be careful about SourceRanges continuing to be value types (caching has to go at the Source level if at all). Also, I didn't immediately see it in the code, but what happens if you query a source range where the originating code comes from many different locations? Is it returning the source location at the beginning of the source range?
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
zdevito
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Combined with @ZolotukhinM's inlining remembering stack changes this will enable a full description of the stack. Keep in mind that We are going to end up in a situation where there are two separate traces: the one from the loaded code and the original one. Their depth is not the same and we need to do a good job reporting it all. We should resolve this when we merge the components together.
| out << getImports(); | ||
| int64_t source_offset = out.tellp(); | ||
| out << body_.str(); | ||
| for (const auto& x : body_.ranges()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is small but this repeating the internal details of source range tracking outside of the the TaggedStream object. This will be a nasty and hard-to-find bug if someone changes the representation and doesn't realize the logic is also repeated here.
|
|
||
| private: | ||
| std::shared_ptr<Source> deserialize_source(const c10::IValue& iv) { | ||
| // TODO: cache these? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we need to do this one because otherwise it is copy the entire source string over and over again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key is just the pointer that is being stored inside in the IValue. I think we have a method that accesses it (see the pickler, which does something similar).
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
[JIT] Load original SourceRanges on import gh-metadata: pytorch pytorch 22180 gh/jamesr66a/9/head
|
@jamesr66a merged this pull request in ffa15d2. |
Summary: Pull Request resolved: pytorch#22180 ghimport-source-id: efa46dc Test Plan: Imported from OSS Differential Revision: D15981425 Pulled By: jamesr66a fbshipit-source-id: bef682bd13c1a5be95bdb97e025690c6f2d523d3
Stack from ghstack:
Resolves #21183
Demo:
Differential Revision: D15981425