Skip to content

perf: skip redundant re-segmentation for sentence blocks in sequence.Run - #1170

Open
theredspoon wants to merge 4 commits into
vale-cli:v3from
theredspoon:perf/sequence-skip-redundant-round-trip
Open

perf: skip redundant re-segmentation for sentence blocks in sequence.Run#1170
theredspoon wants to merge 4 commits into
vale-cli:v3from
theredspoon:perf/sequence-skip-redundant-round-trip

Conversation

@theredspoon

@theredspoon theredspoon commented Sep 2, 2026

Copy link
Copy Markdown

Depends on #1162. Stacked on top of it, so the diff will look larger than the actual change until #1162 merges into v3 and this rebases. Click into this branch's single commit on the Commits tab to see this PR's actual diff.

Problem

Sequence.Run calls f.Sentences(blk.Text) unconditionally, even for a plain rule whose declared scope narrows to sentence, meaning blk is already exactly one sentence a segmenter produced. Re-segmenting it is redundant, and under a configured non-English remote NLP endpoint, wastes a full HTTP round-trip per dispatched sentence.

Trusting a rule's declared scope to justify skipping that call is unsafe on its own: scope-narrowing has had real bugs before. The fix here gates the skip on a structural fact about the block itself, not on what any rule declared.

Fix

Block.IsSentence() reports whether a block's scope carries the sentence. prefix that only the segmentation loop ever produces during block construction. Run skips f.Sentences exactly when blk.IsSentence() is true. Scope.Matches now reads the same helper for its own, unrelated sentence. check, so the two readings of "is this a sentence block" can't drift apart.

Why this is safe where a naive "skip when the rule has no threshold" would not be

Re-segmenting one segmenter's own output with that same segmenter can only reproduce it. The call is skipped exactly when it is truly redundant, not merely when a rule claims not to need it. A rule's own declared scope can be wrong: narrowing logic is code, and code has bugs. A block's actual constructed scope reflects how it was really built. Every block that isn't structurally known to be one sentence still gets the real call, whatever the rule declared.

