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 caches the name value when ProtoUrlConfig reads it. Renames already trigger another ProtoUrlConfig so that's automatically handled. This relies on nothing changing a root node's name value in place during patching, and I couldn't find anything that does.

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 temporary build of all three commits compared the stored and live name on every match during a full repatch and found no difference. The same check, given one deliberately wrong stored name, logged it and changed the output.
  • 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 3 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>
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 read the name value for FullUrl; it now keeps it
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.
This assumes nothing changes a root node's name value in place during
patching: edits and copies modify a copy and wrap it in a new
ProtoUrlConfig, so renames by patches are seen.

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.

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