Record reporting efficiency opportunities
This commit is contained in:
@@ -1,6 +1,8 @@
|
|||||||
# Invocation reporting audit remediation
|
# Invocation reporting audit remediation
|
||||||
|
|
||||||
Status: five open findings. Findings 4 and 5 were added by the interaction audit.
|
Status: five open remediation findings and two efficiency opportunities for
|
||||||
|
consideration. Findings 4 and 5 came from the interaction audit; findings 6 and 7
|
||||||
|
came from the algorithmic audit.
|
||||||
|
|
||||||
## Review scope
|
## Review scope
|
||||||
|
|
||||||
@@ -33,6 +35,34 @@ quiet help/version, aggregate configuration YAML, quiet comparison, concrete
|
|||||||
request validation, and offline aggregate smoke testing. Findings 4 and 5 record
|
request validation, and offline aggregate smoke testing. Findings 4 and 5 record
|
||||||
the additional gaps; findings 1–3 remain open and are not duplicated below.
|
the additional gaps; findings 1–3 remain open and are not duplicated below.
|
||||||
|
|
||||||
|
The algorithmic audit at `ee76b0f` examined the reporting-specific parser passes,
|
||||||
|
delivery bookkeeping, deferred storage, native diagnostic partitioning, and
|
||||||
|
integration report coordination. It used source-level complexity analysis, not
|
||||||
|
benchmarks or real-world performance measurements. Selected algorithms were
|
||||||
|
excluded from reconsideration, including synchronous serialized reporting, FIFO
|
||||||
|
deferred delivery, rendering rich content under quiet mode, fresh backend trees,
|
||||||
|
and the chosen configuration/smoke report preparation boundaries. Findings 6 and
|
||||||
|
7 are implementation opportunities within those decisions.
|
||||||
|
|
||||||
|
No newly introduced quadratic-time algorithm was identified in the reviewed
|
||||||
|
paths. In particular:
|
||||||
|
|
||||||
|
- Reporting selection is linear in the number of flag occurrences. There are
|
||||||
|
four fixed reporting flags and at most two additional reverse searches for
|
||||||
|
selection provenance; these do not create a quadratic pass.
|
||||||
|
- Deferred insertion is amortized constant time plus callback-list copying;
|
||||||
|
draining is linear in queued messages and details, excluding callback/I/O
|
||||||
|
costs. The avoidable storage and handoff costs are described in finding 6.
|
||||||
|
- Native diagnostic snapshotting and partitioning are linear in source records,
|
||||||
|
diagnostic records, and the total related-location entries. Nested copying of
|
||||||
|
related locations is not quadratic in the actual input size.
|
||||||
|
- Explanation combination uses amortized slice appends. Configuration comparison
|
||||||
|
has two sides, and aggregate smoke execution has four selected operations;
|
||||||
|
neither introduces an unbounded nested traversal through all prior results.
|
||||||
|
|
||||||
|
The algorithmic audit found no additional ordinary correctness issue beyond
|
||||||
|
findings 1–5. It made no implementation changes and ran no tests or benchmarks.
|
||||||
|
|
||||||
## 1. Preserve native PromptKit operation failure explanations
|
## 1. Preserve native PromptKit operation failure explanations
|
||||||
|
|
||||||
Priority: P2. Status: open.
|
Priority: P2. Status: open.
|
||||||
@@ -320,3 +350,119 @@ Include a quiet alias followed by another parse error, without suppressing the
|
|||||||
terminal usage explanation. Retain scoped-alias precedence, unavailable-target,
|
terminal usage explanation. Retain scoped-alias precedence, unavailable-target,
|
||||||
terminator/value handling, and discovery-context coverage at their existing
|
terminator/value handling, and discovery-context coverage at their existing
|
||||||
owners.
|
owners.
|
||||||
|
|
||||||
|
## 6. Avoid retaining deferred messages that need no presentation work
|
||||||
|
|
||||||
|
Priority: P3. Status: for consideration.
|
||||||
|
|
||||||
|
### Finding and complexity
|
||||||
|
|
||||||
|
[DeferredReporter.enqueue](../../reporter.go) unconditionally stores the category,
|
||||||
|
heading, and copied detail list. Visibility is checked only when `finalize`
|
||||||
|
drains the queue through `submitMessage`. Consequently, a long-running handler
|
||||||
|
that submits many simple deferred Debug messages in ordinary mode, or simple
|
||||||
|
Info/Warning messages in quiet mode, retains all of them despite there being
|
||||||
|
no output or renderer work to perform.
|
||||||
|
|
||||||
|
Let `M` be the number of these renderer-free hidden messages and `B` the bytes
|
||||||
|
of otherwise unreferenced distinct heading text. Reporter storage can grow as
|
||||||
|
`Θ(M + B)` until finalization. Finalization then makes a second `Θ(M)` queue
|
||||||
|
array copy before visiting and suppressing every entry. This is linear, not
|
||||||
|
quadratic, but the additional reporter storage for this case can be `O(1)`.
|
||||||
|
The `M` public submissions still require `Θ(M)` total call work; this is not a
|
||||||
|
claim that the whole invocation can become sublinear.
|
||||||
|
|
||||||
|
### Refactoring opportunity
|
||||||
|
|
||||||
|
Under the current execution flow, reporting selection is applied before handlers
|
||||||
|
and providers run, with no public operation to change it mid-handler. For a
|
||||||
|
hidden deferred submission with no non-nil detail callback, consider recording
|
||||||
|
the eventual suppression observation without retaining the heading or queue
|
||||||
|
entry. Preserve the existing no-content treatment of an empty heading with no
|
||||||
|
renderer. This removes the avoidable `Θ(M + B)` retention and its later sweep
|
||||||
|
for renderer-free suppressed messages.
|
||||||
|
|
||||||
|
Retain rich hidden submissions: their callbacks must still execute at the
|
||||||
|
selected deferred time against discard so rendering failures are preserved.
|
||||||
|
Retain visible messages in FIFO order. Closed-reporter checks must still run
|
||||||
|
before accepting or discarding any submission. This optimization does not add
|
||||||
|
a public immutability guarantee for future mode APIs or change the selected
|
||||||
|
emission-time visibility rules.
|
||||||
|
|
||||||
|
Independently, `finalize` can detach the reporter-owned queue under its existing
|
||||||
|
mutex instead of copying its array and then clearing the original reference.
|
||||||
|
That makes queue handoff `O(1)` instead of `Θ(M)` extra copying; actual visible/
|
||||||
|
rich message delivery remains linear. An internal delivery path can also avoid
|
||||||
|
recopying callback lists already captured by `enqueue`, while preserving the
|
||||||
|
selected defensive copy when first accepting caller-owned lists.
|
||||||
|
|
||||||
|
These opportunities preserve the chosen queue and synchronous delivery model.
|
||||||
|
They do not justify message caps, dropping visible messages, skipping rich
|
||||||
|
renderers, or a background delivery subsystem.
|
||||||
|
|
||||||
|
### Review evidence
|
||||||
|
|
||||||
|
The retention and copies follow directly from `enqueue`, the array copy at the
|
||||||
|
start of `finalize`, and `submitMessage`'s later visibility check. If selected,
|
||||||
|
use focused behavior checks for suppression observations, FIFO delivery,
|
||||||
|
callback capture, hidden renderer failures, and late submissions. Source-level
|
||||||
|
storage analysis is sufficient; no benchmark or allocation threshold is proposed.
|
||||||
|
|
||||||
|
## 7. Stop normalizing an argument tail that parsing cannot reach
|
||||||
|
|
||||||
|
Priority: P3. Status: for consideration.
|
||||||
|
|
||||||
|
### Finding and complexity
|
||||||
|
|
||||||
|
In [interpretFlagTokens](../../internal/cobra/flags.go), the first unknown long
|
||||||
|
flag or unknown short-cluster member sets `recordingOccurrences = false`, but
|
||||||
|
interpretation continues through the remaining argv. Later recognized flags
|
||||||
|
still populate spelling lists and construct canonical `--name=value` strings.
|
||||||
|
[Session.Parse](../../internal/cobra/parse.go) then calls the backend, which
|
||||||
|
fails at the earlier unknown flag without consuming that normalized tail.
|
||||||
|
The adapter does not enable unknown-flag whitelisting.
|
||||||
|
|
||||||
|
For example, after `--unknown`, a later valid `--message=LARGE_VALUE` still
|
||||||
|
causes the value bytes to be copied into a new canonical argument, even though
|
||||||
|
no parser, converter, or handler reaches that value. The recent occurrence
|
||||||
|
tracking correctly excludes the tail from selection evidence but retains this
|
||||||
|
pre-existing unnecessary normalization work.
|
||||||
|
|
||||||
|
Let `F` and `A` be the flag/alias table sizes, `P` the token/byte processing cost
|
||||||
|
through the first fatal unknown flag, and `T` the remaining normalization work,
|
||||||
|
including copied value bytes. With expected constant-time map lookups, the helper
|
||||||
|
currently takes `O(F + A + P + T)` time on that failure path. It can take
|
||||||
|
`O(F + A + P)` by stopping at the failing token and growing temporary arrays
|
||||||
|
with the prefix instead of preallocating for all argv. The existing backend can
|
||||||
|
still produce the same failure from that prefix. This is an early-failure
|
||||||
|
improvement, not a quadratic-time finding or a change to the successful parse's
|
||||||
|
necessary linear work.
|
||||||
|
|
||||||
|
### Refactoring opportunity
|
||||||
|
|
||||||
|
Consider ending interpretation immediately after preserving the first unknown
|
||||||
|
flag or failing short-cluster suffix. Preserve any earlier valid occurrences and
|
||||||
|
their spellings, including recognized members preceding a bad short member.
|
||||||
|
Let the backend retain its existing failure classification, wording, and
|
||||||
|
precedence; do not replace normal parsing with a reporting-specific scanner.
|
||||||
|
This can also remove the repeated `recordingOccurrences` guards needed solely
|
||||||
|
because the current implementation continues past the failure.
|
||||||
|
|
||||||
|
The helper currently preallocates both output and occurrence slices from total
|
||||||
|
argv length, so an early return alone does not eliminate its `O(argc)` upfront
|
||||||
|
array allocation and initialization. To obtain the prefix-bounded helper cost,
|
||||||
|
grow those arrays with the interpreted prefix rather than reserving for the
|
||||||
|
unreachable tail. This changes allocation strategy, not accepted input sizes.
|
||||||
|
|
||||||
|
Other routing/parser steps still copy argv string headers, so this refactoring
|
||||||
|
does not make the entire invocation prefix-linear. It specifically removes
|
||||||
|
avoidable tail interpretation, spelling storage, and large-value byte copies.
|
||||||
|
There is no need to reconsider the selected backend or fresh-tree strategy.
|
||||||
|
|
||||||
|
### Review evidence
|
||||||
|
|
||||||
|
The unknown-flag branches and continued spelling/string construction establish
|
||||||
|
the extra work statically. Existing prefix-order, missing-value precedence,
|
||||||
|
short-cluster, and terminator/value tests provide relevant behavior coverage if
|
||||||
|
this is selected. Preserve the backend error and consumed occurrence prefix on
|
||||||
|
early failure; no timing threshold or real-world performance data is needed.
|
||||||
|
|||||||
Reference in New Issue
Block a user