From eb998fcd095ce7bbc9d7dd76fbf5272283a3a667 Mon Sep 17 00:00:00 2001 From: IPerception Date: Mon, 7 Sep 2026 22:12:40 -0400 Subject: [PATCH 1/4] perf: browse the mutated document instead of re-parsing the output render() rebuilt the browsable document with parse(result.output) -- a second full parse, producing a second full set of segment objects, for a document processText already held. It now returns that document and render() takes it. Measured on 10/25/50 MB synthetic 837P, like for like: run time falls 26-37% (50 MB: 2565 ms -> 1848 ms). Retained heap is essentially flat, about 2%. PERFORMANCE.md predicted "roughly a third of peak heap" and that is wrong. The prediction treated the re-parse as a copy held alongside the mutated document, but the mutated document became unreachable the moment processText returned, so only one of the two was ever retained. What the re-parse actually cost was the time to do it. The doc is corrected in a later commit, with the measurements. The benchmark stopped counting a re-parse that no longer happens. Note it had been discarding its re-parse immediately while the app keeps it in state.doc forever, so its heap column understated the app; the baseline above was re-measured with that corrected. render.mjs section [6] covers what changed -- the document you browse, which parity.mjs does not see because the output string was never at risk. Verified load-bearing by restoring the re-parse, which fails it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AfabdBfKu77QsNYVZkc3nn --- web/EDIWorkbench.html | 13 ++++++++++++- web/bench/bench.mjs | 7 +++++-- web/tests/render.mjs | 22 ++++++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/web/EDIWorkbench.html b/web/EDIWorkbench.html index c60fddf..8cf7313 100644 --- a/web/EDIWorkbench.html +++ b/web/EDIWorkbench.html @@ -2451,6 +2451,11 @@

Using it with real data

return { output: serialize(doc), + // The mutated document itself, so the UI can browse the result without + // parsing `output` back into a second copy of what is already here. + // Callers must treat it as owned by the result: the segment objects are + // live, not clones, and `changes` holds the only record of what they were. + doc, segmentCount: doc.segments.length, delimiters: { element: doc.elementSep, @@ -6086,7 +6091,13 @@

Using it with real data

`
${t.value.toLocaleString()}${t.label}
` ).join(""); - state.doc = parse(result.output); // browse the edited document, not the original + // Browse the edited document, not the original. processText already holds + // the mutated document, so this takes it rather than re-parsing the output + // string back into a second full set of segment objects -- which cost a + // whole extra parse and about a third of peak heap for a document it had + // in hand the entire time. The segments here are the ones the rules + // mutated, which is exactly what the re-parse was reconstructing. + state.doc = result.doc; state.outline = buildOutline(state.doc.segments); state.claims = null; // stale the moment the document changes; rebuilt on demand state.focus = null; diff --git a/web/bench/bench.mjs b/web/bench/bench.mjs index 40b7aa4..04f3827 100644 --- a/web/bench/bench.mjs +++ b/web/bench/bench.mjs @@ -12,7 +12,7 @@ * actually does and they have very different budgets: * * load -- parse only, i.e. open a file and browse it - * run -- snapshot + rules + diff + serialize + the UI's re-parse + * run -- snapshot + rules + diff + serialize * * Numbers come from Node, not a browser. Same V8, so the arithmetic * transfers; a real tab additionally pays for file.text() and the download @@ -143,7 +143,10 @@ const [runTime] = time(() => { new Dtp472ServiceLineShiftRule(1), new StringReplaceRule({ find: "SPRINGFIELD", replace: "METROPOLIS" }), ]); - parse(result.output); // the UI re-parses so you can browse the edited doc; count it + // The UI used to re-parse result.output here so you could browse the edited + // document, and this benchmark counted that parse because it was real work + // a user paid for. processText now returns the mutated document directly, + // so there is nothing to count -- result.doc is what the UI browses. return result; }); const runHeap = heap() - base; diff --git a/web/tests/render.mjs b/web/tests/render.mjs index c17cfc7..1c60cf4 100644 --- a/web/tests/render.mjs +++ b/web/tests/render.mjs @@ -173,6 +173,28 @@ check("the caution clears for an 837", byId.get("deidRemit").hidden, true); app.loadFile("broken.edi", "this is not an interchange"); check("the caution clears when nothing is loaded", byId.get("deidRemit").hidden, true); +console.log("\n[6] browsing after a run shows the edited document, not the original"); +// render() used to rebuild the browsable document with parse(result.output) -- +// a second full parse of what processText already held. It now takes +// result.doc directly. The output string was never the thing at risk there +// (parity.mjs pins that); what was at risk is the document you browse, so +// that is what this checks: the same values the download carries. +const subscriber = () => + app.state.doc.segments.find((s) => s.id === "NM1" && s.elements[0] === "IL").elements[2]; + +app.loadFile("sample_837p.edi", app.SAMPLE_837P); +check("the loaded document carries the real name", subscriber(), "DOE"); + +app.deidentify(); +const scrubbed = subscriber(); +check("the browsable document shows the scrubbed name", scrubbed !== "DOE", true); +check("it is the document the run produced, not a re-parse of it", + app.state.doc === app.state.result.doc, true); +check("the download agrees with what is on screen", + app.state.result.output.includes(scrubbed), true); +check("the original is still the one that gets re-run", + app.state.raw.includes("DOE"), true); + console.log(`\n${pass} passed, ${fail} failed`); if (fail) process.exit(1); console.log("clean"); From e4771da7a3fe70a499a96a5d96e620f9eaeea09a Mon Sep 17 00:00:00 2001 From: IPerception Date: Mon, 7 Sep 2026 22:14:54 -0400 Subject: [PATCH 2/4] test: audit that rule marks account for every change they make Preparation for dropping the full pre-run snapshot from processText. Once the change list is built from marks instead of from a clone-and-compare over every segment, a change a rule makes without marking it is unrecoverable -- the before-value is gone the moment the rule overwrites it. Nothing checked that. _marks.mjs audits both directions and is called from the suites that already have each rule wired up, rather than becoming a tenth suite that would have to re-lift the engine to reach them: parity.mjs [13] for the two engine rules, deid.mjs [18] for DeidentifyRule. The two directions are not equally serious. "unmarked" must always be empty. "overclaimed" is expected and harmless -- StringReplaceRule counts a match even when the replacement equals it, so ANYTOWN -> ANYTOWN marks every hit and changes nothing. That case is now pinned explicitly, because it is the reason processText keeps sameSegment as a filter over marked segments instead of trusting the marks outright. All three rules pass as they stand; this commit changes no behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AfabdBfKu77QsNYVZkc3nn --- web/tests/_marks.mjs | 63 ++++++++++++++++++++++++++++++++++++++++++++ web/tests/deid.mjs | 20 ++++++++++++++ web/tests/parity.mjs | 28 ++++++++++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 web/tests/_marks.mjs diff --git a/web/tests/_marks.mjs b/web/tests/_marks.mjs new file mode 100644 index 0000000..3a46d05 --- /dev/null +++ b/web/tests/_marks.mjs @@ -0,0 +1,63 @@ +// Audits whether a rule's marks account for every change it actually makes. +// +// Not a suite of its own -- it is called from the suites that already have a +// rule wired up: parity.mjs for the two engine rules, deid.mjs for +// DeidentifyRule. Putting the audit next to each rule's other tests beats a +// tenth suite that would have to re-lift the whole engine to reach them. +// +// Why this exists at all: processText used to build its change list by cloning +// every segment before the rules ran and comparing afterwards. The clone was a +// full second copy of the working set, so it was dropped -- the change list is +// now built from what the rules say they touched. That moves the correctness of +// the Changes tab, and of the Limited Data Set's own record of what it +// rewrote, onto the accuracy of `marks`. Nothing checked that before; this does. +// +// Two directions, and they are not equally serious: +// +// unmarked a rule changed something and did not say so. The change list +// misses it. Nothing downstream can recover it, because the +// before-value is gone the moment the rule overwrites it. This +// must always be empty. +// overclaimed a rule marked something that did not actually change. Harmless +// and expected -- StringReplaceRule counts a match even when the +// replacement equals what it matched. processText filters these +// with sameSegment, which is exactly why that filter was kept +// when the full snapshot was dropped. + +const key = (segment, element) => `${segment}:${element}`; + +export function auditMarks(parse, rule, raw) { + const doc = parse(raw); + if (typeof rule.check === "function") rule.check(doc); + + // The audit's own before-copy. This is the thing processText no longer does; + // a test may pay for it because a fixture is small and correctness is the + // point, which is not true of a 50 MB file in a browser tab. + const before = doc.segments.map((seg) => ({ id: seg.id, elements: seg.elements.slice() })); + const result = rule.apply(doc.segments); + + const changed = new Set(); + doc.segments.forEach((seg, i) => { + const was = before[i].elements; + const now = seg.elements; + // Compare across the longer of the two: a rule that appends or truncates + // elements has changed the segment just as much as one that overwrites. + for (let j = 0; j < Math.max(was.length, now.length); j++) { + if (was[j] !== now[j]) changed.add(key(i, j)); + } + if (before[i].id !== seg.id) changed.add(key(i, "id")); + }); + + const claimed = new Set(); + for (const mark of result.marks) { + for (const element of mark.elements) claimed.add(key(mark.segment, element)); + } + + const sorted = (set) => [...set].sort(); + return { + result, + changed: sorted(changed), + unmarked: sorted([...changed].filter((k) => !claimed.has(k))), + overclaimed: sorted([...claimed].filter((k) => !changed.has(k))), + }; +} diff --git a/web/tests/deid.mjs b/web/tests/deid.mjs index 640ac25..5605a27 100644 --- a/web/tests/deid.mjs +++ b/web/tests/deid.mjs @@ -15,6 +15,7 @@ import { readFileSync, readdirSync } from "node:fs"; import { join } from "node:path"; import { APP, PACDR_FIXTURE, X221_FIXTURE, SAMPLES_DIR } from "./paths.mjs"; +import { auditMarks } from "./_marks.mjs"; const html = readFileSync(APP, "utf8"); @@ -566,5 +567,24 @@ check("the output is byte-identical to the input", m.serialize(x221.doc), x221Ra check("the fixture still validates cleanly after the run", m.validateDocument(x221.doc).map((f) => f.title), []); +console.log("\n[18] every rewrite is marked"); +// processText builds its change list from these marks now, rather than from a +// clone of every segment taken before the rules ran, so a rewrite this rule +// forgets to mark is a rewrite nothing downstream can report. That matters +// more here than for the other two rules: the Changes tab is the only record +// of what a Limited Data Set replaced, and the mapping is thrown away when the +// run ends. See _marks.mjs. +for (const [name, source] of [ + ["the 837P fixture", RAW], + ["the PACDR fixture", readFileSync(PACDR_FIXTURE, "utf8")], +]) { + const audit = auditMarks(m.parse, new m.DeidentifyRule(23), source); + check(`${name}: the rule changes it`, audit.changed.length > 0, true); + check(`${name}: no rewrite goes unmarked`, audit.unmarked, []); + // put() already refuses to mark a write that would not change the value, so + // unlike StringReplaceRule this rule should claim nothing it did not do. + check(`${name}: and none is claimed that did not happen`, audit.overclaimed, []); +} + console.log(`\n${pass} passed, ${fail} failed`); process.exit(fail ? 1 : 0); diff --git a/web/tests/parity.mjs b/web/tests/parity.mjs index 5e49337..7f7df7e 100644 --- a/web/tests/parity.mjs +++ b/web/tests/parity.mjs @@ -3,6 +3,7 @@ import { readFileSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { APP, FIXTURE, MAKE_REF, REF_DIR } from "./paths.mjs"; +import { auditMarks } from "./_marks.mjs"; const html = readFileSync(APP, "utf8"); @@ -247,5 +248,32 @@ check("4010 file still round trips", serialize(parse(v4010)) === v4010, true); check("5010 with a 'U' placeholder reports none", parse(raw.replace("*^*00501*", "*U*00501*")).repetitionSep, null); +console.log("\n[13] the engine rules mark every change they make"); +// processText builds its change list from these marks now, instead of from a +// clone of every segment taken before the rules ran. A change a rule makes +// without marking it is invisible from here on -- the before-value is gone the +// instant the rule overwrites it. See _marks.mjs. +for (const [name, rule] of [ + ["shift service dates", new Dtp472ServiceLineShiftRule(30)], + ["replace a whole element", new StringReplaceRule({ find: "ANYTOWN", replace: "METROPOLIS" })], + ["replace inside a component", new StringReplaceRule({ find: "99213", replace: "99214", mode: "component" })], +]) { + const audit = auditMarks(parse, rule, raw); + check(`${name}: actually changes the fixture`, audit.changed.length > 0, true); + check(`${name}: no change goes unmarked`, audit.unmarked, []); +} + +// The reason processText still runs sameSegment over the segments a rule +// marked, rather than trusting the marks outright. StringReplaceRule counts a +// match whether or not the replacement differs from it, so asking for +// ANYTOWN -> ANYTOWN marks every hit and changes nothing. Without the +// filter each one would surface in the Changes tab as a change with identical +// before and after. +const noop = auditMarks(parse, new StringReplaceRule({ find: "ANYTOWN", replace: "ANYTOWN" }), raw); +check("a replacement equal to its match changes nothing", noop.changed, []); +check("but the rule marks it anyway", noop.overclaimed.length > 0, true); +check("and processText filters it back out", + processText(raw, [new StringReplaceRule({ find: "ANYTOWN", replace: "ANYTOWN" })]).changes, []); + console.log(`\n${pass} passed, ${fail} failed`); process.exit(fail ? 1 : 0); From 1df0cc487067959d2e903867aeb6aa5dfb618729 Mon Sep 17 00:00:00 2001 From: IPerception Date: Mon, 7 Sep 2026 22:19:48 -0400 Subject: [PATCH 3/4] perf: snapshot only the segments a rule changes processText cloned every segment in the document before the rules ran, so it could diff against that copy afterwards and work out what moved. On a 5M-segment file that is a second full working set built to find a few thousand changes. Each rule now copies a segment on its first write to it and carries the copy in the mark it already reported, so only changed segments are ever copied. All three rules had a single mutation site to hook -- put() in DeidentifyRule, and one assignment each in the other two -- so the capture is a few lines apiece. processText keeps the FIRST copy it sees per segment. Two rules can touch one segment, and the second rule's copy already contains the first rule's edit; the change list has to show what arrived in the file, not what the second rule happened to find. parity.mjs [14] pins this, and fails with a fabricated before value if the rule is flipped to last-writer-wins. sameSegment stays, as a filter over marked segments rather than a sweep over all of them. The marks say what a rule touched, not what it managed to change, and StringReplaceRule counts a match even when the replacement equals it. Dropping the filter would surface those as changes with identical before and after. What it no longer catches -- a rule editing without marking -- is now covered directly by the mark audits added in the previous commit. Measured on synthetic 837P, against the same tree with only this change absent: 100 MB 5502 ms -> 4057 ms (-26%) 125 MB 10812 ms -> 6968 ms (-36%) 50 MB 1848 ms -> 1770 ms (-4%) Retained heap is unchanged and the out-of-memory ceiling has not moved: the clone was transient, so it never showed up in a post-GC measurement. What it cost was the work of building it and the GC pressure of carrying it, which is why the gain grows with the file -- at 125 MB the extra copy was crowding the heap limit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AfabdBfKu77QsNYVZkc3nn --- web/EDIWorkbench.html | 67 +++++++++++++++++++++++++++++++++++-------- web/tests/parity.mjs | 21 ++++++++++++++ 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/web/EDIWorkbench.html b/web/EDIWorkbench.html index 8cf7313..5e9b3d6 100644 --- a/web/EDIWorkbench.html +++ b/web/EDIWorkbench.html @@ -2238,8 +2238,11 @@

