Repository navigation
[improvement](be) Remove per-term contention from SNII index build memory accounting - #70007
Conversation
…mory accounting ### What problem does this PR solve? Issue Number: None Related PR: apache#66052, apache#67657 Problem Summary: Stream load into a SNII table stops scaling once flush threads compete. On a log table with ten inverted indexes and mostly unique terms, SNII loaded at 314 MB/s against 854 MB/s for V2, with 3.0 times the CPU and 9.8 times the kernel CPU. Two process-wide hot spots cause it, and both are reached per term. 1. SpimiTermBuffer::report_arena_delta() calls GlobalMemoryLimiter::report() whenever the resident total changes, which is once per new term, and report() takes a process-wide mutex. The limiter keeps only the arena size, which grows one 32 KiB block at a time, so nearly every call repeats the same number. At 30 flush threads that was 476k voluntary context switches per second against 21k for V2, with every futex wait sampled on one address. 2. The Reservations that apache#67657 added to the term encoders change several times per term, and each change updates two process-wide atomics through snii_build_consume_release(). That lambda took 54% of the flush threads' user CPU. The limiter is now told only when the arena size changes. Nothing it stores is lost, and pressure is still re-decided whenever the arena grows. A reporter now holds its changes back from the two process-wide counters until they add up to 1 MiB, and publishes the remainder when it is destroyed. The reporter stays exact, so per-writer budgets and spill thresholds do not change. The process-wide counters can lag each live reporter by less than 1 MiB, so a writer that stays below 1 MiB no longer shows on the SniiIndexBuild tracker, and the forced-spill decision can start late by that lag added up. The index bytes do not change. Results in the same window, 4 rounds each, medians, MB/s for V2 and SNII: - Log data before: 854 and 314, 62.9% slower. - Log data after: 852 and 770, 7.7% slower. - Short logs before: 997 and 389, 61.0% slower. - Short logs after: 1003 and 865, 13.7% slower. ### Release note Improve load throughput of the SNII inverted index format on columns with many unique terms. ### Check List (For Author) - Test: Unit Test, Manual test - 2246 SNII, BKD and inverted index tests pass under ASAN. New tests cover both fixes, and five mutants of the change are each caught by them. - Fix and base binaries were compared in stream loads on one BE. - Forced spills on a real load under a low share, confirmed in the spill directory: eight queries return the same counts as V2 on 11M rows, and the tracker returns to 0. - Behavior changed: Yes. The SniiIndexBuild tracker and its metric lag each writer by less than 1 MiB, and the forced-spill decision can start late by that lag added up. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
TPC-H: Total hot run time: 28810 ms |
TPC-DS: Total hot run time: 151219 ms |
ClickBench: Total hot run time: 24.25 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed apache/doris PR #70007 at head 27adda9f7133b97ed3c397163ae9fab155239d1b against the supplied merge-base diff. The PR reduces per-term contention in SNII build accounting, and the tracker callback's atomic publication, registered/unregistered classification, destructor drain, writer ownership, registry locking, and changed test coverage were checked.
One blocking memory-control issue is reported inline: the new arena-only limiter report can leave a floor-sized eligible arena unflagged when process or other-writer pressure begins while resident vocabulary data grows without allocating another arena block. The local writer cap does not cover a writer below its own cap, so this can miss the process-wide backstop until an unrelated future arena growth.
Critical checkpoint conclusions: data/version/delete-bitmap and FE/BE protocol paths are not involved; no transaction or persistence changes are present; concurrent callback and limiter registry lifetimes were traced and are safe; error propagation and test registration were inspected; the changed tests cover partial-step draining, concurrent callbacks, limiter selection, and spill ownership. I inspected tests and did not execute builds or tests under the static-only review contract. No additional user-provided review focus was supplied.
The review converged after 2 rounds: every assigned reviewer returned NO_NEW_VALUABLE_FINDINGS in round 2, the accepted candidate was independently reverified, and all other risk points were dismissed with evidence.
Review converged after 2 round(s).
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…side one arena block ### What problem does this PR solve? Issue Number: None Related PR: apache#67657 Problem Summary: The first commit of this PR reports to the process-wide limiter only when a buffer's arena grows. A capacity doubling moves the resident total by MiBs in one token without touching the arena: the term slots doubling from 2^17 to 2^18 entries add 3.5 MiB. Before that commit such a token re-decided pressure at once; with the arena gate alone it waited for the next 32 KiB block. The limiter is now also told when the resident total has moved by kBuildBytesPublishStep (1 MiB) since its last decision. That is the step in which the decision's input, the registered build bytes, moves anyway. On 2M tokens of a buffer with 600k new terms this adds 11 decisions to 310, and on 2M new terms 13 to 550; before the first commit every new term was a decision. ### Release note None ### Check List (For Author) - Test: Unit Test - ResidentStepInsideOneArenaBlockReachesTheLimiter finds a term whose resident step lands inside one arena block and checks that it is decided at that term; it fails without the new clause. 2247 SNII, BKD and inverted index tests pass under ASAN. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 28942 ms |
TPC-DS: Total hot run time: 151653 ms |
ClickBench: Total hot run time: 24.31 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Static review of PR #70007 at head 57843ed against base 8ebf20d completed in round 1. No new actionable P0-P3 findings were verified. I reviewed all seven changed files and traced HeldBackBuildBytes through MemoryReporter reservation/report/destructor paths, registered versus unregistered counters, and the GlobalMemoryLimiter/SpimiTermBuffer report, registry, and forced-spill lifecycle. Existing inline P1 comment 4450932617 was independently rechecked: the resident-step trigger catches capacity growth inside an unchanged arena, and fixed 32 KiB arena allocation bounds the delay for new posting growth; idle/no-growth behavior predates this PR, so the existing issue does not still apply. No additional user focus was provided. Static-only validation inspected the authoritative diff, related code, threads, ledger, and exact refs; no builds or tests were executed under the review contract. Changed-file review checkpoints found no new hub include, gate-table, PCH, source-including-test, or comment-condition defect. All assigned subagents returned NO_NEW_VALUABLE_FINDINGS, so convergence was achieved.
Review converged after 1 round(s).
…is empty ### What problem does this PR solve? Issue Number: None Related PR: apache#67657 Problem Summary: The first commit of this PR holds a reporter's changes back from the process-wide counters until they add up to 1 MiB, and publishes the remainder when the reporter is destroyed. A text writer's reporter is not destroyed with the writer: IndexFileWriter keeps it in _snii_memory_reporters, and BetaRowsetWriter keeps the IndexFileWriter of every segment until the rowset writer closes. Each finished segment therefore left up to 1 MiB per text index in g_registered_build_bytes, the input of the forced-spill decision, and in the SniiIndexBuild tracker, for the rest of the load. With many segments the sum can push the decision over its share and force-spill healthy writers, which the merge base never did. A local doris-repo-review run found it. A reporter now also publishes when its live bytes are back at zero. A writer holds its arena for the whole build, so per-term swings never reach zero: this is one publish per finished writer, and a scratch reservation taken and given back leaves nothing to publish. The same commit covers the other points of that review: the per-term swing test now runs for kUnregistered reporters too, the report_arena_delta() declaration describes the limiter half, the resident-step test no longer assumes one decision per token, and the BKD test asserts without slack. ### Release note None ### Check List (For Author) - Test: Unit Test - AnEmptyReporterHoldsNothingBack keeps a reporter alive, takes it back to zero through report() and through a reservation, and checks that nothing is left; it fails without the zero rule. PerTermSwingsStayOffTheSharedCounters now covers both populations and fails when unregistered reporters publish every change. 2248 SNII, BKD and inverted index tests pass under ASAN. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
TPC-H: Total hot run time: 28730 ms |
TPC-DS: Total hot run time: 153906 ms |
ClickBench: Total hot run time: 24.56 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Review converged in round 1 with no new verified findings.
The change meets its stated goal: per-reporter SNII build bytes are batched into process-wide observation counters, flushed when a reporter reaches zero, and used by the global limiter without charging the local reporter twice. I traced the text, BKD, compaction, MemoryReporter, SpimiTermBuffer, GlobalMemoryLimiter, and IndexFileWriter lifecycles. The pending/live atomics preserve the publication invariant at quiescence; reporter destruction and member ordering drain reservations safely; the limiter registry remains mutex-protected and owner-thread spill requests are consumed on the next token. No configuration, wire/protocol, persistence, transaction, or storage-format compatibility change is present, and parallel registered/unregistered populations are handled consistently.
The added unit tests cover threshold batching, zero-drain through report and reservations, destructor cleanup, concurrent callbacks, resident-step limiter refresh, pressure during an unchanged arena, and BKD accounting. Static review inspected the authoritative diff and related call paths; no builds or tests were run under the review contract. No additional user focus was supplied.
Existing P1 inline thread 4450932617 was independently rechecked against this head. The resident-step condition now re-evaluates pressure for active resident-only growth inside an unchanged arena, so that finding is treated as fixed and is not reposted.
Review converged after 1 round(s).
What problem does this PR solve?
Issue Number: None
Related PR: #66052, #67657
Problem Summary: Stream load into a SNII table stops scaling once flush threads compete. On a log table with ten inverted indexes and mostly unique terms, SNII loaded at 314 MB/s against 854 MB/s for V2, with 3.0 times the CPU and 9.8 times the kernel CPU. The adaptive flush pool is on by default and allows up to 4 threads per core, so this is the normal case on a busy BE. With the pool fixed at 6 threads SNII was 23% slower than V2, and at 30 threads 60% slower.
Two process-wide hot spots cause it, and both are reached per term:
SpimiTermBuffer::report_arena_delta()callsGlobalMemoryLimiter::report()whenever the resident total changes, which is once per new term.report()takes a process-wide mutex, looks the writer up and reads the memory signals. The limiter keeps only the arena size, which grows one 32 KiB block at a time, so nearly every call repeats the same number. At 30 threads the flush threads blocked about 476k times per second against 21k for V2, and every futex wait sampled was on one address. That was 13.6 cores of kernel CPU against 3.5.snii_build_consume_release(). That lambda took 54% of the user CPU of the flush threads.Both scale with new terms and with threads, which is why duplicate data and a small pool look mild.
IndexFileWriterkeeps it until the rowset writer closes.What this costs. The reporter itself stays exact, so per-writer budgets and spill thresholds do not change. The process-wide counters can lag each writer that is still building by less than 1 MiB:
SniiIndexBuildandsnii_index_build_mem_consumptionno longer show a writer that stays below 1 MiB while it builds. They are exact again as soon as a writer is empty, so a finished segment leaves nothing behind.inverted_index_ram_buffer_size(2 GiB by default). A writer below 1 MiB could not be a victim anyway, since forced spills only pick arenas abovesnii_forced_spill_min_arena_bytes(64 MiB by default), and the process-level backstops read real memory.The index bytes do not change.
Both binaries were built from master and alternated in one window, 4 rounds each with the order flipped, and the table shows medians. One BE pinned to 32 cores, local disk, stream load of JSON lines with 16 concurrent 100 MB batches. Log data is expanded from a real sample of service logs, where 1% of the rows carry base64 blobs and make up 68% of the bytes. Short logs leave those rows out. V2 is unchanged, as it should be, and kernel CPU falls from 516 s to 43 s on the log data.
An earlier prototype with the same two changes also held up with other shapes and pools: fixed 30 threads -59.6% to -8.7%, fixed 6 threads -23.1% to -14.8%, 1 GB batches -43.8% to -13.7%, and data with few unique terms -20.2% to -11.7%.
Forced spills were checked on a real load: with
snii_index_build_max_memory_limit_percent = 1,snii_forced_spill_min_arena_bytes = 1 MiB, and 16 concurrent 1 GB batches, the spill directory held up to 224 files at once and the limiter logged its shortfall warning. It stayed empty with the default share. Eight MATCH, phrase and equality queries returned the same counts as V2 on the 11M rows, and the tracker went back to 0.Release note
Improve load throughput of the SNII inverted index format on columns with many unique terms.
Check List (For Author)
NewTermsThatKeepTheArenaStayOffTheLimiter: new terms inside one arena block never reach the limiter while the resident total moves;ResidentStepInsideOneArenaBlockReachesTheLimiter: a 1 MiB resident step inside one block is decided at that term;PressureIsDecidedWhenTheArenaNextGrows: pressure that arrives inside a block still ends in a forced spill;PerTermSwingsStayOffTheSharedCounters(both populations),AnEmptyReporterHoldsNothingBack: a retained reporter that is back at zero leaves nothing in the shared counters,HeldBackBytesArePublishedTogetherAtTheStep,DestroyingAReporterPublishesTheLastPartialStep,ConcurrentChangesOnOneReporterLoseNothing: the coalescing, its remainder and concurrent use; existing tracker tests that asserted exact visibility of a few KB now use changes of at least one step; eight mutants of the change each fail tests; 2248 SNII, BKD and inverted index tests pass under ASAN).Manual test: the A/B and the forced-spill load above.
SniiIndexBuildtracker andsnii_index_build_mem_consumptionlag each writer by less than 1 MiB, and the forced-spill decision can start late by that lag added up.🤖 Generated with Claude Code