Commit 321ee41
perf(logs): collapse IPv4 at emit time instead of in a second pass (#54930)
### What does this PR do?
Targets `mwdd146980/tokenize-ipv4-hybrid-token`, not `main`. It keeps
the IPv4 hybrid token exactly as-is and only changes *when* the collapse
happens, to recover a tokenizer throughput regression.
`collapseHybridTokens` walked the whole token list after tokenization
and called `ipv4At` at every position. Two problems:
1. **`ipv4At` isn't inlinable.** Two slice headers plus an index, seven
comparisons — over the budget, confirmed by its absence from the `can
inline` list under `go build -gcflags=-m`. So it was a real function
call at *every* token position, ~250 of them on a 1 KB line.
2. **The pass rewrote both buffers unconditionally.**
`tsBuf[w]=tsBuf[r]` / `idxBuf[w]=idxBuf[r]` ran for every token even
when `w == r` and nothing collapsed — the common case, since most log
lines have no IP.
A dotted quad can only ever be closed by its **final octet**, so the
check doesn't need a second pass at all — it only needs to run when a
1-3 digit run is emitted:
```go
t.tsBuf = append(t.tsBuf, token+Token(r))
if token == D1 && runLen <= maxIPv4OctetDigits {
t.collapseIPv4Tail()
}
```
`collapseIPv4Tail` reads the fields directly (no slice params, and
`isIPv4OctetToken` still inlines), tests separators first (`ts[5] !=
Period` rejects almost every call in one compare), skips re-checking the
trailing octet since the caller already established it, and doesn't
touch the buffers unless there's a real match. On a match it overwrites
one slot and truncates — capacity is preserved, so buffer reuse and the
allocation-free borrowed path are unaffected.
The second commit handles `tokenizeWithoutHybridCollapse`, which called
`emitRuns` to get *uncollapsed* tokens. That half of the IIS test is
really about the scorer — given a token sequence where the client IP is
still seven run tokens, Kadane averages 0.5 and the line stays aggregate
— so the sequence is a fixture, not something the tokenizer needs to
reproduce. It is now written out explicitly, split around the client IP
so the same fixture expresses both the pre- and post-collapse shapes,
with a `require.Equal` against the real `Tokenize` output pinning it so
the baseline cannot drift. No test-only state on `Tokenizer`. Both IIS
tests pass unchanged.
### Motivation
Measured against this PR's parent commit. Binaries built from each
commit and run **interleaved** (alternating each round, so thermal drift
on the M4 Max cancels rather than biasing one side), 10 rounds,
`benchstat` n=10:
| | parent | PR head (`93bfd45`) | this branch |
|---|---|---|---|
| Latency geomean (33 benchmarks) | 193.8 ns | **+38.79%** | **+1.82%**
|
| Throughput geomean (~1 KB lines) | 647 MiB/s | **−35.84%** |
**−2.02%** |
Worst cases on the PR head were `AppLog/Borrowed` 1.43 → 2.43 µs
(+69.9%), `TimestampHeavy` +70.5%, `RealisticApacheLog` +56.7%, all at
`p=0.000`. This is `tokenizeBorrowed`, the per-line hot path for auto
multi-line detection.
With this change, over half the benchmarks are statistically
indistinguishable from the parent (`p>0.05`); the worst remaining is
`TimestampHeavy` at +8.4%.
### Describe how you validated your changes
Collapsing during emission rather than after isn't *obviously*
equivalent on chained quads like `1.2.3.4.5.6.7.8`, so I verified it
instead of reasoning about it:
- **Differential test, 250,042 inputs.** Dumped tokens **and** start
indices from both implementations and diffed — byte-for-byte identical.
51k of those inputs contain at least one quad (16k×1, 13k×2, 12.7k×3,
8k×4, 764 with 5+), generated from a quad-biased grammar plus long
dotted chains. My first corpus only produced 26 quads, which would have
proven nothing, so I regenerated before trusting it.
- Full package suite passes, including both new IIS tests, under
`-count=2` and `-race`.
- `go vet` clean; `gofmt` clean.
- Added `1.2.3.4.5.6.7`, `1.2.3.4.5.6.7.8` and `12.34.56.78.90.12.34.56`
to `TestTokenizerIPv4`. That leftmost-greedy, non-overlapping behavior
is load-bearing for this refactor and was previously unguarded.
- Reworded the `HybridTokenPromotion` invariant in `tokenizer.allium`:
it described the collapse as a scan performed *after* promotion, which
was the two-pass structure. Now states the observable rule (leftmost,
non-overlapping) and documents the chained-quad case. Semantics
unchanged. I couldn't run `allium check` — the binary isn't installed
locally, so that wording is worth a look.
### Additional Notes
- No release note: this is a perf refinement to an unmerged feature that
already carries one.
- No `BUILD.bazel` change — no files or imports added.
- The benchmark numbers are **tokenizer-local**. I didn't measure the
full decoder path, so I can't say what share of real per-line pipeline
cost this represents.
- Feel free to squash this into your branch or cherry-pick it, whichever
is less disruptive.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 93bfd45 commit 321ee41
2 files changed
Lines changed: 75 additions & 53 deletions
Lines changed: 33 additions & 9 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
159 | 159 | | |
160 | 160 | | |
161 | 161 | | |
162 | | - | |
163 | | - | |
164 | | - | |
165 | | - | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
166 | 180 | | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
171 | 190 | | |
172 | 191 | | |
173 | 192 | | |
| |||
184 | 203 | | |
185 | 204 | | |
186 | 205 | | |
187 | | - | |
| 206 | + | |
188 | 207 | | |
189 | 208 | | |
190 | 209 | | |
| |||
193 | 212 | | |
194 | 213 | | |
195 | 214 | | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
196 | 220 | | |
197 | 221 | | |
198 | 222 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
19 | 22 | | |
20 | 23 | | |
21 | 24 | | |
| |||
150 | 153 | | |
151 | 154 | | |
152 | 155 | | |
153 | | - | |
154 | | - | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
155 | 163 | | |
| 164 | + | |
156 | 165 | | |
157 | 166 | | |
158 | 167 | | |
| |||
163 | 172 | | |
164 | 173 | | |
165 | 174 | | |
166 | | - | |
167 | 175 | | |
168 | 176 | | |
169 | 177 | | |
170 | | - | |
171 | | - | |
172 | | - | |
| 178 | + | |
| 179 | + | |
173 | 180 | | |
174 | 181 | | |
175 | 182 | | |
| |||
213 | 220 | | |
214 | 221 | | |
215 | 222 | | |
216 | | - | |
217 | | - | |
218 | | - | |
219 | | - | |
220 | | - | |
221 | | - | |
222 | | - | |
223 | | - | |
224 | | - | |
225 | | - | |
226 | | - | |
227 | | - | |
228 | | - | |
229 | | - | |
230 | | - | |
231 | | - | |
232 | | - | |
233 | | - | |
234 | | - | |
235 | | - | |
236 | | - | |
237 | | - | |
238 | | - | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
239 | 234 | | |
240 | 235 | | |
241 | 236 | | |
242 | 237 | | |
243 | | - | |
244 | | - | |
245 | | - | |
246 | | - | |
247 | | - | |
248 | | - | |
249 | | - | |
250 | | - | |
251 | | - | |
252 | | - | |
253 | | - | |
254 | | - | |
255 | | - | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
256 | 247 | | |
257 | | - | |
258 | | - | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
259 | 257 | | |
260 | 258 | | |
261 | 259 | | |
| |||
0 commit comments