Conversation
no ref
Config is fully loaded by the time loadNconf() returns, and nothing in Ghost
writes to it afterwards, but nconf doesn't know that. Every config.get()
walks all nine stores, and for an object-valued key it collects a hit from
each one and deep-merges them - on every single call. There are 353 get call
sites in core, some on per-request paths.
freeze() makes the instance read-only and memoises get() by key. Because each
cached value is whatever nconf itself returned for that key, a frozen lookup
can't disagree with an unfrozen one, and with writes rejected a cache entry
can't go stale - so there's no invalidation to get wrong. Measured against the
real config: get('database') 3526ns -> 15ns, getSiteUrl() 1276ns -> 63ns.
The mutators throw rather than no-op. nconf's own readOnly flag would have
been the obvious lever, but Provider._execute *skips* read-only stores for a
destructive action and returns undefined, which turns a config write into a
silent failure instead of a loud one.
loadNconf freezes as its last step rather than leaving it to boot, so a write
during boot fails loudly instead of quietly working. Nothing in the tree does
that today - the last two runtime writes were the asset hash, moved into the
asset hash service - so this holds the line rather than changing behaviour.
It's skipped under test, where the suites rewrite config between cases on
purpose, and optimization.freezeConfig turns it off entirely.
A keyless get() is left uncached. nconf's env store holds the whole
environment, so the merged tree materialises it - on a boot here, 139 of 194
top-level keys came from env rather than config - and caching that would mean
a long-lived object holding every environment variable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011s9ZdgxjVXMr7YTh4UtmtH
no ref optimization.freezeConfig was added as a kill switch for freezing config, on the grounds that freezing makes config throw in production. On reflection it earns its config surface: nothing in the tree writes to config after load, so the flag only exists to re-enable a behaviour we don't want back, and a config key to control config's own loading is an awkward thing to reason about. defaults.json is now untouched by this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s9ZdgxjVXMr7YTh4UtmtH
…ects
no ref
Two problems with the freeze helper, both found in review.
required() was in the mutator list, but it isn't one: nconf implements it as a
read - it calls get() for each key and throws when one is missing. Blocking it
would have made post-load config validation throw "Config is frozen" in every
non-test environment. It's out of the list, with a test covering both the
passing and missing-key cases while frozen.
Object-valued reads were cached and handed back by reference, so a caller that
mutated what it got rewrote the cache for every later reader. configure() in
data/db/connection.js does exactly that - it assembles the knex config by
mutating the object it's passed, which is config.get('database'). That made the
two ways of reading a key disagree:
get('database').pool -> {} (whatever knex bootstrap bolted on)
get('database:pool') -> undefined (no store has it)
Cached values are now deep-frozen, so a mutation raises a TypeError naming the
site instead of silently corrupting config, and configure() clones before
mutating. The clone is worth having on its own: mutating the object config
handed back was already writing through to nconf's stores for nested keys,
since nconf's merge shares subtrees by reference.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011s9ZdgxjVXMr7YTh4UtmtH
no ref Repeated frontend translations were reparsing identical ICU messages. Memoize successful formatters by locale and resolved translation content, clear them on theme initialization, and bound the cache to 5,000 entries.
no ref - lazily-construct moment timeNow to avoid moment creation on every render - memoize Intl.Locale maximization
no ref The `t` and `navigation` theme helpers call `labs.isSet()` on every render, and each call rebuilt the whole flag object: a lodash deep clone of the `labs` setting plus a key-by-key overlay of the GA, remote-override and config layers. A CPU profile of the Pro image under a 5,000-request frontend load put ~0.8s (0.5% of busy time) in this path. Caching the result is the wrong fix — labs derives from the settings cache, and module-scoped copies of database state block running several containers per site — so the computation itself is now cheap instead: - `getAll()` shallow-copies the setting. The `labs` value is a flat map of booleans (allowlist-validated on every write path) and the settings cache parses it fresh on each `get`, so a shallow copy is all that is needed to keep callers from mutating shared state. - `isSet()` checks the layers in precedence order for the one key it was asked about instead of building the whole object, so the hot helper call allocates nothing beyond the `config.get` lookup. `labs-flag-overrides` gains a copy-free single-flag accessor for that path. The theme-engine middleware test stubbed only `getAll()` and relied on `isSet()` delegating to it, so it now fakes `isSet()` from the same data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
no ref
Several profile follow-ups memoise a pure derivation of an immutable input:
compiled ICU messages by locale and text, locale candidate lists, parsed NQL
filter trees, an Intl formatter per timezone, the parsed site URL, and lazily
required modules on the request path. Each of those grows its own Map, with its
own (usually absent) bound and no shared way to clear it.
One helper gives them one bound, one kill switch and one test reset, and can
later serve packages such as url-utils once they are pulled in-repo:
- once(compute) computes on first call and returns the same value afterwards,
which is the shape the lazy requires want.
- memoize(compute, key, {max}) is a keyed memo backed by lru-cache, bounded at
500 entries by default and required to be finite. No TTL: entries are pure
derivations of immutable inputs, so an entry is never stale, only evicted.
Errors thrown by compute are not memoised.
- configure({enabled}) is the kill switch, called once at boot from the new
optimization.memoize config key; nothing else reads config from inside the
package.
- resetAll() clears every memo, called from configUtils.restore so tests that
rewrite config or swap modules in the require cache are unaffected.
This is a memo helper, not a cache. Nothing derived from the settings cache,
the URL map or a database row goes through it — that is mutable state and needs
real invalidation, which this deliberately does not have.
The first two consumers are the lazy requires in link-replacer and
generate-excerpt, which also prove the require(esm) path from CommonJS core.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K4KZATm5a3QU1xBM4Zntg4
no ref `memoize` statically imports lru-cache, and `once` has no use for it. Because both lived in one module, every `once` consumer loaded lru-cache — including boot.js, which calls `configure` before it has loaded anything else, so the package put lru-cache on Ghost's critical boot path for a helper that never touches it. lru-cache 11 is dual-format, so the ESM copy imported here is a separate instance from the CommonJS copy glob/path-scurry already loads; the cost was additive, not shared. Splitting `once` and `memoize` into their own modules, with the shared enabled flag and registry in a third, gives `@tryghost/memoize/once` with nothing behind it. Measured from ghost/core in a cold process: 6.8ms and 183KB through /once against 27.9ms and 1094KB through the root. Splitting rather than lazily importing lru-cache inside `memoize` keeps both entry points free of top-level await, which require(esm) needs. Both entry points still share one enabled flag and one registry, so `configure` and `resetAll` cover every memo in the process whichever entry point created it. A test walks the module graph from src/once.ts and fails if anything reachable from it grows an external dependency. Core's boot, the two lazy-require consumers and the test config helper all use /once; nothing in Ghost currently needs the root entry point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4KZATm5a3QU1xBM4Zntg4
no ref Ghost lazily requires a number of heavyweight libraries inside the functions that use them, to keep them off the boot path. That works, but it pays a require-cache lookup on every call, and spam-prevention.js in particular had grown 31 copies of the same two requires across its 16 rate-limiter factories. Wrapping each one in once() from @tryghost/memoize/once keeps the load lazy — the module is still not required until the first call — while collapsing the repeat lookups into a single hoisted loader per module per file. Converted 47 call sites across 12 files: - express-brute and @tryghost/brute-knex in spam-prevention (31 sites) - cheerio/slim in mentions, oembed, the email renderer and the RSS feed (6) - @extractus/oembed-extractor in the oembed service and Twitter provider (3) - @tryghost/image-transform in the lexical and mobiledoc card factories (3) - juice in the email renderer and staff emails (2) - sharp in the gift preview renderer (2) The rule applied: convert a lazy require when it loads a third-party library AND the enclosing function runs more than once. That deliberately leaves alone the lazy requires of internal modules — those mostly exist to break circular dependencies rather than to defer cost, and caching a module reference at module scope is a riskier change than it looks for code that tests swap in the require cache. It also leaves alone the single-site requires in boot, service init and CLI paths, which run once anyway. Every consumer imports the /once entry point, so none of them loads lru-cache. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K4KZATm5a3QU1xBM4Zntg4
…into claude/perf-integration-benchmark
…g' into claude/perf-integration-benchmark
…integration-benchmark
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many -t test:types -p ghost,@tryghost/me... |
❌ Failed | 5s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 4m 54s | View ↗ |
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 8m 20s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 17s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 4m 9s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 14s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 37s | View ↗ |
nx run @tryghost/koenig-lexical:test:acceptance |
✅ Succeeded | 2m 22s | View ↗ |
Additional runs (11) |
✅ Succeeded | ... | View ↗ |
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗.
☁️ Nx Cloud last updated this comment at 2026-09-18 02:27:35 UTC
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #30892 +/- ##
==========================================
+ Coverage 67.81% 67.84% +0.02%
==========================================
Files 1682 1683 +1
Lines 60767 60858 +91
Branches 10500 10513 +13
==========================================
+ Hits 41209 41288 +79
- Misses 17234 17239 +5
- Partials 2324 2331 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
no ref
AdapterCacheRedis folds the adapter's `ttl` into `clusterConfig.options` by
assigning into the object it was handed. That object is the adapter's slice of
config itself — resolveAdapterOptions returns the config subtree directly, and
its other branch only shallow-copies, so `clusterConfig` is the config's own
object either way. Once config is frozen the assignment throws:
TypeError: Cannot assign to read only property 'ttl' of object '#<Object>'
at new AdapterCacheRedis (AdapterCacheRedis.js:44)
at AdapterManager.getAdapter (adapter-manager.ts)
at core/server/lib/image/index.js:11
which kills Ghost during init. Found by booting the Pro image: freeze is skipped
under NODE_ENV=testing so CI never sees it, and config.development.json
configures no adapters at all, so a development boot never constructs this
adapter either. It needs a Redis cache adapter with clusterConfig — which is
production Pro, and nothing else.
The ttl is now folded into a derived clusterConfig rather than assigned into
config. That is worth doing on its own: the old write went through into the
subtree every later reader of `adapters:...` sees, and changed the key the
adapter manager builds by stringifying this same config.
The three new tests freeze their input, because freeze is skipped under test and
without it the mutation passes unnoticed. Two of them fail against the old
constructor; the third guards the path where no ttl is configured.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K4KZATm5a3QU1xBM4Zntg4
…egration-benchmark
Member
Author
|
Closing — this served its purpose as a benchmark vehicle and was never for merging. Result: the six combined PRs came in at −11.5% CPU on the Moya Pro benchmark for a fixed 5,000-request frontend workload (two pairs: −13.3%, −9.6%; arms non-overlapping). Attribution went to #30722 (nconf freeze) and #30704 (theme translation cache); written up on both. It also caught a boot-breaking bug that CI could not: #30722's config freeze made The branch Generated by Claude Code |
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
2 participants
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.
Why are you making it?
Each of the in-flight performance PRs is individually at or below the benchmark's noise floor, so none of them can be measured alone. This branch merges them so the aggregate can be measured in one run.
Measured from the last 20 scheduled runs on
ghost-perf:load.cpu_shas a 4.2% noise floor (pooled within-Ghost-version sd, so it is measurement noise rather than code drift). The expected aggregate effect is ~3% of busy CPU at best — still under that, so even combined this is not resolvable from the headline CPU number in a single A/B pair. The run is worth doing for the profile and require_cost lanes, which report CPU self-time and resident memory by package and are therefore attributions rather than comparisons.What does it do?
Nothing of its own. It is
mainat56c5bd9plus three merges, all conflict-free:claude/labs-helper-perf-5924ceclaude/nconf-config-freeze-ay169gclaude/convert-lazy-requires-to-onceonce()+ #30886@tryghost/memoizeNo conversion work was done here — the PRs are merged exactly as they stand, so the branch measures what is actually up for review.
Why is this something Ghost users or developers need?
It isn't, directly. It is a measurement vehicle. The matching baseline is
mainat the merge base56c5bd9, run on the same runner.Verification
main).GHOST_CI_SHUTDOWN_AFTER_BOOT=1): exit 0,Ghost booted in 3.08s, zero frozen-config writes, zeroTypeErrors. That matters because Added a freeze helper that makes config read-only once loaded #30722's freeze is skipped underNODE_ENV=testing, so CI never exercises it — I confirmed separately thatconfig.set()does throw after load in development, so the frozen path was genuinely live during that boot.🤖 Generated with Claude Code
https://claude.ai/code/session_01K4KZATm5a3QU1xBM4Zntg4
Generated by Claude Code