Repository navigation
Inflate map sightline data with the browser's gzip on the web - #255
Conversation
Opening a map on the web froze the page for 0.6 to 1.7 s. Most of that was the pure-Dart gzip inflate and CRC-32 from package:archive, which on the web run on the UI thread (compute does not leave it there): about 0.4 s per side on Bind. inflateWorldGzip now hands the bytes to the browser's DecompressionStream, which inflates Bind's 19 MB side in about 50 ms. Desktop is unchanged: it still calls decodeWorldGzip (native zlib) inside its background isolate. Browsers without DecompressionStream (Firefox before 113, Safari before 16.4) also keep decodeWorldGzip. The integrity contract holds. The browser rejects a wrong CRC-32 or length, a stream cut before its footer, and bytes after it; the header and the footer length are still checked here, and any failure is the same FormatException as before. Navigation charts read through the same function, so they get the same speedup. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (5)
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 |
| Future<Uint8List> inflateWorldGzip(Uint8List compressed) async { | ||
| // Firefox before 113 and Safari before 16.4 lack it; they keep the slow path. | ||
| if (!globalContext.has('DecompressionStream')) { | ||
| return decodeWorldGzip(compressed); |
There was a problem hiding this comment.
The new browser decompression path and its older-browser fallback have no committed browser test. Both paths decoded valid gzip and rejected damaged input in Chrome, but that check is not part of the test suite. A later regression in either path could therefore go unnoticed. Please commit browser tests for valid and damaged input through inflateWorldGzip, including the fallback. This is a non-blocking coverage concern.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Authored Chrome gzip probe source
- The exact temporary test source calls the existing decoder and new browser entry point with valid, bad-CRC, and truncated input, then removes DecompressionStream to exercise the fallback.
- The exact authored command runs the two focused Chrome test phases, captures their output and exit codes, and removes the temporary test copy.
Committed test coverage inspection
- Git commands list the PR files and committed test references, showing no committed test of the new browser entry point or DecompressionStream branch.
Chrome result with the existing Dart decoder
- The baseline Chrome run decoded valid gzip and rejected bad-CRC and truncated input, establishing comparison behavior with exit code 0.
Chrome result with the new stream and simulated older-browser fallback
- The Chrome run decoded valid gzip and rejected damaged input through both the browser stream and the feature-detected fallback, with exit code 0.
There was a problem hiding this comment.
Agreed it's a gap, but a committed browser test wouldn't close it today. CI's web job only builds the app (flutter build web), and nothing in CI runs @TestOn('browser') tests: the existing one, test/web/web_codecs_video_encoder_test.dart, is run by hand. On this machine flutter test --platform chrome doesn't load at all. So a new browser test would sit unexecuted. Instead, the browser path was checked in Chromium:
- 55 fixtures (concatenated members, trailing data, wrong CRC with correct ISIZE, wrong ISIZE, footer truncation, empty and short input, optional header flags, bad FHCRC), compiled with both dart2js and dart2wasm. Every failure was a
FormatException. - All 26 bundled assets came out byte-identical to the old decoder.
- The fallback path was exercised with
DecompressionStreamremoved.
Running browser tests in CI is worth doing, but as its own change.
There was a problem hiding this comment.
That’s fair. A committed @TestOn('browser') test would not provide regression coverage while CI neither runs browser tests nor can reliably launch them, so it wouldn’t close the practical gap today. The Chromium validation is substantially broader than the proposed test—55 fixtures across dart2js and dart2wasm, all bundled assets checked byte-for-byte, and the fallback exercised—so I’m withdrawing this finding for PR #255. Running browser tests in CI is worthwhile, but belongs in a separate change.
Opening a map on the web froze the page for up to 1.5 s. On Bind, the biggest map, loading now takes half as long, and the longest freeze is about a quarter of what it was:
That's both sides loaded through the real
svgHeightRuntimeProviderin headless Edge, median of 3 runs with before and after alternated. "Longest freeze" is the longest gap between 1 ms timer ticks during the load. In the signed-in web app (release build against production, test account), opening a Bind strategy's longest frame gap went from 924–1,069 ms to 535–563 ms, and on Breeze from 854–1,111 ms to 479–549 ms.What was slow
On the web,
computeruns on the UI thread. Each side's sightline asset was inflated by package:archive's pure-Dart gzip, then CRC-checked in pure Dart. On Bind that was about 330 ms of inflate plus 74 ms of CRC per side. Everything else (fetch,jsonDecode, artwork SHA-256,fromJson,prepareDartQuery) was untouched and is listed below.What changed
inflateWorldGzipuses the browser's ownDecompressionStream('gzip')throughdart:js_interop(no new dependency). Bind's side inflates in about 50 ms, CRC and length included.DecompressionStream(Firefox before 113, Safari before 16.4) keepdecodeWorldGzip, so they don't lose cones.decodeWorldGzip(native zlib) in the same background isolate.Integrity
FormatExceptionas before.What's left
Per side on Bind, about 250–300 ms goes into turning JSON into the model (
jsonDecode+SvgHeightVisibility.fromJson), spread across dart2js JSON conversion, building the ground triangle grid (17 of Bind's 19 MB of JSON is ground mesh) and building the edge tree. Cutting that further needs a different asset format or a web worker. That's a separate design decision.Testing
flutter testsuite passes: 1,763 passed, 9 skipped.flutter test --platform chromedoesn't load on this machine, so there's no committed browser test. The web path was exercised in the Edge runs above.🤖 Generated with Claude Code
The missing browser tests are a non-blocking concern; the behavior checked in Chrome worked, so this finding does not make the PR unsafe to merge.
Findings
Summary
The PR uses the browser’s gzip decompressor for map sightline and navigation data when available, while retaining the Dart decoder for older browsers and native platforms. A Chrome check decoded valid gzip and rejected damaged input through both browser paths. Neither browser path has a committed regression test, so future breakage could go unnoticed.
Reviews (1) · Last reviewed commit: "Inflate map sightline data with the brow..."