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

[REVIEW] New stream-ordered suballocator resources (PTDS support) #449

Merged
merged 49 commits into from
Aug 7, 2020

Conversation

harrism
Copy link
Member

@harrism harrism commented Jul 23, 2020

This PR is bigger than originally planned, but it achieves quite a lot.

  • Factors free lists into a base class defining the free_list abstract interface, and two implementations: fixed_size_free_list (for fixed-size block allocators) and coalescing_free_list (for coalescing pool allocators).
  • Factors stream-ordered suballocation support into a new template abstract base-class stream_ordered_suballocator_memory_resource
  • Reimplements fixed_size_memory_resource and pool_memory_resource in terms of the above classes.
  • Both of these are now thread safe without using a thread_safe_resource_adapter.

TODO:

  • Investigate whether fixed_multisize_memory_resource is thread safe without the adapter.
    Update: fixed_multisize_memory_resource and hybrid_memory_resource allocate/deallocate functions are thread safe as long as their upstream memory resources are thread safe. All currently available upstream resources in RMM are thread-safe after this PR. Therefore there is no longer a current need for thread_safe_resource_adapter.

@harrism harrism added the 3 - Ready for review Ready for review by team label Jul 23, 2020
@harrism harrism self-assigned this Jul 23, 2020
@harrism harrism requested review from jrhemstad and codereport July 23, 2020 07:58
@harrism
Copy link
Member Author

harrism commented Jul 23, 2020

Reviewers: I could use feedback on whether my use of move semantics is correct and consistent (and where I missed it). There are some other new (to me) C++ things in here that I used, like emplacing in a std::unordered_map using std::piecewise_construct and std::forward_as_tuple (In this case I needed to ensure the emplaced object's destructor is not called due to copying)

@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

1 similar comment
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@harrism
Copy link
Member Author

harrism commented Jul 24, 2020

I'm seeing a big slowdown in the random_allocations benchmark. Investigating.

Fixed. But now I'm wondering if using CRTP rather than virtual functions would be faster. It's really too bad profiling CPU code is so hard.

Edit: the more I think about it CRTP makes a lot of sense for these classes (runtime polymorphism isn't really needed), but that should be a followup.

@harrism
Copy link
Member Author

harrism commented Jul 31, 2020

I removed the virtual inheritance in favor of CRTP in the stream_ordered_memory_resource base class.

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

This overall looks great. Love the use of algorithms and const :)

@harrism harrism requested a review from a team as a code owner August 6, 2020 04:39
@harrism harrism mentioned this pull request Aug 6, 2020
11 tasks
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

The shared_ptr is a much nicer solution to the lifetime of the events.

Copy link
Contributor

@codereport codereport 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!

Comment on lines +121 to +124
struct split_block {
void* allocated_pointer; ///< The pointer allocated from a block
block_type remainder; ///< The remainder of the block from which the pointer was allocated
};
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ 👍

@harrism harrism merged commit e0ceca1 into rapidsai:branch-0.15 Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants