perf: skip full decode of non-EVENT frames on the offload gesture path (#47 follow-up)#64
Merged
Merged
Conversation
… path (#47 follow-up) FrameRouter.dispatchLiveGestureIfFresh runs for EVERY frame on the historical-offload path (thousands of type-47 records over a multi-minute sync) purely to catch a rare EVENT gesture (double-tap / wrist). It fully parsed each one — CRC32 + FieldBuilder allocation — only to discard it at `guard typeName == "EVENT"`. Offloads carry far more frames than the ~1Hz live stream #47 targeted, so this is the bigger decode redundancy. Add WhoopProtocol.frameTypeName(_:family:): the packet type NAME only, no CRC/FieldBuilder, family-aware (inner type byte at [4] on WHOOP4, [8] on 5/MG, same lookup each parse uses). dispatchLiveGestureIfFresh now pre-checks `frameTypeName == "EVENT"` and skips the full parse for the ~99% that aren't. Byte-identical: an EVENT frame still gets the full parse + CRC guard; a non-EVENT frame was dropped at the typeName guard anyway. Verified LOCALLY (WhoopProtocol has no Compression wall): a new FastPathParityTests case proves frameTypeName == parseFrame(...).typeName across the whoop4 parity + historical corpora and the whoop5 hardware vectors — passing. So the pre-filter can't diverge from the full-parse guard. iOS/macOS only — Android dispatches gestures INSIDE handleFrame (entangled with routing) and, post-#47, already parses offload frames once, so there's no isolated pre-filter to add there. NEEDS the same on-strap sanity check as #47 (gestures still fire mid-offload) before merge.
…er drops a gesture (#47 re-review) The parity test asserted strict frameTypeName == parseFrame.typeName. That's right for valid frames but would FALSE-FAIL on a malformed/short frame (frameTypeName returns nil; parseFrame calls it 'INVALID/FRAGMENT') — and it left malformed frames untested, the exact case where a false-negative would silently drop a gesture on real BLE (fragments/corruption). Assert the two things that actually matter: exact typeName for VALID frames, and the EVENT *decision* for ANY frame (the property the pre-filter's guard rests on). Add explicit empty / short / wrong-SOF / short-5MG cases — all land on 'not EVENT' in both paths, so the pre-filter skips exactly as the full parse would. Passes locally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #47/#63. The bigger of the two decode redundancies I flagged.
Problem
FrameRouter.dispatchLiveGestureIfFreshfires for every frame on the historical-offload path — thousands of type-47 records over a multi-minute sync — purely to catch a rareEVENTgesture (double-tap / wrist). It fully parsed each one (CRC32 + FieldBuilder allocation) only to discard it atguard parsed.typeName == "EVENT". Offloads carry far more frames than the ~1 Hz live stream #47 targeted, so this is the larger win.Fix
WhoopProtocol.frameTypeName(_:family:): the packet type name only — no CRC verify, no FieldBuilder. Family-aware (inner type byte at[4]on WHOOP4,[8]on 5/MG, using the same lookup each parse path uses).dispatchLiveGestureIfFreshnow pre-checksframeTypeName(...) == "EVENT"and skips the full parse for the ~99% that aren't. Byte-identical: an EVENT frame still gets the full parse + CRC guard below; a non-EVENT frame was dropped at the typeName guard anyway.Verification
FastPathParityTestscase provesframeTypeName == parseFrame(...).typeNameacross the whoop4 parity + historical corpora and the whoop5 hardware vectors — passing. So the pre-filter's== "EVENT"guard provably can't diverge from the full-parse guard.FrameRouterchange.Scope / caveat
handleFrame(entangled with routing) and, post-perf: parse each live WHOOP4 frame once instead of 2-3× (thread ParsedFrame through router/collector) #47, already parses offload frames once — so there's no isolated pre-filter to add there.Follow-up to #47.