claude/optimize-performance-decoders-ifxPy#388
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
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 | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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.
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
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
|
|
||
| /** | ||
| * 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
| /** 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
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.
Performance:
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.
Label_83 to avoid re-allocating patterns inside per-message decode().
the prefix chars before parsing and computes Number(...) once each
for lat/lon (was twice).
re-stripping.
Decoders:
rather than discarding it (full ATN/PM-CPDLC parsing is still TODO).
Tests:
Label_QS, Label_ColonComma, Label_B6, ARINC_702.
Test count: 407 -> 445 passing, 88 -> 95 suites.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor