Repository navigation
🤖 perf: keep the config snapshot while it still matches the file - #5750
Merged
Merged
Conversation
An edit's fresh read no longer drops the snapshot of the unchanged file, and a save keeps a snapshot only when a reader already parsed the file that is on disk after the save. With a reader on every event-loop turn (the startup heal sweep), one edit parsed config.json 4 times; now it parses it twice. Adds configScale.bench.ts (live-shaped synthetic config rows).
…hot mutation The test asserted only snapshot identity and a read count after a failed setUpdateChannel save. With a fresh transform input, a kept snapshot still matches the unchanged file, so that assertion pinned an extra read, not a behavior. The clear exists for an edit that returns the shared snapshot after mutating it (removeWorkspaceFromTestConfig does): without it, readers see the unsaved mutation. The test now runs that edit and asserts readers get the file content. Signed-off-by: Thomas Kosiewski <tk@coder.com>
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. |
Member
Author
|
Dogfood evidence for head Each line below is the driver's raw JSON for one run: startup heal-sweep counts, then the cross-process check ( run1 {"label": "base", "workspaces": 4700, "portOpenS": 2.32, "titlesDoneS": 16.26, "settledS": 31.49, "sweep": {"parses": 85, "saves": 21, "sweepParses": 85, "sweepSaves": 21, "sweepMs": 17619.175097000003, "sweepRuns": 1}, "sweepParsesPerSave": 4.05, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 89, "saves": 22, "sweepParses": 85, "sweepSaves": 21, "sweepMs": 17619.175097000003, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run1 {"label": "head", "workspaces": 4700, "portOpenS": 2.16, "titlesDoneS": 12.21, "settledS": 26.54, "sweep": {"parses": 43, "saves": 21, "sweepParses": 43, "sweepSaves": 21, "sweepMs": 13499.029835000001, "sweepRuns": 1}, "sweepParsesPerSave": 2.05, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 47, "saves": 22, "sweepParses": 43, "sweepSaves": 21, "sweepMs": 13499.029835000001, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run2 {"label": "base", "workspaces": 4700, "portOpenS": 2.37, "titlesDoneS": 16.11, "settledS": 30.74, "sweep": {"parses": 85, "saves": 21, "sweepParses": 85, "sweepSaves": 21, "sweepMs": 17270.552028000002, "sweepRuns": 1}, "sweepParsesPerSave": 4.05, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 89, "saves": 22, "sweepParses": 85, "sweepSaves": 21, "sweepMs": 17270.552028000002, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run2 {"label": "head", "workspaces": 4700, "portOpenS": 2.2, "titlesDoneS": 12.58, "settledS": 27.71, "sweep": {"parses": 43, "saves": 21, "sweepParses": 43, "sweepSaves": 21, "sweepMs": 13871.772016, "sweepRuns": 1}, "sweepParsesPerSave": 2.05, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 47, "saves": 22, "sweepParses": 43, "sweepSaves": 21, "sweepMs": 13871.772016, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run3 {"label": "base", "workspaces": 4700, "portOpenS": 2.16, "titlesDoneS": 15.89, "settledS": 31.53, "sweep": {"parses": 85, "saves": 21, "sweepParses": 85, "sweepSaves": 21, "sweepMs": 17458.416162, "sweepRuns": 1}, "sweepParsesPerSave": 4.05, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 89, "saves": 22, "sweepParses": 85, "sweepSaves": 21, "sweepMs": 17458.416162, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run3 {"label": "head", "workspaces": 4700, "portOpenS": 2.18, "titlesDoneS": 12.4, "settledS": 29.87, "sweep": {"parses": 43, "saves": 21, "sweepParses": 43, "sweepSaves": 21, "sweepMs": 15616.672260000001, "sweepRuns": 1}, "sweepParsesPerSave": 2.05, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 47, "saves": 22, "sweepParses": 43, "sweepSaves": 21, "sweepMs": 15616.672260000001, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run0 {"label": "base", "workspaces": 4700, "portOpenS": 2.18, "titlesDoneS": 2.18, "settledS": 16.61, "sweep": {"parses": 5, "saves": 1, "sweepParses": 5, "sweepSaves": 1, "sweepMs": 5474.685412999999, "sweepRuns": 1}, "sweepParsesPerSave": 5.0, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 9, "saves": 2, "sweepParses": 5, "sweepSaves": 1, "sweepMs": 5474.685412999999, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}
run0 {"label": "head", "workspaces": 4700, "portOpenS": 2.18, "titlesDoneS": 2.18, "settledS": 17.11, "sweep": {"parses": 3, "saves": 1, "sweepParses": 3, "sweepSaves": 1, "sweepMs": 5335.1228790000005, "sweepRuns": 1}, "sweepParsesPerSave": 3.0, "afterApi": "API title", "afterAtomic": "External atomic title", "afterInPlace": "In-place edit, longer title", "totals": {"parses": 7, "saves": 2, "sweepParses": 3, "sweepSaves": 1, "sweepMs": 5335.1228790000005, "sweepRuns": 1}, "tombstonesLeft": 2735, "healedGone": true, "freshKept": true, "healWarnings": 2}counter.cjs (preload)// Preload: count Config parses and saves overall and while the startup heal sweep runs.
const { isMainThread } = require("worker_threads");
const fs = require("fs");
if (isMainThread) {
const repo = process.env.F2_REPO;
const out = process.env.F2_COUNTS;
const { Config } = require(repo + "/dist/node/config/index.js");
const removal = require(repo + "/dist/node/services/workspaceRemoval.js");
const c = { parses: 0, saves: 0, sweepParses: 0, sweepSaves: 0, sweepMs: null, sweepRuns: 0 };
let inSweep = false;
const P = Config.prototype;
const origRead = P.readConfigOrDefault;
P.readConfigOrDefault = function (...a) { c.parses++; if (inSweep) c.sweepParses++; return origRead.apply(this, a); };
const origSave = P.saveConfig;
P.saveConfig = function (...a) { c.saves++; if (inSweep) c.sweepSaves++; return origSave.apply(this, a); };
const origHeal = removal.healRemovalTombstonesForRegisteredWorkspaces;
if (typeof origHeal !== "function") throw new Error("heal sweep export missing");
removal.healRemovalTombstonesForRegisteredWorkspaces = async function (...a) {
c.sweepRuns++; inSweep = true; const t0 = performance.now();
try { return await origHeal.apply(this, a); }
finally {
inSweep = false; c.sweepMs = performance.now() - t0;
fs.writeFileSync(out + ".sweep", JSON.stringify(c));
}
};
process.on("SIGUSR2", () => fs.writeFileSync(out, JSON.stringify(c)));
}driver.py"""F2 startup + cross-process dogfood driver. Synthetic fixture only; never real ~/.xum data."""
import hashlib, json, os, shutil, signal, socket, subprocess, sys, time, urllib.request, random
repo, label, template, port, outdir = sys.argv[1], sys.argv[2], sys.argv[3], int(sys.argv[4]), sys.argv[5]
root = os.path.join(outdir, f"root-{label}")
assert root.startswith(os.environ["XUM_SCRATCH_DIR"] + "/") and os.path.realpath(root) != os.path.realpath(os.path.expanduser("~/.xum")), root
shutil.rmtree(root, ignore_errors=True)
shutil.copytree(template, root, symlinks=True)
cfg_path = os.path.join(root, "config.json")
cfg = json.load(open(cfg_path))
ws = [(p, w) for p, proj in cfg["projects"] for w in proj["workspaces"]]
active = [w for _, w in ws if not w.get("archivedAt")]
assert len(ws) >= 4700, len(ws)
def tomb(wid):
d = hashlib.sha256(wid.encode()).hexdigest()[:32]
return os.path.join(root, "locks", f"workspace-removed-{d}.json")
os.makedirs(os.path.join(root, "locks"), exist_ok=True)
old = time.time() - 86400
rng = random.Random(7)
for i in range(2734):
f = tomb(f"gone-{rng.getrandbits(64):016x}")
json.dump({"workspaceId": os.path.basename(f), "removedAt": 1}, open(f, "w"))
os.utime(f, (old, old))
healed = [active[1]["id"], active[2]["id"]]
for wid in healed:
f = tomb(wid); json.dump({"workspaceId": wid, "removedAt": 1}, open(f, "w")); os.utime(f, (old, old))
fresh = active[3]["id"]
json.dump({"workspaceId": fresh, "removedAt": 1}, open(tomb(fresh), "w"))
# The unregistered markers name ids that are not in config (their own file name).
for f in os.listdir(os.path.join(root, "locks")):
pass
counts = os.path.join(outdir, f"counts-{label}.json")
for p in (counts, counts + ".sweep"):
if os.path.exists(p): os.remove(p)
env = {
"PATH": os.environ["PATH"], "HOME": os.path.join(root, "home"), "USERPROFILE": os.path.join(root, "home"),
"XDG_CONFIG_HOME": os.path.join(root, "home", ".config"), "XDG_CACHE_HOME": os.path.join(root, "home", ".cache"),
"XUM_ROOT": root, "XUM_LOG_LEVEL": "info", "NODE_ENV": "production", "XUM_MOCK_AI": "1",
"XUM_DISABLE_TELEMETRY": "1", "F2_REPO": repo, "F2_COUNTS": counts,
}
log = open(os.path.join(outdir, f"server-{label}.log"), "w")
t0 = time.monotonic()
proc = subprocess.Popen(["node", "--require", os.path.join(os.path.dirname(__file__), "counter.cjs"),
os.path.join(repo, "dist/cli/index.js"), "server", "--host", "127.0.0.1",
"--port", str(port), "--no-auth"], env=env, stdout=log, stderr=subprocess.STDOUT,
stdin=subprocess.DEVNULL, start_new_session=True)
open(os.path.join(outdir, f"server-{label}.pid"), "w").write(str(proc.pid))
def api(route, body):
req = urllib.request.Request(f"http://127.0.0.1:{port}/api/{route}", data=json.dumps(body).encode(),
headers={"content-type": "application/json"}, method="POST")
with urllib.request.urlopen(req, timeout=120) as r:
raw = r.read()
return json.loads(raw) if raw else None
result = {"label": label, "workspaces": len(ws)}
try:
while True:
assert proc.poll() is None, "server exited"
try:
socket.create_connection(("127.0.0.1", port), timeout=0.2).close(); break
except OSError:
time.sleep(0.02)
result["portOpenS"] = round(time.monotonic() - t0, 2)
titles = []
for i in range(int(os.environ.get("F2_TITLES", "20"))):
r = api("workspace/updateTitle", {"workspaceId": active[10 + i]["id"], "title": f"F2 startup title {i}"})
assert r is not None and r.get("success") is True, r
result["titlesDoneS"] = round(time.monotonic() - t0, 2)
deadline = time.monotonic() + 600
while not (os.path.exists(counts + ".sweep") and "housekeeping settled" in open(log.name).read()):
assert proc.poll() is None, "server exited"; assert time.monotonic() < deadline, "timeout"
time.sleep(0.1)
result["settledS"] = round(time.monotonic() - t0, 2)
sweep = json.load(open(counts + ".sweep"))
result["sweep"] = sweep
result["sweepParsesPerSave"] = round(sweep["sweepParses"] / sweep["sweepSaves"], 2) if sweep["sweepSaves"] else None
# Cross-process check: our rename, then another writer's atomic replace, then an in-place edit.
target = active[40]["id"]
assert api("workspace/updateTitle", {"workspaceId": target, "title": "API title"})["success"]
info = api("workspace/getInfo", {"workspaceId": target}); result["afterApi"] = info["title"]
data = json.load(open(cfg_path))
for _, proj in data["projects"]:
for w in proj["workspaces"]:
if w["id"] == target: w["title"] = "External atomic title"
tmp = cfg_path + ".ext"; json.dump(data, open(tmp, "w"), indent=2); os.replace(tmp, cfg_path)
result["afterAtomic"] = api("workspace/getInfo", {"workspaceId": target})["title"]
text = open(cfg_path).read().replace('"External atomic title"', '"In-place edit, longer title"')
with open(cfg_path, "r+") as f: f.write(text); f.truncate()
result["afterInPlace"] = api("workspace/getInfo", {"workspaceId": target})["title"]
os.kill(proc.pid, signal.SIGUSR2); time.sleep(0.5)
result["totals"] = json.load(open(counts))
finally:
if proc.poll() is None:
os.killpg(proc.pid, signal.SIGTERM)
try: proc.wait(20)
except subprocess.TimeoutExpired: os.killpg(proc.pid, signal.SIGKILL); proc.wait()
locks = os.listdir(os.path.join(root, "locks"))
names = set(locks)
result["tombstonesLeft"] = sum(1 for n in locks if n.startswith("workspace-removed-"))
result["healedGone"] = all(os.path.basename(tomb(w)) not in names for w in healed)
result["freshKept"] = os.path.basename(tomb(fresh)) in names
result["healWarnings"] = open(log.name).read().count("Healed a removal tombstone")
print(json.dumps(result)) |
Member
Author
|
Remote UAT (Coder Agents on dogfood,
The remote also found older behavior that this PR does not change (same on base): startup-results.txt |
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
An edit no longer throws away a config snapshot that still matches
config.json. With a reader on every event-loop turn (the startup tombstone heal sweep does this), one edit now parsesconfig.json2 times instead of 4. At 4,712 workspaces, that edit is 66 ms faster (205 ms to 138 ms, 95% CI -40.1% to -23.9%). In the startup scenario at 4,700 workspaces, the heal sweep now does 2 parses per save instead of 4.Refs #5727
Background
The edit's own fresh read (
readConfigOrDefaultinsideenqueueConfigEditEffect) cleared the shared snapshot, and the save cleared it again after the rename. Each clear forced the next reader to parse the same bytes again. So with a reader on every turn, one edit cost 4 parses:This change removes parses 2 and 4. The floor is 2: the edit's fresh read, and one read that publishes the saved file.
Implementation
All in
src/node/config/index.ts(9 changed code lines plus comments):enqueueConfigEditEffect: the transform reads withreadConfigOrDefault({ keepSnapshot: true }).readConfigOrDefault: withkeepSnapshot, it clears the snapshot as before, then gives it back only after a successful, cacheable read. A failed, missing or lenient read leaves it cleared. Otherwise the edit gate'sloadConfigOrDefaultwould hit the old snapshot, never retry the read that clears a recorded load failure, and refuse every edit until a restart.saveConfigEffect: after the write, it keeps a snapshot only if the snapshot's stat key equals the current stat key ofconfig.json. That snapshot came from a reader that parsed the real bytes on disk, whoever wrote them. The cost is onestatSyncper save.The bench file
src/node/config/configScale.bench.tsadds live-shaped synthetic rows (no real data). It is the file from the unmerged bench branch, unchanged.Invariants and the tests that guard them
All tests are in
src/node/config/snapshot.test.ts. The existing cases are unchanged.config.jsonexactly 2 times (main: 4)parses config.json twice per edit while a reader runs on every event-loop turn(new)serves the committed snapshot, never the transform's object, to readers during an edit(new)EIOin the edit's read, the next edit succeeds without a file change or restartaccepts the next edit after the edit's own read fails once on a warm snapshot(new)drops the snapshot when a save fails(new)invalidates on atomic external replacement, including same-size bytes and mtime,isolates edits and reloads the saved snapshot once,sees an external atomic replacement between our rename and save completion(existing, unchanged)does not reuse a lenient structurally invalid load for a strict read,does not cache structurally invalid entries after an unrelated save,treats stat failures as cache misses,does not reuse a cached snapshot after the file disappears(existing, unchanged)Mutation check, run locally against
snapshot.test.ts(each mutant fails only the expected test):index.tsEIOrecovery test"Unsaved")Measurements
Host: shared Coder host (AMD EPYC 9454P), Node 22.19,
make bench-compare ROUNDS=10(interleaved base and head processes, paired 95% CI on per-round log ratio).0c5e35d81638e85ed420c4b7c943d86ed502a2f7(origin/main merge-base), head4fa387b2e78bf32f810d7c21bc1f488bd33ce265.A/A noise floor:
make bench-compare BENCH=config BASE=HEAD ROUNDS=10Both sides are the same commit (
712637f68e, the product code of this PR). Result: 0fasterorslowerverdicts out of 42 rows.A/A table (42 rows)
Base vs head:
make bench-compare BENCH=config ROUNDS=10Result: 4
fasterverdicts, all on the reader-per-turn row. 0slowerverdicts. At the live-shaped size (4,712 workspaces) the reader-per-turn edit takes 66 ms less (budget: at least 35 ms). Plain edits without a concurrent reader do not change, as expected.Full base-vs-head table (42 rows)
Parses per save in the startup heal-sweep scenario (counting harness, 4,700 workspaces)
Setup: a synthetic fixture from
scripts/perf/workspace-scale/generate-fixture.ts(4,700 workspaces, 19 projects, 57% archived, realistic profile, 8.5 MBconfig.json). Before start, the driver writes 2,734 one-day-old tombstones for unregistered ids, 2 one-day-old tombstones for registered ids, and 1 fresh tombstone for a registered id. It startsnode --require counter.cjs dist/cli/index.js server --no-authwith an isolatedXUM_ROOTandXUM_MOCK_AI=1. As soon as the port opens, it sends 20workspace.updateTitlecalls. The preload countsreadConfigOrDefaultandsaveConfigcalls whilehealRemovalTombstonesForRegisteredWorkspacesruns. Counts are exact. Timings come from single runs on a shared host (load average 11-32) and are noisy. Run 1 used a build of712637f68e; runs 2, 3 and the control used the head build. Both have the same product code (the later commit changes only a test).housekeeping settled(s)Dogfood (head
4fa387b2e78bf32f810d7c21bc1f488bd33ce265, synthetic 4,700-workspace root, no real data)The same driver ran a cross-process check after each startup run, on base and head:
workspace.updateTitle:workspace.getInforeturnsAPI title.config.jsonthrough a temp file and a rename, as another backend does:workspace.getInforeturnsExternal atomic title.config.jsonin place with a different length:workspace.getInforeturnsIn-place edit, longer title.All 3 steps passed on base and head in all 3 runs. The harness source and raw results are in a PR comment.
Risks
Low. The change only stops clearing a snapshot whose stat key still matches the file, and every snapshot read compares keys first, as before. The remaining risk is the existing stat-key limit (an in-place rewrite that keeps inode, mtime and size), which this PR does not change. A save still keeps no snapshot of the file it replaced, and a failed save still clears it.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high