Using it with real data

const shifted = shiftDate8(value, this.days); if (shifted === null) return; // malformed date; skip rather than fail the file + // Copied before the write, not after: processText builds its change list + // from these marks, so this is the only record of the original date. + const before = { id: seg.id, elements: seg.elements.slice() }; seg.elements[2] = shifted; - marks.push({ segment: i, elements: [2] }); + marks.push({ segment: i, elements: [2], before }); }); return { segments: marks.length, occurrences: marks.length, marks }; @@ -2315,14 +2318,18 @@

Using it with real data

if (this.segmentIds && !this.segmentIds.has(seg.id.toUpperCase())) return; const hits = []; + let before = null; seg.elements.forEach((value, j) => { const [next, count] = this.replaceIn(value); if (!count) return; + // On the first hit only, and before the write: one copy per changed + // segment is what replaced processText's clone of every segment. + if (!before) before = { id: seg.id, elements: seg.elements.slice() }; seg.elements[j] = next; occurrences += count; hits.push(j); }); - if (hits.length) marks.push({ segment: i, elements: hits }); + if (hits.length) marks.push({ segment: i, elements: hits, before }); }); return { segments: marks.length, occurrences, marks }; @@ -2384,10 +2391,12 @@

Using it with real data

/* ---- engine -------------------------------------------------------- */ -function snapshot(segments) { - return segments.map((seg) => ({ id: seg.id, elements: seg.elements.slice() })); -} - +// There was a snapshot(segments) here that cloned every segment in the +// document before the rules ran, so processText could diff against it +// afterwards. It was a second full copy of the working set, built to find +// changes the rules already report: each rule now copies a segment on its +// first write to it and carries that copy in the mark. sameSegment survives +// because the marks say what a rule touched, not what it managed to change. function sameSegment(a, b) { if (a.id !== b.id || a.elements.length !== b.elements.length) return false; return a.elements.every((v, i) => v === b.elements[i]); @@ -2402,10 +2411,19 @@

Using it with real data

if (typeof rule.check === "function") rule.check(doc); } - const before = snapshot(doc.segments); const countBefore = doc.segments.length; const summary = []; const ruleKindsBySegment = new Map(); + // Segment index -> the segment as it was before any rule touched it. Each + // rule copies a segment on its first write to it and carries that copy in + // the mark, so only segments that actually change are ever copied. This + // replaced a clone of the entire document taken up front, which on a large + // file was a second full working set built to find a handful of changes. + // + // First writer wins. Two rules can hit one segment, and the second one's + // copy has the first one's edit already in it -- the change list has to + // show what arrived, not what the second rule happened to find. + const beforeBySegment = new Map(); for (const rule of rules) { const result = rule.apply(doc.segments); @@ -2417,6 +2435,9 @@

Using it with real data

occurrences: result.occurrences, }); for (const mark of result.marks) { + if (mark.before && !beforeBySegment.has(mark.segment)) { + beforeBySegment.set(mark.segment, mark.before); + } let entry = ruleKindsBySegment.get(mark.segment); if (!entry) { entry = { kinds: [], elements: new Map() }; ruleKindsBySegment.set(mark.segment, entry); } if (!entry.kinds.includes(rule.kind)) entry.kinds.push(rule.kind); @@ -2432,22 +2453,36 @@

Using it with real data

// // The change list below pairs segments by index, which only holds while // the count is unchanged; a rule that changes it will need that revisited. + // Note the counts this repairs are not themselves listed as changes: the + // list is built from what the rules marked, and this repair is not a rule. + // That is moot while no rule changes the count, and is the second thing to + // revisit on the day one does. const countsRepaired = doc.segments.length === countBefore ? 0 : repairControlCounts(doc); + // Only the segments a rule marked can appear here, so this walks those + // rather than sweeping the whole document. sameSegment stays as a filter + // over them: a rule may mark a write that turned out to change nothing -- + // StringReplaceRule counts a match even when the replacement equals it -- + // and such a segment must not reach the Changes tab as a change with an + // identical before and after. What the filter no longer catches is the + // opposite case, a rule editing a segment without marking it; that is + // covered directly now, by the mark audits in parity.mjs and deid.mjs. const changes = []; - doc.segments.forEach((seg, i) => { - if (sameSegment(before[i], seg)) return; + for (const i of [...beforeBySegment.keys()].sort((a, b) => a - b)) { + const was = beforeBySegment.get(i); + const seg = doc.segments[i]; + if (sameSegment(was, seg)) continue; const entry = ruleKindsBySegment.get(i) || { kinds: [], elements: new Map() }; changes.push({ index: i, - before: before[i], + before: was, after: { id: seg.id, elements: seg.elements.slice() }, kinds: entry.kinds, elementKinds: entry.elements, }); - }); + } return { output: serialize(doc), @@ -4142,6 +4177,7 @@

Using it with real data

apply(segments) { const marks = new Map(); // segment index -> Set of element indices + const befores = new Map(); // segment index -> the segment as it arrived const mark = (i, el) => { let set = marks.get(i); if (!set) { set = new Set(); marks.set(i, set); } @@ -4149,6 +4185,13 @@

Using it with real data

}; const put = (i, el, value) => { if (segments[i].elements[el] === value) return; // no-op: don't claim a change + // Copy the segment before the first write lands on it, and only then -- + // processText no longer clones every segment up front, so this is the + // only chance to record what a rewritten value used to be. It has to + // happen here rather than in mark(), which runs after the assignment. + if (!befores.has(i)) { + befores.set(i, { id: segments[i].id, elements: segments[i].elements.slice() }); + } segments[i].elements[el] = value; mark(i, el); }; @@ -4289,7 +4332,7 @@

Using it with real data

const out = []; for (const [segment, elements] of [...marks].sort((a, b) => a[0] - b[0])) { occurrences += elements.size; - out.push({ segment, elements: [...elements] }); + out.push({ segment, elements: [...elements], before: befores.get(segment) }); } return { segments: out.length, occurrences, marks: out }; } diff --git a/web/tests/parity.mjs b/web/tests/parity.mjs index 7f7df7e..0c681fa 100644 --- a/web/tests/parity.mjs +++ b/web/tests/parity.mjs @@ -275,5 +275,26 @@ check("but the rule marks it anyway", noop.overclaimed.length > 0, true); check("and processText filters it back out", processText(raw, [new StringReplaceRule({ find: "ANYTOWN", replace: "ANYTOWN" })]).changes, []); +console.log("\n[14] two rules on one segment report what arrived, not what the second found"); +// The change list is assembled from per-rule copies now, and a second rule +// touching an already-edited segment copies it with the first rule's edit +// already in place. processText keeps the earliest copy. Get this wrong and +// the Changes tab shows a before that never existed in the input file: here +// it would claim 20230102, a value the shift rule invented moments earlier. +const chained = processText(raw, [ + new Dtp472ServiceLineShiftRule(1), // 20230101 -> 20230102 + new StringReplaceRule({ find: "20230102", replace: "19990101" }), // -> 19990101 +]); +const shiftedThenSwapped = chained.changes.filter((c) => c.before.id === "DTP"); +check("the service-line DTPs are listed once each, not twice", shiftedThenSwapped.length, 2); +check("before is the value from the input file", + shiftedThenSwapped.map((c) => c.before.elements[2]), ["20230101", "20230101"]); +check("after is the value both rules left behind", + shiftedThenSwapped.map((c) => c.after.elements[2]), ["19990101", "19990101"]); +check("both rules are credited on the segment", + [...new Set(shiftedThenSwapped.flatMap((c) => c.kinds))].sort(), ["shift", "swap"]); +check("the output agrees with the change list", + (chained.output.match(/DTP\*472\*D8\*19990101~/g) || []).length, 2); + console.log(`\n${pass} passed, ${fail} failed`); process.exit(fail ? 1 : 0); From 8a3f1e9f18ea59b2a0b038407ab6657776169c1a Mon Sep 17 00:00:00 2001 From: IPerception Date: Mon, 7 Sep 2026 22:22:45 -0400 Subject: [PATCH 4/4] docs: record what items 2 and 3 actually did, and correct the old numbers Two things in this file were wrong rather than merely stale. The heap column understated the app. The benchmark discarded its copy of the browsable document immediately; the app keeps it in state.doc for as long as the file is open. The 125 MB row moving from 1.99 GB to 2.90 GB is that correction, not a regression. And the column is heap after a forced GC, not peak heap, though it was labelled peak. That is what made the predictions for both items wrong: they were sold as memory savings on the theory that five copies were held at once, when the two being removed each lived and died inside a single run. Both delivered time instead -- together roughly halving run time at 100-125 MB -- and moved the heap column by about 2%. The reasoning error is written down next to the result, since the same mistake is available to anyone reading the copy table. Also records that the ceiling did not move: 150 MB was out of memory before and still is. Only the streaming parse moves that. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AfabdBfKu77QsNYVZkc3nn --- web/PERFORMANCE.md | 159 ++++++++++++++++++++++++++++----------------- 1 file changed, 101 insertions(+), 58 deletions(-) diff --git a/web/PERFORMANCE.md b/web/PERFORMANCE.md index 9b67ed3..fde66c7 100644 --- a/web/PERFORMANCE.md +++ b/web/PERFORMANCE.md @@ -1,15 +1,16 @@ # EDI Workbench (browser) — performance findings -**Measured 2026-08-03.** Status: findings recorded, no fixes applied yet. +**First measured 2026-08-03. Re-measured 2026-09-07** after backlog items 2 and 3 +landed. Status: those two are done; the rest of the backlog stands. ## Recommendation | Range | Verdict | |---|---| -| **up to 25 MB** | Comfortable. Sub-second load, ~1.3 s to run rules. | -| **25–50 MB** | Usable. Noticeable freeze (1–3 s) but nothing breaks. | -| **50–100 MB** | Degraded. 3–7 s frozen tab; Chrome may offer to kill the page. | -| **100–125 MB** | Hard edge. 15 s and ~2 GB heap at 125 MB. | +| **up to 25 MB** | Comfortable. Sub-second load, under a second to run rules. | +| **25–50 MB** | Usable. Brief freeze (1–2 s) but nothing breaks. | +| **50–100 MB** | Degraded. 2–4 s frozen tab; Chrome may offer to kill the page. | +| **100–125 MB** | Hard edge. 6 s and ~2.9 GB heap at 125 MB. | | **150 MB+** | Out of memory on a 4 GB tab. | For scale, 25 MB of 837P is roughly 30,000 claims. Most real batches sit well under @@ -21,21 +22,38 @@ about headroom and about not freezing the tab, not about unblocking normal use. Synthetic 837P, ~41k segments per MB, ~42 bytes per segment (close to real professional claims). Two rules active: service date shift + find/replace. -| File | Segments | Load & browse | Heap | Run rules | Peak heap | +| File | Segments | Load & browse | Heap | Run rules | Heap after run | |---:|---:|---:|---:|---:|---:| -| 1 MB | 41k | 17 ms | 8 MB | 57 ms | 15 MB | -| 5 MB | 206k | 83 ms | 39 MB | 230 ms | 74 MB | -| 10 MB | 410k | 144 ms | 79 MB | 446 ms | 148 MB | -| 25 MB | 1.0M | 351 ms | 195 MB | 1.3 s | 367 MB | -| 50 MB | 2.0M | 642 ms | 392 MB | 2.6 s | 736 MB | -| 64 MB | 2.6M | 855 ms | 495 MB | 3.1 s | 934 MB | -| 80 MB | 3.3M | 1.0 s | 624 MB | 5.0 s | 1.17 GB | -| 100 MB | 4.2M | 1.4 s | 883 MB | 6.9 s | 1.58 GB | -| 125 MB | 5.2M | 1.8 s | 1.11 GB | 15.3 s | 1.99 GB | +| 1 MB | 42k | 15 ms | 8 MB | 50 ms | 22 MB | +| 5 MB | 208k | 76 ms | 43 MB | 174 ms | 115 MB | +| 10 MB | 415k | 133 ms | 88 MB | 332 ms | 232 MB | +| 25 MB | 1.0M | 366 ms | 221 MB | 862 ms | 579 MB | +| 50 MB | 2.1M | 769 ms | 446 MB | 1.8 s | 1.17 GB | +| 64 MB | 2.7M | 953 ms | 565 MB | 2.2 s | 1.48 GB | +| 80 MB | 3.3M | 1.2 s | 712 MB | 3.3 s | 1.86 GB | +| 100 MB | 4.2M | 1.4 s | 882 MB | 4.0 s | 2.31 GB | +| 125 MB | 5.2M | 2.0 s | 1.11 GB | 6.2 s | 2.90 GB | | 150 MB | — | — | — | out of memory | — | Re-run with `node web/bench/bench.mjs` (see that file's header for options). +### Two corrections to the first round of numbers + +**The heap column was understating the app, and is now larger.** It is not a +regression. The benchmark used to throw away its copy of the browsable document +the instant it built it, while the app keeps that document in `state.doc` for as +long as the file is open. So the old column measured a state the app is never in. +The 125 MB row reading 1.99 GB before and 2.90 GB now is that correction, not a +change in what the code does. + +**It is also "heap after the run", not peak heap, and used to be labelled peak.** +The measurement is taken after a forced GC, so it reports what is still reachable +when the run finishes — it cannot see a copy that is built and discarded during +the run. That is exactly what made the original predictions for items 2 and 3 +wrong: both removed transient copies, so both moved this column barely at all +while cutting the time substantially. Bear it in mind before predicting a memory +saving from a copy that does not outlive `processText`. + ### Caveats on these numbers - **Measured in Node, not a browser.** Same V8, so the arithmetic transfers, but a @@ -49,28 +67,49 @@ Re-run with `node web/bench/bench.mjs` (see that file's header for options). ## Why memory is the constraint, not CPU -Parsing costs about **8x the file size** in heap; running rules about **16x**. CPU is -comparatively cheap — the memory wall arrives first. The 16x is five full-size copies -of the same data: +Parsing costs about **8x the file size** in heap; running rules about **23x** of what +survives the run. CPU is comparatively cheap — the memory wall arrives first. What is +still reachable when a run finishes is three full-size copies of the same data: | # | Copy | Where | |---|---|---| -| 1 | Parsed segment objects | `parse()` — unavoidable, this is the working set | -| 2 | Before-snapshot clone of every segment | `processText()` → `snapshot(doc.segments)` | -| 3 | Serialized output string | `serialize(doc)` | -| 4 | `changes[]` holding before + after per changed segment | `processText()` | -| 5 | Re-parse of the output so the UI can browse the edited doc | `render()` → `state.doc = parse(result.output)` | +| 1 | Parsed segment objects, as the rules left them | `parse()` — unavoidable, this is the working set, and it is what the UI browses | +| 2 | Serialized output string | `serialize(doc)` | +| 3 | `changes[]` holding before + after per changed segment | `processText()` | -Copies 2 and 5 are pure waste and are the cheapest to remove. +Two more used to be built and thrown away during the run — a clone of every segment +taken before the rules ran, and a re-parse of the output so the UI had something to +browse. Both are gone; see below. Neither was reachable at the end of a run, which is +why removing them cut the time without moving the heap column much. + +Copy 3 is the one left with any slack in it: it holds a before and an after for every +changed segment, so a rule that rewrites most of a file approaches a second working +set. It is also the only record of what a run changed, so it cannot simply be dropped. ## Already handled -Not on the backlog — these were dealt with when the browser was built: +Not on the backlog — these were dealt with when the browser was built, or have since +been done: - **Document list is virtualized.** Only rows near the fold exist in the DOM, so rendering is O(viewport) regardless of file size. Rendering is not a cost factor. - **Changes tab caps at 500 rendered segments**, output preview at 400k characters. Both note the truncation; the download is always complete. +- **The output is no longer re-parsed** so the UI can browse it (was backlog item 2). + `processText()` returns the document it already mutated and `render()` takes it. + Cut run time by 26–37%. +- **Only changed segments are copied** (was backlog item 3). Each rule copies a + segment on its first write to it and carries the copy in the mark it already + reported, replacing a clone of the whole document taken up front. Cut run time by a + further 26% at 100 MB and 36% at 125 MB — the gain grows with the file, because the + extra copy was crowding the heap limit and paying for it in GC. + + This moved the correctness of the change list onto the accuracy of each rule's + `marks`. That is not free, and it is why `web/tests/_marks.mjs` exists: it audits, + for every rule, that nothing changes without being marked. `sameSegment` was kept + as a filter over marked segments — a rule marks what it *touched*, not what it + managed to change, and `StringReplaceRule` counts a match even when the replacement + equals it. ## Improvement backlog @@ -79,8 +118,10 @@ In the order I'd do them. ### 1. Get processing off the main thread, and show progress **Problem:** `run()` calls `processText()` synchronously, so the tab is frozen with no -feedback for the entire run — 2.6 s at 50 MB, 6.9 s at 100 MB. This is the worst part -of the current behaviour and it is a UX problem, not a throughput one. +feedback for the entire run — 1.8 s at 50 MB, 4.0 s at 100 MB. Items 2 and 3 roughly +halved those, which makes this the worst remaining part of the behaviour by a wide +margin. It is a UX problem, not a throughput one, and no further copy-removal fixes +it: a 4 s freeze with no progress indication reads as a hung tab whatever the number. **Fix:** move the engine into a Web Worker. The engine block in `EDIWorkbench.html` (between the `engine:start` and `engine:end` markers) has no DOM access precisely so @@ -93,52 +134,51 @@ even before the worker lands. **Expected gain:** no change to the ceiling; the tab stays responsive throughout. -### 2. Stop re-parsing the output - -**Problem:** `render()` does `state.doc = parse(result.output)` so the document -browser shows edited values. That's a second full parse and a second full set of -segment objects. - -**Fix:** `processText()` already holds the mutated document. Return it, and have -`render()` use it directly. Segment indices already align (no rule adds or removes -segments), which is the assumption `state.touched` relies on today — worth an -explicit comment when this changes. +### ~~2. Stop re-parsing the output~~ — done 2026-09-07 -**Expected gain:** removes copy #5, roughly a third of peak heap. +### ~~3. Snapshot only what changes~~ — done 2026-09-07 -### 3. Snapshot only what changes +Both are described under **Already handled** above, with what they actually bought. -**Problem:** `processText()` clones *every* segment up front to diff against. On a -5M-segment file that's a second full working set to detect ~1M changes. - -**Fix:** rules already return `marks` naming the segments and elements they touched. -Capture the before-values inside the rule at mutation time and carry them in the -mark, then drop `snapshot()` and the full comparison sweep entirely. - -**Watch:** the current diff is what makes the change list correct even if a rule -reports its marks inaccurately. Removing it moves that trust to the rules, so the -parity tests need to cover both rules' mark accuracy directly. - -**Expected gain:** removes copy #2, plus the O(n) comparison sweep. +Worth recording, because it is the lesson rather than the result: **both were +predicted as memory savings and both delivered time savings instead.** Item 2 was +expected to remove "roughly a third of peak heap" and moved the heap column by about +2%. The reasoning error was treating five copies as five things held at once, when +the two being removed were each built and discarded inside a single run — so at no +point did the file exist five times over. Before predicting a memory win from +removing a copy, check whether anything still refers to it when the run ends. ### 4. Guard rail for oversized files -After 1–3, put a soft warning in front of files over ~100 MB: state the expected time +After 1, put a soft warning in front of files over ~100 MB: state the expected time and let the user proceed. Better than an unexplained multi-second freeze or an out-of-memory crash. ### 5. Only if still needed: streaming parse Chunked parsing with progress reporting would push past 125 MB, but it's a large -change to a currently simple and well-tested parser. Don't start here — 1–3 should -move the ceiling far enough that this stays unnecessary. +change to a currently simple and well-tested parser. + +Note that items 2 and 3 **did not move the ceiling at all** — 150 MB was out of +memory before them and still is. They removed transient copies, and the ceiling is +set by what a run has to hold simultaneously at its worst moment, which is dominated +by the working set, the output string and the change list. If the ceiling is the +thing that actually needs to move, this item is what moves it, and nothing cheaper +will. Don't start here anyway: 125 MB is far past any real batch. ## How to verify an improvement -1. `node web/bench/bench.mjs` before and after; compare peak heap and run time. -2. Re-run the correctness suites — the engine must stay byte-for-byte identical to - the Python `edi_engine` output. Items 2 and 3 both touch `processText()`, which is - exactly what those tests pin down. +1. `node web/bench/bench.mjs` before and after; compare run time and heap. **Measure + the same tree with only the change absent** — not against the table above, which + was taken on a different machine and, before 2026-09-07, with a benchmark that + modelled the app's retained memory incorrectly. Stashing the change and re-running + is the cheap way to get a comparison worth quoting. +2. Remember the heap column is measured after a forced GC. It will not show you a + copy that dies before the run returns, however large that copy was. +3. Re-run the correctness suites — the engine must stay byte-for-byte identical to + the Python `edi_engine` output. Anything touching `processText()` is exactly what + those tests pin down, and anything touching how rules report `marks` is now pinned + by the audits in `_marks.mjs`. ## Test harnesses @@ -148,3 +188,6 @@ claim index and CSV export, control counts and claim balance, de-identification, virtual-list windowing math, the load path against a throwaway DOM, and the static self-containment lint); `node web/bench/bench.mjs` runs the benchmark that produced the numbers above. + +`web/tests/_marks.mjs` is not a suite of its own — it is the mark audit, called from +`parity.mjs` and `deid.mjs`, which are where each rule is already wired up.