-
Notifications
You must be signed in to change notification settings - Fork 8
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
WIP: automatic bulk handle #287
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #287 +/- ##
=======================================
Coverage 57.65% 57.65%
=======================================
Files 70 70
Lines 10146 10146
Branches 1335 1335
=======================================
Hits 5850 5850
Misses 3462 3462
Partials 834 834 ☔ View full report in Codecov by Sentry. |
This is a little bit more complicated than I expected. I'm trying to think about what things could maybe make it simpler. From the use case description, I think the biggest difference between the conventional pattern and the proposed pattern is on the server side. Could we accomplish something similar without adding a new bulk type, but by instead by just adding two convenience functions on the server, something like I could see that being helpful to hide the transfer and allocation steps. I'm not sure that we need a new type or new push or pull functions for it, though? The underlying functions are available for anyone doing more sophisticated patterns. |
In your scenario, you still need a new bulk type wrapping the local bulk + local buffer, don't you? Then the only difference between my API and yours is that (1) your solution doesn't involve creating an auto-bulk on the client side, and (2) my solution does the transfer when first calling |
The create() can provide both Still under discussion: how (and should we) handle different allocation modes, like being able to use existing memory, or being able to use poolsets, or being able to use a provider-specific allocator? |
Most often when using bulk operations, the code looks as follows:
hg_bulk_t
;hg_bulk_t
to a server;hg_bulk_t
;margo_bulk_transfer
operations between the twohg_bulk_t
.The feature proposed by this PR, called
margo_auto_bulk_t
, is a way to simplify this. With this feature, the above pattern changes into the following:margo_auto_bulk_t
;margo_auto_bulk_t
to a server;margo_auto_bulk_access
to get a pointer to a local buffer (allocated by Margo) mirroring the client's buffer. If the client has specifiedMARGO_PULL_ON_ACCESS
as a flag, the first call tomargo_auto_bulk_access
will pull the content of the client's memory into the server's.margo_auto_bulk_pull
to pull data from the client's memory to the server's local buffer, andmargo_auto_bulk_push
to push data from the server's local buffer to the client's memory;MARGO_PUSH_ON_DESTROY
as a flag,margo_auto_bulk_push
is implicitly called when themargo_auto_bulk_t
is destroyed.In most cases,
margo_auto_bulk_push
andmargo_auto_bulk_pull
are not needed, and transfers will happen automatically onmargo_auto_bulk_access
and/or when callingmargo_free_input
on an RPC input that contains amargo_auto_bulk_t
.Important to note is the fact that the
margo_auto_bulk_t
will optimize the whole thing when client and server live in the same process:margo_auto_bulk_access
will simply provide the server with the address of the buffer provided by the client, rather than do an extra allocation, and all the copies will be bypassed.@carns Before I implement this feature, do you have any comment?