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.
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
Replace GEMM backend: cublas.gemm -> cublaslt.matmul #1736
Replace GEMM backend: cublas.gemm -> cublaslt.matmul #1736
Changes from 8 commits
2cc477b
dc7a9a4
34a9479
71c03c0
a2fb088
699de0c
f994f19
f4d634a
2d1bf5c
e57eebf
d44bf20
facf81d
157d8ae
be68b61
f5ac41a
2d4dcb2
6f58669
a0e93fd
4c0d742
01c3634
abb3f00
e24b1c0
de29580
3835ed0
de60202
fe84fae
f47626a
d7efc0c
dd7ee22
01e62b0
8fdf6cc
324f5c6
ba6883f
a56ea2c
cd4663a
c2f1daa
c976de0
a5de437
7849786
9bec3cf
ceb8d10
1f39534
b2e3b8b
05c64fc
fdbe003
9e08c0f
97f1d49
6164e4f
f6ded84
88ecbb0
ca11f9f
47303b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
While I was thinking about overheads of copying the resources handle in the gemm/matmul, it occurred to me that we hadn't discussed what should happen to user resources when the resources handle is copied. I see three sensible options:
@cjnolet, do you have a preference here?
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.
@achirkin maybe we could provide a flag for this? Perhaps even have a predefined element in the map of user resources containing this flag? This way they user could set/change the flag using raft::resource::set_user_resource_copy_mode()` or something? We can default it to the least overhead option.
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.
@cjnolet do you mean a runtime flag? I've been thinking about that. At first glance, it seems nice to have this flexibility. However, I think, this has a potential to do more harm than good.
From what I've seen so far, most of the "users" of the user_resource are within raft itself (e.g. caching in the algorithms). When we add these use cases, we keep in mind particular behavior of the resource (e.g. cached kernel parameters are shared across the copied handles or kept private). I can imagine this behavior affecting the performance of the algorithms. If an external user is able to change this behavior at runtime (because they want a particular behavior in their piece of code) it will have an adverse effect on overall performance of the other parts of raft codebase.
Therefore, I think it's reasonable to decide on this behavior once and for all and design the uses cases with the chosen behavior in mind.
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.
Your comment here makes me struggle with the name a little bit, because I was absolutely thinking we would want to invite users to use a "user resource". One of the reasons we didn't use an unordered map for the other resources is because lookup has shown to be slow, especially when the map is small.
Rather than a "user resource", this seems to be more of a "runtime shared resource cache". I'd prefer to name it appropriately based on its use and I think that name is generalized enough to tell a potential user (internal or external) what it's for. Docs can help describe it even better for anyone who might have further confusion on the name.
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 think it's ok to invite users to use this resource, but you're right the name implies it's made specifically for users, which is wrong. How about "custom_resource"?
To me, the only difference of the custom_resource to the other resources is that raft can't allocate a slot for it at compile time. I've tried to reflect it in the updated name and docs. How does it look to you now?
Also regarding the resource copy policy: I suggest we stick to the status quo now (option 1) and revise it when we decide something on
raft::resources
and thread-safety issue.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.
Would be nice to reuse the resources' lock here rather than having two different locks for getting the resource and loading the store from the resource. This could be done e.g. by passing an optional lambda to the
resource::get_resource
to apply post-processing while holding the lock.I haven't added this to not make the PR overly invasive.