Record the algorithmic efficiency audit
This commit is contained in:
206
docs/roadmap/audit/02-efficiency.md
Normal file
206
docs/roadmap/audit/02-efficiency.md
Normal file
@@ -0,0 +1,206 @@
|
||||
# Algorithmic correctness and efficiency audit
|
||||
|
||||
Status: four open P3 efficiency findings. Reviewed source:
|
||||
`d8da761a213c82837276266faeebf93beda8eb08`.
|
||||
|
||||
## Scope and architecture
|
||||
|
||||
The [internal overview](../../internal/overview.md) inventories the implemented
|
||||
owners. Core captures declarations, validates installation batches, resolves
|
||||
routes and bindings, coordinates invocations, and supplies completion and shared
|
||||
reporting. Its neutral metadata and internal Cobra/pflag adapter separate public
|
||||
contracts from backend state. ckconfig prepares scopes and comparison inputs and
|
||||
projects ConfigKit evidence. ckprompt assembles PromptKit operations and adapts
|
||||
native requests, catalogs, reports, and smoke results. Applications own loaders,
|
||||
builders, and execution dependencies; upstream libraries own domain algorithms.
|
||||
Release launchers delegate maintainer operations to ReleaseKit.
|
||||
|
||||
This pass reviewed declaration/registry traversal, alias compilation and help
|
||||
discovery, parsing/completion, operand allocation, captured binding replay,
|
||||
report conversion and delivery, integration selection/filtering/projection, and
|
||||
maintained consumer/tooling boundaries. Findings describe source-level scaling,
|
||||
not measured bottlenecks or new performance guarantees. No benchmarks or
|
||||
real-world performance measurements were gathered.
|
||||
|
||||
Explicitly selected approaches were excluded from reconsideration:
|
||||
|
||||
- Fresh mutable backend state per invocation, immutable metadata reuse, and
|
||||
report buffering, as selected in the retired performance records and current
|
||||
[internal overview](../../internal/overview.md).
|
||||
- The finite pushdown termination analysis, longest-prefix routing semantics,
|
||||
canonical flag endpoints, scoped precedence, and intermediate-route discovery
|
||||
selected in the retired alias implementation plan at `f42aefe`.
|
||||
- Completion's explicit routing check for each proposed child, selected in the
|
||||
retired completion implementation plan at `10a799a`.
|
||||
- JSON report normalization, whole-report aggregation, and the composed example's
|
||||
invocation-local list cache using reflect.DeepEqual, selected in the retired
|
||||
adoption implementation plan at `2f43f78`.
|
||||
- The [deferred scope/selector and backend work](../future.md#possible-further-efficiency-work),
|
||||
upstream domain algorithms, and the completed dense occurrence reconstruction,
|
||||
heap merge, validation-filter indexing, and subtree-prefix preparation in
|
||||
[remediation.md](../remediation.md).
|
||||
|
||||
The findings preserve those decisions. The plans prescribe endpoint, precedence,
|
||||
discovery, and validation behavior, but do not require independent repeated
|
||||
chain walks, repeated processing of identical prefixes, full-catalog scans per
|
||||
rewrite, or eager diagnostic path formatting at every valid node.
|
||||
|
||||
## Validation and interpretation
|
||||
|
||||
All three independent module default suites passed with Go 1.27.0 using
|
||||
`GOWORK=off GOTOOLCHAIN=local go test -mod=readonly ./...`, run separately from
|
||||
each module directory. The aligned workspace suite also passed using
|
||||
`GOTOOLCHAIN=local go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
The unchanged source reused successful cached results. Dependency downloads were
|
||||
disabled. No production files or repository tests were modified.
|
||||
|
||||
A temporary external consumer confirmed that the workload shapes below are
|
||||
accepted: a flag-alias chain with an active command endpoint and inactive group
|
||||
contexts; a single-token group-alias chain whose help has no descendant aliases;
|
||||
distinct aliases sharing a discovery-only prefix; and a valid nested declaration
|
||||
chain. These are behavioral checks, not timing experiments. The complexity
|
||||
findings follow from the implementation's loops and copies.
|
||||
|
||||
Complexities below use expected constant-time map operations and count tokens
|
||||
or records separately from their string-byte costs. They describe the identified
|
||||
phase, not the entire construction or execution pipeline. Nested loops over
|
||||
disjoint child collections, required independent input copies, sorted output,
|
||||
and work proportional to actual serialized output are not findings by themselves.
|
||||
|
||||
No additional ordinary correctness defect was established during this pass.
|
||||
The six open findings in [01-correctness.md](01-correctness.md) remain separate;
|
||||
passing default suites do not resolve those findings.
|
||||
|
||||
## E1 — P3: Flag-alias compilation repeatedly resolves shared chain suffixes
|
||||
|
||||
**Code:** [flag_aliases.go](../../../flag_aliases.go), resolveFlagAliases lines
|
||||
226–251 and resolveFlagTarget lines 254–279; compileFlagAliases invokes this work
|
||||
independently for each canonical and discovery context.
|
||||
|
||||
**Circumstances and complexity:** consider A effective mappings forming
|
||||
`--a1 → --a2 → … → --aA → --value`. Each source starts a fresh visited map and
|
||||
chain walk. The suffix traversed for `--a2` was already traversed for `--a1`, and
|
||||
so on. There are Θ(A²) total target examinations and corresponding cumulative
|
||||
chain/visited storage operations per context, despite only A mappings. The same
|
||||
cost occurs when the final flag is unavailable and all these global aliases are
|
||||
inactive. Across C contexts with the same chain shape, this part can cost
|
||||
Θ(CA²), apart from sorting, canonical metadata, and string processing. Peak
|
||||
temporary storage for a single walk is O(A).
|
||||
|
||||
**Potential refactoring:** resolve the effective functional graph once per
|
||||
context using visitation states and memoized terminal results. Cache both an
|
||||
available canonical endpoint and an unavailable endpoint, and detect a cycle
|
||||
using the active traversal stack. This reduces graph traversal to expected O(A)
|
||||
work and O(A) scratch storage per context. Keeping the existing source sort for
|
||||
deterministic validation leaves O(A log A) total per-context work for these
|
||||
steps. A context-local cache suffices; no cross-invocation provider cache is
|
||||
needed.
|
||||
|
||||
Canonical spellings must still terminate target resolution before following a
|
||||
mapping of the same spelling. Resolve each source's target before separately
|
||||
checking active source collisions. Preserve scoped overrides, inactive globals,
|
||||
scoped missing-target failures, useful cycle diagnostics, and revalidation after
|
||||
registration. Results cannot be shared indiscriminately between contexts.
|
||||
|
||||
## E2 — P3: Alias help scans every source for every intermediate route
|
||||
|
||||
**Code:** [help_view.go](../../../help_view.go), routeHelpAliases lines 38–51;
|
||||
[resolver.go](../../../internal/alias/resolver.go), ResolutionPaths lines 139–150.
|
||||
|
||||
**Circumstances and complexity:** let P be the number of paths visited while
|
||||
rewriting the entered help path and A the number of declared command aliases.
|
||||
For every visited path, the function scans all A aliases and copies each source
|
||||
through FromTokens before checking its length and prefix. This performs PA
|
||||
source examinations, plus repeated source-token copying and prefix comparisons,
|
||||
even when no source can contribute an entry.
|
||||
|
||||
For a valid chain of A single-token aliases ending at a canonical group,
|
||||
`help a1` visits Θ(A) paths. Every source is one token long, so every candidate
|
||||
is discarded by the length check. Discovery of an empty alias section still
|
||||
takes Θ(A²) examinations/copies. This cost recurs when requesting that help
|
||||
again even if the routing snapshot is already prepared; it is separate from
|
||||
registration, termination analysis, and backend construction.
|
||||
|
||||
**Potential refactoring:** query an immutable source-prefix index for strict
|
||||
descendants of each visited prefix. The existing internal alias trie is a
|
||||
possible owner; alternatively prepare a source index with the routing snapshot.
|
||||
Building such an index is proportional to total source tokens. Prefix lookup
|
||||
then visits matching descendants rather than the entire catalog. For the
|
||||
single-token-chain example, the identified discovery work becomes O(A), with no
|
||||
descendant entries to enumerate. General cases still pay for prefix lengths,
|
||||
matching candidates, authoritative target resolution, and sorting the result.
|
||||
|
||||
Keep intermediate-route discovery and resolve each surviving composed entered
|
||||
path through the existing resolver. Preserve longer overrides, canonical-target
|
||||
validation, suffix deduplication, and deterministic presentation. A prefix index
|
||||
does not authorize enumerating additional composed routes or introducing the
|
||||
deferred alias-completion feature.
|
||||
|
||||
## E3 — P3: Discovery-context preparation rebuilds shared prefixes repeatedly
|
||||
|
||||
**Code:** [flag_aliases.go](../../../flag_aliases.go), discoverFlagContexts lines
|
||||
181–200, particularly the prefix cloning/key construction at 185–187 and
|
||||
resolution at 194–195.
|
||||
|
||||
**Circumstances and complexity:** R distinct alias sources can share a prefix
|
||||
of depth D, for example `legacy nested … action1`, `legacy nested … action2`,
|
||||
and so on, all targeting an ordinary canonical group. Every source independently
|
||||
enumerates the same D proper prefixes. For each prefix length k, it copies k
|
||||
tokens, builds a key, and may resolve and classify the prefix again. The
|
||||
prefixes map deduplicates only the eventual result; it does not prevent this
|
||||
work, including for prefixes previously classified as ineligible.
|
||||
|
||||
With bounded component lengths and no rewrite within those proper prefixes,
|
||||
the shared-prefix portion costs Θ(RD²), although the source declarations contain
|
||||
only Θ(RD) tokens and there are only D distinct proper prefixes. When R and D
|
||||
both grow, that is cubic work in their common scale for quadratic input size.
|
||||
Additional rewrite work can increase the classification cost further.
|
||||
|
||||
**Potential refactoring:** enumerate distinct proper source prefixes once,
|
||||
preferably by traversing the already compiled source trie, then run the existing
|
||||
classification once per unique prefix. Retaining full paths and keys once per
|
||||
prefix is sufficient; no change to command routing is required. In the example,
|
||||
source traversal plus prefix materialization/classification can be
|
||||
O(RD + D²), instead of O(RD²). More generally, account for total source tokens,
|
||||
each distinct prefix's representation, and one resolution per distinct prefix.
|
||||
|
||||
A seen set consulted only after rebuilding each full prefix key would remove
|
||||
duplicate resolutions but retain the Θ(RD²) cloning/key cost. Use a traversal
|
||||
that actually shares prefix processing. Preserve exclusion of canonical paths,
|
||||
exact alias sources, and prefixes intercepted by valid canonical operand space;
|
||||
cache negative classifications too if using an alternative memoized traversal.
|
||||
The source index proposed in E2 could support this work as well.
|
||||
|
||||
## E4 — P3: Successful declaration validation eagerly constructs every full path
|
||||
|
||||
**Code:** [command.go](../../../command.go), collectDeclarationIssues lines
|
||||
444–470, especially path copying and formatting at 451–453.
|
||||
|
||||
**Circumstances and complexity:** every visited declaration copies its entire
|
||||
ancestral path and clones/joins it again to make a diagnostic label before
|
||||
checking whether any issue needs that label. For N valid commands arranged in
|
||||
a depth-N chain, this performs Θ(1 + 2 + … + N) = Θ(N²) token-copying and label
|
||||
construction work, with quadratic cumulative allocation, while producing no
|
||||
diagnostic. A valid tree can be checked for retained issues and root-only
|
||||
placement in one traversal.
|
||||
|
||||
**Potential refactoring:** maintain one scratch path for the validation walk using
|
||||
an append/truncate stack and format it only when emitting an actual issue or a
|
||||
root-only placement error. With ordinary amortized slice growth, the successful
|
||||
validation walk becomes O(N) work and O(D) path storage for depth D. Error cases
|
||||
also pay for the path strings actually included in their diagnostics.
|
||||
|
||||
Preserve child order, retained error identity, root-only placement checks, and
|
||||
invalid/absent declaration handling. This changes scratch diagnostic preparation,
|
||||
not captured declaration ownership. Other installation phases retain their own
|
||||
metadata/path costs; this finding does not claim that complete App construction
|
||||
would consequently become linear for every tree shape.
|
||||
|
||||
## Recommendation
|
||||
|
||||
Consider E1 first: memoizing a context's flag graph removes repeated suffix
|
||||
work with a small, local change. E2 and E3 can share a private source-prefix
|
||||
traversal/index while retaining the selected resolver and discovery semantics.
|
||||
E4 is an independent simplification of diagnostic traversal. All four are
|
||||
efficiency opportunities for consideration, not evidence of measured operational
|
||||
impact or a reason to reopen the selected algorithms listed above.
|
||||
Reference in New Issue
Block a user