-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-40698: [C++] Create registry for Devices to map DeviceType to MemoryManager in C Device Data import #40699
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
jorisvandenbossche
merged 15 commits into
apache:main
from
jorisvandenbossche:device-registry
Mar 27, 2024
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4c1183c
GH-40698: [C++] Create registry for Devices to map DeviceType to Memo…
jorisvandenbossche 9972eed
test with CUDA
jorisvandenbossche e16e24d
some clean-up and docs
jorisvandenbossche 337e65f
use static local variable instead of call_once
jorisvandenbossche f33872d
some more docs + basic test for failure cases
jorisvandenbossche 8c6acab
Apply suggestions from code review
jorisvandenbossche 1c20d68
fixup and move registry tests
jorisvandenbossche 76ee5b8
remove support for CUDA_HOST and CUDA_MANAGED
jorisvandenbossche 0176f58
register CUDA by default, remove public RegisterCUDADevice
jorisvandenbossche 92ece26
address general feedback
jorisvandenbossche 7a9e30d
deprecate arrow::cuda::DefaultMemoryMapper
jorisvandenbossche 1ac0c76
Merge remote-tracking branch 'upstream/main' into device-registry
jorisvandenbossche a5a6f6c
renaming
jorisvandenbossche 6805f02
fix doc param
jorisvandenbossche a73f4a5
lint
jorisvandenbossche 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
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
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.
Sorry, but a couple more suggestions to unify naming:
MemoryMappertoDeviceMemoryMapper?RegisterDeviceMemoryManagertoRegisterDeviceMemoryMapperGetDeviceMemoryManagertoGetDeviceMemoryMapperThere 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.
Good points, that naming is definitely more consistent.
There is however one problem that we already define a
DeviceMemoryMapperfor the keyword type in the actual bridge.h Import methods:arrow/cpp/src/arrow/c/bridge.h
Lines 218 to 219 in 434f872
and we should probably find a distinct name, given that both are slight different (the one takes device_type+device_id and returns a MemoryManager, while the other is a function already tied to a specific device_type and thus only takes a device_id, returning again a MemoryManager)
It's of course a subtle difference that might be difficult to embody in a name. But at least using distinct names seems best.
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.
Perhaps
DeviceIdMapperthen? Not terribly pretty I admit...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 that sounds good for the function type alias, but then I would personally leave the register/get functions as is? I would find
RegisterDeviceIdMappera bit strange with the focus on theid, because you are also registering a device type, it's just that the value you store for the registered type is the DeviceIdMapper ..Anyway, in the end it doesn't matter that much, happy to go with whatever we come up with.
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.
Or
DeviceMapper/RegisterDeviceMapper/GetDeviceMapper? (that's a bit more generic, but keeps the three consistent with each other)