c10 dispatcher stores autograd kernels #23666
Conversation
pytorchbot
added
the
module: internals
Pull Request resolved: #23666 ghstack-source-id: 87825631 Differential Revision: [D16603130](https://our.internmc.facebook.com/intern/diff/D16603130/)
smessmer
changed the title
dzhulgakov
left a comment
dzhulgakov
left a comment
There was a problem hiding this comment.
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.
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.
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.
dzhulgakov
left a comment
dzhulgakov
left a comment
There was a problem hiding this comment.
Thanks for the comments!
|
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