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

Track ColumnChunk allocations through the BufferManager #3743

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benjaminwinger
Copy link
Collaborator

@benjaminwinger benjaminwinger commented Jul 3, 2024

Implements #3660 (also see #3698)

I've merged the MemoryAllocator and MemoryManager; it wasn't obvious the purpose of the distinction (though it's occurred to me that we could have separate allocators for stuff using malloc like the ColumnChunks and our own allocator). Mainly I just wanted to avoid having to store separate references to the MemoryManager everywhere. The MemoryManager seemed better suited than the memory allocator to pass around to initialize column chunk buffers, however there wasn't an way of getting a MemoryManager from an existing buffer (just a MemoryAllocator). While I could have changed that, merging MemoryManager and MemoryAllocator seemed simpler.

Note that it was necessary to raise the buffer pool size used for the tests; that should be able to be lowered again once spilling to disk is implemented as part of testing that behaviour.

Issues

I was having some issues with hanging instead of throwing exceptions when doing large rel copies with a restricted buffer pool size (looking at the behaviour when there is not sufficient memory).
As far as I can tell, sometimes, with a large buffer pool of ~2000-3000 MB the hanging seems to be it continuously swapping out pages in the hash index. It may be that it will eventually throw an exception, but there sometimes is enough free memory for it to almost continuously try to do a small amount of work.

What's more concerning is that restricting the buffer pool size to the minimum 64MB on the same tests it will hang, and running it with backtraces will show a backtrace from a buffer manager exception (when looking up in the hash index; a couple of times it was the assert in freeUsedMemory), but that exception isn't stopping execution and it continues running with high CPU usage.

I suspect that spilling to disk (#3698) will more or less solve these issues, as we generally shouldn't need to operate in such a memory restricted environment, and with spilling to disk ideally won't need to actually fail unless both disk and memory are full, but I'm concerned that we don't seem to be failing reliably in those circumstances.

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.

None yet

1 participant