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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] host_memory_resource #272

Merged
merged 28 commits into from
Feb 18, 2020
Merged

[REVIEW] host_memory_resource #272

merged 28 commits into from
Feb 18, 2020

Conversation

jrhemstad
Copy link
Contributor

@jrhemstad jrhemstad commented Feb 11, 2020 •
edited
Loading

Towards partial completion of #260, this PR introduces host_memory_resource to control host memory management.

  • Decide if host_memory_resource should have an alignment parameter for consistency with std::pmr::memory_resource
    • Resolved that it should.
  • Implementation
  • Documentation
  • Add pinned memory resource
  • Add new_delete memory resource
  • Tests
  • Add aligned.hpp for aligned memory allocation/free

@jrhemstad jrhemstad added the 2 - In Progress Currently a work in progress label Feb 11, 2020
@jrhemstad jrhemstad requested a review from harrism February 11, 2020 17:33
@jrhemstad jrhemstad requested a review from a team as a code owner February 11, 2020 18:21
@jrhemstad jrhemstad added 3 - Ready for review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Feb 11, 2020
@jrhemstad jrhemstad changed the title [WIP] host_memory_resource [REVIEW] host_memory_resource Feb 11, 2020
@harrism
Copy link
Member

harrism commented Feb 11, 2020

Discussion of this here: rapidsai/cudf#4020 Suggest @vuule or @OlivierNV review this PR for suitability.

@harrism harrism requested review from vuule and OlivierNV February 11, 2020 20:48
harrism
harrism requested changes Feb 11, 2020
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Looks great, mostly straightforward. Couple of comments.

@raydouglass
Copy link
Member

rerun tests

@jrhemstad jrhemstad removed the 3 - Ready for review Ready for review by team label Feb 11, 2020
@jrhemstad jrhemstad changed the title [REVIEW] host_memory_resource [WIP] host_memory_resource Feb 11, 2020
trevorsm7
trevorsm7 suggested changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@trevorsm7 trevorsm7 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few refactoring suggestions.

vuule
vuule approved these changes Feb 11, 2020
View reviewed changes
Copy link

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Looks good. Suggested minor changes to complement one of @trevorsm7 suggestions.

@jrhemstad
Copy link
Contributor Author

See refactored and improved aligned allocation functionality in aligned.hpp which provides aligned_allocate and aligned_deallocate.

This addresses most review comments as well as fixing the UB from the misaligned load/store.

@jrhemstad jrhemstad added 3 - Ready for review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Feb 12, 2020
@jrhemstad jrhemstad changed the title [WIP] host_memory_resource [REVIEW] host_memory_resource Feb 12, 2020
@jrhemstad
Copy link
Contributor Author

rerun tests

trevorsm7
trevorsm7 reviewed Feb 12, 2020
View reviewed changes
include/rmm/mr/host/new_delete_resource.hpp
return static_cast<void *>(ptr);

// If the requested alignment isn't supported, use default
alignment = (detail::is_supported_alignment(alignment))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be an error or warning when the requested alignment isn't supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes straight from the documentation and requirements of std::pmr::memory_resource:

Allocates storage with a size of at least bytes bytes. The returned storage is aligned to the specified alignment if such alignment is supported, and to alignof(std::max_align_t) otherwise.

This is also documented in the host_memory_resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems strange to fail silently, but I suppose that's fine if documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not failing. It's still allocating memory, but with a different alignment.

@jrhemstad jrhemstad requested a review from harrism February 17, 2020 14:32
harrism
harrism approved these changes Feb 17, 2020
View reviewed changes
@jrhemstad jrhemstad added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for review Ready for review by team labels Feb 18, 2020
@harrism harrism merged commit 1d3ab0b into rapidsai:branch-0.13 Feb 18, 2020
This was referenced Jul 20, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

@trevorsm7 trevorsm7 trevorsm7 approved these changes

@vuule vuule vuule approved these changes

@OlivierNV OlivierNV OlivierNV approved these changes

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jrhemstad @harrism @raydouglass @trevorsm7 @vuule @OlivierNV