Repository navigation
perf: add monotomic/slab allocator and improve HSA - #779
Conversation
bfe0d45 to
2c27ab7
Compare
0140d69 to
c111938
Compare
4bfd053 to
cbfff28
Compare
beaa72d to
fc8a7b4
Compare
422f475 to
5b69100
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Summary
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
36bc9403-9e6c-41d7-bce4-4c42799f1cee
📒 Files selected for processing (62)
.pre-commit-config.yamlbolt/benchmarks/tpch/TpchBenchmark.cppbolt/benchmarks/unstable/CMakeLists.txtbolt/benchmarks/unstable/SlabAllocatorBenchmark.cppbolt/common/base/BitUtil.hbolt/common/base/tests/BitUtilTest.cppbolt/common/memory/CMakeLists.txtbolt/common/memory/CompactDoubleList.hbolt/common/memory/HashStringAllocator.cppbolt/common/memory/HashStringAllocator.hbolt/common/memory/MallocAllocator.cppbolt/common/memory/MemoryPool.hbolt/common/memory/MemoryResource.cppbolt/common/memory/MemoryResource.hbolt/common/memory/tests/CMakeLists.txtbolt/common/memory/tests/CompactDoubleListTest.cppbolt/common/memory/tests/HashStringAllocatorTest.cppbolt/common/memory/tests/MemoryPoolTest.cppbolt/common/memory/tests/SlabAllocatorTest.cppbolt/core/QueryConfig.hbolt/core/tests/QueryConfigTest.cppbolt/dwio/dwrf/test/WriterFlushTest.cppbolt/dwio/dwrf/writer/IntegerDictionaryEncoder.hbolt/dwio/dwrf/writer/StringDictionaryEncoder.hbolt/exec/ContainerRow2RowSerde.cppbolt/exec/ContainerRowSerde.cppbolt/exec/GroupingSet.cppbolt/exec/HashBuild.cppbolt/exec/HashTable.cppbolt/exec/HashTable.hbolt/exec/RowContainer.cppbolt/exec/RowContainer.hbolt/exec/SortBuffer.cppbolt/exec/Spill.cppbolt/exec/Spill.hbolt/exec/SpillFile.cppbolt/exec/SpillFile.hbolt/exec/Spiller.hbolt/exec/VectorHasher.cppbolt/exec/VectorHasher.hbolt/exec/WindowPartition.cppbolt/exec/benchmarks/CMakeLists.txtbolt/exec/benchmarks/HashAggregationStringAllocationBenchmark.cppbolt/exec/benchmarks/HashJoinStringAllocationBenchmark.cppbolt/exec/benchmarks/HashStringMonotonicAllocatorBenchmark.cppbolt/exec/benchmarks/RowContainerStoreStringBenchmark.cppbolt/exec/benchmarks/RowContainerVarcharOrderByBenchmark.cppbolt/exec/benchmarks/SortStringAllocationBenchmark.cppbolt/exec/benchmarks/StlContainerAllocatorBenchmark.cppbolt/exec/meta/MetaRowSorterApi.hbolt/exec/tests/HashJoinTest.cppbolt/exec/tests/HashTableTest.cppbolt/exec/tests/OrderByTest.cppbolt/exec/tests/RowContainerTest.cppbolt/functions/lib/SubscriptUtil.hbolt/jit/RowContainer/RowContainerCodeGenerator.cppbolt/jit/RowContainer/RowContainerCodeGenerator.hbolt/jit/RowContainer/RowEqVectorsCodeGenerator.cppbolt/jit/tests/RowEqRowIRTest.cppbolt/jit/tests/ThrustJITv2Test.cppbolt/type/tests/StringViewTest.cppbolt/vector/ComplexVector.h
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments: 1
bolt/dwio/dwrf/test/WriterContextTest.cpp: 1
50-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the stale comment, and shrink
WideTableDictionaryMemory.The comment on Line 89 says that per-column resources "would charge about 80 GiB". That figure assumes a 1 MiB first reservation for each resource. The first reservation is now 64 KiB (
kInitialReservationBytes), so 80,000 per-column resources would charge about 5 GiB.The test does not need 80,000 columns to detect the regression. It builds 80,000 column writers and uses up to about 512 MiB of real memory. That makes it slow, and it can fail with OOM on shared CI runners. Assume 8,000 columns and a 128 MiB root pool. Per-column resources would then need about 500 MiB and would still exhaust the pool. Expected real usage drops to about one tenth of the current value. Scale the
GENERALbound to match.♻️ Proposed change
- constexpr uint32_t kNumColumns = 80'000; + constexpr uint32_t kNumColumns = 8'000; constexpr int64_t kMiB = 1 << 20; auto root = memory::memoryManager()->addRootPool( - "wide_table_dictionaries", 512 * kMiB); + "wide_table_dictionaries", 128 * kMiB); ... - // Per-column slab resources would charge about 80 GiB and exhaust the - // 512 MiB pool before reaching this point. Only one reservation's worth + // Per-column slab resources would charge at least 64 KiB each (about + // 500 MiB) and exhaust the 128 MiB pool. Only one reservation's worth // of unused slab space may be charged across the entire writer.Also lower the
384 * kMiBbound on Line 99 to about48 * kMiB.Also applies to: 89-91
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1d9af30b-c80a-48a4-8d20-548024e7aceb
📒 Files selected for processing: 17
bolt/common/base/BitUtil.hbolt/common/memory/MemoryResource.cppbolt/common/memory/MemoryResource.hbolt/common/memory/tests/SlabAllocatorTest.cppbolt/dwio/dwrf/test/TestDictionaryEncodingUtils.cppbolt/dwio/dwrf/test/TestEncodingSelector.cppbolt/dwio/dwrf/test/TestIntegerDictionaryEncoder.cppbolt/dwio/dwrf/test/TestStringDictionaryEncoder.cppbolt/dwio/dwrf/test/WriterContextTest.cppbolt/dwio/dwrf/writer/ColumnWriter.cppbolt/dwio/dwrf/writer/IntegerDictionaryEncoder.hbolt/dwio/dwrf/writer/StringDictionaryEncoder.hbolt/dwio/dwrf/writer/WriterContext.cppbolt/dwio/dwrf/writer/WriterContext.hbolt/exec/Spiller.cppbolt/exec/Spiller.hbolt/vector/tests/VectorTest.cpp
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
ba23cd8 to
3d20ac9
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve the shared-allocator constructor overload. · RowContainer.h:339
bolt/exec/RowContainer.h:339
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the shared-allocator constructor overload.
Existing callers that pass a
std::shared_ptr<HashStringAllocator>as the final argument cannot compile with the newRowContainerParamsignature. Keep a forwarding overload for that argument while adding the new configuration overload. This preserves the stated no-breaking-changes contract.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6c4cc2d9-6665-47af-b4cb-8a7632b9e579
📒 Files selected for processing: 3
bolt/exec/RowContainer.cppbolt/exec/RowContainer.hbolt/exec/tests/RowContainerTest.cpp
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
kexianda
changed the title
HashStringAllocator and memory::StlAllocator are slow for small, frequent allocations. Use `MonotonicMemoryResource`/`MonoAllocator` for append-only variable-width data whose lifetime matches a batch of rows. Use `SlabMemoryResource`/`SlabAllocator` for frequently allocated small objects that require individual deallocation. Microbenchmarks show that SlabAllocator is about 4x faster than HashStringAllocator and about 40x faster than memory::StlAllocator. ``` ============================================================================ [...]s/unstable/SlabAllocatorBenchmark.cpp relative time/iter iters/s ============================================================================ HashStringAllocatorFifoAllocFree256K_8Threads 58.47ns 17.10M AlignedStlAllocatorFifoAllocFree256K_8Threads 10.372% 563.70ns 1.77M SlabAllocatorFifoAllocFree256K_8Threads 384.63% 15.20ns 65.79M ``` In the append-only scenario, the monotonic allocator is about 12x faster than HashStringAllocator. ``` ============================================================================ [...]StringMonotonicAllocatorBenchmark.cpp relative time/iter iters/s ============================================================================ HashStringAllocatorAllocateNoFree 6.22s 160.90m MonotonicMemoryResourceAllocateNoFree 1287.9% 482.60ms 2.07 ``` TPC-DS 100 GB benchmark results on a development machine show a 1.4% performance improvement.
Optimize bitmap queries and free-list mutation on the allocator's hot paths. - Vectorize BitUtil range queries. isAllSet(), findFirstBit(), and findLastBit() scan complete words with AVX2, Arm SVE, or RISC-V Vector instructions when available, while scalar prefix/suffix handling preserves arbitrary bit-range boundaries and prevents out-of-bounds vector loads. Add exhaustive range-query coverage for set and unset bits. - Replace allocator-side linear bitmap scanning with a two-level bitmap. Use freeNonEmpty_ for per-list availability and freeBitmapIndx for non-empty bitmap words, with direct 64-bit register operations for bit updates. Only update the first-level index when a bitmap word transitions between empty and non-empty. - Reduce CompactDoubleList pointer work by caching decoded links in insert() and remove(), returning the post-removal empty state from remove(), and adding a specialized popFront() for allocation. The allocation path avoids decoding the known sentinel predecessor and avoids a separate empty() check. - Reuse the already-known free-list index while allocating, cache Header size and physical-next state before mutating the Header, and avoid redundant size, index, and Header reloads. - Specialize freeRestOfBlock() instead of routing the remainder through the general free() path. Handle only the required forward coalescing, update counters and both bitmap levels directly, cache the final remainder size and successor Header, and write free/trailer metadata without recomputing physical neighbors. - Extend BitUtil, CompactDoubleList, and HashStringAllocator tests for range boundaries, pop-front behavior, empty-state reporting, bitmap consistency, and allocation/free coalescing invariants. Microbenchmark results show a 52.9% increase in throughput.
3 participants
What problem does this PR solve?
HashStringAllocator and memory::StlAllocator are very slow:
add monotomic/slab allocator to accelerate them.
HashStringAllocator is still used in Agg, optimize the implementation.
Use SIMD to optimize common BitUtil.h
Issue Number: close #789
Type of Change
Description
Describe your changes in detail.
For complex logic, explain the "Why" and "How".
Performance Impact
No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).
Positive Impact: I have run benchmarks.
Click to view Benchmark Results
Negative Impact: Explained below (e.g., trade-off for correctness).
Release Note
Please describe the changes in this PR
Release Note:
Checklist (For Author)
Breaking Changes
No
Yes (Description: ...)
Click to view Breaking Changes