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

Fix get_pool_memory_resource return type #1483

Closed

Conversation

tfeher
Copy link
Contributor

@tfeher tfeher commented May 2, 2023

The return type of get_pool_memory_resource was changed in #1469 from pool_memory_resource to device_memory_resource. There are debug logs in the code (example), which query the pool size, that would fail when debug logging is enabled. This PR restores the output type of get_pool_memory_resource.

@tfeher tfeher requested a review from a team as a code owner May 2, 2023 10:41
@github-actions github-actions bot added the cpp label May 2, 2023
@tfeher tfeher requested a review from ahendriksen May 2, 2023 10:41
@tfeher tfeher self-assigned this May 2, 2023
@tfeher tfeher added bug Something isn't working breaking Breaking change non-breaking Non-breaking change and removed breaking Breaking change labels May 2, 2023
@tfeher
Copy link
Contributor Author

tfeher commented May 2, 2023

@ahendriksen was it just an oversight to change the return type, or is there a reason (compile time) for such change? In the latter case we could just remove the printouts (longer term we consider removing such pools anyway).

@ahendriksen
Copy link
Contributor

The return type of get_pool_memory_resource was changed specifically to avoid the inclusion of the pool_memory_resource.hpp header, which is one of the few RMM headers which unconditionally includes spdlog. Including spdlog slows down compile times considerably.

The memory_pool.hpp file is included from cudart_utils.cuh, so this PR in current state would undo all compile time gains from splitting out memory_pool-inl.hpp.

As an alternative, would it be possible to dynamically cast to pool_memory_resource in those places where the debug logging takes place?

@tfeher
Copy link
Contributor Author

tfeher commented May 2, 2023

Thanks @ahendriksen for the explanation. I am closing this in favor of #1484.

@tfeher tfeher closed this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cpp non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants