perf(canon): Reconcile parses the documents seven times, not twelve

.claude/skills/fix-issue/findings/modelsdk/2026-11-13-reconcile-parses-both-documents-per-step.json

@@ -0,0 +1 @@
+{"area": "modelsdk/canon", "date": "2026-11-13", "symptom": "canon.Reconcile was 23 s of a 77 s settled re-run of 600 pages, about 38 ms for each rebuilt page compared with its stored unit (ako/mxcli#2160)", "cause": "Reconcile is a row of steps from bytes to bytes - carry translations, transplant ids, carry PersistentIds, carry what MDL cannot state, compare - and each parses both documents for itself: twelve whole-document parses for one write. Five of them served steps that either only read the stored document or did not need a tree at all: the id substitution parsed the document before and after patching to read its ids, the PersistentId carry parsed both documents to find that a page has no PersistentId, and the transplant and the final comparison each parsed the stored document", "file": "modelsdk/canon/identity.go (Reconcile, parseStored, equalToStored, reconcileInPasses), modelsdk/canon/transplant.go (transplantIDs, idMappingWith, elementIDSetOf), modelsdk/canon/persistentid.go (persistentIDElement), modelsdk/canon/canon.go (digestOfDoc)", "insight": "The obvious change - parse each document once and hand the trees to every step - is unsafe here, and reading the steps showed why before any test did: the translation carry and the unexpressed carry copy parts of the stored tree into the new one, so a shared stored tree could be changed under the steps after them, and the new document changes between steps. Only the steps that provably just read the stored document share its parse (the transplant's pairing and the final digest); the carries keep a parse of their own. The other two savings need no tree: the ids are read off the raw bytes, and a document that does not contain the bytes of a binary PersistentId element has nothing to carry. Twelve parses become seven. The old Reconcile is kept verbatim as reconcileInPasses and compared byte for byte, with the verdict, on every PedApp unit against itself, a rebuild with new ids, rebuilds with properties left out, and the reverse: 2,214 comparisons. The fixture has no workflow, so the PersistentId path was unexercised on both sides; a workflow-shaped pair is compared separately, and always skipping the carry fails three tests. A first version of that control did not compile (the mutation left an import unused) and printed nothing, which reads like a pass: a control has to be seen to fail. End to end, a script changing all 600 pages run with the old and the new binary leaves 1,719 byte-identical units, 600 of which differ from before the run. Measured: settled page re-run 74.3 s -> 61.6 s, changed-pages run 72.1 s -> 65.6 s."}

CHANGELOG.md

