perf: cut cold-load FCP 70% and audit-log navigation 57% - #232
Merged
Conversation
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.
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.
…o-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.
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%.
… 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.
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.
…ions 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.
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.
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.
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.
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.
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.
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.
.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.
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.
Deploying simple-module-python with
|
| Latest commit: |
197ec1a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2bc0808.simple-module-python.pages.dev |
| Branch Preview URL: | https://features-perf-2inxd.simple-module-python.pages.dev |
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.
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.
antosubash
added a commit
that referenced
this pull request
Aug 3, 2026
#232 merged with the lint job overridden, bringing one new unformatted doc onto main. Formats it so `ruff format --check` is clean against the merged tree. Verified zero prose lines changed. Claude-Session: https://claude.ai/code/session_01TtYUkaUAJmUcCwB5QGxPqN
antosubash
added a commit
that referenced
this pull request
Aug 3, 2026
* style: format Python code blocks in Markdown (ruff 0.16) ruff 0.16 began formatting Python code blocks inside Markdown files. `ruff>=0.8` is unpinned, so CI picked the behaviour up and `make ci-python-lint` started failing on 74 documentation files the formatter had never touched before. Changes are confined to ```python fences: verified that zero prose lines changed across all 74 files (every added/removed non-blank line falls inside a python fence, using CommonMark fence rules). Also fixes 2 RUF036 errors (`None` not at the end of a type union) in modules/settings/settings/contracts/accessor.py, surfaced by the same ruff bump and independently failing the lint job. Annotation order only; runtime behaviour is unchanged and the settings suite (112 tests) passes. Claude-Session: https://claude.ai/code/session_01TtYUkaUAJmUcCwB5QGxPqN * style: format Python blocks in markdown added by #232 #232 merged with the lint job overridden, bringing one new unformatted doc onto main. Formats it so `ruff format --check` is clean against the merged tree. Verified zero prose lines changed. Claude-Session: https://claude.ai/code/session_01TtYUkaUAJmUcCwB5QGxPqN
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.
Summary
Measured navigation performance end-to-end, then fixed what the measurements
actually pointed at. All four of the hypotheses this started with were dropped
on evidence; every fix here was found by measuring, not by guessing.
Cold page load (4 Mbps / 40 ms throttled, production build):
−70% FCP, −75% transfer, −76% requests.
Navigation (Postgres: 10k users / 100k audit rows):
What was actually wrong
GZipMiddlewarewas never installed — the built CSS shipped as 139 KB instead of 21 KB. Asset caching was correct; compression simply didn't exist./audit_log→/audit_log/), costing a full round trip on every navigation. Nothing caught it because everything still rendered.basedefaulted to/while the host serves under/static/dist/, so every lazy page load fired 404s and MIME errors. The real import resolved relatively, so no test failed and no page broke.render_error_pageskippedinertia.share(), so a 404 showedhost.error.not_found_titleinstead of "Page Not Found"./admin/background-tasks/workerstook 4.1 s. Four sequential Celeryinspectbroadcasts, each waiting a full 1 s timeout — worst exactly when an admin opens it because workers are down. Now concurrent: 1.02 s.SELECT DISTINCTscaled with rows. Postgres skip scan for the audit-log filter dropdown: 5.26 ms → 0.48 ms (11×).Also included
/audit_log/and/dashboard/.make new-modulebugs fixed. Scaffolded modules could not boot (wrong dependency name, empty wheel, missingsettings.py) and could not passmake lint(no README/LICENSE/metadata). Each has a regression test.Note on the
catalogmoduleEarlier revisions of this branch added a
catalogsample module as thebenchmark fixture. It was a debugging aid and has been removed — the
measurements above were taken with it installed, but every fix is framework-
or build-level and none depended on it. The harness now runs against
/audit_log/, a real list page backed by 100k rows. See §13 of the perf doc.Verification
ty,biome,tsc, file-size, hardcoded-string, metadata and README checks all greennpm ciaccepts the lockfilemake doctorback to its original 1 pre-existing error (SM020), no new warningsSix regression guards protect these wins, because every one is invisible to a normal test — the app renders correctly either way, just slower: compression ratio, cold-load request count, menu-URL canonicality, asset-path integrity, dialect branch selection, and worker-probe concurrency.
Notes for the reviewer
docs/perf/2026-08-02-baseline.mdhas the full method and every number, in 13 parts. Part 6 explicitly corrects Parts 1–3: the initial SQLite numbers badly understated server cost, and I left that correction visible rather than quietly rewriting it.reltuples, and apg_trgmindex for search (needs a Postgres-only migration).ruff>=0.8is unpinned and now breaks CI. "Python lint & format" fails on 68 markdown files — ruff 0.16.x formats Python code blocks inside markdown, which older versions did not.mainhas 74 such files; this branch has 68 only because 6 were incidentally reformatted. All 519.pyfiles are clean. This is pre-existing and unrelated to this branch, but it does block the merge — worth its own PR, or pin ruff.Test plan
/audit_log/, confirms the page works and the network tab is clean