web-next: Reduce per-interaction allocations#308
Conversation
📝 WalkthroughWalkthroughThis PR implements module-level memoization of ChangesIntl Formatter Caching and Performance Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces performance optimizations by caching expensive Intl.DisplayNames and Intl.RelativeTimeFormat instances, and switching to non-keyed <Show> components in PostEngagementBar and PublicTimeline to prevent unnecessary DOM remounts. A review comment identifies a behavioral inconsistency in Timestamp.tsx where the fallback formatting hardcodes { numeric: "auto" } instead of respecting the passed options object.
Two CPU/allocation hotspots surfaced by a Firefox profiler trace of
`/feed` opening NoteComposer (+10 MB/s sustained during the
interactive window, vs. ~0 MB/s while idle on the same route).
LanguageSelect.tsx
- `locales` was a plain function, so every reactive read from the
`<Combobox>` re-ran a `.map()` over ~200 `POSSIBLE_LOCALES`
entries that allocated a fresh `Intl.DisplayNames(locale,
{ type: "language" })` per iteration. Each `Intl.DisplayNames`
loads CLDR tables for its target locale, so the steady-state cost
of opening the composer was ~200 fresh CLDR-backed instances per
reactive tick.
- Wrap `locales` in `createMemo`, hoist `englishNames` to module
scope (its locale doesn't depend on anything), and memoise
`displayNames` so it only rebuilds when `i18n.locale` actually
changes. Cache per-locale `Intl.DisplayNames` instances at module
scope so the per-row native-name lookup costs one allocation
over the lifetime of the page.
- Drive-by: the pre-existing guard pushed `props.value.baseName`
onto the locale list only when it was *already* present, which
produced a duplicate entry and never extended the list to cover a
non-standard locale. Invert the condition so the value is appended
when missing — that matches the apparent intent (the comboboxneeds
to be able to show the active value even if it's outside the
canonical set).
Timestamp.tsx
- `formatRelativeTime` constructed a new `Intl.RelativeTimeFormat`
every render, which fires once per visible timestamp every
1s/30s/1min depending on age. Cache instances keyed by
`(locale, numeric, style)` at module scope. Apply the same
`options` in the value=0 fallback path that the main loop uses,
so the fallback's `numeric`/`style` no longer drifts from the
caller (the previous code happened to pass no options in the
fallback, defaulting to Intl's `"always"`, while the loop honoured
the caller's `"auto"` — pulling the same `options` through keeps
them consistent).
Not included here: the original draft of this branch also dropped
`keyed` from two `<Show keyed when={…()}>` wrappers (PostEngagementBar
and PublicTimeline). The local lint plugin `keyed-show.ts` is right
to enforce `keyed` for solid-relay-backed accessors — non-keyed
risks the documented "Stale read from <Show>" race when a fragment
flips to null inside the same tick as a downstream reactive read.
The perf benefit that motivated those drops (avoiding mass remount
of Kobalte primitives on every Relay snapshot tick) was real, but
its root cause is upstream: `createFragment` in solid-relay
pre-clears `data` to `undefined` before applying its
identity-preserving `reconcile`, defeating the merge and producing a
fresh top-level reference on every snapshot. Fixed in
nyanrus/solid-relay#fix/reconcile-preserve-identity (proposed
upstream at XiNiHa/solid-relay#68). Once that lands, the `keyed`
Shows in this codebase stop re-mounting on field updates without
any change needed here. The remaining follow-ups (Tooltip and
ActorHoverCard lazy-mount, Skeleton/Separator/Button cleanup) are
captured in `web-next/PERF_TODO.md`.
Assisted-by: Claude Code:claude-opus-4-7
86e1ec3 to
5ac7f33
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-next/src/components/Timestamp.tsx (1)
91-108: 💤 Low valueCache implementation reduces allocations effectively.
The module-level cache and key generation are correct for the current usage. One optional robustness improvement: the cache key could include
localeMatcherfromIntl.RelativeTimeFormatOptionsto guard against future code passing different values for that option.♻️ Optional: include localeMatcher in cache key
- const key = `${locale}|${options.numeric ?? ""}|${options.style ?? ""}`; + const key = `${locale}|${options.numeric ?? ""}|${options.style ?? ""}|${options.localeMatcher ?? ""}`;This ensures the cache key reflects all options that affect formatter behavior, preventing subtle bugs if
localeMatcheris used in the future.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/components/Timestamp.tsx` around lines 91 - 108, Update the cache key for Intl.RelativeTimeFormat to include the localeMatcher option so different localeMatcher values produce distinct entries; modify the key construction inside getRelativeTimeFormat (which uses relativeTimeFormatCache) to append options.localeMatcher (or a safe default) along with options.numeric and options.style before reading/setting the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web-next/src/components/Timestamp.tsx`:
- Around line 91-108: Update the cache key for Intl.RelativeTimeFormat to
include the localeMatcher option so different localeMatcher values produce
distinct entries; modify the key construction inside getRelativeTimeFormat
(which uses relativeTimeFormatCache) to append options.localeMatcher (or a safe
default) along with options.numeric and options.style before reading/setting the
map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df5b8d87-6791-443a-8883-e47a8c067cd1
📒 Files selected for processing (3)
web-next/PERF_TODO.mdweb-next/src/components/LanguageSelect.tsxweb-next/src/components/Timestamp.tsx
✅ Files skipped from review due to trivial changes (1)
- web-next/PERF_TODO.md
|
for the nitpick, there seems no call site passes localeMatcher |
5ac7f33 to
c7ea755
Compare
Summary
Firefox profiler traces of
https://hackers.pub/feedshowed a sustained +10 MB/s heap growth during interaction while the same route was flat (~0 MB/s) when idle. This PR fixes the two app-level hotspots that explain a large share of that. The remaining follow-ups are tracked in #333 (see "What's not in this PR").The single biggest interaction-time allocator was opening the
NoteComposerand triggeringLanguageSelect; further re-mount churn of Kobalte primitives traces to an upstream solid-relay bug (see below).What's in this PR
LanguageSelect.tsx— memoize the locale list, cache Intl formatterslocaleswas a plain function (not a memo), so every reactive read from<Combobox>re-ran a.map()over ~200POSSIBLE_LOCALESentries. Each iteration allocated a freshIntl.DisplayNames(locale, { type: "language" }), which loads the CLDR table for its target locale. Steady-state cost: ~200 CLDR-backed instances per reactive tick.localesincreateMemo, hoistenglishNamesto module scope (its locale is constant), and memoizedisplayNamesso it only rebuilds wheni18n.localeactually changes.Intl.DisplayNamesinstances at module scope so the native-name lookup costs one allocation over the lifetime of the page.Drive-by fix: the existing guard pushed
props.value.baseNameonto the locale list only when it was already present (producing a duplicate) and never extended the list to cover a non-standard locale. The condition is inverted so the value is appended when missing, which matches the apparent intent (the combobox needs to be able to show the active value even if it's outside the canonical set).Timestamp.tsx— cacheIntl.RelativeTimeFormatformatRelativeTimeconstructed a newIntl.RelativeTimeFormaton every render, which fires once per visible timestamp every 1s / 30s / 1min depending on age. Cache instances by(locale, numeric, style)at module scope.options, so itsnumeric/stylestays consistent with the loop above (the previous code passed no options in the fallback, falling through to Intl's"always"default while the loop honoured the caller's"auto").What's not in this PR
The original draft also dropped
keyedfrom two<Show keyed when={…()}>wrappers (PostEngagementBar,PublicTimeline). The local lint pluginweb-next/lint-plugins/keyed-show.tscorrectly enforceskeyedfor solid-relay-backed accessors — non-keyed risks the documented "Stale read from<Show>" race when a fragment flips to null inside the same tick as a downstream reactive read — so that draft has been reverted.The perf cost that motivated the original drop is real: with the current
solid-relay@1.0.0-beta.25,<Show keyed>over a Relay fragment re-mounts the entire subtree on every snapshot tick, including pure field updates such as a reaction toggle or a polled count delta. Profiler runs showed_$createComponentdominating the leaf-sample list during interaction, with the Kobalte chunks (@kobalte/core/dist/chunk/*) at ~1,200 inclusive samples per ~1,733 hackers.pub samples.The root cause is upstream:
createFragmentpre-clearsdatatoundefinedimmediately before applying its identity-preservingreconcile({ key: "__id", merge: true }). Solid'sreconcileearly-exits to "return the new value as-is" when the current state isn't wrappable (seesolid-js/storemodifiers.ts), so the pre-clear guarantees a fresh top-level reference on every snapshot — defeating the merge.Upstream fix proposed at XiNiHa/solid-relay#68. Once that ships, the
keyedShows in this codebase stop re-mounting on field updates with no change needed here. As an interim, the workspace could vendor it viapnpm-workspace.yaml#patchedDependencies(the workspace already patches@kobalte/core,@solidjs/start, andsolid-js).Other follow-ups from this investigation, tracked in #333 rather than an in-repo TODO file:
TooltipinPostEngagementBar(~100 primitives mounted at all times on a 25-card timeline).ActorHoverCard(×25 author avatars + per-mention).ui/skeleton.tsxwith a plain element (Kobalte'sSkeletonis<div role="status" aria-busy="true">; the 18 in-app uses are decorative shapes).ui/separator.tsxwith<hr>(one call site).ui/button.tsxwrapper for non-polymorphic uses (~46 call sites; mechanical but high churn for small gain).