Conversation
…d fixture Why: apache#4677 item 4 asks for a Usage settings tab-entry median/p95 number plus renderer-side evidence that the activity table mounts a bounded page instead of every row. Fixture (test infra only): - scenarios-usage.ts pads the 5 handcrafted turns with 394 zero-tool turns (132/131/131 across the three sessions) so the ledger holds exactly 409 activity rows: 399 model rows + 10 tool rows. Synthetic turns reuse the handcrafted generators on the fixed clock (60..453 minutes ago). - e2e-fixture.ts: one catchUpModelCallProjection() pass bounds how many lagging runs it folds, so a padded seed with hundreds of queued runs left most of them unfolded after seeding. Fold in a loop until pendingRuns reaches 0, which is generic for any large canonical seed. Spec (perf/usage-tab-entry.spec.ts): a custom usageWindow fixture injects the settings-usage scenario; the test asserts the bounded mount (DOM rows <= 50 with the tab badge at 409) and then samples 12 warm re-entry rounds entirely inside the renderer with rAF — the clock runs from the synthetic click to the activity table's next painted frame, excluding Playwright round-trips. Results land in $TEMP/usage-tab-entry-results.json. Numbers (this branch, apache/main base): median 103.9 ms, p95 142.6 ms (n=12, p95 = max), first-entry DOM rows 50/409. Reference numbers from apache#4677 (comment issuecomment-5880047554, measured on a feature branch): median 134.1 ms, p95 201.3 ms — same magnitude; the branch difference is noted rather than compared. Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed ebc8eb6. The PR adds a 409-activity-row Usage fixture, repeatedly folds the fixture's canonical model-call projection, and adds a real-Electron warm tab-entry measurement. No product code path or schema changes. I found two issues in the new measurement spec (inline), so the current reported median and the intended slow-path timeout should not be relied upon as written.
On this head, a local Node 24 real Electron/Host/storage measure passed with 50 mounted rows and a 409-row badge; its 12 entry samples were 191.7–274.6 ms on this machine, not a head-to-head comparison with the author's host. Targeted Biome, ASF headers, diff check, current-head hosted test, and a merge-tree against fetched main 2f32205 passed. I also reproduced the hanging timeout branch with an actual Playwright page. I did not run macOS/Wayland or measure a cold app start. This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| return; | ||
| } | ||
| frames += 1; | ||
| if (frames > MOUNT_POLL_FRAMES) { |
There was a problem hiding this comment.
P2: Pass MOUNT_POLL_FRAMES into each page.evaluate callback. Playwright serializes the callback into the renderer but does not carry this Node-side lexical constant. The fast path passes because it returns before this line; when the activity table fails to unmount or mount promptly, the first poll reaching this guard throws ReferenceError from requestAnimationFrame. The surrounding Promise remains pending instead of rejecting after 600 frames, and this perf test can then wait for its 40-minute test timeout. I reproduced the pending Promise and page error in a real Playwright page with the table held present. The entry poll at line 147 has the same issue.
There was a problem hiding this comment.
Fixed in 1128be4: both page.evaluate callbacks now receive the poll budget as an explicit argument ([ACTIVITY_TABLE, <NAV>, MOUNT_POLL_FRAMES] → ([tableSelector, navSelector, pollBudgetFrames])), so the guard compares against a real number inside the renderer. When the table fails to unmount/mount in time, the poll now rejects after 600 frames as designed instead of throwing ReferenceError from the rAF callback and leaving the Promise pending. A fresh run of the corrected spec completes normally (1 passed, 12 rounds sampled).
| } | ||
|
|
||
| const sorted = [...entries].sort((left, right) => left - right); | ||
| const median = sorted[Math.floor(sorted.length / 2)]; |
There was a problem hiding this comment.
P2: Compute the median of both middle samples for the fixed 12-round run. This selects the upper middle value, which is not the median of an even-sized sample: the PR's raw entries sort to middle values 98.1 and 103.9 ms, so the median is 101.0 ms, not the reported 103.9 ms. The requested tab-entry median is the primary output of this spec; use the mean of sorted indices 5 and 6 (or label a different quantile explicitly) and update the reported number.
There was a problem hiding this comment.
Fixed in 1128be4: the summary now computes the standard even-sample median — (sorted[n/2 - 1] + sorted[n/2]) / 2 for even runs, the single middle value for odd runs. A fresh run of the corrected spec reports median 81.2 ms (entries: 89.7, 84.1, 80.7, 80.1, 85.5, 93, 73.5, 76.7, 81.6, 87.8, 74.1, 77.6 → middle values 80.7 / 81.6) and p95 93 ms. The PR body's Measured numbers table and raw entries were updated to the corrected run.
…n-sample median Two review fixes on the usage-tab-entry perf spec: - Playwright serializes page.evaluate callbacks without their Node-side closure, so the MOUNT_POLL_FRAMES guard referenced an undefined constant inside the renderer: the first poll reaching it threw a ReferenceError from a requestAnimationFrame callback, leaving the surrounding Promise pending forever instead of rejecting after 600 frames. Both the unmount and the entry poll now receive the budget as an explicit evaluate argument, so the slow path rejects as designed. - The 12-round summary picked sorted[n/2], the upper middle value, as the median. For an even-sized sample the median is the mean of the two middle values; the summary now averages sorted[n/2 - 1] and sorted[n/2] for even runs (odd runs keep the single middle value). The PR body's measured numbers were refreshed from a new run of the corrected spec. Refs apache#4677 Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 1128be4496e42a88abd14c0cd92a3f90669fea60.
The two findings from my previous review are addressed. Both page.evaluate callbacks now receive the frame budget as a serialized argument (apps/desktop/perf/usage-tab-entry.spec.ts:107-159), so their timeout branches no longer reference a missing Node-side closure. The 12-sample median now averages the two middle values (apps/desktop/perf/usage-tab-entry.spec.ts:169-173); the refreshed PR numbers use that calculation. I found no additional substantiated P0–P3 in this revision. The prior current-head COMMENTED reviews by another account are not approvals.
The measurement-only PR still seeds 409 activity rows, asserts a 50-row first page and a 409 badge, and measures 12 warm re-entries in the renderer. I reran the real Electron/Host/storage perf spec on Node 24 under Xvfb: 1/1 passed, 50 DOM rows and 409 total; local median was 221.2 ms and p95 272.3 ms. These times are not directly comparable to the author's machine. Focused Biome, ASF headers, diff-check, and merge-tree against current main passed; current-head hosted test passed. I did not force either 600-frame timeout branch, or test macOS/Wayland and cold startup. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Checked before responding: neither the description of this PR nor #5800's (whichever applies) has ever contained an If the |
This branch has not been deployed
2 participants
Summary
#4677 item 4 asks for a Usage settings tab-entry median/p95 number, plus renderer-side evidence that the activity table mounts a bounded page instead of every row. This PR adds both as a perf-tier measurement (test fixture + new spec only, no product code paths changed):
apps/desktop/perf/usage-tab-entry.spec.ts(new) — injects thesettings-usagescenario via a customusageWindowfixture, asserts the bounded mount (DOM rows <= 50, tab badge 409), then samples 12 warm re-entry rounds: click off the Usage section, wait for the table to unmount, click back in. Each round is timed entirely inside the renderer withrequestAnimationFrame— the clock runs from the synthetic click to the activity table's next painted frame, excluding Playwright round-trips. Results land in$TEMP/usage-tab-entry-results.json.apps/desktop/src/main/e2e-fixture/scenarios-usage.ts— pads the 5 handcrafted turns with 394 zero-tool turns (132/131/131 across the three sessions), so the ledger holds exactly 409 activity rows = 399 model rows + 10 tool rows. Synthetic turns reuse the handcrafted generators on the fixed clock (60..453 minutes ago), so every table stays deterministic.apps/desktop/src/main/e2e-fixture.ts(infra fix) — onecatchUpModelCallProjection()pass bounds how many lagging runs it folds, so a padded seed with hundreds of queued runs left most of them unfolded after seeding. It now folds in a loop untilpendingRuns === 0, which is generic for any large canonical seed.Measured numbers
Raw entries (ms): 89.7, 84.1, 80.7, 80.1, 85.5, 93, 73.5, 76.7, 81.6, 87.8, 74.1, 77.6. For the even 12-sample run the median is the mean of the two middle values (80.7 / 81.6 → 81.2); an earlier head of this PR reported the upper middle value (103.9 ms) instead — corrected per review. The reference numbers were measured on a different branch state, so they are quoted as the magnitude anchor, not compared head-to-head.
Verification
npm run measure -- usage-tab-entry(real Electron + Host + storage)npx biome checkon the 3 touched filesnpm run check:asf-headersnode scripts/protocol-epoch-check.mjs --stagedgit diff --check --cachedHonest boundaries
AI use
Select exactly one:
Tool(s) and scope: GLM-5.3-Flash (ZCode) — authored the fixture padding, the projection-fold fix, and the measurement spec; numbers above come from a real local run of the spec.
Checklist
Does this PR entail a change in behavior?
Refs #4677, #4531, #4539