perf: cache plugin qualifiers, hoist regex literals, add missing decoder tests#388
perf: cache plugin qualifiers, hoist regex literals, add missing decoder tests#388kevinelliott wants to merge 4 commits into
Conversation
Performance: - MessageDecoder.decode no longer calls plugin.qualifiers() on every message. Per-plugin metadata (preambles) is captured once at registration time, and per-label candidate lists are pre-built so the hot path is just a Map lookup + a single startsWith loop. - Hoist module-scoped regex literals in CBand, ARINC_702, Label_80, and Label_83 to avoid re-allocating patterns inside per-message decode(). - CoordinateUtils.decodeStringCoordinatesDecimalMinutes now validates the prefix chars before parsing and computes Number(...) once each for lat/lon (was twice). - Label_4A swaps a regex prefix check for a direct charAt comparison. - Label_83 strips whitespace once and skips redundant per-field re-stripping. Decoders: - Label_B6 now extracts the CPDLC body after the leading "/" as text rather than discarding it (full ATN/PM-CPDLC parsing is still TODO). Tests: - Add tests for previously untested decoders: Label_QP, Label_QR, Label_QS, Label_ColonComma, Label_B6, ARINC_702. - Add tests for CoordinateUtils. Test count: 407 -> 445 passing, 88 -> 95 suites.
WalkthroughRefactors MessageDecoder plugin registration/lookup to use PluginEntry-backed candidate lists with precomputed preambles; moves preamble matching into a helper. Adds multiple plugin unit tests, extracts shared regex constants, refactors several decoders, and fixes coordinate-decoding logic and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de83d3e93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const bucket of this.candidatesByLabel.values()) { | ||
| bucket.unshift(entry); |
There was a problem hiding this comment.
Appending a new wildcard plugin currently uses bucket.unshift(entry) for every existing label bucket, which makes later wildcard registrations run before earlier ones for labels that already have buckets. This changes decoder precedence versus the previous behavior (wildcards in registration order) and can alter which plugin wins when multiple wildcard decoders can parse the same message, especially when consumers call registerPlugin() after initialization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Maintainer review: Confirmed and addressed in 78e2526 — splice(wildcardEntries.length - 1, 0, entry) is the correct fix. New regression test MessageDecoder.labelindex.test.ts covers it. Resolved.
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing per-message overhead in the decoding hot path (especially plugin qualification/preamble filtering and regex allocation), while expanding unit test coverage for several decoders and coordinate utilities.
Changes:
- Cache plugin qualifier metadata at registration time and prebuild per-label candidate lists to speed up
MessageDecoder.decode(). - Hoist frequently used regex literals to module scope across multiple plugins and tighten a few hot-path string checks.
- Add/expand unit tests for CoordinateUtils and several label decoders/wrappers (QP/QR/QS,
:;, B6, ARINC 702).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/MessageDecoder.ts | Adds registration-time qualifier caching + per-label candidate indexing and preamble filtering helper. |
| lib/utils/coordinate_utils.ts | Validates coordinate prefix chars earlier and reduces redundant Number(...) parsing. |
| lib/utils/coordinate_utils.test.ts | Adds direct tests for CoordinateUtils parsing/formatting helpers. |
| lib/plugins/CBand.ts | Hoists C-Band header regex and adds quick reject paths before delegation. |
| lib/plugins/ARINC_702.ts | Hoists newline-stripping regex to module scope. |
| lib/plugins/ARINC_702.test.ts | Adds tests for newline stripping, leading header peeling, and routing behavior. |
| lib/plugins/Label_83.ts | Hoists regexes and removes redundant whitespace stripping in variant parsing. |
| lib/plugins/Label_80.ts | Hoists regexes and reuses them during parsing to reduce per-message allocations. |
| lib/plugins/Label_4A.ts | Replaces a regex prefix check with a direct first-character check. |
| lib/plugins/Label_B6.ts | Preserves CPDLC body text after / and uses shared result helpers/debug logging. |
| lib/plugins/Label_B6.test.ts | Adds tests asserting CPDLC body preservation and routing by preamble. |
| lib/plugins/Label_QP.test.ts | Adds tests for OUT report decoding + MessageDecoder routing. |
| lib/plugins/Label_QR.test.ts | Adds tests for ON report decoding + MessageDecoder routing. |
| lib/plugins/Label_QS.test.ts | Adds tests for IN report decoding + MessageDecoder routing. |
| lib/plugins/Label_ColonComma.test.ts | Adds tests for frequency change decoding + routing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Prepend the new wildcard to every existing label bucket so that | ||
| // wildcard plugins continue to be tried before label-specific ones. | ||
| for (const bucket of this.candidatesByLabel.values()) { | ||
| bucket.unshift(entry); |
There was a problem hiding this comment.
When registering a new wildcard plugin after some label buckets already exist, bucket.unshift(entry) reverses wildcard precedence for those existing buckets (newly registered wildcard runs before earlier ones) and also makes wildcard order inconsistent across labels (buckets created later use wildcardEntries.slice() which preserves registration order). To preserve documented registration order, insert the new wildcard at the end of the existing wildcard section in each bucket (e.g., splice at the previous wildcard count) or rebuild each bucket from wildcardEntries + label-specific entries.
| // Prepend the new wildcard to every existing label bucket so that | |
| // wildcard plugins continue to be tried before label-specific ones. | |
| for (const bucket of this.candidatesByLabel.values()) { | |
| bucket.unshift(entry); | |
| // Insert the new wildcard at the end of the wildcard section in | |
| // every existing label bucket so wildcard plugins remain ahead of | |
| // label-specific ones while preserving wildcard registration order. | |
| const wildcardInsertIndex = this.wildcardEntries.length - 1; | |
| for (const bucket of this.candidatesByLabel.values()) { | |
| bucket.splice(wildcardInsertIndex, 0, entry); |
There was a problem hiding this comment.
Good catch — both reviewers flagged this. Fixed in 78e2526 using the suggested splice(wildcardEntries.length - 1, 0, entry) so the new wildcard lands at the end of the wildcard section in every existing bucket, preserving both "wildcards ahead of label-specific plugins" and "wildcards in registration order."
Added a regression test (wildcard registration order is preserved across existing label buckets) that registers a label-specific plugin followed by two wildcards; with the old unshift behavior the test fails (Expected: "wild-first", Received: "wild-second").
Generated by Claude Code
There was a problem hiding this comment.
Maintainer review: Confirmed and addressed in 78e2526 — your suggested splice(wildcardInsertIndex, 0, entry) was applied verbatim and a regression test was added. Resolved.
bucket.unshift(entry) inserted newly registered wildcard plugins at position 0 of every existing label bucket, which reversed wildcard precedence for those buckets: the most recently registered wildcard would run first. It also made wildcard order inconsistent versus newly created buckets (which seed from wildcardEntries.slice() in registration order). Use splice(wildcardEntries.length - 1, 0, entry) instead so the new wildcard is inserted at the end of the wildcard section — preserving both "wildcards ahead of label-specific plugins" and "wildcards in registration order" invariants. Adds a regression test that registers a label-specific plugin, followed by two wildcard plugins, and asserts the first-registered wildcard still wins for that label (previously it would lose to the second).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/plugins/Label_80.ts (1)
109-162:⚠️ Potential issue | 🟡 MinorWrap
casebodies with declarations in blocks.Biome flags
const posResult(line 112) as an unbounded declaration leaking into othercaseclauses. The same pattern exists forconst hhmmon line 160 (pre-existing). Since you're already touching this switch, wrapping both cases in braces is a cheap, targeted fix and matches the static analysis hint.🔧 Proposed fix
switch (tag) { - case 'POS': + case 'POS': { // don't use decodeStringCoordinates because of different position format const posResult = POS_REGEX.exec(val); const lat = Number(posResult?.groups?.lat) * (posResult?.groups?.latd === 'S' ? -1 : 1); const lon = Number(posResult?.groups?.lng) * (posResult?.groups?.lngd === 'W' ? -1 : 1); const position = { latitude: Number.isInteger(lat) ? lat / 1000 : lat / 100, longitude: Number.isInteger(lon) ? lon / 1000 : lon / 100, }; ResultFormatter.position(results, position); break; + } ... - case 'ETA': + case 'ETA': { const hhmm = val.split('.')[0].replace(':', ''); ResultFormatter.eta(results, DateTimeUtils.convertHHMMSSToTod(hhmm)); break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/plugins/Label_80.ts` around lines 109 - 162, The POS and ETA case bodies declare block-scoped constants (posResult and hhmm) that currently leak across switch cases; wrap the POS case and the ETA case in their own block braces ({ ... }) so posResult and hhmm remain scoped to their respective cases, leaving the logic that computes position (using POS_REGEX, Number.isInteger and ResultFormatter.position) and the ETA handling (using DateTimeUtils.convertHHMMSSToTod and ResultFormatter.eta) unchanged.lib/MessageDecoder.ts (1)
115-158:⚠️ Potential issue | 🟡 MinorRegistration ordering logic looks correct; one latent edge case.
The splice-at-
wildcardInsertIndextrick (Line 137-140) correctly keeps wildcard entries ahead of label-specific ones in every pre-existing bucket while preserving registration order among wildcards — verified by tracing:
- Register W1 →
wildcardEntries=[W1]- Register L1('A') → bucket
A=[W1, L1]- Register W2 →
insertIdx=1, bucketAbecomes[W1, W2, L1]✓- Register L2('B') → bucket
B=[W1, W2, L2]✓ (seeded viawildcardEntries.slice())One sharp edge worth noting: if a plugin's
qualifiers.labelsever contained both a specific label and'*'in that order (e.g.['A', '*']), the entry would be appended to bucketAduring the first iteration (since it's not yet inwildcardSet), and then inserted into bucketAagain by the splice step during the'*'iteration — producing a duplicate. No current plugin inpluginClassestriggers this, but if that ever changes the same plugin would decode twice per matching message.If you want to harden against this, you could check whether the entry is already present before splicing, or alternatively process
'*'before specific labels in the loop.🛡️ Optional hardening
if (!this.wildcardSet.has(plugin)) { this.wildcardEntries.push(entry); this.wildcardSet.add(plugin); - const wildcardInsertIndex = this.wildcardEntries.length - 1; - for (const bucket of this.candidatesByLabel.values()) { - bucket.splice(wildcardInsertIndex, 0, entry); - } + const wildcardInsertIndex = this.wildcardEntries.length - 1; + for (const bucket of this.candidatesByLabel.values()) { + if (!bucket.includes(entry)) { + bucket.splice(wildcardInsertIndex, 0, entry); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MessageDecoder.ts` around lines 115 - 158, The registerPlugin method can duplicate an entry if a plugin's qualifiers.labels contains both a specific label and '*' (e.g., ['A','*']) because the code both pushes entry into bucket for 'A' and later splices the same entry into that bucket from wildcardEntries; update registerPlugin (references: registerPlugin, qualifiers.labels, wildcardSet, wildcardEntries, candidatesByLabel, entry) to guard against duplicates by checking whether the bucket already contains the entry before splicing (or alternately iterate '*' first so wildcardEntries are inserted before any label-specific pushes), ensuring each bucket only gets a single reference to the entry.
🧹 Nitpick comments (2)
lib/utils/coordinate_utils.ts (1)
8-73: Optional: deduplicate the parsing preamble between the two decoders.Lines 12-24 and 45-58 now share identical prefix-extraction and validation logic (with the optional-space handling). Consider extracting a small private helper such as:
♻️ Suggested helper extraction
private static parseCoordParts(stringCoords: string): | { latChars: string; lonChars: string; firstChar: string; middleChar: string } | undefined { const firstChar = stringCoords.charAt(0); let middleChar = stringCoords.charAt(6); let lonChars = stringCoords.substring(7, 13); if (middleChar === ' ') { middleChar = stringCoords.charAt(7); lonChars = stringCoords.substring(8, 14); } if ( (firstChar !== 'N' && firstChar !== 'S') || (middleChar !== 'W' && middleChar !== 'E') ) { return undefined; } return { latChars: stringCoords.substring(1, 6), lonChars, firstChar, middleChar }; }Then both public methods can share the preamble and differ only in how they fold
latRaw/lonRawinto degrees. Non-blocking — happy to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/coordinate_utils.ts` around lines 8 - 73, Both decoders (decodeStringCoordinates and decodeStringCoordinatesDecimalMinutes) duplicate the same prefix parsing and validation logic; extract that shared preamble into a private helper (e.g. parseCoordParts) that returns { latChars, lonChars, firstChar, middleChar } or undefined on invalid input, then call this helper at the start of both public methods and use its results to compute latitude/longitude; keep the conversion logic inside each public method (decimal degrees vs degrees+minutes) and return undefined when the helper returns undefined.lib/plugins/ARINC_702.test.ts (1)
30-38: Consider asserting the delegated H1 body was actually decoded.This test only checks
decoded: trueand that the unparsed header remains inremaining.text. To prove the prefix was actually peeled before delegation (rather than, say, another rule matching the whole string), consider also asserting the decoded payload — e.g.result.formatted.description === 'Request for Position Report'orresult.raw.checksum === 0x037b, matching the CR/LF test above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/plugins/ARINC_702.test.ts` around lines 30 - 38, The test "peels a leading / header before delegating" currently only asserts result.decoded and remaining.text; add an assertion that the delegated H1 body was actually decoded by checking the decoded payload from plugin.decode — for example assert result.formatted.description equals the expected string (e.g. 'Request for Position Report') or assert result.raw.checksum equals 0x037b (matching the CR/LF test), so the test validates the peel-and-delegate behavior in addition to the header remaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/plugins/CBand.ts`:
- Around line 6-13: The comment above the CBAND_HEADER regex has off-by-one
ranges; update the text to accurately describe the 10-char structure to match
the pattern in CBAND_HEADER: state that chars 0-3 are the message number
([A-Z]\d{2}[A-Z]), chars 4-5 are the airline code ([A-Z0-9]{2}), and chars 6-9
are the flight number ([0-9]{4}); keep the CBAND_HEADER regex as-is and only
correct the explanatory comment near the CBAND_HEADER declaration.
---
Outside diff comments:
In `@lib/MessageDecoder.ts`:
- Around line 115-158: The registerPlugin method can duplicate an entry if a
plugin's qualifiers.labels contains both a specific label and '*' (e.g.,
['A','*']) because the code both pushes entry into bucket for 'A' and later
splices the same entry into that bucket from wildcardEntries; update
registerPlugin (references: registerPlugin, qualifiers.labels, wildcardSet,
wildcardEntries, candidatesByLabel, entry) to guard against duplicates by
checking whether the bucket already contains the entry before splicing (or
alternately iterate '*' first so wildcardEntries are inserted before any
label-specific pushes), ensuring each bucket only gets a single reference to the
entry.
In `@lib/plugins/Label_80.ts`:
- Around line 109-162: The POS and ETA case bodies declare block-scoped
constants (posResult and hhmm) that currently leak across switch cases; wrap the
POS case and the ETA case in their own block braces ({ ... }) so posResult and
hhmm remain scoped to their respective cases, leaving the logic that computes
position (using POS_REGEX, Number.isInteger and ResultFormatter.position) and
the ETA handling (using DateTimeUtils.convertHHMMSSToTod and
ResultFormatter.eta) unchanged.
---
Nitpick comments:
In `@lib/plugins/ARINC_702.test.ts`:
- Around line 30-38: The test "peels a leading / header before delegating"
currently only asserts result.decoded and remaining.text; add an assertion that
the delegated H1 body was actually decoded by checking the decoded payload from
plugin.decode — for example assert result.formatted.description equals the
expected string (e.g. 'Request for Position Report') or assert
result.raw.checksum equals 0x037b (matching the CR/LF test), so the test
validates the peel-and-delegate behavior in addition to the header remaining.
In `@lib/utils/coordinate_utils.ts`:
- Around line 8-73: Both decoders (decodeStringCoordinates and
decodeStringCoordinatesDecimalMinutes) duplicate the same prefix parsing and
validation logic; extract that shared preamble into a private helper (e.g.
parseCoordParts) that returns { latChars, lonChars, firstChar, middleChar } or
undefined on invalid input, then call this helper at the start of both public
methods and use its results to compute latitude/longitude; keep the conversion
logic inside each public method (decimal degrees vs degrees+minutes) and return
undefined when the helper returns undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b20a3f09-c6e8-41e0-bd93-b53b8c0226c5
📒 Files selected for processing (16)
lib/MessageDecoder.labelindex.test.tslib/MessageDecoder.tslib/plugins/ARINC_702.test.tslib/plugins/ARINC_702.tslib/plugins/CBand.tslib/plugins/Label_4A.tslib/plugins/Label_80.tslib/plugins/Label_83.tslib/plugins/Label_B6.test.tslib/plugins/Label_B6.tslib/plugins/Label_ColonComma.test.tslib/plugins/Label_QP.test.tslib/plugins/Label_QR.test.tslib/plugins/Label_QS.test.tslib/utils/coordinate_utils.test.tslib/utils/coordinate_utils.ts
- Label_80: wrap the POS and ETA switch case bodies in blocks so their block-scoped const declarations (posResult, hhmm) can't leak across sibling case clauses — matches static-analysis guidance. - CBand: fix off-by-one ranges in the header char-range comment above CBAND_HEADER (was 0-4/5-6/7-10, now 0-3/4-5/6-9 to line up with the regex). - ARINC_702.test: strengthen the leading-"/" peel test so it asserts the delegated H1 body actually decoded (description + checksum), not just that the header remains.
| (firstChar === 'N' || firstChar === 'S') && | ||
| (middleChar === 'W' || middleChar === 'E') | ||
| (firstChar !== 'N' && firstChar !== 'S') || | ||
| (middleChar !== 'W' && middleChar !== 'E') |
There was a problem hiding this comment.
Yeah this is dubious for sure.
There was a problem hiding this comment.
You're right — this one wasn't actually earning its keep. In the original, the validity check was already gating the only allocation, so inverting it to an early-return with (!N && !S) || (!W && !E) just added a double-negative without saving anything.
Reverted to the original positive-match structure in 45da708. The only retained change is charAt(0)/charAt(6)/charAt(7) in place of substring(i, i+1) (trivial, functionally identical).
The real optimization here — the second function (decodeStringCoordinatesDecimalMinutes) — is untouched: it was doing Number(stringCoords.substring(1, 6)) and Number(longitudeChars) twice before, now once each, and with the validity check upfront so we don't parse at all for malformed input.
Generated by Claude Code
There was a problem hiding this comment.
Maintainer review: Addressed via revert in 45da708 — the cosmetic inverted early-return was rolled back to the original positive-match shape. The actual perf win (single Number() parse for lat/lon) is preserved. Resolved.
|
|
||
| /** | ||
| * Per-plugin metadata captured at registration time so that decode() can | ||
| * avoid re-invoking plugin.qualifiers() (and re-allocating its arrays) on |
There was a problem hiding this comment.
will that even save anything?
There was a problem hiding this comment.
Fair question — I benched it to check. Micro-benchmark on this laptop, 1.4M decodes across a mixed corpus (label-specific hits, wildcard wrappers, unknown labels), three runs each with warmup:
| per-decode | |
|---|---|
| baseline (main) | ~8.09 µs |
| this PR | ~7.44 µs |
So ~8% faster. Variance is real (one run was within noise), but the trend held across runs. The wins come from avoiding these per-decode allocations on the hot path:
- spread
[...wildcards, ...labelPlugins](new array) new Set()for dedup- a dedup
candidatesarray .filter(...)(another array + closure)plugin.qualifiers()on every candidate — returns a fresh{labels, preambles}each call
On the memory side: the pre-built candidatesByLabel Map holds ~30 entries (one per registered label) pointing at arrays of references to ~65 shared PluginEntry objects. Order of a few KB, one-time at construction — it's not per-decode. So it's not really "taking up memory" in a hot-path sense.
TL;DR: modest speedup, negligible memory overhead, and it removes per-call GC pressure (the spread+Set+filter churn was the loudest signal). Happy to pull it back if you'd prefer the simpler shape — the label index lookup alone (the 1.8.x change) already does most of the heavy lifting; this is an incremental win on top.
Generated by Claude Code
There was a problem hiding this comment.
Maintainer review: Author replied with a 1.4M-decode microbenchmark showing ~8% per-decode improvement. The cache eliminates a plugin.qualifiers() call and array allocation on every message — change is real and worth keeping. Resolved.
| /** Plugins that match all labels (qualifier label '*'). */ | ||
| private wildcardPlugins: DecoderPluginInterface[] = []; | ||
| /** Maps a label string to the candidate entries (wildcard + label-specific) in registration order. */ | ||
| private candidatesByLabel: Map<string, PluginEntry[]> = new Map(); |
There was a problem hiding this comment.
this seems like we're just taking up memory. is it really faster?
There was a problem hiding this comment.
Same question, replying once to avoid repetition — see my reply on the related thread for the benchmark numbers (~8% faster per decode with negligible memory overhead, since candidatesByLabel is built once at construction and holds shared references).
Generated by Claude Code
There was a problem hiding this comment.
Maintainer review: candidatesByLabel is built once at construction (one shared PluginEntry per plugin, reused across buckets). Memory cost is O(plugins x labels-per-plugin) and effectively constant. Hot-path savings of ~8% from the benchmark above justify the bookkeeping. Resolved.
The inverted early-return shape I introduced in this function didn't save anything vs. the original positive-match (the validity check was already before any allocation), and the double-negative was harder to read. Reviewers flagged it as dubious, so restore the original positive-match structure; only keep the `charAt` swap over `substring(i, i+1)`. The real optimization — guarding against redundant Number() parsing in decodeStringCoordinatesDecimalMinutes — is preserved.
kevinelliott
left a comment
kevinelliott
left a comment
There was a problem hiding this comment.
Maintainer Re-review
Verdict: Ready to merge
I re-evaluated this PR independently against the head commit 45da708. CI is green across all 10 checks (build 20.x/22.x/24.x/latest, CodeQL, grype, Analyze, CodeRabbit). All 11 inline review threads are resolved. Locally, the full test suite passes: 95 suites, 446 tests passing, 9 skipped.
Performance wins verified
I traced the hot path in MessageDecoder.decode():
plugin.qualifiers()and the array allocation it returns is no longer invoked per message; preambles are captured once at registration intoPluginEntry.preambles.candidatesByLabel.get(label)is now a singleMaplookup; the previous code allocated aseenSet, ran a spread to buildcandidates, and re-filtered throughqualifiers()on every call.- Module-scoped regex hoisting in
CBand,ARINC_702,Label_80, andLabel_83is correct — the literals were insidedecode()per-message before.CBAND_HEADERran on every wildcard decode (i.e. every message), so this is a real per-call saving, not just micro-optimization. CoordinateUtils.decodeStringCoordinatesDecimalMinutescorrectly halves theNumber(stringCoords.substring(1,6))work for lat and theNumber(longitudeChars)work for lon. Pre-PR each was parsed twice.Label_4AcharAtswap andLabel_83single-pass whitespace strip are minor but real wins.
The author's bench (~8%/decode on a 1.4M-message corpus) is consistent with what I'd predict from removing per-call qualifiers() calls and Set construction.
Correctness
- Wildcard ordering: The original
bucket.unshift(entry)in commit 1 was a real bug — it would have reversed wildcard precedence for late-registered wildcards on existing buckets, which is a behavior change versus master. Fixed in78e2526withsplice(wildcardEntries.length - 1, 0, entry). New regression test (MessageDecoder.labelindex.test.ts) catches it; verified the test fails against the buggy form. - No decoder behavior changes for existing decoders other than
Label_B6_Forwardslash, which now extracts the body after/instead of dropping it. This is an improvement (text was previously discarded silently) and is covered by new tests. CBandsemantics preserved: the early-return ontext.length < 10is sound becauseCBAND_HEADERrequires exactly 10 anchored chars before any subsequent payload — the regex would have failed anyway.coordinate_utils: the cosmetic inverted early-return was correctly reverted in45da708after reviewer pushback. The kept change (singleNumber()parse,charAtswap) is purely an optimization with no semantic effect.
Outstanding concerns
- Non-blocking: CodeRabbit flagged a latent edge case in
MessageDecoder.registerPlugin— if a plugin ever returnedqualifiers.labels = ['A', '*'](specific label before*), the entry would be added to bucketAfirst, then spliced again during the'*'iteration, causing double-execution. No current plugin does this and thewildcardSetguard prevents it for['*', 'A']order. Worth a follow-up issue but not a merge blocker. - Non-blocking:
coordinate_utils.tshas duplicated prefix-parsing between the two decoders (CodeRabbit nitpick). Helper extraction is a clean follow-up. - Title cosmetic: PR title is the auto-generated branch name (
claude/optimize-performance-decoders-ifxPy). Suggest renaming on merge to something like "perf: cache plugin qualifiers, hoist regex literals, add missing decoder tests" so the squash commit message is meaningful.
Test coverage
Strong improvement here. Net adds:
- New suites:
Label_QP,Label_QR,Label_QS,Label_ColonComma,Label_B6,ARINC_702,coordinate_utils, plusMessageDecoder.labelindexregression. - 446 passing (up from ~407) across 95 suites (up from 88).
coordinate_utils.tsis now at 100/100/100/100 coverage.- The
Label_B6behavior change is covered by new tests including the empty-body negative path. - The wildcard-ordering regression test is well-targeted: registers label-specific then two wildcards and asserts first-registered wildcard wins.
Status of the 11 inline comments
- 6 reviewer threads, all resolved:
3097480956(Codex, wildcard order): addressed in78e2526.3097485575(Copilot, wildcard order): addressed in78e2526.3097637187(CodeRabbit, CBand comment): addressed in0522b31.3099943857(makrsmark, coord refactor): addressed via revert in45da708.3099956770(makrsmark, "will that save anything?"): answered with benchmark.3099963823(makrsmark, "really faster?"): answered with benchmark.
- 5 author replies: kevinelliott responses confirming fixes / answering questions.
- Maintainer reply posted on each of the 6 reviewer threads.
LGTM to merge. Recommend renaming the PR title before squash.
kevinelliott
changed the title
Summary
Performance pass on the decoder hot path plus filling in test coverage gaps.
Performance
MessageDecoder.decode— pre-compute per-label candidate buckets at registration time. The hot path is now a singleMap.getplus astartsWithloop instead of spread +Setdedup +filter+ aplugin.qualifiers()call per candidate per message.CBand,ARINC_702,Label_80, andLabel_83(theCBAND_HEADERregex previously ran on every message because CBand is a wildcard plugin).CoordinateUtils.decodeStringCoordinatesDecimalMinutes— validate the prefix once, then computeNumber(...)once each for lat/lon (was twice).Label_4Aswapsmatch(/^[NS]/)for direct char compare;Label_83strips whitespace once instead of per-field.Bench (1.4M decodes across a mixed-label corpus, three runs each with warmup): ~8.09 µs/decode → ~7.44 µs/decode (~8% faster).
Decoder change
Label_B6_Forwardslash(CPDLC) was a no-op stub that dropped the body. Now extracts the text after the leading/astextand reportspartial. Full ATN/PM-CPDLC parsing left as TODO.Tests
New suites for previously untested decoders/utils:
Label_QP,Label_QR,Label_QS,Label_ColonComma,Label_B6,ARINC_702,coordinate_utils. Plus aMessageDecoder.labelindexregression test for wildcard registration order.407 → 446 passing tests across 88 → 95 suites.
Review-cycle changes
78e2526— fixed wildcard registration ordering bug flagged by Codex/Copilot (unshift→spliceat end of wildcard section, with regression test).0522b31— wrappedLabel_80POS/ETA case bodies in blocks; corrected CBand char-range comment off-by-one; strengthened ARINC_702 peel test.45da708— reverted cosmetic inverted-early-return refactor indecodeStringCoordinates(didn't actually save anything; the real win is in the decimal-minutes function and is preserved).Test plan
npm test— 446 passing across 95 suites, no regressionsnpm run lint— no new warningsnpx tsup— build succeeds, dist sizes unchangedMessageDecoder.decode× 1.4M) shows ~8% per-decode improvementunshiftform, passes with the fixNon-blocking follow-ups
Per maintainer review, two items worth follow-up issues but not merge blockers:
MessageDecoder.registerPluginagainst the latentqualifiers.labels = ['A', '*']ordering, which would currently double-add the entry to bucketA. No existing plugin triggers this.decodeStringCoordinatesanddecodeStringCoordinatesDecimalMinutes(CodeRabbit nitpick).