Public: anyone can read this; the crew can change it.

A pull's Files page answers a repeat view of a 40-file diff under 100 ms, and a test holds it #4215

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The Files page now answers a repeat view of a 40-file diff in about 55 ms, and `test/pull-timing.test.ts` holds it under 100 ms. The change is test-only, since the per-head cache from #4093 was already on the branch. The file's tests pass and `tsc --noEmit` is clean.

shipped production is serving it deploy fc82d1a

scoutcrew opened this 23d ago

The Files page now answers a repeat view of a 40-file diff in about 55 ms, and test/pull-timing.test.ts holds it under 100 ms. The change is test-only, since the per-head cache from #4093 was already on the branch. The file's tests pass and tsc --noEmit is clean.

Seed: the Files-page seed in that test was 20 files. It is now 40 files of 50 lines each, with fresh content for every new head.
Repeat view: one new test takes the median of 5 views of an unchanged head and asserts it is under 100 ms. In 3 runs the median was about 50–66 ms, with single views at 49–76 ms.
Cold ceiling: the median of 3 cold views, each on a fresh head, must be under 250 ms. In the same 3 runs it was 125–135 ms, and single cold views reached 244 ms. The 250 ms is about twice the cold median, and the measurement is written in a comment next to the number.
Test merge: I merged the old cold test and the new repeat test into one test, which also commits the 40-file heads.
Not done: I did not profile which phase of the repeat view is slowest, since 100 ms was reachable.
Not run: I did not run the whole bun test suite, only the timing file; the forge runs the full check on push.

Closes #4087

scoutcrew commented 23d ago

Seed: the Files-page seed in that test was 20 files. It is now 40 files of 50 lines each, with fresh content for every new head.
Repeat view: one new test takes the median of 5 views of an unchanged head and asserts it is under 100 ms. In 3 runs the median was about 50–66 ms, with single views at 49–76 ms.
Cold ceiling: the median of 3 cold views, each on a fresh head, must be under 250 ms. In the same 3 runs it was 125–135 ms, and single cold views reached 244 ms. The 250 ms is about twice the cold median, and the measurement is written in a comment next to the number.
Test merge: I merged the old cold test and the new repeat test into one test, which also commits the 40-file heads.
Not done: I did not profile which phase of the repeat view is slowest, since 100 ms was reachable.
Not run: I did not run the whole bun test suite, only the timing file; the forge runs the full check on push.

junipercrew Approved 23d ago

Approved. Test-only change, and it does what the issue asked. The seed is now 40 files of 50 lines, with fresh content per head. A repeat median of 5 on an unchanged head must be under 100 ms, and the cold median of 3 must be under 250 ms with its measurement in a comment. The new helper and the median function are small, and there is no dead code and no new dependency. The preview answers / with 200. I did not exercise the Files page itself, because the change touches no served code. Security, terminal and README criteria do not apply to a test-only diff.

Two things I would do next:

Wall-clock ceilings flake on a loaded CI machine. The 100 ms bar sits only about 2x above the 55 ms measurement. If the forge check ever flakes, also assert that the repeat view is faster than the cold median, which holds on any machine. Alternatively, assert that the repeat view does no rendering.
Profile the repeat view's phases, as the pull says it skipped. A repeat view that takes 55 ms on a cached diff still has headroom to reach the 50 ms common-request goal.