Repository navigation
Perf/cache skill and grammar parses - #221
Merged
swedishborgie merged 2 commits intoSep 25, 2026
Merged
Conversation
added 2 commits
September 24, 2026 13:14
parser() and paramsParser() rebuilt the participle grammar/lexer from scratch on every call via lexer.MustSimple + participle.MustBuild. Since the grammar is static (both functions take no arguments), this repeated compilation work on every parseGrammar/paramsParser.ParseString call for no benefit. Hoist construction into a sync.OnceValue-memoized package var, built once on first use and reused for every subsequent call, including concurrently (participle.Parser[T] is immutable after Build; all per-parse mutable state lives in a parseContext constructed fresh inside ParseString/ParseFromLexer). Verified safe under -race with a concurrent reuse benchmark. Measured on a lint run over a large context tree (~400 tasks against a plugin registry contributing several thousand skills), comparing pprof captures before/after: - newTagLexer cumulative allocations: 176.7M objects -> 0 (no longer invoked per call) - lexer.Upgrade cumulative allocations: 985.1M -> 640.7M objects (-35%) - total heap objects for the run: 10.60B -> 9.41B (-11.2%, ~1.19B fewer) - wall-clock: 625s -> 564.9s (-9.7%)
discoverSkills re-parses every SKILL.md on each call, and callers that construct a Context per unit of work re-walk the whole skill tree each time. A lint-all-tasks loop does exactly that: one (or two, when the task imports a plugin) full discovery pass per task, over an unchanged tree. The same files were parsed once per task rather than once per run. Add an optional SkillCache, attached to the context via NewContextWithSkillCache and keyed by skill file path, so repeated discovery reuses earlier parse results. lenient does not affect parsing, so the path alone is a sufficient key; results are captured before validateAndAddSkill's lenient name inference, keeping the cached value mode-independent. A context without a cache attached behaves exactly as before: the cache methods are nil-receiver-safe, so discoverSkills parses every call. Only deterministic outcomes are memoized. A *markdown.ParseError means the file was read and its content rejected, so every later call would reject it identically and caching it saves a re-parse per task. An I/O error says nothing about the next attempt — permissions, fd exhaustion, a flaky network filesystem, a file that vanished mid-walk — so those are left uncached and retried; memoizing one would let a single transient blip early in a run fail every later task. On a cached error the frontmatter is stored zero rather than as parsed, since parsing fills it field by field and can fail partway, leaving a half-built struct that a later reader might trust. Cached frontmatter is shared by value, and copying the struct copies only the map headers of Content and Metadata. Nothing writes to either map in production today, and the sharing is safe only while that holds, so both fields are documented read-only. Consumers in the lint path run sequentially, so a future write there would surface as cross-task contamination rather than a race the detector would catch. Measured on a lint run over a large context tree (~400 tasks against a plugin registry contributing several thousand skills): ~559-579s before, 113s after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
perf: cache skill parses and grammar construction across Context instances
Two independent caching fixes for callers that construct many
Contextinstances against the same tree. Both are opt-in or transparent; no behavior changes.1. Reuse participle grammars (
taskparser/grammar.go)The task and params grammars were rebuilt on every parse. Now built once via
sync.OnceValue.Profiling showed
newTagLexereliminated,lexer.Upgradedown 35%, and ~1.19B fewer heap objects allocated (~11% of total allocations).2. Optional skill parse cache (
skillcache.go)discoverSkillsre-parses everySKILL.mdon each call. A caller that constructs oneContextper unit of work re-walks the whole skill tree each time — the same files parsed once per task rather than once per run.Adds
SkillCache, attached viacontext.ContextwithNewContextWithSkillCacheand keyed by file path. A context without one behaves exactly as before: the cache methods are nil-receiver-safe, so discovery parses on every call.Only deterministic outcomes are memoized. A
*markdown.ParseErrormeans the file was read and its content rejected, so the verdict is stable and worth caching. I/O errors — permissions, fd exhaustion, a flaky network filesystem, a filethat vanished mid-walk — describe one attempt, not the file, so they are left uncached and retried.
Cached frontmatter is shared by value, and copying the struct copies only the map headers of
ContentandMetadata. Nothing writes to either map today; both are now documented read-only, since the sharing is only safe while that holds.Measured
On a downstream lint-all-tasks run (~400 tasks against a registry contributing several thousand skills): ~559–579s → ~113s.
Tests
New coverage for cache sharing across contexts, cached parse errors, and non-cached transient I/O errors. Full suite passes under
-race.