[improvement](load) reduce MemTable per-row memory overhead - #66545
[improvement](load) reduce MemTable per-row memory overhead#66545liaoxin01 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
Pull request overview
This PR optimizes BE load-path memtable sorting by replacing MemTable’s per-comparison virtual/indirect multi-key sort with the vectorized ColumnSorter used by the query engine, and updates unit tests to validate the new ordering and aggregation behavior.
Changes:
- Implement
_sort_permutation_by_key_columns()and useColumnSorter-based permutation sorting in_sort()and_sort_by_cluster_keys(). - Remove now-unused
Tieand_sort_one_column()helpers. - Rewrite memtable sort unit tests to exercise real
MemTable::insert()/to_block()sorting and key semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| be/src/load/memtable/memtable.cpp | Replaces memtable sorting with ColumnSorter permutation sorting and updates cluster-key sort accordingly. |
| be/src/load/memtable/memtable.h | Removes unused Tie and declares the new permutation-sorting helper. |
| be/test/load/memtable/memtable_sort_test.cpp | Reworks tests to validate multi-key ordering, NULL handling, batching independence, and UNIQUE/AGG behaviors via real MemTable flow. |
Suppressed comments (2)
be/test/load/memtable/memtable_sort_test.cpp:150
- If
mem_table.insert()fails, the test should stop immediately; otherwise later steps may operate on a partially-initialized MemTable and produce confusing follow-up failures. PreferASSERT_TRUEhere.
Status st = mem_table.insert(&block, payload);
EXPECT_TRUE(st.ok()) << st;
be/test/load/memtable/memtable_sort_test.cpp:156
- If
to_block()fails (or doesn't setout), the helper should abort; usingEXPECT_TRUEcan return a nulloutand crash the caller. UseASSERT_TRUEand assertoutis non-null.
std::unique_ptr<Block> out;
Status st = mem_table.to_block(&out);
EXPECT_TRUE(st.ok()) << st;
return out;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
225cc5e to
103c5b5
Compare
|
run buildall |
103c5b5 to
5ccb379
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 28783 ms |
TPC-DS: Total hot run time: 166522 ms |
ClickBench: Total hot run time: 23.82 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
sollhui
left a comment
There was a problem hiding this comment.
This change may significantly increase the peak memory usage of MemTable sorting, especially for narrow tables with many rows.
The old implementation sorted _row_in_blocks in place and its main row-count-dependent temporary allocation was Tie, which uses roughly 1 byte per row.
The new implementation introduces several additional O(N) allocations:
-
perm: N * sizeof(size_t), about 8 bytes/row;
-
EqualFlags: about 1 byte/row;
-
ColumnSorter’s inline permutation: approximately 8 bytes/row for INT32, 16 bytes/row for INT64, 24 bytes/row for strings, and potentially 32 bytes/row for Decimal128 due to alignment;
-
sorted_rows: another N * sizeof(shared_ptr), normally 16 bytes/row, while the original _row_in_blocks storage is still alive.
For the 7.05M-row benchmark mentioned in the PR, the old sort-specific temporary allocation was only about 7 MB. The new peak is approximately:
-
at least 169 MB during the perm + sorted_rows phase;
-
around 176 MB for an INT64 key during ColumnSorter;
-
around 233 MB for a string key;
-
potentially around 289 MB for a Decimal128 key.
The inline permutation is released between key columns, so these numbers do not multiply by the number of keys, but the peak is still much larger than before. Concurrent MemTable flushes could amplify this and potentially hit the load/process memory limit.
5ccb379 to
3568ae0
Compare
|
Thanks, the arithmetic is right and I reproduced every term. Two things: one of them I removed, and I want to put the rest next to what the memtable already spends. Removed: The rest is what it is. Peak for the 7.05M-row shape, before and after this commit:
So it only moves the needle where Context for the remainder. It is also accounted: If the peak is still the blocker, the honest fix is not to shave the sort but to drop |
|
run buildall |
TPC-H: Total hot run time: 28683 ms |
TPC-DS: Total hot run time: 158329 ms |
ClickBench: Total hot run time: 23.68 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3568ae0 to
6983af1
Compare
|
run buildall |
TPC-H: Total hot run time: 29165 ms |
MemTable keeps one RowInBlock per loaded row in _row_in_blocks, held as a
shared_ptr, so there is one make_shared per row. Measured at 80 bytes per
row: 16 for the pointer in the vector, plus a 64 byte heap chunk holding
the 16 byte control block and the 40 byte struct.
Two of the struct's five fields do not need to be there.
_agg_state_offset held _offsets_of_aggregate_states.data(), the same
pointer for every row, so it belongs on the MemTable. _has_init_agg said
exactly what _agg_mem being non-null already says. That leaves 24 bytes,
small enough to keep in the vector directly and drop the per-row
allocation with it.
For a 7.05M row memtable that is 564 MB down to 169 MB, and building the
array drops from 401 ms to 111 ms.
Dropping the shared_ptr means the rows in _row_in_blocks and the copies
_aggregate() works on are no longer the same object. Two places relied on
that aliasing:
- prev_row now points into temp_row_in_blocks, which is where
_finalize_one_row() will read it from.
- _aggregate() adopts temp_row_in_blocks unconditionally, so the
entries left behind cannot name an aggregate state that
_finalize_one_row() has already released. Without this, a memtable
that aggregates across a shrink_memtable_by_agg() round and then
again in to_block() destroys those states twice -- confirmed by
instrumenting ~MemTable.
memtable_sort_test.cpp only covered class Tie. It now also drives a
MemTable through insert()/to_block() and covers multi-column and nullable
key ordering, the DUP_KEYS tie-break direction, batching independence,
UNIQUE_KEYS last-writer-wins, AGG_KEYS aggregation, and aggregate state
surviving shrink_memtable_by_agg() rounds. The AGG_KEYS schema carries a
BITMAP BITMAP_UNION column so those last cases run over an aggregate
state that owns heap memory rather than a trivially destroyed one.
6983af1 to
f73e312
Compare
|
Splitting this in two, since the memory question and the sort question are independent and the memory one deserves to be judged on its own. This PR is now only the row diet: The Two things in here worth your eye, both consequences of dropping the
|
|
run buildall |
TPC-DS: Total hot run time: 158484 ms |
ClickBench: Total hot run time: 23.8 s |
TPC-H: Total hot run time: 29158 ms |
TPC-DS: Total hot run time: 158117 ms |
ClickBench: Total hot run time: 24.01 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Problem Summary:
MemTablekeeps oneRowInBlockper loaded row in_row_in_blocks, held as ashared_ptr, so there is onemake_sharedper row. That costs 80 bytes perrow, measured: 16 for the pointer in the vector, plus a 64 byte heap chunk
holding the 16 byte control block and the 40 byte struct. A 7.05M row memtable
spends 564 MB on row bookkeeping alone, resident for its whole life.
Two of the struct's five fields do not need to be there:
size_t* _agg_state_offset_offsets_of_aggregate_states.data(), the same pointer for every rowbool _has_init_agg_agg_membeing non-null already saysThat leaves 24 bytes, small enough to keep in the vector by value and drop the
per-row allocation with it. The remaining fields are unchanged, and so is
everything the memtable does with them.
_row_in_blocksBoth are measured rather than derived:
sizeof(RowInBlock)plus the residentsize of 5M rows for the first, a build loop of 7.05M rows for the second.
What the shared_ptr was quietly providing
Rows in
_row_in_blocksand the copies_aggregate()works on used to be thesame object. They are not any more, and two places depended on that:
prev_rownow points intotemp_row_in_blocks. Aggregation mutates thegroup representative, and the copy
_finalize_one_row()later reads is theone in
temp_row_in_blocks, so that is the one it has to point at._aggregate()adoptstemp_row_in_blocksunconditionally rather thanonly on the non-final path, so the entries left behind cannot still name an
aggregate state that
_finalize_one_row()has released. Without this, amemtable that aggregates across a
shrink_memtable_by_agg()round and thenagain in
to_block()releases those states twice. That is not hypothetical:instrumenting
~MemTableshowed it destroying two states that_finalize_one_row<true>()had already destroyed.Related
#66588 replaces the memtable sort with the vectorized
ColumnSorterand isstacked on this one. Its scratch grows with the key width, so it wants the rows
themselves to be cheap first; the ordering and the resulting peaks per key type
are spelled out there. Neither change needs the other to be correct.
Release note
None
Check List (For Author)
be/test/load/memtable/memtable_sort_test.cpponly coveredclass Tie. Itstill does, and now also drives a
MemTablethroughinsert()/to_block(),covering multi-column key ordering, nullable key ordering (NULL first), the
DUP_KEYStie-break direction, independence from how rows are split acrossinsert()calls,UNIQUE_KEYSlast-writer-wins,AGG_KEYSaggregation, andaggregate state surviving
shrink_memtable_by_agg()rounds. TheAGG_KEYSschema carries a
BITMAP BITMAP_UNIONcolumn so those last cases run over anaggregate state that owns heap memory rather than a trivially destroyed one.
Two gaps worth stating plainly:
Those cases reach the double-release path but do not fail on it in a
release build, where a double free is not reliably fatal. ASAN is what turns
it into a failure.
The flexible-partial-update paths are the largest part of this diff and have
no unit coverage, before or after. I checked them line by line against the
previous semantics, but that is a reading, not a test, and P0 is what has to
cover them.
Behavior changed:
Does this need documentation?