@@ -35,6 +35,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - **`check` resolves an `alter page` target against the building blocks the page holds** (ako/mxcli#2182, mendixlabs/mxcli#1835) — an `alter page` / `alter snippet` on a document the same script creates was never resolved, so `set … on a_container99` after `use building block PlatformDesignSystem.Alert_Info as a_` passed `check -p` and exec then stopped on `widget "a_container99" not found`, with the page already created. A plain misspelt widget passed the same way. Check now replays the documents the script creates, expanding each `use building block` and `use fragment` as exec does (prefix included), and refuses a target they do not hold at that point as MDL-ALTER02, suggesting the nearest name (`did you mean a_container1`). On a stored page, the names a block inserted by `alter page … insert` are now its expanded names: `insert after s_container2` after inserting a block `as s_` is no longer refused, and `set … on s_text99` is no longer waived. Without a project a block cannot be expanded, so every name under its prefix is accepted (every name, for an unprefixed block), and a page holding DataGrid 2 columns or object-list items, whose addresses are derived, is not judged. The duplicate-name side (ako/mxcli#559) was already fixed.
 - **A pluggable widget's template is prepared once per widget type** (ako/mxcli#2160) — every data grid, filter and other pluggable widget a script builds loaded its template by deep-cloning the built-in one and merging the project's widget package into the clone, the same work each time. The merged template is now made once for each widget type and package and reused; each widget still gets IDs of its own. Creating 600 pages: 29.5 s before, 26.5 s after; re-running them settled: 83.1 s before, 73.9 s after. The pages written are the same: all 1,719 units of the two projects compare equal once IDs are set aside.
 - **`check` no longer decodes every microflow of the project before it starts** (mendixlabs/mxcli#1842, ako/mxcli#2160) — two of its checks, flow access (who may run a flow) and entity-access calls (MDL-SEC22), each read every microflow in full once per run, so a `check` of five statements cost seconds on a large project whatever the script held. They now read the few values they need from each stored flow, and decode a flow's body only when it contains a call. What `check` reports is unchanged. On a project of 5,373 units, with files already read once: `check` of 5 `create or modify microflow` statements 2.0 s before, 0.9 s after; of 200, 3.9 s before, 2.8 s after.
+- **Comparing a write with the stored unit parses the documents seven times where it parsed them twelve** (ako/mxcli#2160) — every unit mxcli writes is first reconciled with the stored one: stored element ids and translations are carried over, and a write that changes nothing is dropped. Each step of that parsed both documents for itself. The steps that only read the stored document now share one parse, the ids are read without parsing, and the workflow-only step is skipped for a document that is not a workflow. The bytes written are the same: a script that changes 600 pages leaves the same 1,719 unit files, byte for byte, with the previous build and this one. Re-running 600 settled pages: 74.3 s before, 61.6 s after.
 - **A pluggable widget's own `caption`, `label`, `buttonStyle` and `icon` are the widget's, not the core properties of that name** (ako/mxcli#2180, mendixlabs/mxcli#1833) — PlatformDesignSystem's Button declares property keys named `caption`, `buttonStyle` and `icon`, its Form label one named `label`, and mxcli read those names as the core widget properties first. `alter page … set (caption: 'Save') on btn` failed with "widget has no Caption property"; `set (buttonStyle: 'outline')` printed "Altered page" and wrote nothing; `set (label: …)` wrote a core label beside the widget's own one, and `create page` did the same, so `describe` printed two labels; `check` refused the widget's own `buttonStyle: 'fillBlue'` as an unknown Mendix button style (MDL-WIDGET02) and its `icon: 'bookmark'` as "not a qualified reference"; and `create page` — and a `use building block` copy of a block holding one — dropped the `icon`. Where the widget's definition declares the key (not as a system property, as a combo box declares `Label`), the name now means that key in all three: ALTER writes it, `create page` stores it, and `check` judges the value against the widget's own enumeration. Core buttons, and pluggable widgets without such a key, are unchanged. Measured on Mendix 11.12.1 with a widget package shaped like the PDS Button: before, two `check` errors, a dropped icon and a second LabelTemplate per widget; after, `check` clean, every value stored in the widget's own property, `mx check` 0 errors.
 - **`describe page` loads the page it describes** (ako/mxcli#2160) — it loaded every page with the same local name, in whatever module, before picking the one asked for, and read the page's document twice. `Overview`, `NewEdit` and `Home` repeat across the modules of most projects, and rewriting a page describes the stored one up to three times, so a script that re-runs its pages paid for it per statement. Output is unchanged. Re-running a settled script of 600 `create or modify page` statements: 87.8 s before, 77.2 s after.
 - **The decode cache compares a unit's bytes once per run** (ako/mxcli#2160) — the cache that spares a script decoding the same unit again recognises the unit by its bytes. It kept the buffer of an earlier pass, so in the next pass every listing compared every unit of the type byte for byte: all of a project's pages, twice per page statement. It now keeps the buffer it matched. Re-running a settled script of 600 `create or modify page` statements: 103.7 s before, 86.8 s after.

modelsdk/canon/canon.go

@@ -67,11 +67,17 @@ func DigestMasking(raw []byte, mask map[string]bool) (string, error) {
 	if err := bson.Unmarshal(raw, &doc); err != nil {
 		return "", fmt.Errorf("unmarshal BSON (%d bytes): %w", len(raw), err)
 	}
+	return digestOfDoc(doc, mask), nil
+}
+
+// digestOfDoc is DigestMasking for a document already parsed. It only reads
+// doc.
+func digestOfDoc(doc bson.D, mask map[string]bool) string {
 	ids := collectElementIDs(doc)
 	var b strings.Builder
 	renderValue(&b, doc, ids, mask)
 	sum := sha256.Sum256([]byte(b.String()))
-	return hex.EncodeToString(sum[:]), nil
+	return hex.EncodeToString(sum[:])
 }
 
 // collectElementIDs walks containment in a deterministic order and numbers every

modelsdk/canon/identity.go

@@ -4,9 +4,12 @@ package canon
 
 import (
 	"bytes"
+	"fmt"
 	"os"
 
 	"go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore"
+
+	"go.mongodb.org/mongo-driver/v2/bson"
 )
 
 // Reconcile is ADR-0008 decision 1, as a single decision both storage engines
@@ -131,8 +134,14 @@ func Reconcile(contents, stored []byte, opts ...Option) (out []byte, unchanged b
 	// version control as a whole-document replacement. TransplantIDs puts the
 	// stored ids back on the elements that still correspond, rewriting every
 	// reference with them.
+	//
+	// The stored document is parsed once here for the two steps that only read
+	// it, this pairing and the comparison at the end (parsedStored). The carries
+	// in between copy parts of the stored document into the new one and keep a
+	// parse of their own.
+	storedDoc := parseStored(stored)
 	if !o.contentsOwnElementIDs {
-		contents = TransplantIDs(contents, stored)
+		contents = transplantIDs(contents, storedDoc)
 	}
 
 	// And the nested identity property the transplant does not cover: every
@@ -152,6 +161,70 @@ func Reconcile(contents, stored []byte, opts ...Option) (out []byte, unchanged b
 	// (ako/mxcli#1897).
 	contents = CarryUnexpressed(contents, stored)
 
+	if alwaysWrite() {
+		return contents, false
+	}
+	eq, err := equalToStored(contents, storedDoc)
+	if err != nil {
+		return contents, false
+	}
+	return contents, eq
+}
+
+// reconcileInPasses is Reconcile as it was before its read-only steps shared a
+// parse of the stored document: every step on the bytes alone. It is what
+// Reconcile is defined by and what its test compares it with.
+func reconcileInPasses(contents, stored []byte, opts ...Option) (out []byte, unchanged bool) {
+	if len(stored) == 0 {
+		return contents, false
+	}
+	var o reconcileOpts
+	for _, fn := range opts {
+		fn(&o)
+	}
+
+	// Identity is carried unconditionally. MXCLI_ALWAYS_WRITE turns off *eliding*
+	// the write, not preserving what the document is: a forced write that
+	// re-minted StableId would renumber the deployed model's operation ids, which
+	// is a change to the app rather than a debugging aid.
+	contents = CarryIdentity(contents, stored)
+
+	// The translations the statement had no way to say. MDL carries ONE string
+	// per text, so a rebuild drops every other language the document held — a
+	// describe→exec round-trip deleted a page's Dutch title from a real project.
+	// Runs before the transplant because it is the one carry that changes the
+	// document's length; the transplant patches fixed-width binaries in place and
+	// must see the final framing.
+	if !o.contentsOwnTranslations {
+		contents = CarryTranslations(contents, stored)
+	}
+
+	// The same reasoning one level down, for the writes that do land: a rebuild
+	// mints a fresh $ID for every sub-element, so a two-line change reads in
+	// version control as a whole-document replacement. TransplantIDs puts the
+	// stored ids back on the elements that still correspond, rewriting every
+	// reference with them.
+	if !o.contentsOwnElementIDs {
+		contents = transplantIDsInPasses(contents, stored)
+	}
+
+	// And the nested identity property the transplant does not cover: every
+	// Workflows$* element carries a PersistentId that both engines re-mint on
+	// every write, so without this a workflow document never equals itself and
+	// no-op elision could never fire for one (issue #949).
+	contents = carryPersistentIDsUnfiltered(contents, stored)
+
+	// A property MDL cannot state: a tab container's default tab. The writer
+	// points it at the first tab, so a round trip rewrote a page whose stored
+	// default was null and reset any other choice (ako/mxcli#1897). Needs the
+	// transplanted $IDs, so it runs after TransplantIDs.
+	contents = CarryDefaultTabPages(contents, stored)
+
+	// The rest of what MDL cannot state: an expression's surrounding whitespace
+	// and an empty screen-reader label Studio Pro stores as a ClientTemplate
+	// (ako/mxcli#1897).
+	contents = CarryUnexpressed(contents, stored)
+
 	if alwaysWrite() {
 		return contents, false
 	}
@@ -162,6 +235,34 @@ func Reconcile(contents, stored []byte, opts ...Option) (out []byte, unchanged b
 	return contents, eq
 }
 
+// parsedStored is the stored document parsed for the steps of Reconcile that
+// only read it. ok is false for bytes that are not a document, which those
+// steps treat as they treat a failed parse of their own.
+type parsedStored struct {
+	doc bson.D
+	ok  bool
+}
+
+func parseStored(stored []byte) parsedStored {
+	var doc bson.D
+	if err := bson.Unmarshal(stored, &doc); err != nil {
+		return parsedStored{}
+	}
+	return parsedStored{doc: doc, ok: true}
+}
+
+// equalToStored is Equal(contents, stored) with the stored side already parsed.
+func equalToStored(contents []byte, stored parsedStored) (bool, error) {
+	da, err := Digest(contents)
+	if err != nil {
+		return false, fmt.Errorf("canonicalise left: %w", err)
+	}
+	if !stored.ok {
+		return false, fmt.Errorf("canonicalise right: not a document")
+	}
+	return da == digestOfDoc(stored.doc, nil), nil
+}
+
 // alwaysWrite disables no-op elision. An escape hatch for bisecting a suspected
 // elision bug ("does it still reproduce if every write lands?") without building
 // a patched binary — not a supported knob.

modelsdk/canon/persistentid.go

@@ -3,6 +3,7 @@
 package canon
 
 import (
+	"bytes"
 	"go.mongodb.org/mongo-driver/v2/bson"
 	"go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore"
 )
@@ -43,6 +44,24 @@ const persistentIDKey = "PersistentId"
 //
 // A document that cannot be read on either side passes through untouched.
 func CarryPersistentIDs(contents, stored []byte) []byte {
+	// Only a workflow's elements have the property. A pair needs it on the new
+	// side as a 16-byte binary, stored as this byte sequence: without it there
+	// is nothing to carry, and the two documents need not be parsed to find
+	// that out — as they were for every page, microflow and domain model
+	// written (ako/mxcli#2160).
+	if !bytes.Contains(contents, persistentIDElement) {
+		return contents
+	}
+	return carryPersistentIDsUnfiltered(contents, stored)
+}
+
+// persistentIDElement is how a binary PersistentId property starts in a stored
+// document: the binary type byte, the key, and the key's terminator.
+var persistentIDElement = []byte("\x05" + persistentIDKey + "\x00")
+
+// carryPersistentIDsUnfiltered is CarryPersistentIDs without the check that
+// the document has the property at all.
+func carryPersistentIDsUnfiltered(contents, stored []byte) []byte {
 	m := persistentIDMapping(contents, stored)
 	if len(m) == 0 {
 		return contents