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 high water mark coalescing in memory pools #908

Closed

Conversation

msimberg
Copy link
Contributor

@msimberg msimberg commented Sep 6, 2024

Fixes #906.

This adds two methods, getAlignedSize and getAlignedHighwaterMark to QuickPool and DynamicPoolList. The m_current_bytes/m_current_size members have been renamed m_aligned_bytes.

getAlignedHighwaterMark is now used instead of getHighWatermark in the hwm coalescing heuristics.

…oalesced pool size

The non-actual high water mark does not include the overallocated size
when rounding user allocations up to the alignment. This means that the
non-actual high water mark may be too small when repeatedly allocating
and deallocating the same sizes in a pool, leading to unnecessary
reallocation of the backing buffers.
@msimberg msimberg marked this pull request as ready for review September 6, 2024 12:03
@msimberg
Copy link
Contributor Author

msimberg commented Sep 6, 2024

Thinking about this a bit more, this isn't quite right either. There's a missing counter, which is the high water mark of "current bytes". In QuickPool, m_current_bytes is what is used for getCurrentSize which correctly accounts for overallocation. m_current_size from AllocationStrategy does not take into account overallocation. Should this rather introduce a new counter in QuickPool which does the high water mark for m_current_bytes instead of m_current_size?

@davidbeckingsale
Copy link
Member

Thinking about this a bit more, this isn't quite right either. There's a missing counter, which is the high water mark of "current bytes". In QuickPool, m_current_bytes is what is used for getCurrentSize which correctly accounts for overallocation. m_current_size from AllocationStrategy does not take into account overallocation. Should this rather introduce a new counter in QuickPool which does the high water mark for m_current_bytes instead of m_current_size?

I had started looking at this, and I think that's the right approach. Let me know if you would like to tackle this. Thanks!

@msimberg
Copy link
Contributor Author

msimberg commented Sep 6, 2024

I had started looking at this, and I think that's the right approach. Let me know if you would like to tackle this. Thanks!

Good to hear, thanks! If you already have something started and you have specific thoughts on how to do it, by all means go for it. You do know the codebase better than I do after all! I'm happy to continue in this direction if it helps though, I probably just need a bit more time and support, so if you're happy to wait for me to get this into a good state I'll keep at it.

@msimberg
Copy link
Contributor Author

msimberg commented Sep 9, 2024

@davidbeckingsale I've pushed some updates hopefully in the direction that you had in mind.

One open question is if the non-hwm heuristics should use getAlignedSize instead of getActualSize? It's not really a affected by #906 since getActualSize is is definitely big enough for the user-facing allocations, but perhaps it should change for consistency? I'm not sure...

src/umpire/strategy/DynamicSizePool.hpp Outdated Show resolved Hide resolved
src/umpire/strategy/QuickPool.cpp Outdated Show resolved Hide resolved
@davidbeckingsale
Copy link
Member

@davidbeckingsale I've pushed some updates hopefully in the direction that you had in mind.

One open question is if the non-hwm heuristics should use getAlignedSize instead of getActualSize? It's not really a affected by #906 since getActualSize is is definitely big enough for the user-facing allocations, but perhaps it should change for consistency? I'm not sure...

Looks good. I think the non-hwm can stick with actualSize since that is already including the alignment. (actual size is the actual amount of bytes allocated by the pool).

@davidbeckingsale
Copy link
Member

Merged in #917

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.

Pathological reallocation in memory pools
2 participants