Testing

  • TestScopeMatchesOnlyHandsSequenceSentenceBlocks: the invariant this design relies on, over the whole scope grammar.
  • TestIsSentence: contract test pinning Block.IsSentence against exactly what block construction produces.
  • TestSequencePlainRuleAddsNoSegmentCallsBeyondCompute: proves the round-trip savings by real request count against a mocked endpoint, and separately proves a real same-sentence match still resolves correctly under the gated path (verified this assertion has teeth by breaking the gate's fallback and confirming it fails).
  • TestSequenceRoundTripSkipDoesNotReintroduceNegatedScopeFalsePositive: confirms this optimization composes correctly with the scope-narrowing fix it depends on for negated scopes.

No internal/e2e case. This changes network-call volume under a remote endpoint. Lint output stays the same for every correctly-configured rule, so a vale run's printed results show nothing different here.

Full repo suite and -race are clean, including internal/e2e.

Performance

BenchmarkSequenceRunPlainRuleSentenceBlock measures the local, built-in-tagger cost: the map lookup this change avoids. Compared against the commit immediately before this one with benchstat (n=6):

                                    │   before    │              after                 │
                                    │   sec/op    │    sec/op     vs base               │
SequenceRunPlainRuleSentenceBlock-11  1.737µ ± 1%    1.729µ ± 0%   ~ (p=0.132 n=6)

                                    │   before    │              after                 │
                                    │    B/op     │     B/op      vs base               │
SequenceRunPlainRuleSentenceBlock-11 1.814Ki ± 0%   1.814Ki ± 0%   ~ (p=1.000 n=6)

                                    │   before    │              after                 │
                                    │ allocs/op   │  allocs/op    vs base               │
SequenceRunPlainRuleSentenceBlock-11   33.00 ± 0%     33.00 ± 0%   ~ (p=1.000 n=6)

No measurable difference locally, as expected: the actual savings this PR targets is the eliminated remote round-trip, which the call-count test above proves directly rather than by wall-clock benchmark.

Related

Follow-up to #1162, found during its review.

The remote-tagging and remote-segmentation call sites (TextToTokens,
Info.Compute) panicked on a failed /tag or /segment request instead of
returning an error, crashing the whole vale process rather than
surfacing a normal, reportable error. TextToContext and the `tag` CLI
command are updated to thread the error through rather than let it
panic.

The shared HTTP transport also ignored response status codes: a non-2xx
response with a technically-valid JSON body (e.g. `500 {"sents":[]}`)
was silently decoded as a successful, empty result instead of a failure.

Each fix has its own regression test.

Two internal/e2e scenarios cover the user-visible behavior end to end,
both against a closed local port so the failure (connection refused) is
deterministic and needs no network or mock server: a lint run whose
Info.Compute hits a failed /segment request during block construction,
and the `tag` CLI command's /tag request. Verified against the pre-fix
commit that both currently fail this way (a panic with a goroutine
stack trace) before this fix, and pass cleanly after it.
sentenceScope's negation branch was a no-op for a bare negated term:
`~list` narrowed to `~list`, itself, via strings.CutPrefix re-adding
the same prefix it had just stripped. A negated term never mentions
`sentence`, so asksForSentence (scope.go) then skipped every
`sentence.*` fragment block for such a rule, and Scope.Matches
instead matched both the whole-block copy and its own paragraph
wrapper for the same text. One real match dispatched to Run twice,
once per block, and produced two identical alerts.

The negation branch now AND-s `sentence` in front of the term instead
of leaving it untouched, so `~list` narrows to `sentence&~list`,
sentences outside a list, the same as every other declared scope
already does.
@theredspoon
theredspoon force-pushed the perf/sequence-skip-redundant-round-trip branch from b5f22cf to da8fff9 Compare September 2, 2026 03:39
NewSequence unconditionally narrowed every declared scope to
sentence-level, so a rule using max/min could never aggregate matches
across a paragraph's sentences. Threshold-opted-in rules now keep
their real declared scope; Run tags each sentence of that scope
separately instead of tagging the whole block once and inferring
sentence boundaries afterward, so a match can never span two
sentences by construction.

An undeclared scope on a threshold rule now defaults to paragraph
plus every other prose-container scope, matching what a plain
sequence rule's undeclared scope already reaches, via one shared
list in internal/core instead of two independently-maintained copies.

Built on vale-cli#1167 (fixes three pre-existing panics on remote NLP
endpoint failures this feature's own paths would otherwise have hit)
and vale-cli#1169 (fixes a sentenceScope bug that review of this feature
found as a real, dispatched double-report).
Run called f.Sentences(blk.Text) unconditionally, even for a plain
rule whose declared scope narrows to `sentence`, paying a redundant
segmentation pass (a full remote round-trip under a configured
non-English endpoint) on text that had already been segmented once,
by the same segmenter, to build the block Run was handed.

Trusting a rule's declared scope unconditionally to justify skipping
that call is unsafe: sentenceScope's negation branch was a real bug
(fixed separately in vale-cli#1169) that could otherwise have made this
optimization reintroduce a cross-sentence false positive. Gated
instead on a structural fact about the block itself: Block.IsSentence
reports whether blk's scope was built by doNLP's segmentation loop,
the only place that produces a `sentence.`-prefixed scope.
Re-segmenting one segmenter's own output with that same segmenter can
only ever reproduce it, so the call is skipped exactly when it is
truly redundant. Scope.Matches reads the same helper for its own,
unrelated `sentence.` check, so the two readings of "is this a
sentence block" cannot drift apart.

Covered by a new invariant test over the whole scope grammar, a
dispatched regression test proving this doesn't reopen the
negated-scope false positive vale-cli#1169 fixed, an IsSentence contract
test, and a call-count test proving the round-trip savings directly
against a mocked remote endpoint.
@theredspoon
theredspoon force-pushed the perf/sequence-skip-redundant-round-trip branch from da8fff9 to c058ccf Compare September 2, 2026 03:50
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.

1 participant