fix(columnar): append filter output under one mutation lease per storage epoch - #2073
Merged
Merged
Conversation
…age epoch Since the #2037 mutation-lease series, col_rel_append_row acquires, validates and finishes a full single-relation mutation set for every row, and the filter operator appended its selected rows that way. Under the main sanitizer job (gcc, ASan/UBSan, debug) on 2-vCPU hosted runners, tdd_recursive and session then exceeded their 30 s and 180 s limits. Pinned to two CPUs locally, tdd_recursive went from 8.8 s at a8b5b5d to 26.0 s on main, with user CPU tripling. perf put col_rel_mutation_set_acquire, col_rel_mutation_lease_validate and their helpers above 40 % of the run, reached through wl_columnar_filter_op and fill_filtered_rel. PR #2058, merged in the same push, measured within noise of its parent. Add a held-lease batch appender: col_rel_append_batch_begin acquires the lease, col_rel_append_batch_row appends exactly as col_rel_append_row would, and col_rel_append_batch_end finishes the lease. A lease may publish one storage transition, so the appender starts a fresh lease only before a row that would replace storage again. That condition is col_rel_append_replaces_storage, which the append path now shares. Generation publication, copy-on-write, admission and per-row denial evidence stay in the append path, unchanged. The writer gates are held across the fill, so readers get EBUSY until the batch ends. Every fill in filter.c now appends through a batch: the timestamped simple-predicate path, the untimestamped bulk copy, the compiled slow path and both loops of fill_filtered_rel. Each exit ends the batch before the output is destroyed or published, through one discard helper on the error paths. Filter results, growth policy and error codes are unchanged. An empty selection now takes one lease where it took none. Measured locally, pinned to two CPUs, sanitizer debug build: test a8b5b5d main this change tdd_recursive 8.8 s 26.0 s 10.9 s session 61.6 s 187.0 s 104.7 s This restores the filter operator's append cost. session is still about 1.7x its pre-#2037 time, because col_rel_set per cell (join.c col_join_append_pair, ops.c aggregation) and other per-row col_rel_append_row callers still take a lease each. Tests in relation_mutation_set: - The batch appender against per-row col_rel_append_row: identical columns including FLOAT -0.0, capacities and generation deltas, at most one lease per storage transition, and reader exclusion while active. - The right-side filter and wl_columnar_filter_op on all three fills: at most 16 mutation sets for 4096 rows where per-row appends need at least 3945, the selected rows and timestamps, and a clean destroy. These fail against the previous filter.c. docs/THREADING.md audits the new test-only nonce peek (220 sites).
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Main's
Sanitizers / ubuntu-latest / gccjob times outtdd_recursive(30 s) andsession(180 s) on 2-vCPU GitHub-hosted runners. The cause is the #2037 mutation-lease series, not #2058, which was merged in the same push:tdd_recursive, pinned to 2 CPUs, sanitizer debugUser CPU tripled, so the extra time is compute, not waiting.
perfput the mutation-set acquire, validate and finish functions above 40 %. They are reached from the filter operator, which appended each selected row throughcol_rel_append_rowand so took a full mutation lease per row. CI attributed the break to #2058 because that push's runs alternated between a fast self-hosted runner, where they passed, and a hosted one, where they timed out.This PR adds a held-lease batch appender (
col_rel_append_batch_begin/row/end). It acquires one lease per storage epoch and renews it only before a row that would replace storage again; the append path uses the same predicate. Everyfilter.cfill now appends through it, and every exit ends the batch before the output is destroyed or published. Filter results, growth policy and error codes are unchanged.Measurements
Pinned to 2 CPUs, sanitizer debug build:
tdd_recursivesessionThis restores the filter operator's append cost.
sessionis still about 1.7x its baseline. Per-cellcol_rel_set(join and aggregation) and other per-rowcol_rel_append_rowcallers still take a lease each. That work is tracked in #2072.Tests
relation_mutation_set: the batch appender is compared against per-rowcol_rel_append_rowon columns (including FLOAT-0.0), capacities and generation deltas. It takes at most one lease per storage transition, and readers get EBUSY while it is active.wl_columnar_filter_op, on all three fills: at most 16 mutation sets for 4096 rows, where per-row appends need at least 3945. Each checks the selected rows and timestamps and a clean destroy. These tests fail against main'sfilter.c.meson test: 416 OK, 0 failed. The full suite under the CI sanitizer configuration (gcc,-Db_sanitize=address,undefined, debug): 414 OK, 0 failed.--suite tidy: 4/4.Review
Ran through the implementation harness with an independent Architect, Critic and Reviewer.
7895ed30, which is this commit's tree.Refs #2072