Track ColumnChunk allocations through the BufferManager #3743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
a017e6b to
27bbaa5
Compare
benjaminwinger
force-pushed
the
track-column-chunks
branch
4 times, most recently
from
77d13d8 to
a72b813
Compare
benjaminwinger
force-pushed
the
track-column-chunks
branch
2 times, most recently
from
ecae6e3 to
1e1bd9d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3743 +/- ##
==========================================
- Coverage 87.32% 87.25% -0.07%
==========================================
Files 1327 1327
Lines 51287 51316 +29
Branches 6884 6886 +2
==========================================
- Hits 44786 44776 -10
- Misses 6329 6368 +39
Partials 172 172 ☔ View full report in Codecov by Sentry. |
ray6080
approved these changes
Aug 22, 2024
benjaminwinger
force-pushed
the
track-column-chunks
branch
9 times, most recently
from
0d55c2d to
4a5cf76
Compare
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
a47e21b to
a6481c0
Compare
This was referenced Sep 4, 2024
Merged
benjaminwinger
force-pushed
the
track-column-chunks
branch
2 times, most recently
from
510fa03 to
bde85c7
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
track-column-chunks
branch
2 times, most recently
from
a36a164 to
ecbfa15
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
ecbfa15 to
f552893
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
f552893 to
b5413de
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
b5413de to
91e2153
Compare
Benchmark ResultMaster commit hash:
|
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
91e2153 to
5a126bd
Compare
benjaminwinger
force-pushed
the
track-column-chunks
branch
from
5a126bd to
cbd8165
Compare
Benchmark ResultMaster commit hash:
|
ted-wq-x
pushed a commit
to ted-wq-x/kuzu
that referenced
this pull request
Nov 14, 2024
(cherry picked from commit bc3cc71)
ted-wq-x
pushed a commit
to ted-wq-x/kuzu
that referenced
this pull request
Nov 14, 2024
(cherry picked from commit bc3cc71)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 (possibly related to #3459).
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.