Repository navigation
perf(ut): reduce table dump verification cost and bound race parallelism - #30686
Conversation
Add a raw go test report gate for duplicate or incomplete roots, make embedded Iceberg cleanup ownership-safe, and keep native CGo linking deterministic for UT builds.\n\nRefs matrixorigin#29249
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? |
|
mo-bot review · Commit: "interrupted" |
|
mo-bot review · Commit: "interrupted" |
|
mo-bot review · Commit: Review completed. Read the assessment. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: b819d7ceb284c26ff533f8ddc3c7c0a5dd877c6b
Assessment: request_changes
Reviewed all 15 changed files and relevant callers. Per-verification caching and the canonical-expression fast path are reasonable designs, but eager timezone loading introduces a confirmed performance regression for early matches. The full optools race suite passed.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
- P2
pkg/sql/plan/table_dump_time_zones.go:46: Cross-timezone DUMP/LOAD verification finds an early named-zone witness—for example, INT DEFAULT (HOUR(FROM_UNIXTIME(1704067200))) created at +03:00 and verified from UTC with a UTC local zone.
Impact: The iterator loads all 597 named zones before yielding Africa/Addis_Ababa, where the previous implementation stopped after three loads. This adds unnecessary timezone I/O and allocation to successful verification and repeats for every affected table. Populate the cache incrementally while preserving early termination and its per-verification lifetime.
Evidence: The isolated exact-source comparison measured 22,126 ns/op, 13,336 B/op and 33 allocations at base versus 5,133,004 ns/op, 6,262,332 B/op and 8,712 allocations at head. Production reachability is through AnalyzeTableDumpBindings and frontend handleDumpTable/handleLoadTable. Reused measurements were verified against unchanged current/base inputs, command receipts and terminal logs; see attempt-1/evidence-reuse-ledger.json and reused-evidence/timezone.log.
Validation:
-
Current-head isolation preflight passed. Full optools package passed with -race -count=1: 382 test/subtest starts, 23.578s, no failure or race-warning output. Reconciled checks/1792522972276891947-3.json with optools-race.log.
-
Verified retained focused report-validator tests and timezone measurements against identical semantic inputs, clean source fingerprints, commands and successful terminal output. Verification receipt: checks/1792522978993449241-4.json.
-
Shell/Python syntax, changed Go formatting and final source cleanliness passed; receipt checks/1792523091190447824-4.json. Detailed review coverage is retained in review-record.json.
-
Frozen CI evidence records successful native builds on Linux amd64/arm64 and native contract checks on Linux/macOS.
Limitations:
-
The timezone measurement isolates the helper; it is not an end-to-end SQL timing. Ordinary SELECT/DML/DDL paths do not call this verifier.
-
Native plan/embed tests and SQL entry points were reviewed but not executed locally. Full UT, SCA and coverage were pending in the frozen snapshot; local_validation contained no author artifact inventory to corroborate the reported full-run and performance results.
-
Linked issue #29249's body was not included in the supplied evidence.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: b691e23ef99966d5f1347f368213c07baaf08b26
Assessment: approve
Reviewed the complete 15-file delta and relevant owners/callers. Incremental timezone caching closes the prior eager-loading regression while preserving enumeration order, early termination and per-verification reload behavior. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
Validation:
-
Receipts, logs, source hashes and review coverage are reconciled in /home/xupeng/.local/state/mo-bot/jobs/239ab45c59d9f0a2994ee7f9/attempt-1/review-record.json. Frozen range, clean source, patch identity and syntax checks passed.
-
Exact-source timezone component tests passed, including full enumeration matching the base's 2,278 witnesses across four dates. Early-match benchmark: candidate 24,419 ns/op and 13,392 B/op; base 23,916 ns/op and 13,336 B/op. Receipt: 1792523828842618364-3.json.
-
All three timezone tests passed with the race detector, then individually for 100 repetitions. Commands, nonempty selections and successful terminal events were verified.
-
Full current-head optools race suite passed: 382 test/subtest executions, no skips or failures. Its actual JSON report passed strict lifecycle validation. Receipts: 1792523909599754445-3.json and 1792524018499468671-3.json.
-
Reused retained optools normal-mode and mocked native build-contract results after verifying unchanged relevant inputs, receipts and log hashes; evidence-reuse-ledger.json records the scope.
Limitations:
-
Timezone execution was component-level; complete plan/embed packages and public SQL entry points were not executed locally.
-
Native contract evidence uses mocks; actual native linking and GPU execution were not performed.
-
Author full-suite and workload-performance claims were not independently corroborated by raw artifacts. Frozen current-head CI was pending, local_validation was empty, and linked issue #29249's body was not supplied.
|
mo-bot review · Commit: Review completed. Read the assessment. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: b7d2208f83e60c7b2a9f86ea8a73260a8871a482
Assessment: approve
Reviewed the complete 16-file delta and relevant callers. Lazy per-verification caching closes the prior regression. Expression comparison preserves compatibility, and shorter embedded test batches reuse existing admission, deadline and cleanup owners. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
Validation:
-
Current-head preflight passed. Full optools race suite passed: 387 test/subtest executions, 23.182s. Its JSON report passed strict lifecycle validation.
-
Changed scheduler matrix passed 35 repetitions under race: 1,085 test/subtest executions, 24.474s. Verified commands, source fingerprints and successful receipts.
-
Recovered the interrupted job’s completed native plan race run: 118 executions passed in 116.216s. Verified unchanged production inputs before reuse.
-
Reused three timezone tests with 100 race repetitions each. Early-witness benchmark matches base latency at approximately 22 microseconds, with 80 additional bytes per operation.
-
Reconciled all 12 retained embedded workload streams: identical 138 roots and 497 terminal/action pairs. Eight batches reduced sampled anonymous-memory peak 5.9% and wall time 9.8%, costing 17.85 additional CPU seconds.
-
Patch identity, formatting, syntax and source cleanliness passed. Evidence is retained under attempt-1 in final-check-ledger.json, review-evidence-ledger.json and review-record.json.
Limitations:
-
Current-head full CI UT, SCA and coverage were pending in the frozen snapshot.
-
Embedded performance measurements use an independent precompiled-binary workload runner; they do not establish whole-CI resource improvements.
-
No fresh public SQL or GPU execution. Historical full-UT evidence was reused only for unchanged inputs.
-
Linked issue #29249’s body was not supplied.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: b7d2208f83e60c7b2a9f86ea8a73260a8871a482
Assessment: approve
Reviewed the complete 16-file delta, callers, ownership, failure paths and supplied comments. No actionable findings remain. Eight embed batches reuse the existing four-worker executor, shared deadline and drain handling. Lazy timezone loading closes the earlier regression.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
Validation:
-
Source identity, review coverage, receipts and log hashes are reconciled in /home/xupeng/.local/state/mo-bot/jobs/eaa2367a185f946ad6c556fc/attempt-1/review-record.json. Final audit passed; source remains unchanged.
-
Isolation preflight passed. Full current-head optools suites passed normally and under race: 387 test/subtest executions each. Receipts: 1792529825211562959-3.json and 1792529969651645509-3.json.
-
TestEmbeddedAuditedBatchScopeAndFailure passed 40 race repetitions, covering 1,240 test/subtest executions. Receipt: 1792530008445280716-3.json.
-
The production report validator accepted the full race report with 387 starts/terminals and zero malformed or incomplete lifecycles. Receipt: 1792530032458278633-3.json.
-
Reused five retained timezone checks after independently verifying unchanged component inputs, base oracle, commands, receipts and logs. Coverage includes 2,278 witnesses across four dates, early-match benchmarks and 100 race repetitions per timezone test. Details: evidence-reuse-ledger.json.
-
Shell/Python syntax, frozen range and patch content checks passed. Final audit receipt: 1792530148598713667-4.json.
Limitations:
-
Timezone execution was component-level; scheduler tests use shell mocks. Complete plan/embed packages and public SQL paths were not executed locally.
-
Actual native linking, GPU execution and full UT workloads were not performed locally. Author performance and full-suite claims were not independently corroborated by retained raw artifacts; local_validation was empty.
-
Linked issue #29249's body was not supplied.
What type of PR is this?
Which issue(s) this PR fixes:
Refs #29249.
What this PR does / why we need it:
Recent UT race jobs grew from roughly 55 minutes to 68–70 minutes. Two slow jobs spent 1082–1091 seconds in three table-dump roots. Avoid cloning already-canonical bound expressions, lazily reuse visited timezone rules within one verification, and run independent table-dump cases at bounded in-process parallelism 3. Process admission and the heavy-package budget remain unchanged.
Keep every original scenario and comparison rule, cancellation behavior and candidate budget. Named rules and failed loads expire after each verification; UTC/Local matches require no named loads, and an early named witness never loads later candidates. Fixed numeric offsets have a finite shared cache. Differential tests retain the old expression comparator as an oracle.
The same change also enforces nonempty, unique and complete raw test/package lifecycles, restores Iceberg fixture state according to ownership, fixes legacy UT cleanup under nounset, and places native object inputs before static-library dependencies during shared linking.
Controlled race measurements: identical 91 original scenarios, precompiled binaries, GOMAXPROCS=8, baseline parallelism 1 vs current parallelism 3. The baseline is main
180608ea8c; relevant baseline inputs are unchanged by the subsequent frontend-only main merge. Current treatment isb691e23ef9.Focused wall decreased 68.6%, CPU 27.3%, cumulative Go allocation 31.3%. Peak RSS increased 12.4% (~33 MiB). Allocation traffic is not peak residency and excludes C/race-shadow allocation. All 91 named scenarios match exactly, and both current timing/allocation reports pass strict lifecycle validation.
Applying the wall ratio only to the three expensive historical CI roots estimates ~12.4 minutes saved if other stages are unchanged. This is an estimate, not measured end-to-end CI improvement. Real-loader early-witness benchmarks also verify that laziness is preserved: ~22us and 13.4KB per operation, versus ~4.8ms and 6.24MB with eager cache population.
Validation:
7572bb36a7: passed in 25m56.7s under 8 CPU / 16 GiB, 511 packages, 82,156 explicit test starts, zero failures. Strict report validation: zero malformed/incomplete lifecycles, five known terminal-only events. Observed scope peak 12.24 GiB, swap/OOM/OOM-kill zero. Warm local caches prevent direct comparison to CI total duration.774d0eda08was merged, both new frontend BIT export tests passed focused race (1.154s).Current-head (
b7d2208f83) ordinary UT CI job 118880250801 passed on the sameamd64-mo-shanghai-8c16grunner class as both slow baseline jobs:Observed whole-job saving is 19m17s–20m42s; CPU decreases 5.5%–6.6%, anonymous occupancy 9.4%–12.6%, and sampled anonymous peak 10.6%–16.1%. These historical jobs have different source SHAs and hosts; this is an observational same-runner-class comparison supported by controlled focused measurements, not a randomized whole-CI A/B. The incremental 8-batch policy is not credited with the entire difference between CI runs.
The current strict raw-report gate passed: 974,310 events, 82,195 starts, 82,200 terminals, five known terminal-only events, zero malformed/incomplete lifecycles. No OOM/OOM-kill. Five additional starts match the five new scheduler matrix rows; package events increase by eight as expected from four additional fresh embed processes.
Memory limits: anonymous peaks are minute-heartbeat samples, and occupancy uses trapezoids only between the first and last emitted heartbeat, without extrapolation. Mean sampled anonymous occupancy rises to 2.64 GiB from 1.98/2.11 GiB despite lower total occupancy and sampled peaks. Exact total cgroup peak remains approximately 16 GiB, including file cache; the current memory-limit event counter is six but OOM/OOM-kill remain zero. This does not establish lower average residency, exact RSS peak or lower total cgroup peak. Current-head full CI SCA also passed (job 118880250719).
Coverage failure is triggered by this PR's native link-order change interacting with the reusable workflow's missing cgo runtime path. A successful coverage run on another PR uses the same workflow revision and linker flags. Under controlled
--as-neededlinking, old-order libmo is dropped from the test executable and passes, while new-order libmo is retained and fails to load; adding only the cgo rpath restores success. The failure was reproduced with actual MatrixOne coverage compiler flags and closed locally by adding only that rpath. Companion matrixorigin/CI#465 fixes the shared workflow and adds an automated real-loader regression. It must be merged and consumed by a fresh coverage run before coverage CI can be called green. Ordinary UT and SCA in run 39481820248 both succeeded. Companion CI#465 workflow validation is green, including the new loader regression.Bounded embedded process generations
On the measured four-worker path,
pkg/embednow uses eight fresh race-test batches while retaining at most four active children, GOMAXPROCS=2, the existing shared deadline, inventory/report validation and failed-drain handling. Other packages and worker counts retain four batches; the whole-package rollback and prebuild paths are unchanged.Matched original-inventory-order full-package comparison using the same compiled race binary (8 CPU/16 GiB, no swap):
This reduces sampled anonymous peak by 5.9%, anonymous occupancy by 8.9% and wall by 9.8%, at a cost of 17.85 CPU seconds for this package. These are observed whole-wave samples including the observer, not exact memory peaks or a claim of a production leak. All 138 roots ran exactly once; all 497 named terminal/action pairs, including the same two existing skips, matched. All twelve JSON streams passed strict lifecycle validation, all process groups drained, and neither arm swapped or OOMed. Exploratory sorted partitions also improved memory but are not used as the production baseline.
The incremental scheduler matrix covers four-worker embed selection, lower-worker rollback, mixed packages and stop-before-next-package on undrained children. Complete optools race passed in 23.439s and incremental optools SCA reported zero issues. The current-head CI measurements above now cover the complete UT workload; these local controls explain the scheduler tradeoff without substituting for that whole-run evidence.
Current head
b7d2208f83ordinary UT passed in run 39495715240. SCA job 118880250719 passed. Coverage job 118880524894 failed with the confirmed native-link-order/runtime-path interaction; CI#465 remains open, so coverage success remains unverified.Recovery after CI#465 merges: the caller references the shared coverage workflow by
main. GitHub reuses the first-attempt reusable-workflow SHA when re-running only failed/specific jobs. Use a fresh workflow run or re-run all jobs to consume the updated shared workflow; confirm the resolved workflow revision in the job log. Preserve the existing UT/SCA run first rather than cancelling its resource measurement. Reference: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations#behavior-of-reusable-workflows-when-re-running-jobsFinal check audit: no running jobs remain. The three failed checks are the coverage producer loader failure and its dependent coverage-merge/CI-Required gates; the merge log explicitly reports no complete producer profile and no coverage verdict. CI#465 has green workflow validation, is mergeable, and awaits GitHub-required review (
REVIEW_REQUIRED/BLOCKED). Its normal approval and merge are the external prerequisite for fresh consumer verification. No default-branch mutation or review bypass has been performed.