-
Notifications
You must be signed in to change notification settings - Fork 26.3k
c10 dispatcher stores autograd kernels #23666
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
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Pull Request resolved: #23666 ghstack-source-id: 87825631 Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
dzhulgakov
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.
It seems a bit over-engineering for the purpose and it makes code much less understandable.
| // before older registrations and the list head is the autograd kernel | ||
| // which is currently used. | ||
| std::list<void*> unboxedAutogradKernels_; | ||
| std::mutex unboxedAutogradKernelsMutex_; |
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.
again, I think it's fine to assume that library loads don't happen concurrently. Even if they did - having a top-level mutex is a much easier solution that also doesn't blow up OperatorEntry size.
btw, since you already have kernelsMutex_ above - why not just use that (even though we don't really need mutex)
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 mutex costs us even less, the complexity added by just the mutex it is negligible, but then we get parallel and race-free library loading for free, don't see a reason for not doing it.
The default pattern for mutexes is to bind them directly to the data they protect and trying to keep these data units small. OperatorEntry size doesn't matter, this is not an entry in the dispatch table.
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
dzhulgakov
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.
Thanks for the comments!
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
|
This pull request has been merged in 211bafc. |
Summary: Pull Request resolved: pytorch/pytorch#23666 ghstack-source-id: 88050430 Differential Revision: D16603130 fbshipit-source-id: bc77c218a4664ad3b57d6918043c93c9df3b42ca
This reverts commit 211bafc.
Stack from ghstack:
Differential Revision: D16603130