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

Make pool_memory_resource::pool_size() public #962

Merged
merged 2 commits into from
Jan 28, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Jan 27, 2022

It's useful to know how much memory RMM has "to itself" at any given time. When using a pool_memory_resource, being able to query the current pool size is the only way to know that.

This is to enable the use cases described in #956 (combined with a CallbackMemoryResource, for which I'll open a subsequent PR).

@shwina shwina requested review from a team as code owners January 27, 2022 16:40
@shwina shwina added this to PR-WIP in v22.04 Release via automation Jan 27, 2022
@github-actions github-actions bot added cpp Pertains to C++ code Python Related to RMM Python API labels Jan 27, 2022
@shwina shwina added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function and removed Python Related to RMM Python API cpp Pertains to C++ code labels Jan 27, 2022
@shwina
Copy link
Contributor Author

shwina commented Jan 27, 2022

@gpucibot merge

Comment on lines 304 to 305
return (<pool_memory_resource[device_memory_resource]*>(
self.c_obj.get()))[0].pool_size()
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a lot to cram into one line. Would it make sense to break this up a bit by assigning to variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to clean this up a bit. Unfortunately, unlike C++, I can't create a type alias for pool_memory_resource[device_memory_resource], so this is still a bit awkward looking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Cython doesn't let you ctypedef inside a function)

Copy link
Member

Choose a reason for hiding this comment

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

The change looks much better! Thanks Ashwin 😄

Interesting guessing you tried ctypedef? Did you try DEF? In any event this seems less critical at least from my perspective

@github-actions github-actions bot added cpp Pertains to C++ code Python Related to RMM Python API labels Jan 28, 2022
v22.04 Release automation moved this from PR-WIP to PR-Reviewer approved Jan 28, 2022
@rapids-bot rapids-bot bot merged commit 81d523a into rapidsai:branch-22.04 Jan 28, 2022
v22.04 Release automation moved this from PR-Reviewer approved to Done Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp Pertains to C++ code improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants