Unify invoking JIT functions #57851
Closed
Unify invoking JIT functions #57851
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
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
lw
requested review from
H-Huang,
mingzhe09088,
mrshenli,
pritamdamania87,
rohan-varma,
wayi1 and
zhaojuanmao
as code owners
facebook-github-bot
added
oncall: distributed
💊 CI failures summary and remediationsAs of commit 9baf610 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
| Job | Step | Action |
|---|---|---|
 pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build |
Build | 🔁 rerun |
This comment was automatically generated by Dr. CI (expand for details).
Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group.
This was referenced May 7, 2021
Closed
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
This was referenced May 17, 2021
mrshenli
approved these changes
May 18, 2021
| bool hasOp() const; | ||
| std::shared_ptr<Operator> op() const; | ||
| bool hasQualifiedName() const; | ||
| // FIXME It's weird to return a const _value_, should this return a reference? |
There was a problem hiding this comment.
Yep, returning const reference look better to me too.
|
|
||
| // Helpers to run user-defined functions, operators and other computations. | ||
|
|
||
| c10::intrusive_ptr<JitFuture> runJitFunction( |
There was a problem hiding this comment.
similar as the comment in the previous PR, this does not need to be a member function?
There was a problem hiding this comment.
It will be used by the subclass later in the stack
There was a problem hiding this comment.
Oh I see, we are adding a new subclass of RequestCallbackImpl? IIUC, it does not have a subclass today?
There was a problem hiding this comment.
Oops, no, got confused with the previous PR. This method in fact will just be used in here, so it could perhaps be a simple function, though I think it might use some other methods in turn, and also making a method is more "consistent" with runJitOperator. Does this make sense?
| .jitCompilationUnit() | ||
| ->get_function(scriptRemoteCall.qualifiedName()) | ||
| .runAsync(stack); | ||
| if (jitFuture->completed()) { // short-cut. |
There was a problem hiding this comment.
we are skipping fast passes, let's measure the impact.
There was a problem hiding this comment.
What benchmarks would you like me to run?
Also, I don't remember if I said it clearly before, so let me explain why I think it's better to avoid these fastpaths/shortcuts: they are terribly dangerous for CUDA sync, because at no point we're synchronizing the events contained in the future with the current streams at the callsite. (It can be done, one just has to call future->wait(), but we didn't do it anywhere, and it's super easy to forget about).
Moreover, I believe they hurt readability, because they force us to lay out the code in a different order than how it's executed. Namely, now we need to do something like this:
continuation = []() { bar(); };
auto fut = foo();
if (fut->completed()) {
continuation();
} else {
fut->addCallback(continuation);
}
Note how in the code bar() appears before foo(), whereas in the "logical flow" of the program bar() comes after foo(). This requires the reader to "jump around" and makes it harder to quickly understand what the code is doing. Without shortcuts, the above code becomes:
auto fut = foo();
fut->addCallback([]() { bar(); });
As for the supposed performance benefit of these fastpaths, I honestly don't think that we're introducing a regression. The addCallback method itself already has such a shortcut logic internally, so in fact these fastpaths were redundant. Moreover, after the changes in #57638, invoking addCallback should come with literally zero overhead, and the compiler will most likely inline it all the time. (I said addCallback but I also mean then since they're basically the same).
There was a problem hiding this comment.
IIRC, @jjlilley added this a while ago when optimizing PyPer perf. So, I assume there was a reason to introduce fast path at least at that time.
Moreover, after the changes in #57638, invoking addCallback should come with literally zero overhead, and the compiler will most likely inline it all the time.
If we are confident there is no additional overhead introduce, this looks good to me.
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
This was referenced May 20, 2021
The same as the previous PR, but for JIT functions. Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/) [ghstack-poisoned]
dgl-intel
pushed a commit
to dgl-intel/pytorch
that referenced
this pull request
May 21, 2021
Pull Request resolved: pytorch#57851 The same as the previous PR, but for JIT functions. ghstack-source-id: 129567069 Differential Revision: [D28253841](https://our.internmc.facebook.com/intern/diff/D28253841/)
|
This pull request has been merged in bfdc279. |
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.
Stack from ghstack:
The same as the previous PR, but for JIT functions.
Differential Revision: D28253841