liaoxin01 commented on PR #66545: URL: https://github.com/apache/doris/pull/66545#issuecomment-5212206666
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: `sorted_rows` (16 B/row).** That one was avoidable and is gone in the latest commit. The permutation is now applied to `_row_in_blocks` in place by following its cycles, using `perm` itself as the "already moved" marker since it is dead afterwards. That is also one move per row instead of two (gather into `sorted_rows`, then `std::move` back), so it is not a trade. **The rest is what it is.** `perm` is 8 B/row because `IColumn::Permutation` is `PaddedPODArray<size_t>`, and the inline permutation is the whole point of the change -- the speedup comes from the key sitting next to the row id. `EqualFlags` is 1 B/row. Note the inline permutation is a `std::vector` local to `ColumnSorter::_sort_by_inline_permutation`, so it is released between key columns and the peak holds one of them, as you said. Peak for the 7.05M-row shape, before and after this commit: | key type | before | after | | --- | --- | --- | | INT32 | 169 MB | 120 MB | | INT64 | 176 MB | 176 MB | | string | 233 MB | 233 MB | | Decimal128 | 289 MB | 289 MB | So it only moves the needle where `sorted_rows` was the peak; for wider keys the inline permutation dominates and the number is unchanged. **Context for the remainder.** `_row_in_blocks` holds one `shared_ptr` (16 B) plus one `make_shared<RowInBlock>` allocation per row -- `sizeof(RowInBlock)` is 40, and libstdc++ puts a 16 B control block in front of it, so 72 B/row. At 7.05M rows that is ~507 MB, resident for the whole life of the memtable rather than transient. Against that, a 120-289 MB transient during `_sort()` is real but it is not the term that decides whether a load fits. It is also accounted: `to_block()` runs under `SCOPED_SWITCH_THREAD_MEM_TRACKER_LIMITER(write_tracker)` + `SCOPED_CONSUME_MEM_TRACKER(memtable->mem_tracker())` in `MemTableFlushExecutor`, so these allocations land on the load tracker and `MemTableMemoryLimiter` sees them. If the peak is still the blocker, the honest fix is not to shave the sort but to drop `_row_in_blocks` in favour of a plain `IColumn::Permutation` -- it exists mostly to carry `_row_pos`, and the agg-state fields are only used on the aggregating paths. That removes ~450 MB of the 507 MB above and makes the sort temporaries look small. I would rather do that as its own change than fold it in here. Happy to hold this PR until that one lands if you prefer that order. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
