perf(log-viewer): 24% faster Call Tree grids, and a highlight that follows your row - #973
Merged
lcottercertinia merged 12 commits intoAug 28, 2026
Merged
Conversation
`keyIdOf` keeps an event's interned bucket key in an `Int32Array` indexed by `eventIndex`. A frame built rather than parsed has no index at all, and writing under one lands an ordinary property on the typed array rather than being dropped, so every other such frame read back the first one's key and merged into its bucket. The slot is now checked before it is read or written. A frame with no slot is keyed but not kept, which costs only the key being built again.
The grid's Aggregated and Bottom Up builders interned their own key strings once per build, and a bottom-up caller row then found the calls it stands for by splitting and comparing those strings again. Both builders now take the log's key table, group by integer, and stamp each row with the interned bucket path that names it. A caller row derives its calls through `KeyPathIds.chainReaches`, which composes each occurrence's chain a step at a time and stops at the row's own depth, so `bottomUpOccurrences.ts` and the path a row used to rebuild from its Tabulator parents both go. | 95MB log, 864k lines, 431k calls | Before | After | | --- | --- | --- | | Bottom Up build | 497ms | 377ms | | Aggregated build | 337ms | 308ms | Two things a merged bottom-up row reports change with it. `calledBy` names the row picked at whatever caller depth it sits, because a deeper row narrows the same calls and so needs the row to tell one depth from the next. The calls themselves are read through the log the row was built from, since a path id means nothing to another log. Also drops the degenerate `Multiset` from the aggregated build: it only ever held the one parent frame, so the check is an integer compare.
…nces Clicking a finding revealed its bucket by asking every row on the grid whether it held the frame, and then asking for every active row to see whether the Show Details filter hid it. The grid's root buckets hold every call in the log between them, so a click that revealed nothing read all 431,307 of them on a 95MB log. The grid is bottom-up, so the frame heads a top-level bucket its own key finds. `findRootBucket` is that lookup, split out of `findBucketRow` so neither carries a parameter it cannot use: only a top-down walk expands a row, and only it needs to wait for one to render. Whether the filter hides the bucket is one read of `_hasDetailsDeep`.
A caller row reported the calls it counts, so every caller depth pointed at the same leaf frames: the flame chart lit the leaves and the call tree selected one, whatever depth was picked. A row now reports the frames it is. `frameEventIndexes` climbs the row's own path depth from each counted call and dedupes, so stepping down the callers walks the highlight up the stack. Details still describes the calls the row counts, which ride on a field of their own.
The tree control is not focusable, so clicking it dropped focus to the body, and the key bindings only answer while the table body holds focus. The arrows scrolled the table instead of moving down it. Focus now returns on every pointer expand and collapse, rather than only the first one before any row is selected. A keyboard collapse declares itself as the code's, as the keyboard expand already did.
Picking a deep inspector row landed on an ancestor and needed a second click. The descent read a row's children without waiting whenever that row was already open, and the wait it did use resolved on the first of `renderComplete` or two frames, so a long render lost the race. `RowNavigation` already tracks whether a render is pending, so it now exposes `waitForRenderComplete` and the view's wait delegates to it. The descent waits for any row whose children have not arrived.
The grid marked the bucket but never scrolled to it: only an `inspector:reveal` from a finding moved the grid, so picking a row in the inspector's call tree did nothing visible. It now reveals on a sticky locate and then marks, as the Call Tree does.
Both correct behaviour that has shipped. The highlight and Analysis fixes in this branch correct the Inspector, which is still unreleased, so they belong to its own entry rather than a new one.
`waitForRenderComplete` was meant to stop the bucket descent racing a frame count against a pending render. But Tabulator dispatches `renderStarted` and `renderComplete` in one synchronous call, so the pending flag always reads false where the wait was awaited, and it fell through to the same two frames as before. Its one live branch could only be entered by a `renderStarted` whose `renderComplete` never came, and one throwing subscriber is enough to cause that: `_dispatch` has no try/catch and five modules subscribe. The wait would then never settle, stalling the descent, the reveal and the mark behind it for the life of the table. The descent still waits for any row whose children have not arrived, which is what fixed the second click.
Sharing one handler with the expand meant a collapse selected the row whenever nothing else was selected, and that selection re-scoped the inspector to the row the user had just closed. A collapse now only hands focus back. The reason it has to is not the one the comment gave: Tabulator sets a `tabIndex` on the tree control, so working it moves focus onto the control rather than dropping it. Either way the key bindings answer only while the table body is the event target.
The contract said a locate never scrolls or selects. That stopped being true when the Call Tree began revealing on a pick, and the Analysis grid now does the same. The Database grids and the flame chart still only mark.
The mark was read before the reveal's awaits and applied after them, so dropping the pick while a reveal was in flight cleared the mark and then had it put back with nothing picked. Hovering another row landed the same way. The mark now goes on only while its own report is still the last one.
lcottercertinia
approved these changes
Aug 28, 2026
15 tasks
lcottercertinia
pushed a commit
that referenced
this pull request
Sep 1, 2026
…de (#980) > **Stacked on #977.** Its commit shows in this diff until that merges. Review from `be6412ea` onward. # 📝 PR Overview Two bugs in the inspector's row mark, both about which rows light up. **A grid row told the inspector the wrong thing.** A row under the pointer emitted every call it counts, so a Bottom Up caller row marked the inspector rows for the leaf calls underneath it rather than for the caller itself. The forward direction stopped doing that in #973; the reverse direction was still on the old rule, so the two disagreed depending on which side you pointed at. **A mark could come back after being dropped.** The sweep reaches only the rows a table has attached. A row lit while on screen, then scrolled out, kept the class when the mark moved away, and the renderer re-attaches such a row without running the row formatter again, so the stale highlight returned with it. ## 🛠️ Changes made - **`rowFrames(row, root, direction)`** — a bottom-up caller row climbs to its own depth, `depthOf(_pathId) - 1` hops above each call it counts. A top-down row already sits at its frames' depth, so it climbs nothing. - **`LogStore.framesAbove`** — the climb, next to `stackByEventIndex`, which already owned this parent-pointer walk. The inspector's `frameEventIndexes` now reads it too, so the two sides cannot drift. - **The direction is read at hover time**, from `directionOf(this.viewMode)`, which the sibling `_emitDetailSelection` already uses. No new parameter and no second source of truth. - **`litByHost`** — what each table's mark has lit, whichever half lit it, so a new mark can un-light an element the renderer has since detached. - **The `detail:locate` doc** described the old rule; it now says what the protocol carries. ## 🧩 Type of change (check all applicable) - [x] 🐛 Bug fix - something not working as expected - [x] ♻️ Refactor - internal changes with no user impact - [ ] ✨ New feature – adds new functionality - [ ] ⚡ Performance Improvement - [ ] 📝 Documentation - README or documentation site changes - [ ] 🔧 Chore - dev tooling, CI, config - [ ] 💥 Breaking change ## 📷 Screenshots / gifs / video [optional] N/A. What changes is which rows carry the highlight. ## 🔗 Related Issues Follows #973, which changed the forward direction, and #975, which shipped the mark mechanism the second fix corrects. ## ✅ Tests added? - [x] 👍 yes - [ ] 🙅 no, not needed - [ ] 🙋 no, I need help Three `rowFrames` cases and one for the detached row. Each guard was proven by reverting the code it covers: dropping the direction check makes a top-down row climb; removing the shared climb fails one `rowFrames` test **and** two `frameEventIndexes` tests, which also shows the inspector test runs the real method rather than a copy of it; restoring the old clearing fails the detached-row test. ## 📚 Docs updated? - [ ] 🔖 README.md - [x] 🔖 CHANGELOG.md - [ ] 📖 help site - [ ] 🧪 Marked any pre-release-only features - [ ] 🙅 not needed No entry: both fixes correct the unreleased Inspector, so they belong to its existing entry. Ticked to record that it was considered. ## Anything else we need to know? [optional] **Where to start.** `LogStore.framesAbove` is the mechanism; `rowFrames` is the grid's use of it and `frameEventIndexes` the inspector's. **Why the mark has to remember what it lit.** Tabulator builds a row's element once (`Row.create()` is guarded by \`this.created\`) and \`Row.initialize()\` re-uses it, and both \`deinitialize()\` calls in Tabulator are inside \`reinitializeRows()\`, a column-layout path. So an ordinary scroll neither rebuilds the element nor re-runs the formatter: the class persists, and clearing it has to reach elements the query cannot see. **On the dedupe.** A merged row aggregates distinct caller frames that share a signature, so the climb is many-to-many and the answer can be as long as what was asked about. It is not safe to climb from one call and assume the rest agree. **Test plan.** - \`pnpm lint\` and \`pnpm test\`. - Call Tree, **Bottom Up**, Inspector open. Hover a bucket row, then a caller row one level down: the inspector mark moves up the stack with you rather than staying on the leaf calls. - Step two and three levels up: one frame per level. - Analysis: same, hovering a row under a method bucket. - **Aggregated** and **Time Order**: unchanged, since a row there already sits at its own frames' depth. - Hover an inspector row so a grid row lights, scroll that row out of view, move the pointer off the inspector row, then scroll back: no highlight. That is the second fix. - Both themes, Inspector docked at the side and at the bottom. **Known unrelated failure locally.** \`lana/src/services/__tests__/servicesRuntime.test.ts\` cannot resolve \`effect\` in a worktree not installed since #951. It passes in CI. **Follow-up, not in this PR.** `deriveCalls` already walks each call's parents inside `chainReaches` and stops exactly at the frame the row is, then keeps the index and throws the frame away, so `framesAbove` re-walks the same edges. Fusing the two means having `chainReaches` return the node it stopped at.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 PR Overview
A merged call-tree row was identified by joining its bucket keys into a string, which was most of the cost of the grid builds. Separately, a bottom-up caller row reported the calls it counts rather than the frames it is, so every caller depth highlighted the same leaf frames however deep you picked.
The grids now group on the interned key ids the inspector already uses, and a row now reports the frames it stands for, so stepping down the callers walks the highlight up the stack. Three navigation bugs found while walking that change are fixed here too.
🛠️ Changes made
KeyPathIdsand group on integers instead of joined key strings. On a 95MB log (864,216 lines, 431,307 calls)toBottomUpTreegoes 497ms → 377ms andtoAggregatedCallTree337ms → 308ms. Medians of four runs; run-to-run variance on this log is about 50ms, so read the second figure as directional.keyIdOfwrote past the end of itsInt32Array, which lands as an ordinary property, so every frame built rather than parsed read back the first one's id.frameEventIndexesclimbs the row's own path depth from each counted call and dedupes, so the flame chart and call tree point at the frames the row is. Details still describes the calls it counts, which ride on a field of their own.🧩 Type of change (check all applicable)
📷 Screenshots / gifs / video [optional]
N/A. Nothing new appears on screen: what changes is build timing and which frames light up.
🔗 Related Issues
None.
✅ Tests added?
📚 Docs updated?
Two entries, for the keyboard and navigation fixes. The highlight and Analysis fixes correct the Inspector, which is unreleased, so they belong to its existing entry rather than a new one.
Anything else we need to know? [optional]
Where to start.
log-viewer/src/core/log/keyPathIds.tscarries the vocabulary; read its class doc first. The two id spaces matter: the aggregated build composes outermost-first and the bottom-up build innermost-first, so ids from the two directions must never be compared.Two questions, two accessors.
locatableEventIndexesis the calls a row counts, which its totals describe.frameEventIndexesis the frames the row is, which a highlight points at. They differ only for a bottom-up caller row.Test plan.
pnpm lintandpnpm test.Called bynames the row hovered.Known unrelated failure locally.
lana/src/services/__tests__/servicesRuntime.test.tscannot resolveeffectin a worktree that has not been installed since #951. Nolanafile is touched here.Reverted during review. A first attempt routed that wait through
RowNavigation's pending-render flag. Tabulator dispatchesrenderStartedandrenderCompletein one synchronous call, so the flag always read false where the wait was awaited and the indirection bought nothing, while its one live branch could only be entered by arenderStartedwhoserenderCompletenever came — which one throwing subscriber causes, since_dispatchhas no try/catch and five modules subscribe. The wait would then never settle. The inline wait is back.Follow-ups, not in this PR.
DatabaseViewandApexLogTimelinemark without revealing, so "does a pick move this view" deserves to be an argument rather than a property of which handler was copied last.