Take schema-scale style work off the render path, and fix what review found - #2128
Draft
kmcginnes wants to merge 11 commits into
Draft
Take schema-scale style work off the render path, and fix what review found#2128kmcginnes wants to merge 11 commits into
kmcginnes wants to merge 11 commits into
Conversation
Style data varies only by type, so within a render pass N elements of a type now cost one `vertexStyleData` call rather than N. The memoizing closure lives in a plain `create*Resolver` factory because the React Compiler lint rules reject a hook that returns a closure mutating its own captured cache. The vertex resolver takes the styles whose icons are in scope, since the canvas needs only the types it draws while the schema view needs every type. Names the atom lookups (`VertexStyleLookup`, `EdgeStyleLookup`) so that contract is explicit, drops the now-callerless `useAllEdgeStyles`, and tightens the style-context test to assert the exact selector set.
…types `useRenderedEdges` called `useRenderedVertices()` while `GraphViewer` also called it directly, so the whole vertex pipeline — icon resolution included — ran twice per render. The filter predicate is now a derived atom both pipelines read, so the store computes it once. Canvas style and icon resolution is scoped to the vertex types actually drawn rather than every type in the schema: the stress schema carries 10,044 vertex types to draw 3, and resolving all of them each render was the dominant app-side render cost. The schema view keeps `useAllVertexStyles`, since drawing every type is its job. Measured over matched 10s windows on a live 10k-type schema, expanding a node: `useBackgroundImageMap` self time 59.7ms -> ~29ms, style work in `renderedEntities` 5.2ms -> ~0.7ms. Total busy time and INP were within run-to-run noise; the dominant expansion cost remains cytoscape rendering and the fcose layout, which this does not touch.
An imported style file can carry an optional key present but set to `undefined`. Spreading it over the defaults overwrote them, and the resulting `data()` mapper has no missing-value fallback on the cytoscape side, so the element rendered unstyled for that field. Only keys with a value override now. Also drops the `labelTextColorFor` memo: profiling put it at 0.1ms over a 10s expansion, far below the per-type resolution that dominates, so the cache only bought global state shared across stores and tests. Pins the default style fixtures with `toStrictEqual` so the tests that depend on them are trustworthy.
`cy.json({ elements })` merges element data: `ele.data(obj)` adds and overwrites
keys but never deletes ones missing from the new object. Both optional fields
were therefore unclearable once applied — a node whose icon stopped resolving
(an uploaded icon that fails to load, retried three times and given up on) kept
rendering the previous icon while its colour updated around it, with no error
and no signal to the user that anything had failed.
`ge_iconUrl` now carries `"none"` when a type has no icon and `ge_lineDashPattern`
carries cytoscape's default for solid lines, so both live on the base `node` /
`edge` rule and always reflect current state. That also moves the icon gate out
of the generic `components/Graph` component, next to the producer that feeds it,
and renames `__iconUrl` to match its siblings.
`displayVerticesSelector` was an `atomFamily` keyed on a freshly allocated `Vertex[]`, and `displayVertexSelector` on `Vertex` object identity. `atomFamily` caches by parameter identity forever unless `remove`/`setShouldRemove` is called, and nothing here calls either — so every node expansion, add, or remove interned a new entry that could never be reached again, each retaining a `DisplayVertex` for every node on the canvas. Retained memory grew with mutations × nodes for the tab's lifetime, driven by the app's most common interaction. The array-keyed family is gone: `displayVerticesInCanvasSelector` iterates `nodesAtom` through the per-id family in a single pass. `displayVertexSelector` is keyed on `VertexId`, so it holds one entry per node. Vertices that are not on the canvas — search results, and details for a vertex fetched on demand — cannot be served by an id-keyed family, so the derivation is extracted into a pure `toDisplayVertex(vertex, context)` over a non-family context atom. Those callers now intern nothing at all. Retention itself is not asserted: `atomFamily` exposes no size. The added test pins the observable proxy — an unchanged node keeps its `DisplayVertex` identity across an unrelated mutation.
Three passes over the canvas vertices became one. The visible-ids atom iterated every vertex, the style-scope atom iterated them again re-testing membership, and the render hook iterated a third time re-testing it once more — two passes existed only to re-discover a decision the first had already made. `canvasVerticesAtom` returns the drawn vertices, their ids, and the styles of the types they draw from a single loop, so the style scope and the drawn set cannot disagree: a drawn vertex's `primaryType` is in `stylesByType` by construction. That also collapses the reason a missing icon used to be silent. Style data was assembled from two inputs of different totality — scalar fields from a lookup that answers for any type, the icon from a partial scope list — so a scope miss was indistinguishable from a type that genuinely has no icon. The style data now derives entirely from the scoped style list, and a type absent from it is a loud throw rather than a node quietly drawn without its icon. The memoizing resolver layer is gone with it: five exported symbols, two factories and two cache closures replaced by one hook returning a plain per-type map. Edge style data is resolved on first sight of a type inside the existing filter loop, since the drawn edge types are only known while filtering.
The ADR records that a new per-type style property has to be added in two places together — the `ge_*` field and its producer, and the matching `data(…)` mapper — and that editing one side alone silently drops the style. Nothing enforced it, and the label background/border and arrow-colour mappings were asserted nowhere. Asserts both directions by collecting the key sets rather than listing names, so the test cannot rot the way a hand-written list does: every `ge_*` key the producers emit must appear as a mapper somewhere in the stylesheet, and every `ge_*` mapper must have a producer. Non-`ge_*` mappers such as `displayName` come from the rendered entity rather than these producers, so they are excluded by prefix. Walks every rule, so a legitimately re-added gated selector still counts.
`renderedEntities.ts` had accumulated four unrelated concerns: a pure string ID codec, the canvas visibility atom, the style scope, and the Cytoscape render hooks. Only the last is what the module name describes. The codec has no coupling to the rest — no React, no Jotai, no state — and is exactly the unit the first four test blocks covered, so it moves out whole along with them. 263 lines down to 193.
`react-colorful` fires `onChange` on every pointermove and the picker was wired straight to the style atom, so a color drag committed at pointer rate. Each commit rebuilds every rendered element's data and re-serializes the whole canvas through `cy.json` — measured at one dropped frame per commit, 167-217ms on a 76 node graph, so a drag cannot keep up. The picker now tracks the pointer in local state and commits on a 150ms delay, the same shape as the display-name field in `modules/Styles/VertexStyleRow.tsx`. The swatch still follows the pointer at full rate; only the canvas lags. The per-commit cost above is measured. The improvement is not: driving rapid trusted input at the Radix slider is not reachable from the automation harness — synthetic key events are ignored, and real ones arrive too far apart to exercise the debounce — so the reduction in commit count is by construction rather than observed.
This was referenced Aug 14, 2026
The comment cited `vertexStyleByTypeAtom`, which the atom does not reach. The actual path is `displayVerticesInCanvasSelector` -> `displayVertexContextSelector` -> `vertexStyleAtom`, and it is why a style edit still recomputes the canvas pipeline. Points at the follow-up issue rather than leaving a dead end.
kmcginnes
force-pushed
the
graph-style-render-performance
branch
from
August 14, 2026 21:15
181ecfd to
d6bfe25
Compare
Both style-data lookups guarded coverage with a throw. In render, that unwinds to the app-level boundary in `DefaultLayout` — neither graph view has a local one — so the whole UI is replaced by the error page whose only affordance is a reload. That is a wildly disproportionate outcome for an invariant whose worst alternative is a node drawn without its icon. The schema view's throw was reachable. Its two sides read the same schema by different paths: the type configs read the schema atom directly, while the style scope came from `useAllVertexStyles` -> `useActiveSchema`, which is deferred. A sync that added a vertex label could therefore render a config whose style had not arrived and take down the view. Verified by reproducing the old shape against the new test, which throws on the added type. Note the pre-branch code read `vertexStyleAtom` directly, which is total, so an unknown type simply got default styling — this branch had converted a self-healing case into a crash. Both are now structural rather than asserted. `canvasVerticesAtom` pairs each drawn vertex with its style so there is no lookup to miss, and style data is resolved on first sight of a type from the style in hand — the same shape `useRenderedEdges` already used. The schema view scopes styles from the same type configs its loop iterates. Both throws are gone. `useAllVertexStyles` has no callers left and is deleted; the canvas covers blank nodes through the synthetic type on the vertex itself, which is already tested.
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
1 participant
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.
Description
Follow-up to #2112, targeting its branch so that PR is unaffected. Two threads: the performance work the reported expansion choppiness prompted, and the correctness bugs a review of that work turned up.
Performance
Moving styling onto element data in #2112 moved the per-type cost from the stylesheet into the render path, and two things there scaled with the schema rather than with what is drawn:
GraphViewercallsuseRenderedVertices()directly anduseRenderedEdges(), which calleduseRenderedVertices()internally to get its valid endpoints. Each hook has its own render-scoped memo, so everything — icon resolution included — ran once per call site.useBackgroundImageMap(useAllVertexStyles())iterated all of them every render: on the stress schema, 10,044 vertex types resolved to draw 3.Both are now derived from one atom that walks the canvas vertices once and yields the drawn vertices, their ids, and the styles of the types they draw. Style data resolves per type instead of per element.
Measurement
Chrome DevTools against a live 10k-type Neptune schema, expanding a 244-neighbour airport with limit 75 (76 nodes / 150 edges). Matched 10-second windows from the click, ~81k samples each:
useBackgroundImageMaprenderedEntitiesThe halving matches the predicted 50% for the double-call exactly. The drop to zero is the scoping — the function no longer appears in the profile at all, and the whole icon path totals ~1.3ms.
Important
This is not a fix for the reported expansion choppiness. Total busy JS and INP were within run-to-run noise across repeated runs. The dominant expansion cost is cytoscape rendering plus the fcose layout — a ~390-470ms long task — which this does not touch. What this delivers is the removal of schema-scale work from the render path. Read the table as "the app's own render hooks are now ~0% of the profile", not "expansion is faster".
Correctness fixes found by review
cy.json({ elements })merges element data and never deletes a missing key, so the two conditionally-absent fields were unclearable once applied. A user replaces an icon, it fails to load,iconRegistrygives up after three attempts, the field goes absent — and the node silently keeps rendering the previous icon while its colour updates around it. Both fields are now always set (ge_iconUrl: "none",ge_lineDashPatternat cytoscape's default for solid lines), so there are no gated selectors left. Note that addingbackground-image: noneto the base rule alone would not fix this: the gated selector keeps matching while the key is present.displayVerticesSelectorwas anatomFamilykeyed on a freshly allocatedVertex[].atomFamilycaches by parameter identity forever unlessremove/setShouldRemoveis called, and nothing here calls either — so every expansion interned an entry that could never be reached again, each retaining aDisplayVertexfor every node on the canvas.undefinedclobbered style defaults. An imported styling file can carry an optional key present but set toundefined; spreading it overwrote the default, and the resultingdata()mapper has no missing-value fallback. A test pinning this was deleted during the migration, which is how it slipped through.modules/Styles/VertexStyleRow.tsx.ge_labelBackground*,ge_labelBorder*, arrow colours) and was verified with a deliberate two-direction probe.Structural
The memoizing resolver layer an earlier iteration introduced is gone — five exported symbols, two factories and two cache closures replaced by one hook returning a plain per-type map. Lazy memoization bought nothing when every caller knows its key set up front, and the resolver function shape was what forced a missing icon to be silent: it had to answer for any argument, where a map need not. A type absent from the scope now throws instead of a node being quietly drawn without its icon. The ID codec also moves out of
renderedEntities.ts, which had accumulated four unrelated concerns (263 → 193 lines).How to read
Ten commits, each self-contained and green; reviewing commit-by-commit will be much easier than the combined diff.
Resolve graph style data once per type+Share one visible-vertex computation and scope canvas icons— the performance change and its first shape.Resolve canvas vertices, ids, and style scope in one pass— supersedes the resolver layer from (1) with an eager per-type map and fuses three passes into one. (1) and (3) are add-then-replace; kept as separate commits because the intermediate states are green and the narrative explains why the second shape is better.Always set every style data field— the stale-icon fix.Stop interning a dead atom per node-set mutation— the memory fix.undefineddefaults fix, the lockstep test, the codec extraction, and the picker debounce.Caveats
Three claims in here are reasoned rather than measured, and are marked as such at their commits:
atomFamilyexposes no size API, so "entry count does not grow" is not observable from a test. The added test pins identity stability as the closest proxy.toIconImageUrl(the profiler showed the existing render-local dedup already made it free) andstyle.setPropertyfor the SVG colour (CSSOM normalises#FF0000torgb(255, 0, 0), changing the emitted data URI for a Nit-severity gain).Related
atomFamilysites,neighbors.tsbeing the same shape and severity as the one fixed here, plus decoupling the canvas atom from style changes.