Repository navigation
🤖 perf: skip the config save clone when no slot holds Cyber - #5770
Merged
Merged
Conversation
Every config save cloned the whole config before encoding Cyber reasoning modes for disk. Without a Cyber slot the encode is a no-op, so check the same slot list first and stringify the config directly (about 27 ms per save at ~4,700 workspaces). The check costs about 0.9 ms at 4,712 workspaces (new cyberReasoningModeDisk bench). Refs #5727 Signed-off-by: Thomas Kosiewski <tk@coder.com>
The mid-save tests awaited the paused realpath alone, so a save that stops calling the CommonJS fs.realpath hung until the 5 s test timeout and `expect(pausedOnce).toBe(true)` could never fail. Race the pause against the edit so that case fails fast on the pausedOnce assertion. Refs #5727
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Refs #5727
Summary
Every config save cloned the whole config with
structuredClonebefore it encoded Cyber reasoning modes for disk. The clone keeps the encoder from changing runtime state. This PR skips the clone and the encode when no reasoning-mode slot holds"cyber", so the encode would change nothing anyway. A newhasCyberReasoningMode(doc)incyberReasoningModeDisk.tswalks the samecollectReasoningModeSlotslist as the encoder, so the check and the encoder cannot disagree about which slots exist. Configs with a Cyber slot still take the clone-and-encode path, as before.The original 20 ms target was not met.
This PR ships under a budget exception that the perf owner declared in advance and recorded on #5727:
Results
The plan's budget was "
editConfigat least 20 ms faster at the live-shaped size (4,712 workspaces)". The primary row iseditConfig, same-value edit (4712). It saved 17.8 ms, 18.7 ms and 16.6 ms in the three base-vs-head runs (run 1, run 2, confirmation), so the target was not met. Run 2's CI for that row includes 0. The confirmation run's CI excludes 0, which is the exception's gate.editConfig, same-value edit(primary)de7e1c35d8)editConfig, same-value edit(primary)de7e1c35d8)editConfig, same-value edit(primary)c14e682ec3)editConfig, then loadConfigOrDefaultde7e1c35d8)editConfig, then loadConfigOrDefaultde7e1c35d8)editConfig, then loadConfigOrDefaultc14e682ec3)editConfig, reader on every event-loop turnde7e1c35d8)editConfig, reader on every event-loop turnde7e1c35d8)editConfig, reader on every event-loop turnc14e682ec3)All runs: Node,
make bench-compare ROUNDS=10, basea4e2d5c44933b17bbf01e455fc42346999bdaa47(merge base with origin/main), the same synthetic fixtures (configScale.bench.ts, never real data).structuredClonegone from the save path on head..gc("inner")), which can hide part of the clone's cost from the plain edit row. This is an unverified guess.hasCyberReasoningModetakes 926.77 µs at 4,712 workspaces and 1.02 ms at 5,000 (A/A run). The limit was 2 ms.getWorkspaceMetadataById(last id) (4712)+2.5%slower(CI [+0.5%, +4.4%]). Run 1 marked twogetAllWorkspaceMetadata, last-known probesrows +4.1% and +2.5%slower, and run 2 did not. This PR does not touch that code. The A/A run, which compares one commit with itself, marked 4 rowsslowerat +1% to +2.7%, so a gap of that size can come from noise on this shared host. I did not prove that it does. I did not rerun: the exception allowed one confirmation run.BENCH='src/node/config/config*.bench.ts', not plainBENCH=config.bench-compareruns the head's bench files on both sides, and the newcyberReasoningModeDisk.bench.tsimportshasCyberReasoningMode, which does not exist on base. The glob selects the sameconfig.bench.tsandconfigScale.bench.tsrows in all three runs.Load averages (1, 5 and 15 min):
loadavg start 11.43 12.92 14.18 | end 30.02 26.21 20.79loadavg start 28.35 26.02 20.82 | end 16.52 15.43 16.40loadavg start 13.54 14.79 16.16 | end 16.27 32.64 37.32loadavg start 11.00 11.51 12.20 | end 4.30 5.89 8.05Full bench-compare tables (A/A, run 1, run 2, confirmation)
A/A (
make bench-compare BENCH=config BASE=HEAD, on6e526e29af, which has the same product change as head, compared with itself):Run 1:
Run 2:
Confirmation run on the final head:
Same bytes
For a config without Cyber modes, head writes exactly the bytes base writes. A throwaway script wrote the
configScale.bench.tsfixtures through the publicConfigAPI on each tree and hashedconfig.jsonafter each save.writeIdis a random UUID per save, so the script pins it to a constant. Base ran twice with identical results.a4e2d5c449sha256c14e682ec3sha256cb15542f45d5c2be05d277bf25a1f4920d128140a2813a828f444c3328ecc661cb15542f45d5c2be05d277bf25a1f4920d128140a2813a828f444c3328ecc66133726d53306b6d3763f3f53aa155bddc0ac906c93f696053a8a6cb8f58911d0633726d53306b6d3763f3f53aa155bddc0ac906c93f696053a8a6cb8f58911d0619ce7d11a08fbb9119d60896d2ff51d3ca6d7d63cd4fb333701927ceb66b09eb19ce7d11a08fbb9119d60896d2ff51d3ca6d7d63cd4fb333701927ceb66b09eb3fb0327948522139320526089cd2dd563183ab5ef6887b8526257a4bbb87fb193fb0327948522139320526089cd2dd563183ab5ef6887b8526257a4bbb87fb193ae7bf134a606d68bdd75884c2ac94d88433d2ad5d6895c6729c1a6dc74909673ae7bf134a606d68bdd75884c2ac94d88433d2ad5d6895c6729c1a6dc7490967e40731874a44d008f71684bed2606f17eb5469b8f07e155720bd8c372dc5ac83e40731874a44d008f71684bed2606f17eb5469b8f07e155720bd8c372dc5ac8392684d58994d187bb0227b29aa87975d9ef81b937c617f13f3ac63ffb009d09092684d58994d187bb0227b29aa87975d9ef81b937c617f13f3ac63ffb009d09049e8f35463e58137bacd99b1f885aaa298d687f6ea7a939e5dc3212563cee37e49e8f35463e58137bacd99b1f885aaa298d687f6ea7a939e5dc3212563cee37ebd2fe5fced05263852450a48ebdbad9672acb85de0556ee954e48dd6f90564dbbd2fe5fced05263852450a48ebdbad9672acb85de0556ee954e48dd6f90564dba7ee746a21d270e5d11c72dd294949d2208c2a595c55a7f76d6078941e940c0ba7ee746a21d270e5d11c72dd294949d2208c2a595c55a7f76d6078941e940c0b94b2cc60bfc68937a7742435c891ee93d8c69ace47d7c6a3c261e3c0554b324094b2cc60bfc68937a7742435c891ee93d8c69ace47d7c6a3c261e3c0554b3240cf8e078a4bc898b93e050689da656d0c20e75b5a9dd96b27c269706de3d1d090cf8e078a4bc898b93e050689da656d0c20e75b5a9dd96b27c269706de3d1d090Serialization-point test
Without the clone, the save works on objects it shares with runtime state. So the save must finish
JSON.stringifybefore its first suspension, or a change made during the write could reach disk.de7e1c35d8makes the existing serialization order explicit: it movesJSON.stringifyout of theEffect.tryPromisethunk to right after the Cyber check. It does not fix a gap. In effect 4.0.0-rc.112,tryPromisealready calls its thunk in the same synchronous step, so the payload was already serialized before the first await on6e526e29afand on main.The new test, "writes the returned state even if settings change mid-save" (both "no Cyber" and "Cyber" cases), pauses the save at the atomic write's first await: a spy on the CommonJS
fs.realpath, held by a deferred promise, with no timers. It then changes the shared settings object (reasoningMode: "cyber"and a new model), resumes, and checks that the file holds the state the edit returned.These mutant results show that the test catches a future regression. They do not prove that an earlier commit was broken.
111a6a1339with head's test filefs.realpathon config.json) added beforeJSON.stringifydiskData = data(no clone)"cyber")Raw step 1 output
The independent test auditor also checked two more mutations. When the check misses nested slots, the runtime-state test and the Cyber mid-save case fail. When an await runs between the check and
structuredClone, the Cyber mid-save case fails. The auditor made one test-only change (c14e682ec3): the test now races the pause against the edit, so a save that never reaches the pausedrealpathfails at once instead of hanging until the timeout.Invariants and the tests that guard them
src/node/config.test.ts). Its only Cyber slot is nested (taskAiPins), so it also fails when the check misses that slot. No earlier test checked the in-memory object after a save: "never writes reasoning mode cyber to disk and restores it on reload" reloads through a newConfig.hasCyberReasoningMode, no Cyber slotrows in the newcyberReasoningModeDisk.bench.ts.Dogfood
Remote dogfood through Coder Agents on the exact final head
c14e682ec3: PASS. Chat: https://dogfood.cdr.dev/agents/23706166-482e-4b69-bdb6-1c9b2bf41759XUM_DISABLE_TELEMETRY=1from its first start, and its logs have 0 PostHog matches. The agent printed no tokens, and the attached logs are redacted.promode reach disk and the API, with 0"cyber"strings and 0 markers.cyberReasoningMode: truewith noreasoningModekey, and the API keeps returning"cyber". That survives an unrelated edit, a restart, 20 sequential and 5 concurrent edits, and a switch back tostandardorpro. Cyber only inagentAiDefaults.exec.subagent,agentAiDefaults.planoradvisorReasoningModeis encoded and decoded the same way.writeId. With it replaced by a constant, the sha256 values match.structuredCloneunder the saveThis run supersedes round 1 (on
6e526e29af). In round 1 the first server ran with telemetry on, and the agent printed its own loopback server's token in the chat. That server was stopped, and no token from it was reused.SHAs
a4e2d5c44933b17bbf01e455fc42346999bdaa47(merge base with origin/main)c14e682ec3144f41916703e5f52b28f6b588ee25de7e1c35d8. The commits after it touch onlysrc/node/config.test.ts.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$15.66