-
Notifications
You must be signed in to change notification settings - Fork 951
perf: cache getSiteSetting() per request + document perf patterns #663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "emdash": patch | ||
| --- | ||
|
|
||
| Cache `getSiteSetting(key)` per-request. It was firing an uncached `options` table read on every call, so templates that pull several settings (or `EmDashHead` reading `seo` on every page render) paid N round-trips to the D1 primary instead of sharing one. Noticeable on colos far from the primary — APS/APE were seeing ~30–100 ms of avoidable warm-render latency per page. | ||
|
|
||
| Wraps each key in `requestCached("siteSetting:${key}", ...)` so concurrent callers in a single render share the in-flight query. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -296,6 +296,56 @@ Handlers in `api/handlers/*.ts` contain business logic. Routes should be thin wr | |||||
| - Entire body wrapped in try/catch. Errors return `{ success: false, error: { code, message } }`. | ||||||
| - Error codes are `SCREAMING_SNAKE_CASE`: `NOT_FOUND`, `VALIDATION_ERROR`, `CONTENT_CREATE_ERROR`, etc. | ||||||
|
|
||||||
| ### Performance: caching and query patterns | ||||||
|
|
||||||
| EmDash runs on D1 with the Sessions API. Anonymous reads go to the nearest replica; writes and authenticated reads route to the primary. The primary is thousands of miles from some CF colos -- every round-trip matters, especially on cold isolates. | ||||||
|
|
||||||
| A few rules and patterns cover 90% of the footguns. | ||||||
|
|
||||||
| **Always add requestCached to query helpers called from templates.** Page-level template code runs inside the ALS request context, so the per-request cache (`src/request-cache.ts`) deduplicates identical calls within a single render. A single un-cached helper called from three widgets turns into three primary-routed reads on a page that should have made one. Rule of thumb: if a helper takes stable arguments (slug, key, entry ID) and can be called from multiple components, wrap it. | ||||||
|
|
||||||
| ```typescript | ||||||
| // WRONG — every caller re-queries | ||||||
| export async function getSiteSetting(key: string) { | ||||||
| const db = await getDb(); | ||||||
| return db.selectFrom("options").where("name", "=", key)... | ||||||
| } | ||||||
|
|
||||||
| // RIGHT — shared within one render | ||||||
| export function getSiteSetting(key: string) { | ||||||
| return requestCached(`siteSetting:${key}`, async () => { | ||||||
| const db = await getDb(); | ||||||
| return ...; | ||||||
| }); | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| The cache key must include every argument that changes the result. Missing an argument means wrong values get served; including too much just means more cache misses. | ||||||
|
|
||||||
| `requestCached` caches the _promise_, so concurrent callers share the in-flight query. On error the entry is deleted so the next call retries. | ||||||
|
|
||||||
| **Module-scope singletons must live on `globalThis`.** Vite duplicates modules across chunks during SSR bundling. A plain `let cache: X | null = null` in a module becomes _two_ variables if two chunks inline the module -- defeating the singleton. Use a `Symbol.for` key on `globalThis`, as `request-context.ts` does. See also `packages/core/src/bylines/index.ts` (`bylinesHolder`) for the pattern applied to a boolean cache. The fix cut ~2 cold-start queries per D1 isolate. | ||||||
|
||||||
| **Module-scope singletons must live on `globalThis`.** Vite duplicates modules across chunks during SSR bundling. A plain `let cache: X | null = null` in a module becomes _two_ variables if two chunks inline the module -- defeating the singleton. Use a `Symbol.for` key on `globalThis`, as `request-context.ts` does. See also `packages/core/src/bylines/index.ts` (`bylinesHolder`) for the pattern applied to a boolean cache. The fix cut ~2 cold-start queries per D1 isolate. | |
| **Module-scope singletons must live on `globalThis`.** Vite duplicates modules across chunks during SSR bundling. A plain `let cache: X | null = null` in a module becomes _two_ variables if two chunks inline the module -- defeating the singleton. Use a `Symbol.for` key on `globalThis`, as `packages/core/src/request-context.ts` and `packages/core/src/request-cache.ts` do. The fix cut ~2 cold-start queries per D1 isolate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AGENTS.md refers to the per-request cache as
src/request-cache.ts, but the implementation lives atpackages/core/src/request-cache.ts(and is imported throughout core as../request-cache.js). Updating the path here will make the guidance easier to follow.