-
Notifications
You must be signed in to change notification settings - Fork 56
Add Module Setup and Teardown Callback to Linkable Code Interface #145
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
Merged
Merged
Changes from all commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
2827d0d
initial
isVoid 7806707
mocking object type using cuda.core objects
isVoid bc10ce3
Update to ctypes module wrapper and type checks
isVoid 54bc46c
add managed module object
isVoid 067ea82
Merge branch 'fea-link-callback' of https://github.com/isVoid/numba-c…
isVoid 51dd293
add two kernels test
isVoid d05f76b
add kernel finalizer
isVoid b329831
removing kernel finalizers
isVoid c6a9b73
add docstring
isVoid 881648c
add doc
isVoid 6f7fe2a
Update docs/source/user/cuda_ffi.rst
isVoid 856536e
Merge remote-tracking branch 'NVIDIA/main' into fea-link-callback
gmarkall d4cc4ac
add a test that involves two streams
isVoid af2d618
add wipe all module call
isVoid a5a3671
use context reset is a better option to unload modules
isVoid 577c8ff
add test for stream completeness
isVoid d8f2f23
move logic into CtypesModule
isVoid bb13580
update docstrings
isVoid 379a69b
consolidate changes into create_module_image
isVoid c5d21fe
explicitly delete kernel reference
isVoid 36ad115
remove stream from setup and teardown callbacks
isVoid 2cc4c28
remove counter
isVoid 01d2d85
add API coverage test
isVoid 24e4260
asserting types of passed in module handle
isVoid 328c430
update documentation
isVoid 43bcfa2
address review comments
isVoid 7663093
update linkable code doc
isVoid 97367ce
update the tests to acommodate nvidia bindings
isVoid 1535233
setup should raise an error if module is already initialized
isVoid 79dd76e
Update docs/source/user/cuda_ffi.rst
isVoid b0ff099
add input type guards for linkable code
isVoid 497f0eb
Fix docstrings
gmarkall df7427e
add lock to protect initialization secton
isVoid 6aa1120
add documentation
isVoid 40f4e9b
add multithreaded callback behavior test
isVoid 0ba5dee
Merge branch 'fea-link-callback' of github.com:isVoid/numba-cuda into…
isVoid c24e0b2
Replace flake8 with ruff and pre-commit-hooks
ZzEeKkAa 075b7bd
Apply precommit
isVoid 4b3650a
Merge remote-tracking branch 'origin' into fea-link-callback
isVoid 28d9dc8
apply compile lock to make sure modules are not compiled more than on…
isVoid 6781263
Merge branch 'main' of https://github.com/NVIDIA/numba-cuda into fea-…
isVoid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| from threading import Lock | ||
| from functools import wraps | ||
|
|
||
| # Thread safety guard for module initialization. | ||
| _module_init_lock = Lock() | ||
|
|
||
|
|
||
| def module_init_lock(func): | ||
| """Decorator to make sure initialization is invoked once for all threads.""" | ||
|
|
||
| @wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| with _module_init_lock: | ||
| return func(*args, **kwargs) | ||
|
|
||
| return wrapper |
Oops, something went wrong.
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.
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.
Is there any risk where at destruction / unloading not acquiring this lock that we end up in a bad situation?
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'm crafting a concurrent test so that we can get a more in depth view of the behavior of setup and teardown with threads. I'll report back.
Uh oh!
There was an error while loading. Please reload this page.
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.
When I delved into the multi-threading kernel launches, this is what I discovered:
cumoduleLoadDataExon their own copy of the pointer. SoNmodules will be created if there areNthreads.What this means for the setup callback function:
What this means for the teardown callback function (Today):
What this means for the teardown callback function (If we implement #171)
[EDITED]
del kernelonly reduces the count where each thread increments. The main thread still holds the initial reference to the kernel. In this case kernel is still finalized at interpreter shut down and the above still holds.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.
Does calling
cumoduleLoadDataExconsume valuable resources like global device memory? I would assume so.Do we have one CUDA Context being used by multiple threads?
If the answer is yes to both of these questions, maybe we should implement a lock where the first thread acquires the lock and creates the module and then all future threads can retrieve the module from a cache? Then we presumably only need to call the setup callback function once instead of N times?
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.
Yes and yes, I think Graham and I touched these questions before and we agreed there were some subtle bug with module creation in Numba. Your suggestion makes sense to me.
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 thought this sounded odd, and it's definitely not what we want. There should be one module per context, not per thread - I noted why in the main PR comments: #145 (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.
I'm happy to address the issue in this PR.