perf: Add a reusable promise cache abstraction - #2230
Open
jeswr wants to merge 4 commits into
Open
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
Many of the performance PRs introduce ad-hoc caching, repeating the same "look up a cached promise, otherwise compute and cache it" logic. This adds a small shared abstraction under src/util/caching so those PRs stop re-writing similar code: - PromiseCache<TKey, TValue>: caches the promise (not just the resolved value) so concurrent callers deduplicate onto a single in-flight computation. It is backed by an injected Map or WeakMap, letting each call site pick process-lifetime caching of a bounded key space (Map) or memory-safe caching keyed on object identity (WeakMap). Entries whose promise rejects are evicted by default so transient failures are retried. - cached(...): a method decorator wrapping a PromiseCache so adoption is a single, minimally invasive annotation. Each instance gets its own cache; the key defaults to the first argument and can be derived via `key`, and `weak` opts into a WeakMap for request-scoped object keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The added JSDoc/comments were more verbose than the surrounding util source; condense to the load-bearing facts (promise cached for concurrent dedup, store choice sets entry lifetime, per-instance decorator caches) and drop the type-parameter tags the codebase does not use. No behaviour change. Model: claude-fable-5 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 tasks
jeswr
marked this pull request as ready for review
August 23, 2026 09:06
jeswr
marked this pull request as draft
August 23, 2026 09:07
Copy link
Copy Markdown
There was a problem hiding this comment.
Pull request overview
Adds reusable promise-based caching utilities for asynchronous computations, including Map/WeakMap support and a per-instance decorator.
Changes:
- Adds PromiseCache with rejection eviction.
- Adds the cached decorator with custom key derivation.
- Exports utilities and adds focused unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Summary |
|---|---|
| test/unit/util/caching/PromiseCache.test.ts | Promise cache tests; no issues noted. |
| test/unit/util/caching/CachedDecorator.test.ts | Decorator tests; no issues noted. |
| src/util/caching/PromiseCache.ts | Moderate issue: avoid wrapping cached promises in a new async promise (3 votes). |
| src/util/caching/CachedDecorator.ts | Moderate issue: remove the unnecessary async wrapper to preserve promise identity (3 votes). |
| src/index.ts | Public exports; no issues noted. |
src/index.ts:531
- These exports add a new public API, so they are a feature-level change. The description says the intended semver level is minor, while the checked checklist says this is a patch change; please align the semver label and release targeting with the actual API addition.
export * from './util/caching/CachedDecorator'; export * from './util/caching/PromiseCache';
src/index.ts:531
- These lines add a new public minor API, but RELEASE_NOTES.md is unchanged while the PR checklist marks the release-notes item complete. Please add the utility to the upcoming release notes (or correct the checklist/description) so this exported feature is discoverable to consumers.
export * from './util/caching/CachedDecorator'; export * from './util/caching/PromiseCache';
src/util/caching/PromiseCache.ts:45
- The rejection handler deletes by key without checking that this promise is still the current entry. If the injected store evicts or replaces the key while this computation is pending, an old rejection can delete the newer promise and force an unnecessary recomputation. Only delete when this.store.get(key) === promise.
promise.catch((): void => {
this.store.delete(key);
});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Comment thread
src/util/caching/CachedDecorator.ts
Outdated
Comment thread
src/util/caching/PromiseCache.ts
Outdated
jeswr
marked this pull request as ready for review
August 24, 2026 00:35
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.
📁 Related issues
Addresses #2229.
✍️ Description
Adds two small asynchronous caching utilities:
computations. Its injected Map/WeakMap-compatible store determines entry
lifetime, and rejected promises are evicted by default.
It supports custom key derivation and WeakMap storage for object keys.
The implementation is exported through src/index.ts. This PR introduces the
shared abstraction only; performance measurements belong in changes that
adopt it on an existing hot path.
The utilities are used by these PRs on my fork:
uses PromiseCache to cache compiled EJS and Handlebars templates by path.
uses WeakMap-backed promise caches to deduplicate effective ACL and store
lookups by ResourceIdentifier.
Both PRs are stacked on this work and should be rebased after it lands.
Local verification on current versions/next-major:
covering promise identity, concurrent deduplication, rejection behavior and
eviction races, Map/WeakMap storage, key derivation, and per-instance
isolation
The intended semver level is minor.
✅ PR check list
Before this pull request can be merged, a core maintainer will check whether