Skip to content

refactor(app): avoid deep equality in timeline row reconciliation - #48435

Open
Priyansh4444 wants to merge 2 commits into
anomalyco:devfrom
Priyansh4444:timeline-row-equality
Open

refactor(app): avoid deep equality in timeline row reconciliation#48435
Priyansh4444 wants to merge 2 commits into
anomalyco:devfrom
Priyansh4444:timeline-row-equality

Conversation

@Priyansh4444

@Priyansh4444 Priyansh4444 commented Sep 11, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #48434

Type of change

  • Bug fix
  • Refactor / code improvement
  • New feature
  • Documentation

What does this PR do?

reuseTimelineRows compares every incoming timeline row against the previous one with Equal.equals, so Effect hashes and walks the whole row, including every PartGroup ref, on each streamed delta. This replaces that call with a comparator that switches on the row's _tag and compares only that variant's fields. The PartGroup check is shared with session-ui instead of copied, and DiffSummary.diffs still uses Equal.equals.

How did you verify your code works?

  • packages/app/src/pages/session/timeline/timeline-row-equality.test.ts asserts the new comparator agrees with Equal.equals for every row variant pair. It runs in the normal unit suite, so CI covers it on every PR.
  • bun run test:unit 726 pass. bun test ./e2e/performance/unit 43 pass.
  • Isolated benchmark, 1,288 row timeline, 1,000 updates per run, separate process per build: 11.5 ms per update before, 0.64 ms after. perf stat on the same harness: 137.6B to 13.2B retired instructions.
  • Browser run on the production build with the same mocked stream: script time 6.47 s to 3.76 s, long task total 982 ms to 72 ms, rendered text and row structure unchanged.
  • Skipping the hash pre-check in Effect's compareObjects moved a one-off benchmark by about 3%, so the fix stays local.

Screenshots / recordings

Not a UI change. Rendered output parity was checked in the browser run and the unit tests.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions github-actions Bot added the needs:compliance This means the issue will auto-close after 2 hours. label Sep 11, 2026
Compare the nine timeline row variants field by field instead of calling
Equal.equals for every row on every stream delta. The PartGroup
comparison now comes from session-ui (message-part-groups) instead of a
local copy, and DiffSummary.diffs still uses Equal.equals.

Add a unit test asserting the comparator agrees with Equal.equals for
every variant pair, so it runs in the normal CI unit suite.
@Priyansh4444

Priyansh4444 commented Sep 11, 2026

Copy link
Copy Markdown
Author

The comparator here used to be a 15 byte stub that called Effect's Equal.equals. It is now a 359 byte function that checks each of the nine row types directly.

before: load "equals", call it       15 bytes
after:  switch on "_tag", compare   359 bytes

The group check is shared with session-ui, and the diff rows still use Equal.equals.

Numbers from the benchmark, which runs the real modules over a 1,288 row timeline:

  • 1,000 updates per run, medians of 5: 11.5 ms per update before, 0.64 ms after. That is a 94% drop.
  • Running the same build against itself moved less than 2%, so the gap is not noise.
  • Browser run, production build, 30x CPU throttle: script time 73.0 s to 35.7 s, longest task 1.73 s to 0.36 s, total long tasks 88.9 s to 49.9 s. Text, rows, parts, and scroll position were unchanged.
  • 726 unit tests pass, including a new test that compares this function with Equal.equals for every row type. It runs on every PR. 43 perf tests pass.

Why this path matters: the timeline rebuilds its row list in a memo whenever messages or parts change, so every streamed update compares the whole timeline with the previous one. The fixture does 1,288 comparisons per update. Before, Effect's equal/hash code held 34.4% of JS samples; after, none.

I tested fixing Effect instead. Skipping the hash check inside compareObjects changed a one-off benchmark by about 3%, so the expensive part is the generic walk itself. A function that knows the row types skips it without touching Effect.

Each build runs in its own process. When two builds share one process, the engine's optimizations for one can distort the other's numbers.

An AI agent wrote this change while following a documented process. I reproduced the benchmark and tests myself; the browser numbers come from the agent's run.

@github-actions github-actions Bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Sep 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

@Priyansh4444
Priyansh4444 marked this pull request as ready for review September 11, 2026 05:35
@Priyansh4444

Copy link
Copy Markdown
Author

I am sorry if this ends up being burden review; but this was what my agent found and it seemed correct especially the benching aspect of it!
Memo is not doing a shortcut on the Tags directly!
Equal.equals() under the hood since it does not implement Hash trait, it does the whole structural check, (O(n^2)) iirc and then a double traversal where it checks the two Object Keys and the whole WeakMap traversal for both objects!
Also this reduces the amount of times you have to hit that WeakMap traversal considering most of the changes are just the actual Tag's.

Correct me if I am wrong. And thank you for taking the time to review it!

This also does sacrifice some readability for performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

app: timeline row reconciliation uses Effect deep equality on every stream delta

1 participant