Commit dabc56e
authored
perf: cut cold-load FCP 70% and audit-log navigation 57% (#232)
* docs(perf): design spec for navigation performance measurement
* docs(perf): implementation plan for navigation performance work
* feat(catalog): scaffold sample module for perf benchmarking
Fixes two scaffolder bugs that made `make new-module` produce a broken
module for any name:
- update_host_pyproject wrote the bare module name as the dependency, but
modules declare the distribution name simple_module_<name>. uv failed with
"references a workspace ... but is not a workspace member".
- The generated pyproject.toml omitted [tool.hatch.build.targets.wheel]
packages, so hatchling could not infer the package directory from the
mismatched distribution name and built an empty wheel — the entry point
then failed with ModuleNotFoundError.
* feat(catalog): rich product/category entity with list, search and detail
Replaces the scaffold's placeholder entity with one that exercises the
realistic worst case for the navigation benchmark: FK relation, indexed
text search, enum status, audit + soft-delete mixins, and a composite
(status, created_at) index matching the default browse ordering.
Service uses the column-query pattern from 026c146/878f51f — select the
DTO's columns and count the conditions directly, no ORM hydration.
Also splits _templates_py.py (over the 300-line cap after the settings
template) into _templates_module.py for the module-definition layer, and
adds regression tests for all three scaffolder bugs.
* test(perf): navigation benchmark harness, catalog seed, locust + micro-benchmarks
- tests/perf: Playwright benchmark measuring click->painted for Inertia
client-side navigations, plus per-navigation payload breakdown. Hooks
Inertia's public inertia:start/inertia:finish document events rather than
exposing the router, so no production code carries a test-only hook.
Navigations are driven by clicking links, not page.goto() — a goto is a
full document load, a different and much heavier path than what a user
feels clicking around.
- tests/loadtest/seed_catalog.py: idempotent faker seed, ~5k products.
- locustfile: catalog list/search/detail tasks.
- tests/benchmarks: shared-props micro-benchmarks for the middleware path.
Gated by 'perf and e2e' markers so the default suite skips them; new
bench-nav make target since these need a live server and browser.
* perf(hosting): compress responses — halves First Contentful Paint
Measured first, per the plan. All three a-priori suspects were dropped on
evidence:
S1 (shared props built for /api/* that discard them) — server time is
1.5-5.5ms of a ~32ms navigation; the wasted work is a fraction of that.
S2 (menus/permissions recomputed per request) — 18us combined, ~0.05% of
a navigation. Three orders of magnitude too small to matter.
S3 (static shared props re-sent every navigation) — real at ~15% of
payload, but total navigation time is payload-insensitive: a 2.2KB
page costs 31.2ms and a 16.4KB page costs 35.1ms.
S4 (Vite dev overhead) — falsified: prod 32.5ms vs dev 33.8ms.
The actual problem was not on the list and was invisible until transfer size
was measured: no Content-Encoding header anywhere, and no GZipMiddleware in
the pipeline. Asset caching was correct (984443b); compression was never
added. The built CSS shipped as 139KB instead of 21KB.
Installs GZipMiddleware inside CorrelationId/RequestLogging but outside
everything producing a body, including the /static mount.
A/B at 4Mbps/40ms, varying only Accept-Encoding so both arms hit the same
server:
transfer 858KB -> 259KB (-69.8%)
FCP 2268ms -> 1168ms (-48.5%)
load 1487ms -> 582ms (-60.9%)
On localhost the same change measures ~0ms — bandwidth is infinite there,
which is why this stayed invisible. Guarded by a regression test that fails
if transfer saving drops below 40%.
* perf(menus): link sidebar items at canonical paths — kills a redirect per navigation
Five of eight sidebar links pointed at a path that 307-redirected: menu items
used the bare view prefix (/catalog) while routes register at /catalog/.
Everything rendered correctly, so no functional test caught it — each
navigation to those pages just paid an extra full round trip.
Affected: /admin/background-tasks /audit_log /branding /catalog
/feature_flags /file-storage /settings
Measured on Postgres (10k users, 100k audit rows, 5k products), prod build,
20 rounds, median:
audit_log 86.2ms -> 50.8ms (-41%)
catalog_list 49.8ms -> 33.0ms (-34%)
On localhost the redirect costs ~8ms; on a 40ms-latency link it roughly
doubles the navigation.
Guarded by framework/hosting/tests/test_menu_urls_are_canonical.py, which
fails if any registered menu URL redirects or 404s, so this cannot recur for
future modules.
Also corrects the baseline doc: the earlier SQLite numbers understated server
cost badly. On Postgres at real volumes, ttfb is 57% of the audit-log
navigation, not the 4-15% SQLite suggested.
* perf(build): group chunks — cold-load FCP down another 34%
The bundle was over-split, not over-sized: 88 JS chunks, 40 of them under
2KB holding just 32KB between them. uvicorn speaks HTTP/1.1, so the browser
opens ~6 connections and 55-63 requests became ~10 serial round trips —
roughly 400ms of pure waiting at 40ms latency.
Declares advancedChunks groups: react-vendor (split out because it changes
only on a dependency bump, so a normal deploy leaves it cached), vendor, and
shared packages/ui components.
Two Rolldown specifics, both hit while doing this:
- Vite 8 bundles with Rolldown, so Rollup's experimentalMinChunkSize does
not exist and type-errors. The equivalent is advancedChunks.
- Rolldown silently ignores minSize unless groups is also declared.
Measured at 4Mbps/40ms, prod build:
FCP /catalog/ 1128ms -> 748ms (-34%)
requests 55 -> 13 (-76%)
transfer 259KB -> 247KB (-5%)
Transfer barely moved and FCP dropped a third — the cost was round trips,
not bytes. Client-side navigation is unchanged (within noise).
Cumulative across all three fixes, cold /catalog/ at 4Mbps/40ms:
2268ms -> 748ms FCP, a 67% reduction.
* test(perf): guard cold-load request count against chunk-group regressions
Locks in the 55->13 request reduction. Over HTTP/1.1 each request past the
browser's ~6-connection limit is another serial round trip, so a regression
here costs latency directly rather than bytes — which makes it invisible to
any size-based check.
* perf(audit_log): Postgres skip scan for distinct entity types
The browse view calls distinct_entity_types() on every render to fill a
filter dropdown. SELECT DISTINCT walks every index entry to return 8 values
— cost proportional to rows, so it degrades as the table grows.
A recursive skip scan (loose index scan) hops value-to-value through
ix_audit_entry_entity_type: one seek per distinct value, so cost tracks
distinct values rather than rows and stays flat as the table grows.
SQLite rejects that CTE form, so _distinct_stmt_for_dialect() returns the
plain query there. Dialect comes from session.bind.dialect.name, whose values
match DatabaseProvider exactly.
Verified against the live 100k-row table — both paths return identical
results, best-of-5: 5.26ms -> 0.48ms (11x).
End to end:
audit_log ttfb 35.9ms -> 23.9ms (-34%)
audit_log total 52.8ms -> 37.0ms (-30%)
Cumulative on that route across all fixes: 86.2ms -> 37.0ms, -57%.
count(*) is now the largest remaining piece of that ttfb (11.4ms). Postgres
could estimate it via reltuples, but that makes the pager's total
approximate — a product decision, so left alone.
* ci: gate the asset-delivery perf guards
Adds a perf-guards job running the Playwright cold-load guards against the
PRODUCTION build, and makes it a required check.
These are invisible to every other job: drop compression or the chunk groups
and the app still renders correctly, just slower. Only a browser measuring the
built bundle catches it. Asserts on structure (request count, compression
ratio), never milliseconds, so runner noise can't make it flaky.
Correcting an earlier claim of mine: three of the four guards already ran in
CI. test_response_compression, test_menu_urls_are_canonical and
test_distinct_entity_types are plain pytest under existing testpaths, so
python-tests covers them (17 tests). Only tests/perf needed a job.
Simulating the job locally first caught a failure that would otherwise have
landed on someone else's PR: with SM_ENVIRONMENT=production exported job-wide,
pytest cannot start at all — the simple_module_test plugin eagerly builds
BackgroundTasksSettings() at import and those reject a localhost broker. The
production config is therefore scoped to the Start API step alone: the server
needs it, the test process must not see it.
Verified locally against the real job steps: 13 requests, 72.6% transfer
saving, 15s runtime.
* perf(static): pre-compressed brotli/gzip assets
GZipMiddleware re-compressed the same immutable, content-hashed bundle on
every request, and on-the-fly compression must use a fast (worse) level.
Compressing once at build time fixes both, and brotli is 13.6% smaller than
gzip-9 across this bundle (248.5KB vs 287.6KB).
- compress-assets.ts: Vite build plugin emitting .gz/.br at max level via
Node's built-in zlib (no new dependency). Skips a variant that fails to
beat its original.
- static_files.py: PrecompressedStaticFiles serves those siblings, brotli
first. Split out of _phase_helpers, which was near the 300-line cap; it
subsumes ImmutableStaticFiles.
Two traps, both now pinned by tests:
- Serving app.js.br directly makes Starlette type it from the .br
extension, and browsers refuse to execute a script sent as
application/octet-stream. The original file's type must be preserved.
- StaticFiles signals a missing file by RAISING HTTPException(404) rather
than returning one, so a missing variant cannot be detected from a status
code. The first implementation looked right and 404'd every asset.
Vary: Accept-Encoding is set on negotiated responses so a shared cache cannot
serve a compressed body to a client that did not ask for one.
Measured at 4Mbps/40ms:
FCP 712ms -> 672ms (-6%)
transfer 244KB -> 215KB (-13%)
Cumulative: 2268ms -> 672ms FCP (-70%), 858KB -> 215KB (-75%).
brotli is declared in the dev group — it was previously only present as a
transitive dep of locust's geventhttpclient, which the new tests would have
silently depended on.
* test(perf): layout-stability benchmark, with a self-verifying observer
Adds CLS measurement for cold load and Inertia client navigation. Result:
CLS 0 across every route, both paths — the layout is genuinely stable, since
server-rendered props mean content arrives before paint rather than reflowing
in afterwards. Nothing to fix.
That zero is only worth reporting because the instrument was validated. The
first version reported all zeros too and was completely broken: forcing an
unmistakable 500px reflow produced no observer entries at all.
Two causes:
- Observers installed after page.goto() miss the whole load, because
longtask does not replay via buffered:true. Now armed with
add_init_script, which runs before any page script.
- The longtask observer never fires in Playwright's Chromium regardless. It
arms without error and records nothing — a deliberate 250ms blocking loop
goes unrecorded in both chromium-headless-shell and channel=chromium.
Long-task measurement is therefore removed rather than shipped. A metric that
always reads zero is worse than no metric; it manufactures confidence that
nothing is wrong.
test_layout_shift_observer_is_live forces a reflow and asserts it is caught,
so a future browser change fails loudly instead of the suite quietly
reporting a perfect score forever.
* fix(build,hosting): correct asset base path and error-page shared props
Found by driving the whole app through Playwright against the production
build and correlating with the backend log.
1. Every lazy page load fired broken preload requests.
Vite records a lazy chunk's preload deps as base-relative paths
("assets/Browse-x.js") and prefixes them with `base` at runtime. The
default "/" sent them to /assets/..., but the host serves the build under
/static/dist/ — so each fell through to the SPA fallback, returned HTML,
and produced a 404 plus a MIME-type console error.
Nothing caught it because the actual dynamic import uses a relative "./"
specifier and resolved fine. Only the preloads were broken: no test
failed, no page broke, the network tab just filled with errors.
Fixed with base: command === 'build' ? '/static/dist/' : '/'. Build-only,
since in dev the host points <script> at ${SM_VITE_DEV_URL}/main.tsx.
Full journey: 91 -> 56 requests, 15 -> 0 errors, 20 -> 0 console errors.
2. Error pages rendered raw translation keys.
render_error_page builds its own Inertia instead of using get_inertia, so
it skipped inertia.share(**shared). The page got no shared props at all —
no i18n, auth or menus — so a 404 displayed host.error.not_found_title
instead of "Page Not Found".
Both guarded by new tests. The asset guard was verified non-vacuous: reverting
the fix, rebuilding clean and restarting makes all three fail.
* perf(background_tasks): run worker inspect probes concurrently
Found by visiting every page: /admin/background-tasks/workers took 4110ms,
35x the next slowest page.
WorkerInspector.snapshot() issued four inspect broadcasts (ping, stats,
active_queues, active) sequentially, each with a 1s timeout. Every call waits
its full timeout for replies because it cannot know whether a slow worker is
still coming, so with the broker up and no workers running that is 4 x 1s.
Worst possible case for this page: an admin opens it BECAUSE workers are
down, and it takes four seconds to say so.
The probes are independent, so they now run concurrently on separate inspect
handles: 4110ms -> 1020ms (-75%).
A short-circuit (skip the rest when nothing answers the ping) was tried first
and rejected — it broke test_worker_in_stats_but_not_ping_is_offline, which
covers a degraded worker answering stats() but not ping(). That worker would
have vanished from the page instead of showing as offline. Concurrency gives
the same speedup with no behaviour change.
Guarded by a timing test that fails if the probes go serial again, plus one
asserting all four are still issued.
* chore: drop unrelated local config from this branch
.claude/settings.local.json and .gitignore were already modified in the
working tree before this work started, and a 'git add -A' in 7b30594 swept
them into the branch. They are machine-local emdash tooling hooks plus a
gitignore entry for that same file — unrelated to the performance work, and
the pair was self-contradictory (the commit both tracked the file and
gitignored it).
Reverted to origin/main so this branch contains only the perf changes.
Whether .claude/settings.local.json should be untracked repo-wide is a
separate decision.
* fix(scaffold): emit README, LICENSE and complete package metadata
Caught by local CI: check_metadata.py and check_readmes.py failed on the new
catalog module with 5 violations — missing README.md, and missing readme /
license / keywords / project.urls in pyproject.toml.
The cause is a fourth scaffolder gap: make new-module emitted neither a README
nor a LICENSE, and its pyproject template omitted the metadata both checkers
require. Every scaffolded module therefore failed 'make lint' until an author
wrote those by hand.
Fixes the catalog module and the templates behind it. The generated README
satisfies check_readmes.py as-is (H1, Install and Usage sections, >=500 bytes)
while clearly marking the parts an author should replace.
Regression tests assert a scaffolded module passes both checkers.
* revert: remove the catalog module
The catalog module was a debugging and benchmarking fixture, not something to
ship. Removes modules/catalog, its migration, tests/loadtest/seed_catalog.py,
the locust catalog tasks and the loadtest-seed-catalog target, and unregisters
it from the workspace, host deps, testpaths, ty paths and the CI module list.
Every performance fix stays — none depended on the module: response
compression, pre-compressed brotli assets, chunk grouping, the asset base-path
fix, canonical menu URLs, error-page shared props, concurrent worker probes
and the Postgres skip scan.
The benchmark harness stays too, repointed from /catalog/ to /audit_log/ — a
real list page backed by 100k rows, so the measurements still run against
realistic data. The catalog list->detail drill-down test was dropped rather
than faked against another module; it relied on data-testid hooks that only
existed in the catalog pages.
The four make new-module scaffolder fixes also stay. They were found because
this module was scaffolded, but they are defects in the scaffolder and affect
every module anyone creates.
Verified after removal: 1461 python tests, 41 js tests, 10/10 perf suite,
lint/typecheck/metadata/readme checks all clean, and 'make doctor' back to its
original 1 pre-existing error (SM020) with no catalog warnings.
* fix: restore package-lock.json to origin/main
Removing the catalog module, I regenerated the lockfile with 'rm -f
package-lock.json && npm install' rather than letting npm prune the workspace
entry. That rewrote it wholesale — 1138 deletions, 0 insertions — which would
have changed what 'npm ci' installs in CI for reasons unrelated to this
branch.
origin/main's lockfile never referenced catalog (the module only ever existed
on this branch), so the correct state is simply origin/main's file. Now
byte-identical to it; 'npm ci' accepts it and the build and JS tests pass.1 parent 76f2cb1 commit dabc56e
51 files changed
Lines changed: 5630 additions & 157 deletions
File tree
- .github/workflows
- docs
- perf
- superpowers
- plans
- specs
- framework
- core
- hosting
- simple_module_hosting
- tests
- host/client_app
- modules
- audit_log
- audit_log
- tests
- background_tasks
- background_tasks
- tests
- branding/branding
- feature_flags
- feature_flags
- file_storage
- file_storage
- permissions
- settings/settings
- users
- scripts
- tests
- tests
- benchmarks
- perf
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
209 | 209 | | |
210 | 210 | | |
211 | 211 | | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
212 | 304 | | |
213 | 305 | | |
214 | 306 | | |
| |||
252 | 344 | | |
253 | 345 | | |
254 | 346 | | |
| 347 | + | |
255 | 348 | | |
256 | 349 | | |
257 | 350 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
70 | 70 | | |
71 | 71 | | |
72 | 72 | | |
73 | | - | |
| 73 | + | |
74 | 74 | | |
75 | 75 | | |
76 | 76 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
52 | 52 | | |
53 | 53 | | |
54 | 54 | | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
55 | 64 | | |
56 | 65 | | |
57 | 66 | | |
| |||
77 | 86 | | |
78 | 87 | | |
79 | 88 | | |
| 89 | + | |
80 | 90 | | |
81 | 91 | | |
82 | 92 | | |
| |||
0 commit comments