-
Notifications
You must be signed in to change notification settings - Fork 25
Tighten data_batch
#120
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
Open
dhruv9vats
wants to merge
9
commits into
NVIDIA:main
Choose a base branch
from
dhruv9vats:synchronized-data-batch
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Tighten data_batch
#120
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
eccef60
introduce synchronized_data_batch
dhruv9vats 0e9ed73
Add state access methods to synchronized_data_batch
dhruv9vats 5eadd0c
rename data_batch to data_batch_core and synchronized_data_batch to d…
dhruv9vats 3524f5d
modify add_data_batch_impl to not have to handle unique_ptr data_batch
dhruv9vats bffa10d
Migrate data batch tests to synchronized API
dhruv9vats d14fec6
Merge main
dhruv9vats 1566c28
move clone and clone_to to data_batch_core
dhruv9vats b6370b6
dont make the get_data and get_memory_space return const ptrs yet
dhruv9vats e6db4f8
Merge main
dhruv9vats 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
Oops, something went wrong.
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.
Why do you have
mut->convert_toinstead ofmut.convert_to? its not a pointer or an optional.Similarly you have several places where you also have
ro->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.
Great observation! The reason for that is, these RAII handles now delegate to the data_batch_core via the overloading of operators, like for read only:
and not via forwarding member functions. So there's only a single definition of a function, and the end user just uses that, just via that handles. And these RAII handles don't have any public member variables and will have minimal member functions.
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 see.... I find that confusing and a leaky abstraction. I feel like data_batch_core should be an implementation detail designed to make it easier to maintain data_batch and read_only_data_batch and mutable_data_batch, but I think it should be a private detail. Using it this way exposes this internal implementation detail
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.
That is the idea with this, so there's only a single source of truth for what can be done with/on a data_batch -- the public members exposed via the data_batch_core. Its just that the only way to get to them would via the RAII handle. Because in this the the data_batch_core becomes a POD, and only holds onto the core underlying and is not concerned about thread safety or concurrent access, which, IMHO, makes is easier to understand what the data_batch_core can actually do.
If we make it private, we are again at wrapping/duplicating the underlying data_batch_core methods. The RAII handles basically act as stencils, providing guided access without being concerned with about what all methods are const vs mutable, simply passing it on. I am thinking of this as constrained access to the underlying object, as opposed to a leaky-abstraction, because, the underlying data is what we want to ultimately access/modify.