Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Merging this PR will not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| 🆕 | bench_session_with_self[false] |
N/A | 22 ns | N/A |
| 🆕 | bench_session_with_self[true] |
N/A | 20 ns | N/A |
Comparing bench/session-repeatability (4667b45) with main (f1080df)
Footnotes
-
860 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
📦 Binary size report
.text per crate
Baseline: |
Experimental draft; not ready to merge. This investigation has not yet demonstrated a coverage-preserving fix for the original warnings. The short root-key batch fails the controlled-slowdown test; the longer successive-update workload detects extra work but measures a different cache regime.
This PR preserves the original benchmark identities and bodies, adds deterministic 32-session candidates and explicit extra-work controls, and tests separate identity/root-key binaries. Production behavior is unchanged. The only manifest changes are the two approved
[[bench]]entries. Temporary diagnostic workflows collect fixed-revision CodSpeed simulation and walltime evidence; the existing benchmark workflow and check thresholds are unchanged.Original warning
The reported comment compares base
bd1f3069ac0fccec0060c1b1ee7aa54f4fa217a0with head06578d79877b69ded4f1e6a09464e20109e5c06e. CodSpeed comparison:The differences are entirely modeled cache/memory costs. These fixtures already use local fixed seeds; key generation occurs outside the measured region. The source diff does not change libsignal, its benchmarks, manifests, or workflows. Unlike earlier reports, CodSpeed explicitly attributes a libm metadata difference to these simulation rows. Missing metadata versus a loading difference remains unresolved. This evidence does not establish that the warning is a false positive.
Metric and supported alternatives
Simulation's headline is weighted instruction cost plus cache/memory penalties, not elapsed time. Walltime comparisons below use CodSpeed's minimum-seconds headline. Batch results are totals, not per-call estimates; counts are encoded in their identities.
--cycle-estimation=falsedoes not remove cache costs.Repeatability and sensitivity evidence
All comparative measurements use CodSpeed. Independent same-commit runs have reproduced identical simulation components; stochastic simulation variation has not been reproduced.
At diagnostic head
f36956de3695444c64acb2e3188678a6899118f3, three environment-matched runs (A, B, D) produced original root-key walltime minima 3/2/3 ns versus 114/107/113 ns for a 32-session batch. The range divided by minimum fell from 50% to 6.54%. This was not universal: self-identity was 24/24/24 ns originally versus 776/748/745 ns batched. Small samples on shared hosts limit inference.At
308c20425fced37189979a85950e81e90c0d36e3, the short root batch failed sensitivity: doubling work changed instruction cost 0.290093 → 0.575915 µs, but memory penalty fell 5.777778 → 4.944444 µs, leaving total 6.480371 → 6.496748 µs (+0.25%). Run. Within-run walltime detected the extra work. This failure rejects the batch as a reliable simulation replacement.At
4c6b11fa18a0b214c3702f45ebefc2ea94bed3db, an environment-matched pair produced:Thus longer work improves timer resolution and retains sensitivity in that pair, but isolation alone does not stabilize the original cold-call walltime.
Earlier fixed-source validation
That validation head is
35c60b9efadb5471924f71229618fe471907f4ef. Actual diagnostic checkout is immutable merge66e29dbad39bab7e8760d024508947b90b48d629, with parents39d3b776531230abf2f22e2c7b8544df6debc291and the PR head. Diagnostics now use the normal workflow's merge checkout and four-package proto-signal build graph, adding walltime as a second measurement mode.Earlier normal/scoped comparisons had head-versus-merge checkout and package-feature-unification differences. They are exploratory execution-context observations, not controlled same-source false-positive evidence, even when CodSpeed reported matching environments.
Three corrected runs (1, 2, 3) have identical SHA-256 hashes for all 22 recorded binaries and identical simulation components. They landed on three different CPUs; cross-run walltime is excluded. Within each run, longer controls retain sensitivity:
The short batch still fails simulation sensitivity. Run 2's isolated cold root-key walltime is flagged under 1 ns/optimized-out by CodSpeed and is unusable, not a performance improvement. Two same-revision diagnostic reruns have completed; attempt-specific results follow. The normal proto-signal job has completed successfully.
Matched rerun snapshot
CodSpeed reuses a workflow run ID across GitHub reruns. Values in the first-attempt table above were retrieved before rerunning; those links now display the latest attempt. The following snapshot was retrieved through CodSpeed MCP at 2026-10-05 17:46–17:47 UTC, comparing diagnostic workflow 2 attempt 1 (job 111883601873) with workflow 1 attempt 2 (job 111894994440). Both built merge
66e29dbad39bab7e8760d024508947b90b48d629; binary hashes match. CodSpeed reported no environment differences.All simulation components match. The long root candidate detects doubling within each run while reducing relative walltime variation versus the short batch in this pair. This supports the companion workload, not replacement of the cold-call metric or a reduced simulation false-positive rate. Workflow 2's second attempt has now replaced that endpoint's previous result; workflow attempt/job IDs identify this historical snapshot. The final matched pair below remains available at its linked comparison.
Full-suite result at the corrected checkout
The normal benchmark workflow passed all four jobs. Its proto-signal log confirms merge
66e29dbad39bab7e8760d024508947b90b48d629, cargo-codspeed 5.0.1 and runner 5.4.0.In the full/scoped comparison, CPU metadata matches, but CodSpeed reports
libm.so.6metadata absent → build IDd53fcbff9f855463606b25cfbe2a2e10856ea3a7, explicitly affecting all 20 simulation rows. This remains an environment caveat, not a warning to suppress.All 11 isolated simulation cases have exactly identical components across full and scoped execution, including the cold-call copies and long-work controls. Combined-target totals change by −7.40% to +6.07% with unchanged instruction components (original non-self/self −3.95%/−3.94%; root-key 0%). This is encouraging evidence for process isolation, but the metadata mismatch prevents presenting it as a fully matched causal test. Nor does it establish a lower false-positive rate across future code/layout changes.
Final matched repeat
All five diagnostic executions passed and rebuilt identical binaries at merge
66e29dbad39bab7e8760d024508947b90b48d629. The final matched pair compares workflow 3 attempt 1 with workflow 2 attempt 2 (job 111895120446). CodSpeed reports no environment differences; all simulation components are identical.The same-commit cold/short walltime rows trigger regressions; the long companion does not, and it retains sensitivity to doubled work. This is a concrete improvement in the repeatability of the different, longer workload. It does not justify removing the original cold-call metric or asserting that the original simulation warnings were false.
Checks and readiness
All 248 libsignal unit tests, benchmark compilation, scoped Clippy with warnings denied, formatting and diff checks passed. Earlier-head CI completed at
35c60b9efadb5471924f71229618fe471907f4ef(2026-10-05 18:17 UTC). Normal CodSpeed, all five diagnostic executions, build-and-test, all-features build/Clippy/tests, all eleven per-crate feature-matrix checks, MSRV, WASM, binary-size, generated artifacts, Miri, E2E, both API-host checks, Rustdoc, serde and dependency checks passed. The only failure is informational semver, reporting published-release API differences in untouched production files. No implementation change was needed during final monitoring. All review threads are resolved; CodeRabbit skipped review because the PR is draft.The original warnings remain observable. Additive stable companion workloads do not by themselves prove a reduction in false simulation warnings. Matched current-build repeats and a same-source full-suite comparison are complete, with the library-metadata caveat above. No tested supported design yet fixes the original warnings while retaining their workload and demonstrated sensitivity. Readiness still requires that substantive fix and removal of temporary diagnostic workflows. Functional CI completion does not establish the requested warning fix. Until then this stays draft.
Recommendation and upstream clarification
Keep this PR as an experimental draft. Useful outputs are the isolated cold-call copies, deterministic companion fixtures, explicit slowdown controls, component-level diagnosis, and fixed-checkout/binary-hash evidence. The temporary repeat workflows and the disproven short-batch replacement are not a validated production gating change. Do not replace the original workload with the resident-session metric.
The minimum unresolved CodSpeed clarification is the provenance and meaning of linked-library metadata: does
libm.so.6: N/Amean not loaded, not observed in a profile, or unavailable metadata, and is that metadata attached to the measured executable/benchmark or aggregated from other executables in the shard? The fixed-source full/scoped pair above gives a concrete reproduction without production edits. Until answered, this environment warning cannot safely be classified as noise.For more precise automated reporting, the minimum supported capability would be a check/comment breakdown that exposes instruction, L1 and last-level penalties together with benchmark-specific environment comparability. MCP already exposes the numeric components; the missing piece is documented CI-check policy/reporting for them. If instruction-based gating is offered, cache/memory changes must remain separately visible and reviewable. An instruction-only pass must not silently clear a cache regression. No external support request has been opened, and no unsupported runner option or threshold change is proposed.
Bounded R03 original-workload walltime validation
Diagnostic head
4667b45d1c0cfd067f2a7777a548a56660d3f2e8adds a single label-triggered set of six workflows: three measurements of basef1080df166bbc2c88e762eda6eb5bb7f6efc2f05and three of headc42dd44c59acff7825eae7d09e940a08c71d91a2. Only the unchanged originalbench_session_with_self[false/true]cases execute. No candidate, batching, root-key or production change is part of this phase.Both revisions use the same four-package build graph, nightly-2026-06-16, cargo-codspeed 5.0.1, runner 5.4.0, allocator settings and ubuntu-24.04 label. Actual CPU/flags, OS/library metadata and binary/source hashes are checked before accepting cross-run walltime comparisons. Three independent workflows per revision preserve separate CodSpeed run IDs. Normal pushes do not trigger this phase; it runs only when
r03-walltime-fixed-revisionsis added. Only the two allowlisted same-repository commits can execute; checkout credentials are not persisted, no secrets are inherited, and existing OIDC permissions are unchanged. Actionlint and diff checks passed. All six initial measurements passed. Both final reruns passed, completing the cap of eight executions. No more benchmark jobs are planned. Current-head CI is now complete; all executable checks passed except informational semver, as detailed below.CodSpeed labels these runs with diagnostic PR #1655's head; each linked job's recorded checkout, rather than that label, identifies the measured R03 source. Run links and actual revisions will be listed together.
R03 metadata correction: in the latest exact R03 simulation comparison, the two session Simulation rows have an AMD EPYC 7763 → Intel Xeon 8573C CPU mismatch. The report's
libmmismatch applies to other Memory rows, not these session Simulation rows. Their weighted instruction costs are unchanged; +111.111 ns last-level penalty and −2.778 ns L1 penalty produce +108.333 ns total. The rerun's merge544316b81492c5cc75cb3a4cd2db613a9ea0d79ahas the same tree as the specified R03 head. Earlier #1653/#1655 metadata attributions above belong to different comparisons and must not be transferred to R03.R03 walltime results: initial six executions
Base means exact checkout
f1080df166bbc2c88e762eda6eb5bb7f6efc2f05; head means exact checkoutc42dd44c59acff7825eae7d09e940a08c71d91a2. Every run contains only the two original self/non-self cases. This table records first attempts; base 1 and head 2 were rerun, so their CodSpeed links now display the second attempt while the linked job IDs preserve attempt identity.The base 2 → head 3 comparison reports no environment differences, with both benchmarks unchanged: non-self 27 → 27 ns, self 26 → 25 ns (medians are the same respective values). Other initial base/head pairs have CPU or CPU-flag differences and are excluded. In particular, the two EPYC 9V74 runs have different flags despite matching model names.
All six builds produced the identical libsignal walltime executable SHA-256
64144b38286dd95686b96027d0a70b7f283fb9aac038cb60fce6a480e3e1fd49. This is evidence about the unchanged original benchmark executable, not all production behavior or the simulation instrument. One matched pair shows no practical slowdown; it is not a proof of exact equivalence. Each case uses the original harness's 100 rounds, without warmup or workload modifications. The two final attempts are recorded below; no further harness iteration is planned.R03 bounded validation conclusion
All eight executions passed. The final base rerun (job) checked out
f1080df166bbc2c88e762eda6eb5bb7f6efc2f05; the final head rerun (job) checked outc42dd44c59acff7825eae7d09e940a08c71d91a2. Both retained the same benchmark executable hash listed above. The head rerun used Intel 8573C (22/20 ns minima) and is excluded from comparisons with the current AMD baseline runs.The accepted EPYC 7763 set has no reported environment differences in either base-to-head comparison or the same-source baseline repeat:
No practical walltime regression attributable to R03 is established for these two original benchmarks. Non-self is identical across the matched set, and head self-time lies inside the repeated-baseline range. All eight native benchmark executables are byte-identical. This supports proceeding with #1656's readiness assessment without treating these two unmatched-environment simulation alerts as evidence of an established real slowdown. The alerts remain visible and unacknowledged; no gate, threshold or production code changed.
Limits: this is two matched baseline executions and one matched head execution on ordinary hosted runners, not a formal equivalence test or a bound on sub-nanosecond changes. Original native-harness semantics, 100-round sampling, and deterministic fixture setup were preserved. This scoped walltime check does not recreate the full-suite simulation cache model, establish download-path performance, or validate #1655's proposed harness candidates. #1655 stays draft and unmerged; the separate benchmark-warning fix remains unproven. The R03 Simulation rows' actual mismatch is CPU metadata; the
libmwarning applies to other Memory rows.Final diagnostic CI and review status
At
4667b45d1c0cfd067f2a7777a548a56660d3f2e8, all checks are terminal (2026-10-05 19:10 UTC): 43 success, 2 skipped, 1 neutral, 1 failure. The full WASM job passed, including the durability probe, SQLite retry timer, VoIP core and client runtime builds. The only failure is informational semver, retaining the 24/3/7 major-check categories against published releases in untouched production files.Greptile reviewed the current changes with zero comments. All inline review threads are resolved. CodeRabbit explicitly skipped this draft; Cubic's neutral result is a quota limit, not a completed review or approval. No diagnostic implementation defect required a further commit. No additional benchmark runs were launched after the eight-execution cap.
This remains an open, unmerged diagnostic draft. The exact R03 source SHAs, attempt-specific run links, metadata correction, matched comparison limits and original workload coverage remain documented above. Completed CI and useful R03 evidence do not turn this PR into a general validated fix for recurring simulation warnings.