Skip to content

Avoid redundant cloning on fresh session store loads#60595

Closed
efww wants to merge 1 commit intoopenclaw:mainfrom
efww:perf/260403-session-store-no-clone-clean
Closed

Avoid redundant cloning on fresh session store loads#60595
efww wants to merge 1 commit intoopenclaw:mainfrom
efww:perf/260403-session-store-no-clone-clean

Conversation

@efww
Copy link
Copy Markdown

@efww efww commented Apr 3, 2026

Summary

loadSessionStore(..., { skipCache: true }) already forces a fresh disk read, but it still returns structuredClone(store) afterward.

On this path, the parsed store is not shared with the cache, so the extra clone is unnecessary. This change returns the parsed store directly when skipCache is true, while preserving the defensive clone behavior for normal cached reads.

Why

Session store update paths currently re-read the full store from disk with skipCache: true, mutate one entry, and then write the full JSON back out. The full rewrite is still the dominant cost, but the extra clone adds avoidable O(n) overhead on the read side of that hot path.

Tests

  • corepack pnpm install --frozen-lockfile on Linux
  • corepack pnpm protocol:check on Linux
  • targeted src/config/sessions.test.ts regression run attempted locally; final pass/fail should come from GitHub CI

Note

  • This PR intentionally contains only the original session-store skipCache minimal change in src/config/sessions/store-load.ts and src/config/sessions.test.ts.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 3, 2026

Greptile Summary

Removes the redundant structuredClone on the skipCache: true path of loadSessionStore. When skipCache is set, the store is parsed directly from disk and never written into the in-memory object cache (writeSessionStoreCache is guarded by !opts.skipCache), so there is no shared reference to protect and the clone is unnecessary. The accompanying test verifies that mutating the returned store does not affect subsequent reads on either path.

Confidence Score: 5/5

This PR is safe to merge — the optimization is correct and the test validates the key aliasing invariant.

The skipCache: true path never writes the parsed store into the object cache (writeSessionStoreCache is gated on !opts.skipCache), and setSerializedSessionStore stores only the raw string. The returned store object is therefore not shared with anything else, making the removed structuredClone genuinely redundant. All remaining findings are P2 or lower.

No files require special attention.

Reviews (1): Last reviewed commit: "perf(sessions): avoid redundant clone on..." | Re-trigger Greptile

@efww ChatGPT Codex Connector
Copy link
Copy Markdown
Author

efww commented Apr 4, 2026

This PR is intentionally minimal and only changes 2 files on top of the latest main:

  • src/config/sessions/store-load.ts
  • src/config/sessions.test.ts

check is green on this PR.

The current failing check-additional signals appear to come from extension boundary guards outside this diff (for example extensions/irc/src/runtime-api.ts, extensions/imessage/src/test-plugin.ts, and extensions/slack/src/outbound-payload-harness.ts), and several other failures are timeout/cancellation shaped (build-artifacts, build-smoke, checks-fast-bundled, checks-fast-contracts-protocol, checks-fast-extensions-shard-*).

Given that this branch is a clean 2-file session-store change, I believe the PR itself should be evaluated separately from those repo-wide extension/timeout failures.

@steipete steipete mentioned this pull request Apr 4, 2026
Closed
@efww efww mentioned this pull request Apr 4, 2026
Closed
@BingqingLyu BingqingLyu mentioned this pull request Apr 28, 2026
Open
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 1, 2026

Thanks for the context here. I did a careful shell check against current main, and this is already implemented.

Close as implemented on main: current main and release v2026.4.29 already avoid the redundant full-store clone on the hot locked session-store update paths via an explicit clone: false option, while preserving defensive cloning for ordinary skipCache readers.

So I’m closing this as already implemented rather than keeping a duplicate issue open.

Review details

Best possible solution:

Keep the current explicit clone: false API and hot-path callers, since it removes the redundant clone where mutation is intentional without weakening the default defensive clone behavior for general session-store readers.

Do we have a high-confidence way to reproduce the issue?

Not applicable as a performance PR rather than a failing bug. The relevant behavior is source-verifiable on current main: locked update paths force a fresh disk read and opt out of cloning with clone: false.

Is this the best way to solve the issue?

Yes. Current main is a narrower maintainable solution than the PR's blanket skipCache return change because it preserves defensive cloning by default and removes the extra clone only for the mutable read-modify-write paths.

Security review:

Security review cleared: The PR diff only changes session-store TypeScript logic and a unit test, with no CI, dependency, package, secret, or supply-chain surface touched.

What I checked:

  • Current load option: LoadSessionStoreOptions now includes clone?: boolean, and loadSessionStore returns the parsed store directly when opts.clone === false. (src/config/sessions/store-load.ts:22, 3c4851037b6c)
  • Hot update path avoids clone: updateSessionStore re-reads under the lock with { skipCache: true, clone: false }, covering the read-modify-write path described in the PR body. (src/config/sessions/store.ts:455, 3c4851037b6c)
  • Entry update path avoids clone: updateSessionStoreEntry also uses { skipCache: true, clone: false }, so single-entry updates do not pay the defensive clone cost after the fresh disk read. (src/config/sessions/store.ts:678, 3c4851037b6c)
  • Default defensive behavior remains tested: The cache tests still verify ordinary loadSessionStore(storePath, { skipCache: true }) returns an isolated clone from serialized JSON, which makes current main narrower than the PR's blanket skipCache change. (src/config/sessions.cache.test.ts:195, 3c4851037b6c)
  • Release provenance: Tag v2026.4.29 contains the same clone: false load option and hot-path callers; git tag --contains a448042c2edd94a4e8ee86d5ed90a5ed9fe8e4cd returns v2026.4.29. (a448042c2edd)
  • PR discussion considered: The Greptile review agreed with the PR's aliasing rationale: skipCache reads are not written into the object cache, so the optimization is safe for the intended mutable update path. (e05be9ffd09a)

Likely related people:

  • steipete: Local blame and tag inspection attribute the currently shipped session-store load/update/cache-test lines used as the implementation proof to this maintainer. (role: recent maintainer and release proof owner; confidence: medium; commits: 65c94df872b9, a448042c2edd; files: src/config/sessions/store-load.ts, src/config/sessions/store.ts, src/config/sessions.cache.test.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3c4851037b6c; fix evidence: release v2026.4.29, commit a448042c2edd.

@clawsweeper clawsweeper Bot closed this May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@efww