perf(disttae): eliminate exact catalog lookup probe allocations - #27546
Conversation
|
Addressed the review findings on head
The PR remains Draft for now because the new-head MatrixOne ALL CI is pending and there is no approval/review decision yet. No SQL/multi-CN QA result is being inferred from the local consumer smoke tests. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review completed on exact head a8efb132191943b7b1a40a00f023dd9e276ad7ae.
The production change preserves the existing exact-BTree comparator, pivot fields, callback checks, and consumer semantics. The pooled pivot is not returned until synchronous Ascend completes, is fully overwritten on acquisition and zeroed before reuse; panic drops the active probe to GC, while sync.Pool GC eviction only restores the cold allocation path. CatalogCache remains pointer-owned, and no persistent/wire/public contract changes. Q1-Q3 and reuse-generation review found no leak, wait, stale-state, or unbounded-retention blocker.
Fresh reviewer evidence: all nine new focused cache/frontend/RC tests passed; the concurrent-reader test passed under -race -count=100; the cache owning package passed in full under -race; five-run table/database benchmarks at histories 1/16/4096 showed the pooled paths at 0 B/op, 0 allocs/op with consistently lower latency than the legacy 1 alloc/op controls. Exact-head CI is green. BVT is correctly N/A for this internal allocation optimization.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
What type of PR is this?
Which issue(s) this PR fixes:
issue #27235
What this PR does / why we need it:
This Draft PR preserves the existing exact-BTree catalog invalidation semantics while reusing the temporary lookup probes used by
CatalogCache.HasNewerVersion.sync.Poolprobe, fully overwrite its fields, run the existingAscendand identity/timestamp checks, clear the probe through a shared release helper, and return it to the pool.Ascendcompletes; a panic drops it instead of returning a potentially dirty object. Pool eviction by GC only causes a cold allocation and cannot change the result.The first commit adds a legacy exact oracle, deterministic differential coverage, concurrent-reader/lifecycle checks, and a benchmark matrix for table/database histories 1/16/256/4096 in warmed-negative and changed states. The follow-up commits add automatic
testing.AllocsPerRungates, deterministic release/reset coverage, a frontendCheckTableDefChangesmoke, and an RCgetCachedTableByKeyreplay/version/GC/identity smoke.This PR deliberately does not include the rejected account-bucket named-dependency scheme (147 measured false positives), the deferred precise shadow watermark, or any instrumentation/report API. #27447 remains Draft and is not rebased or merged.
Historical profile numbers (
17,060,022,442 Band52,996,163 objects) are owner attribution only. The A+C TPCC result (+0.7026%) is directional combination evidence; this PR makes no independent TPS, P95/P99, or+20%claim.Validation
testing.AllocsPerRungates require0allocations while asserting both negative and changed boolean results; cold and post-GC measurements are logged separately.pkg/vm/engine/disttae/cachefocused/full tests and full race: PASS through the repository CGo wrapper.pkg/vm/engine/disttaeRC consumer focused/full tests and full race: PASS through the repository CGo wrapper.pkg/frontendCheckTableDefChangefocused/full tests: PASS through the repository CGo wrapper.make build-with-prebuilt-native: PASS ona8efb132191943b7b1a40a00f023dd9e276ad7ae.TableItem/DatabaseItemprobe escape; only coldsync.Pool.Newallocation remains.benchstat: pooled warmed paths report0 B/op, 0 allocs/op; final-head longer database-history control shows no stable regression.a8efb132191943b7b1a40a00f023dd9e276ad7ae, diff hashf06d8c32c25b5f0c4b1eb04ab5a7c44b6e500a1184245994150639d6b4b93c37.GOWORK=off go buildandgo vetremain environment-blocked: a clean base reproduces the existingpkg/common/docfilterCGo declaration errors (C.mo_cbitmap_*/C.mo_croaring_*). They are not claimed as passes.BVT: N/A — the changed contract is an internal lookup-probe allocation optimization with unchanged SQL semantics; SQL cannot assert probe allocation. Exact invalidation behavior is covered by the cache differential tests plus the frontend and RC consumer smoke tests. Full multi-CN SQL DDL validation remains a QA task.QA required: yes. The direct consumer tests close the local wrapper/eviction path; prepared-plan and multi-CN RC DDL smoke remain QA validation. This PR is ready for human review only after the required remote checks and approval; it makes no claim that local consumer smoke replaces that QA.