perf: resolve decofile state/revision synchronously when already resolved#1089
Open
hugo-ccabral wants to merge 1 commit intomainfrom
Open
perf: resolve decofile state/revision synchronously when already resolved#1089hugo-ccabral wants to merge 1 commit intomainfrom
hugo-ccabral wants to merge 1 commit intomainfrom
Conversation
…lved Both state() and revision() in fs.ts and fetcher.ts used .then() on already-resolved promises, which schedules unnecessary microtasks. Cache resolved values synchronously so subsequent reads return Promise.resolve(cachedValue) without scheduling a microtask through .then(). Called 50+ times per page load via resolve() in mod.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe PR introduces caching mechanisms in decofile management to reduce asynchronous operations. In Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
engine/decofile/fetcher.ts (1)
249-256:onChangefallback discards the innerDisposable.When
resolvedProvideris not yet set,decofileProviderPromise.then((r) => r.onChange(cb))discards theDisposablereturned by the inner provider, so the caller's no-op[Symbol.dispose]can never actually unsubscribe the callback. This is a pre-existing issue not introduced by this PR, but it's worth tracking: the resurrection bug above can cause callbacks registered via this path to linger even after the object is disposed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/decofile/fetcher.ts` around lines 249 - 256, The onChange fallback currently discards the Disposable from decofileProviderPromise; change it to capture and forward that inner Disposable so callers can unsubscribe: when resolvedProvider is falsy, call decofileProviderPromise.then(r => { const inner = r.onChange(cb); store inner in a local variable }); return a Disposable whose [Symbol.dispose] calls the stored inner?.[Symbol.dispose]() (and guard for it being set later), so both resolvedProvider.onChange and the promise-based path return disposables that actually unsubscribe; reference onChange, resolvedProvider, decofileProviderPromise and [Symbol.dispose].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/decofile/fetcher.ts`:
- Around line 229-268: The promise resolution currently assigns resolvedProvider
unconditionally which can resurrect a provider after dispose; add a local
boolean flag (e.g., isDisposed = false) and in the decofileProviderPromise.then
callback check isDisposed before assigning resolvedProvider—if isDisposed then
call r?.dispose?.() and skip caching; in the returned dispose() implementation
set isDisposed = true, null out resolvedProvider and perform the existing
cleanup so the then-handler will not cache a disposed provider. Ensure
references to resolvedProvider/state/revision/set/onChange continue to work with
this guard.
---
Nitpick comments:
In `@engine/decofile/fetcher.ts`:
- Around line 249-256: The onChange fallback currently discards the Disposable
from decofileProviderPromise; change it to capture and forward that inner
Disposable so callers can unsubscribe: when resolvedProvider is falsy, call
decofileProviderPromise.then(r => { const inner = r.onChange(cb); store inner in
a local variable }); return a Disposable whose [Symbol.dispose] calls the stored
inner?.[Symbol.dispose]() (and guard for it being set later), so both
resolvedProvider.onChange and the promise-based path return disposables that
actually unsubscribe; reference onChange, resolvedProvider,
decofileProviderPromise and [Symbol.dispose].
Comment on lines
+229
to
268
| let resolvedProvider: DecofileProvider | null = null; | ||
| decofileProviderPromise.then((r) => { | ||
| resolvedProvider = r; | ||
| }); | ||
|
|
||
| return { | ||
| set(state, revision) { | ||
| if (resolvedProvider) return resolvedProvider.set?.(state, revision) ?? Promise.resolve(); | ||
| return decofileProviderPromise.then((r) => r?.set?.(state, revision)); | ||
| }, | ||
| notify() { | ||
| if (resolvedProvider) return resolvedProvider.notify?.() ?? Promise.resolve(); | ||
| return decofileProviderPromise.then((r) => | ||
| r?.notify?.() ?? Promise.resolve() | ||
| ); | ||
| }, | ||
| state: (options) => decofileProviderPromise.then((r) => r.state(options)), | ||
| state: (options) => { | ||
| if (resolvedProvider) return resolvedProvider.state(options); | ||
| return decofileProviderPromise.then((r) => r.state(options)); | ||
| }, | ||
| onChange: (cb) => { | ||
| if (resolvedProvider) return resolvedProvider.onChange(cb); | ||
| decofileProviderPromise.then((r) => r.onChange(cb)); | ||
| return { | ||
| [Symbol.dispose]: () => { | ||
| }, | ||
| }; | ||
| }, | ||
| revision: () => decofileProviderPromise.then((r) => r.revision()), | ||
| revision: () => { | ||
| if (resolvedProvider) return resolvedProvider.revision(); | ||
| return decofileProviderPromise.then((r) => r.revision()); | ||
| }, | ||
| dispose: () => { | ||
| decofileProviderPromise.then((r) => r?.dispose?.()); | ||
| if (resolvedProvider) { | ||
| resolvedProvider.dispose?.(); | ||
| } else { | ||
| decofileProviderPromise.then((r) => r?.dispose?.()); | ||
| } | ||
| resolvedProvider = null; | ||
| delete decofileCache[endpoint]; |
There was a problem hiding this comment.
dispose() before promise resolution resurrects a disposed resolvedProvider.
The .then that populates resolvedProvider (line 230) is registered before dispose() can register its cleanup .then. Microtask ordering therefore guarantees:
resolvedProvider = rfires → live reference is cached.r?.dispose?.()fires → provider is disposed.
resolvedProvider is left pointing to the disposed provider, so any call to state(), revision(), set(), etc. on the returned object after dispose() will route through the disposed instance.
🐛 Proposed fix — guard the cache-setter against post-dispose execution
let resolvedProvider: DecofileProvider | null = null;
+ let disposed = false;
decofileProviderPromise.then((r) => {
- resolvedProvider = r;
+ if (!disposed) {
+ resolvedProvider = r;
+ }
});
// … inside dispose: …
dispose: () => {
+ disposed = true;
if (resolvedProvider) {
resolvedProvider.dispose?.();
} else {
decofileProviderPromise.then((r) => r?.dispose?.());
}
resolvedProvider = null;
delete decofileCache[endpoint];
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/decofile/fetcher.ts` around lines 229 - 268, The promise resolution
currently assigns resolvedProvider unconditionally which can resurrect a
provider after dispose; add a local boolean flag (e.g., isDisposed = false) and
in the decofileProviderPromise.then callback check isDisposed before assigning
resolvedProvider—if isDisposed then call r?.dispose?.() and skip caching; in the
returned dispose() implementation set isDisposed = true, null out
resolvedProvider and perform the existing cleanup so the then-handler will not
cache a disposed provider. Ensure references to
resolvedProvider/state/revision/set/onChange continue to work with this guard.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="engine/decofile/fetcher.ts">
<violation number="1" location="engine/decofile/fetcher.ts:230">
P2: `resolvedProvider` can be repopulated after `dispose()` if the promise resolves later, leaving a stale disposed provider cached. Guard the `.then` assignment with a disposed flag (or clear in the dispose promise) so the cached provider cannot be restored after disposal.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
|
|
||
| let resolvedProvider: DecofileProvider | null = null; | ||
| decofileProviderPromise.then((r) => { |
There was a problem hiding this comment.
P2: resolvedProvider can be repopulated after dispose() if the promise resolves later, leaving a stale disposed provider cached. Guard the .then assignment with a disposed flag (or clear in the dispose promise) so the cached provider cannot be restored after disposal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At engine/decofile/fetcher.ts, line 230:
<comment>`resolvedProvider` can be repopulated after `dispose()` if the promise resolves later, leaving a stale disposed provider cached. Guard the `.then` assignment with a disposed flag (or clear in the dispose promise) so the cached provider cannot be restored after disposal.</comment>
<file context>
@@ -225,26 +225,46 @@ export const fromEndpoint = (endpoint: string): DecofileProvider => {
);
+
+ let resolvedProvider: DecofileProvider | null = null;
+ decofileProviderPromise.then((r) => {
+ resolvedProvider = r;
+ });
</file context>
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
engine/decofile/fs.tsresolvedStateandresolvedRevisionas synchronous variablesset()/doUpdateState()is called), store the values directlystate()andrevision()returnPromise.resolve(cachedValue)instead ofdecofile.then(r => r.state)— avoids scheduling a microtask through.then()engine/decofile/fetcher.tsDecofileProviderreference once the promise completesstate(),revision(),set(), etc.) go directly to the resolved provider instead of.then()on the promiseContext
CPU profile showed decofile state/revision at ~0.79% of total CPU (0.19%
fs.ts state+ 0.13%fetcher.ts state+ 0.20%revision+ others). Called 50+ times per page load viaresolve()inmod.ts.Even when a Promise is already resolved, calling
.then()schedules a microtask in the event loop. Under 30k req/min with 50+ resolves per page, that's 1.5M+ unnecessary microtask schedulings per minute.Test plan
Summary by cubic
Cache the resolved decofile state, revision, and provider so reads return immediately without scheduling microtasks. This removes .then() on already-resolved promises in hot paths and reduces CPU overhead.
Written for commit 8f34400. Summary will update on new commits.
Summary by CodeRabbit