Skip to content

Patcher optimizations - #194

Draft
sparr wants to merge 3 commits into
sarbian:masterfrom
sparr:faster-mm-patching
Draft

sparr wants to merge 3 commits into
sarbian:masterfrom
sparr:faster-mm-patching

Conversation

@sparr

@sparr sparr commented Oct 30, 2026 •

Copy link
Copy Markdown

Reduces an uncached launch of a large RP-1 install by about 55% to the main menu, 76% off ModuleManager's patching time. The patched output is identical. Launches with an not-invalidated cache are unaffected.

Commits

1. Speed up patch node matching (bb61d3a).
This commit can be broken up into smaller commits if requested.

  • Currently, WildcardMatch builds a regex pattern string and a closure on every call before checking its cache, and runs a regex for patterns with no * or ?. This PR compares such names directly and caches regexes by pattern.
  • Currently, CheckConstraints re-parses its constraint string for every node. This PR introduces a new parsed constraint class and caches after the initial parse of each constraint.
  • This PR walks matching subnodes in one pass, instead of calling FindConfigNodeIn for each index. FindConfigNodeIn no longer builds a list.
  • Generally, allocations removed by all of the above changes result in fewer GC waits on the patching thread.

2. Index root nodes by type for edit patches (904fc4b). Currently, every edit patch tests every root node, although a matcher only accepts one exact node type. This PR indexes nodes grouped by type, in list order, and edit patches visit only their own type's group. Edits replace values in place and cannot rename a root node, so they leave the index valid. Copy, delete, and insert patches mark the affected type stale, and it is rebuilt when next needed. IPatch.Apply is unchanged.

3. Match patches against each config's stored name value (ccec578). Currently, NodeMatcher calls GetValue("name") on every node of the patch's type, for every patch with a name pattern. This PR keeps the node's name Value object when ProtoUrlConfig reads it. Renames by edit and copy patches create a new ProtoUrlConfig, and the Special command's in-place writes (*@PART[foo]/name = bar) change that same Value object, so both are seen.

Benchmarks

Time for a forced full repatch. "Patching" is ModuleManager's logged Ran in time. "Total" runs from launch to the main menu scene change.

Patches applied Patching Total
None 463 s 642 s
1: faster matching 204 s 387 s
2: type index 416 s 595 s
3: stored name value 416 s 592 s
1 + 2 131 s 307 s
1 + 2 + 3 111 s 285 s
  • The install is KSP 1.12.5 with RP-1 and various other mods, 164,005 applied patches, on a Ryzen 5 5600X under Linux. It ran on a virtual display through VirtualGL, without an attached profiler or debugger.

Verification

  • ModuleManager.ConfigCache, .TechTree, .Physics, and .ConfigSHA were identical to the unpatched build's in all six configurations.
  • A new test covers a root node renamed in place by *@.
  • The test suite passes under Mono: 476 tests, 14 of them new.

Behavior differences

  • CheckConstraints stops at the first matching subnode. A later subnode with a null name would previously have thrown, and now is not reached. Null node names are not expected in practice.

AI Disclosure

Claude Opus 5.5 produced 99% of the code, 100% of the commit messages, and 50% of this PR description. I have lightly reviewed the code and am proceeding with a more thorough review. I will un-draft this PR when I am confident in its implementation. I won't be offended at all if this is considered an issue with POC code instead of a PR and someone reimplements it by hand instead.

sparr and others added 2 commits October 30, 2026 11:05
Matching each patch against every node spent most of its time
allocating, and the allocations stalled the patching thread in Boehm GC
waits. In a profiled RP-1/RO repatch, NodeMatcher.IsMatch was 55% of
the patching thread, WildcardMatch 36%, and GC waits 22%.

- WildcardMatch compares strings directly when the wildcard has no * or
  ?, keeping the regex's match before a final newline, and caches its
  regex by the wildcard instead of building the pattern on every call.
- CheckConstraints parses each constraint string once and caches it.
- CheckConstraints visits the matching subnodes in one pass instead of
  calling FindConfigNodeIn with each index in turn.
- FindConfigNodeIn no longer builds a list of the matching subnodes.
- KeyValueCache.FetchWithKey takes a delegate that need not capture.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every edit patch tested every root node, and NodeMatcher.IsMatch
rejects most of them by type alone.

PatchApplier now keeps a PatchTypeIndex of the database's list nodes,
grouped by node type in list order, and gives it to edit patches, which
visit only the nodes of their matcher's type. Edits replace values in
place and cannot rename a root node, so they leave the index valid; it
is invalidated if a type changes anyway. Copy and delete patches mark
their matcher's type stale and insert patches their node type, and a
stale group is rebuilt when next needed. IPatch.Apply is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sarbian

sarbian commented Oct 30, 2026

Copy link
Copy Markdown
Owner

Nice. Those numbers looks great. I will let you test it.

@sarbian

sarbian commented Oct 30, 2026

Copy link
Copy Markdown
Owner

One correctness issue in the third commit (ccec578): the assumption that nothing changes a root node's name value in place doesn't hold.

The Special * command can write to another root node directly. RecurseVariableSearch returns the live ConfigNode.Value from the database config, and ModifyNode assigns to it (val.value = modVal.value) without creating a new ProtoUrlConfig, so NameValue keeps the old name.

@PART[source] { *@PART[target]/name = renamed }
@PART[renamed] { x = 1 }
@PART[target] { y = 2 }

On master, @PART[renamed] applies to that node and @PART[target] doesn't. With this PR it's the reverse, with no error logged, until another edit happens to rewrap the node. The temporary stored-vs-live check wouldn't catch this unless the install has such a patch, and no test covers *@; TestIsMatch__ProtoUrlConfig__UsesNameValue actually pins the stale behaviour.

Possible fixes: have the Special branch refresh the affected ProtoUrlConfig, or fall back to the live name when a patch run contains a *@ write that targets a root node.

Found by reading the code; I haven't run this scenario.

NodeMatcher.IsMatch looked up a node's name value for every patch with
a name pattern, once per node of the patch's type. With the previous
two commits applied, in a profiled RP-1/RO repatch, that lookup was
about 15% of the patching thread.

ProtoUrlConfig already looked up the name value for FullUrl; it now
keeps the Value object and exposes its value as NameValue.
NodeMatcher.IsMatch(IProtoUrlConfig) matches against it, and edit,
copy, and delete patches call that through an INodeMatcher extension
that falls back to IsMatch(ConfigNode) for other matchers. A special
command (*@part[foo]/name = bar) writes to that Value object in place,
so its renames are seen. Edits and copies modify a copy and wrap it in
a new ProtoUrlConfig, so their renames are seen too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sparr
sparr force-pushed the faster-mm-patching branch from ccec578 to 46e359d Compare October 31, 2026 16:46
@sparr

sparr commented Oct 31, 2026 •

Copy link
Copy Markdown
Author

Pushed a fix. Instead of caching the name string and refreshing when it changes, ProtoUrlConfig now caches a reference to the underlying name object, so they can't be out of sync after a name change. Added a test to cover the name change case. Benchmark results are +/-1%, within the noise margin on my PC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants