perf: cache getSiteSetting() per request + document perf patterns#663
Merged
Conversation
getSiteSetting(key) was firing an options-table read on every call, which meant EmDashHead's new per-render seo lookup (PR #613) added a D1 round-trip to every page. On APS/APE colos that's 30-100ms of avoidable warm-render latency. Wrapping in requestCached("siteSetting: ${key}", ...) dedupes concurrent callers and matches the pattern getSiteSettings() already uses. Also adds a "Performance: caching and query patterns" section to AGENTS.md covering requestCached, the globalThis+Symbol singleton pattern (bundler-safe), the anti-pattern of "has any" probes, after() for deferring bookkeeping, and the query-count harness. Everything I had to relearn today.
🦋 Changeset detectedLatest commit: f479526 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | f479526 | Apr 19 2026, 02:35 PM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
Improves render-path performance by deduplicating getSiteSetting(key) lookups within a single request, and adds internal documentation capturing common caching/query pitfalls and preferred patterns.
Changes:
- Wrap
getSiteSetting(key)inrequestCachedto avoid repeatedoptionstable reads per render. - Add a new “Performance: caching and query patterns” section to
AGENTS.md. - Add a changeset documenting the per-request caching behavior change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/settings/index.ts | Cache getSiteSetting(key) per-request via requestCached to reduce repeated DB reads. |
| AGENTS.md | Document caching/query-count patterns and operational guidance for perf-sensitive helpers. |
| .changeset/cache-get-site-setting.md | Patch changeset describing the performance fix and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| 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. |
There was a problem hiding this comment.
AGENTS.md refers to the per-request cache as src/request-cache.ts, but the implementation lives at packages/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.
Suggested change
| **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. | |
| **Always add requestCached to query helpers called from templates.** Page-level template code runs inside the ALS request context, so the per-request cache (`packages/core/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. |
|
|
||
| `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. |
There was a problem hiding this comment.
This sentence points to packages/core/src/bylines/index.ts and mentions a bylinesHolder globalThis singleton pattern, but that file currently has no bylinesHolder (or any globalThis/Symbol.for singleton cache). Either update the reference to a file that actually demonstrates the pattern (e.g. packages/core/src/request-context.ts / packages/core/src/request-cache.ts) or remove the example so the docs don’t send readers on a dead end.
Suggested change
| **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. |
Merged
ascorbic
added a commit
that referenced
this pull request
Apr 19, 2026
Adds peekRequestCache() to request-cache.ts so a narrower query can
opportunistically satisfy itself from a broader one already loaded.
getSiteSetting(key) now first peeks for the "siteSettings" batch
result (populated by getSiteSettings()) and reads the requested key
from there if present. Falls back to a per-key cached query if the
batch hasn't been loaded.
Net effect on the blog-demo layout: getSiteSettings() is already
called by Base.astro, so the EmDashHead getSiteSetting("seo") call
from PR #613 now costs zero extra queries instead of one
primary-routed round-trip per render. Query-count snapshot is
unchanged — the +1 queries PR #663 couldn't dedupe are now gone.
ascorbic
added a commit
that referenced
this pull request
Apr 19, 2026
Adds peekRequestCache() to request-cache.ts so a narrower query can
opportunistically satisfy itself from a broader one already loaded.
getSiteSetting(key) now first peeks for the "siteSettings" batch
result (populated by getSiteSettings()) and reads the requested key
from there if present. Falls back to a per-key cached query if the
batch hasn't been loaded.
Net effect on the blog-demo layout: getSiteSettings() is already
called by Base.astro, so the EmDashHead getSiteSetting("seo") call
from PR #613 now costs zero extra queries instead of one
primary-routed round-trip per render. Query-count snapshot is
unchanged — the +1 queries PR #663 couldn't dedupe are now gone.
8 tasks
ascorbic
added a commit
that referenced
this pull request
Apr 19, 2026
* perf: getSiteSetting(key) piggybacks on cached getSiteSettings()
Adds peekRequestCache() to request-cache.ts so a narrower query can
opportunistically satisfy itself from a broader one already loaded.
getSiteSetting(key) now first peeks for the "siteSettings" batch
result (populated by getSiteSettings()) and reads the requested key
from there if present. Falls back to a per-key cached query if the
batch hasn't been loaded.
Net effect on the blog-demo layout: getSiteSettings() is already
called by Base.astro, so the EmDashHead getSiteSetting("seo") call
from PR #613 now costs zero extra queries instead of one
primary-routed round-trip per render. Query-count snapshot is
unchanged — the +1 queries PR #663 couldn't dedupe are now gone.
* add changeset
0aveRyan
pushed a commit
to 0aveRyan/emdash
that referenced
this pull request
Apr 27, 2026
…dash-cms#663) getSiteSetting(key) was firing an options-table read on every call, which meant EmDashHead's new per-render seo lookup (PR emdash-cms#613) added a D1 round-trip to every page. On APS/APE colos that's 30-100ms of avoidable warm-render latency. Wrapping in requestCached("siteSetting: ${key}", ...) dedupes concurrent callers and matches the pattern getSiteSettings() already uses. Also adds a "Performance: caching and query patterns" section to AGENTS.md covering requestCached, the globalThis+Symbol singleton pattern (bundler-safe), the anti-pattern of "has any" probes, after() for deferring bookkeeping, and the query-count harness. Everything I had to relearn today.
0aveRyan
pushed a commit
to 0aveRyan/emdash
that referenced
this pull request
Apr 27, 2026
…-cms#664) * perf: getSiteSetting(key) piggybacks on cached getSiteSettings() Adds peekRequestCache() to request-cache.ts so a narrower query can opportunistically satisfy itself from a broader one already loaded. getSiteSetting(key) now first peeks for the "siteSettings" batch result (populated by getSiteSettings()) and reads the requested key from there if present. Falls back to a per-key cached query if the batch hasn't been loaded. Net effect on the blog-demo layout: getSiteSettings() is already called by Base.astro, so the EmDashHead getSiteSetting("seo") call from PR emdash-cms#613 now costs zero extra queries instead of one primary-routed round-trip per render. Query-count snapshot is unchanged — the +1 queries PR emdash-cms#663 couldn't dedupe are now gone. * add changeset
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
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.
What does this PR do?
Two things in one PR — a small fix and the docs update it should have come with the first time.
Fix: cache `getSiteSetting(key)` per request
`getSiteSetting` was firing an un-cached `options` table read on every call. PR #613 added a call from `EmDashHead.astro` (to surface `seo.googleVerification` / `seo.bingVerification` as meta tags), which means every page render now pays one extra D1 round-trip just for SEO settings.
On colos close to the primary (EUW/USE) that's ~10–20 ms. On APS/APE, where the primary is thousands of kilometres away, it's 30–100 ms of avoidable warm-render latency. Perf monitor data shows APS warm_mw stepped from ~90 ms → ~140–200 ms around the PR #613 deploy at 11:02 UTC today.
Wrapping each key in `requestCached("siteSetting:${key}", ...)` dedupes concurrent callers within a render. `getSiteSettings()` (plural) already uses the same pattern.
Docs: "Performance: caching and query patterns" in AGENTS.md
Captures what we've been rediscovering all day:
Type of change
Checklist
AI-generated code disclosure