Skip to content
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

Avoid unnecessary allocations around buffer storage #115

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Apr 25, 2022

The various get_data and set_data functions on buffer storage objects take and return data in raw_buffer_storage instances. While this is good for memory safety in APIs, it forces unnecessary copies when that data is taken from or written into larger data structures as happens in #94 and #114.

This PR replaces raw_buffer_storage with raw pointers for those APIs. Where ownership is required, the new type-erasing smart pointer type unique_payload_ptr is used which either manages an ad-hoc allocation (like raw_buffer_storage without the metadata) or takes ownership of a unique_frame_ptr, but points to its payload.

@fknorr fknorr self-assigned this Apr 25, 2022
@fknorr fknorr force-pushed the remove-raw-buffer-storage branch 2 times, most recently from a73c773 to 986e449 Compare April 28, 2022 08:37
include/mpi_support.h Outdated Show resolved Hide resolved
@fknorr fknorr force-pushed the remove-raw-buffer-storage branch from 986e449 to 738a789 Compare July 15, 2022 15:20
Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The only thing I find maybe a bit iffy is that we leak the idea of a "payload" into the buffer manager (solely based on the type name), even though it doesn't really need to know about payloads. Not only that, half of the time the thing doesn't actually point to a payload, making it even weirder.

@fknorr fknorr force-pushed the remove-raw-buffer-storage branch from 3e72250 to 061cb6c Compare July 18, 2022 15:30
@fknorr
Copy link
Contributor Author

fknorr commented Jul 18, 2022

As discussed offline, the name unique_payload_ptr is going to stay, but both unique_frame_ptr and unique_payload_ptr now have their own headers to decouple the concept of "owning type-erased blobs" from "networking frames".

@fknorr fknorr force-pushed the remove-raw-buffer-storage branch from 061cb6c to ea2f140 Compare July 18, 2022 15:35
@fknorr fknorr merged commit 9a099d2 into master Jul 18, 2022
@fknorr fknorr deleted the remove-raw-buffer-storage branch July 19, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants