From 673faea097fb9a178b5743e71cbdcbecd215568f Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 9 Oct 2026 00:34:21 +0000 Subject: [PATCH] Reduce path formatting allocations --- docs/roadmap/startup-benchmark.md | 175 +++++++++++++++++++++++++++++- docs/roadmap/startup.md | 10 +- internal/engine/path.go | 13 +++ internal/engine/path_test.go | 12 +- 4 files changed, 200 insertions(+), 10 deletions(-) diff --git a/docs/roadmap/startup-benchmark.md b/docs/roadmap/startup-benchmark.md index db82b3c..232cf48 100644 --- a/docs/roadmap/startup-benchmark.md +++ b/docs/roadmap/startup-benchmark.md @@ -1,6 +1,7 @@ # Startup construction baseline and first optimization proposal -Status: Stages 1–3 complete; optimization has not started. +Status: Stages 1–3 complete; the first Stage 4 experiment is retained. Further +optimization and complete consumer-workload verification remain outstanding. Measured on 2026-10-09 under the [startup roadmap](startup.md). The [consumer request](promptkit-performance-request.md) owns the submitted workload @@ -310,12 +311,174 @@ change only if repeatable benefit justifies its complexity. If it does not, return to profiling and assess pointer reparsing or repeated traversal rather than assuming the next optimization in advance. -## Validation and checkpoint +## Initial validation and checkpoint The maintained fixture's preflight and measured runs succeeded at every size. `./scripts/check.sh` passed formatting and whitespace checks, the offline suite (including local documentation links and facade boundaries), full vet, and -package builds. Stages 4–6 remain unstarted. No loading semantics, public API, -dependency, release gate, or consumer adoption decision changes in this work. -The checkpoint has sufficient evidence for one focused optimization experiment; -wider representation changes would need their own measured justification. +package builds. At that checkpoint, Stages 4–6 remained unstarted and no +production implementation had changed. The evidence supported one focused +optimization experiment; wider representation changes needed their own measured +justification. + +## Focused experiment: Pre-size `Path.String` + +Status: Retained on 2026-10-09. + +### Change and comparison setup + +The fresh unoptimized baseline is `91ae4fa7eff1571e7c05de01f9e922d75df7453d`, +including the maintained benchmark above. An isolated checkout preserved that +baseline while the working candidate changed only engine `Path.String` and +extended the existing path test. Toolchain, CPU, runtime settings, fixture sizes, +and timing boundaries are as recorded above. Fresh measurements used the same +benchmark commands: five 500-ms observations for construction/PromptKit, three +200-ms observations for complementary cases, and separate two-second profiles. +All measurement families ran sequentially. Logs and binaries are outside the +repositories at `/tmp/configkit-path-sizing.JpbMpD`. + +The sizing pass counts separators, token bytes, and the extra bytes required for +`~` and `/` escaping. A local 20-byte scratch array with `strconv.AppendInt` +measures index text without allocating another string. `strings.Builder.Grow` +reserves the calculated capacity. The original escaping, index-writing, root, +and invalid-path behavior remain intact. There is no cache, pool, shared state, +new validation, or change to segment ownership. The sizing pass adds work; +benefit is measured rather than assumed. + +The PromptKit checkout remained at +`eb830bb61346672f0f6c5bce6119b76ccc6d61ac`. Its temporary replacement first +selected the isolated baseline and then the ConfigKit working candidate. No +consumer repository or committed dependency changed. + +### Fresh construction results + +Each cell below is the median of that metric across five observations. Timing +improvements describe this run, without claiming statistical significance or +machine-independent speedups. + +| Workload | Before ms | After ms | Before B | After B | Before allocations | After allocations | +| --- | ---: | ---: | ---: | ---: | ---: | ---: | +| Local / 32 pairs | 18.251 | 17.495 | 11,999,426 | 11,036,879 | 129,136 | 80,465 | +| Local / 128 pairs | 68.096 | 60.859 | 47,320,688 | 43,490,008 | 513,720 | 319,131 | +| Local / 512 pairs | 262.823 | 252.779 | 188,764,416 | 173,640,328 | 2,051,979 | 1,273,697 | +| PromptKit / independent | 73.386 | 65.899 | 50,127,948 | 46,171,529 | 538,541 | 338,171 | + +Allocated bytes fell by 8.02%, 8.10%, and 8.01% locally; allocation counts fell +by 37.69%, 37.88%, and 37.93%. PromptKit showed 7.89% fewer allocated bytes and +37.21% fewer allocations. These reductions recur across observations and catalog +sizes. The downstream total continues to include its adapter and post-return +processing; it is not an isolated ConfigKit cost. + +Timing ranges (ms), keeping the first PromptKit outlier: + +| Workload | Before range | After range | +| --- | ---: | ---: | +| Local / 32 pairs | 17.468–18.518 | 16.719–19.131 | +| Local / 128 pairs | 66.211–79.363 | 58.048–63.724 | +| Local / 512 pairs | 256.783–287.161 | 241.361–272.743 | +| PromptKit / independent | 68.308–134.999 | 63.664–74.520 | + +The local 128-pair median and downstream median were approximately 10% lower; +the other local medians were 4% lower. Timing remains sensitive to shared-host +load, so the allocation reduction supplies the stronger evidence for retention. + +### Complementary workloads and possible timing regressions + +The original comparison's per-metric medians follow. These fixtures retain the +different boundaries explained above. + +| Benchmark / size | Before ms | After ms | Before B | After B | Before allocations | After allocations | +| --- | ---: | ---: | ---: | ---: | ---: | ---: | +| ComposeWideMappings / 128 | 0.883 | 0.806 | 848,507 | 797,562 | 9,658 | 5,962 | +| ComposeWideMappings / 256 | 1.781 | 1.708 | 1,703,582 | 1,595,293 | 19,652 | 11,860 | +| ComposeWideMappings / 512 | 4.460 | 3.963 | 3,626,263 | 3,403,282 | 39,643 | 23,659 | +| UnderlayBroadCatalog / 128 | 1.718 | 1.520 | 1,265,027 | 1,229,413 | 15,904 | 12,628 | +| UnderlayBroadCatalog / 256 | 3.707 | 3.231 | 2,580,999 | 2,459,362 | 33,473 | 24,821 | +| UnderlayBroadCatalog / 512 | 7.094 | 6.676 | 5,268,929 | 4,975,269 | 68,581 | 49,177 | +| InheritanceInspectedLoad / 16 | 1.124 | 1.296 | 876,204 | 835,688 | 11,050 | 8,060 | +| InheritanceInspectedLoad / 32 | 2.248 | 2.180 | 1,545,178 | 1,466,920 | 19,943 | 14,145 | +| InheritanceInspectedLoad / 64 | 4.030 | 3.784 | 2,906,020 | 2,752,258 | 37,778 | 26,360 | +| UnderlayRenderedHistory / 32 | 0.289 | 0.305 | 504,547 | 503,459 | 3,294 | 3,221 | +| UnderlayRenderedHistory / 64 | 0.624 | 0.735 | 1,006,052 | 1,003,942 | 6,529 | 6,392 | +| UnderlayRenderedHistory / 128 | 1.879 | 1.283 | 2,031,854 | 2,027,694 | 12,997 | 12,732 | + +Allocations improved in every case. Rendering benefits are smaller because its +load is outside timing. The first batch's 16-entry inheritance and smaller +rendering times were higher after the change, so they received longer follow-up +runs in reverse order (after, then before). + +Inheritance / 16 used `-benchtime=500ms -count=5`: before times were +2.537, 1.153, 1.148, 1.155, and 1.153 ms; after times were 1.308, 1.088, 1.086, +1.112, and 1.082 ms. Its median changed from 1.153 to 1.088 ms, reversing the +initial apparent regression, with the same allocation reduction. + +Rendering used `-benchtime=500ms -count=3`. Its timing medians were: + +| History size | Before ms | After ms | +| ---: | ---: | ---: | +| 32 | 0.290 | 0.302 | +| 64 | 1.538 | 0.610 | +| 128 | 7.213 | 1.329 | + +The before / 64 observations spanned 0.643–3.004 ms and before / 128 spanned +2.211–7.909 ms, exposing substantial host variation. Rendering / 32 remained +about 4% slower in this second batch, but the ranges overlapped (before +0.288–0.317 ms; after 0.299–0.307 ms). A small extra cost from the sizing pass +on short inspection paths cannot be ruled out. There is insufficient evidence +to claim every operation is faster; the stronger construction allocation benefit +and limited implementation complexity justify this tradeoff. These measurements +concern cumulative allocation; retained heap was not measured separately. The +change introduces no cache or altered data lifetime. + +### Reprofiled attribution and retention decision + +Fresh profiles used the local 128-pair fixture. The before profiled run measured +72.765 ms, 47,297,038 B, and 513,721 allocations; after measured 64.367 ms, +43,532,356 B, and 319,132 allocations. These remain separate from ordinary +timing comparisons. + +`Path.String` accounted for 14.38% of sampled allocation bytes before and 7.43% +after. The line profile previously attributed 62.00 MB to separator writes and +183.01 MB to token writes across the run; afterward it attributed 123.00 MB to +the initial `Grow`, with no sampled allocation at those writes. The profile +runs completed different operation counts (36 and 39), so raw MB totals must +not be treated as per-load differences. Direct benchmark measurements above +establish the per-load reduction. `Path.String`'s cumulative CPU share changed +from 21.15% to 18.96%; that share includes the added sizing work. + +Retain the change: a short local sizing pass removes approximately 195,000 +allocations and 3.83 MB from a representative 128-pair construction, with +matching benefits in the consumer and complementary cases. The existing +segment ownership and public behavior stay intact. The extra scan and uncertain +small-rendering timing cost are recorded rather than converted into a new +performance promise. + +### Next candidate and remaining work + +`clonePathSegments` remains the largest flat allocation site: 31.29% before and +34.99% after. Its larger share reflects other allocations disappearing; the +underlying copies were not changed. `ParsePath` remains at 4.92% after, while +`effectivePathIndex.func1` accounts for 3.84%. GC and map operations remain +prominent. Pointer formatting still accounts for nearly 19% of sampled CPU, +including `strings.Count` work, so further formatting changes would need to +justify their own extra complexity. + +The recommended next experiment is to investigate combining the two walks of +the final tree in `finalizeMetadataPaths`: it first calls `effectivePathIndex` +and then walks the same final tree again for historical paths. One traversal +could populate both indexes and format each final path once, while retaining +the separate records, contextual segment kinds, ownership copies, and all walks +of earlier/application/inheritance trees. This is a concrete redundancy to +measure before considering broader path representation or ownership changes. +Its expected benefit is not established yet. A generic `Keys` constructor +rewrite is smaller, but only accounted for 0.78% of sampled allocated bytes in +the after profile, so it is a lower-priority candidate. + +The path/metadata/provenance/query/explanation/secret tests passed with the +candidate. The existing path test now additionally checks decimal formatting +at three-digit and signed integer limits, including negative invalid indexes. +`./scripts/check.sh` passed formatting, whitespace, the offline suite (including +documentation links and facade boundaries), vet, and builds. This experiment +does not complete Stage 5: PromptKit's file-backed, shared-base, +and layered fixtures plus its applicable configuration tests still belong to +the final consumer verification. No consumer upgrade or release is published. diff --git a/docs/roadmap/startup.md b/docs/roadmap/startup.md index ac8c671..17d0692 100644 --- a/docs/roadmap/startup.md +++ b/docs/roadmap/startup.md @@ -1,7 +1,8 @@ # Startup Loading Performance Roadmap -Status: Stages 1–3 complete; optimization has not started. Findings and -reproduction commands are in [startup-benchmark.md](startup-benchmark.md). +Status: Stages 1–3 complete; Stage 4 is in progress with the first optimization +retained. Findings and reproduction commands are in +[startup-benchmark.md](startup-benchmark.md). Audience: ConfigKit maintainers implementing and reviewing this work. @@ -121,7 +122,10 @@ finding and recommend whether further investigation is justified. ### 4. Implement and measure focused changes -Status: Not started. +Status: In progress. `Path.String` buffer sizing is retained after fresh +construction, consumer, inspection, and profile comparisons. See the +[focused experiment](startup-benchmark.md#focused-experiment-pre-size-pathstring). +The next candidate is duplicate final-tree traversal during metadata finalization. 1. Implement the selected internal change while preserving the supported API and established behavior. Audit any reduced copying or reused data for diff --git a/internal/engine/path.go b/internal/engine/path.go index faff696..a956dab 100644 --- a/internal/engine/path.go +++ b/internal/engine/path.go @@ -86,7 +86,20 @@ func (p Path) String() string { return "" } + // Include each separator and the extra byte introduced by escaping a token. + // Use stack storage for sizing indexes without allocating another string. + size := len(p.segments) + var indexText [20]byte + for _, segment := range p.segments { + if segment.kind == pathSegmentIndex { + size += len(strconv.AppendInt(indexText[:0], int64(segment.index), 10)) + } else { + size += len(segment.key) + strings.Count(segment.key, "~") + strings.Count(segment.key, "/") + } + } + var b strings.Builder + b.Grow(size) for _, segment := range p.segments { b.WriteByte('/') if segment.kind == pathSegmentIndex { diff --git a/internal/engine/path_test.go b/internal/engine/path_test.go index e47d60b..9995a69 100644 --- a/internal/engine/path_test.go +++ b/internal/engine/path_test.go @@ -1,6 +1,9 @@ package engine -import "testing" +import ( + "strconv" + "testing" +) func TestPathRoundTripsSemanticKeys(t *testing.T) { t.Parallel() @@ -36,6 +39,13 @@ func TestPathRoundTripsSemanticKeys(t *testing.T) { if got := Keys("items").Index(12).Key("name").String(); got != "/items/12/name" { t.Fatalf("indexed path = %q", got) } + maximum := int(^uint(0) >> 1) + for _, index := range []int{100, maximum, -1, -maximum - 1} { + want := "/items/" + strconv.Itoa(index) + if got := Keys("items").Index(index).String(); got != want { + t.Fatalf("indexed path = %q, want %q", got, want) + } + } } func TestPathPreservesSegmentIntentAndImmutability(t *testing.T) {