Repository navigation
Conversation
The Perfetto trace was exported as a JSON array of numbers, which makes jsonEncode do roughly two string writes per trace byte and can exhaust the browser's memory for large traces. Export it as a base64 string under a new key instead, and keep reading the legacy list format so older files still load. Also: - Only encode the bytes in a ByteData view when converting to base64. - Allocate Uint8ListRingBuffer.merged once at its exact size. - Show an error instead of crashing when the data is too large to export. - Revoke the download Blob URL after the download starts. Fixes flutter#10010
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes memory usage and file size when saving and loading performance trace data. It introduces base64 encoding for the Perfetto trace binary instead of the legacy JSON array of numbers, while maintaining backwards compatibility. It also optimizes buffer merging in Uint8ListRingBuffer, ensures only the active view of ByteData is encoded, handles potential RangeError during large exports, and defers revoking object URLs on the web to prevent download cancellations. There are no review comments, so I have no feedback to provide.
srawlins
left a comment
srawlins
left a comment
There was a problem hiding this comment.
This looks like a great feature, thanks!
|
@kenzieschmoll I'd love your review here, if not the code itself, then the feature idea. |
| return ExportControllerWeb(); | ||
| } | ||
|
|
||
| const _revokeObjectUrlDelay = Duration(seconds: 1); |
There was a problem hiding this comment.
one second seems arbitrary - how did you come to this value and test that this is enough time?
| // numbers. | ||
| final legacyTrace = json[OfflinePerformanceData.traceBinaryKey]; | ||
| return switch (legacyTrace) { | ||
| final Uint8List bytes => bytes, |
There was a problem hiding this comment.
nit: Remove the Uint8List case and the loads a legacy Uint8List test. A Uint8List under traceBinaryKey only came from the old in-memory toJson() for Review History, which never outlives a session, so nothing can produce this value anymore.
| } on RangeError catch (e) { | ||
| // The data is too large to be encoded as a single JSON string (the | ||
| // maximum string length in the browser is about 2^29 characters). | ||
| notificationService.pushError( | ||
| 'Unable to export this screen\'s data because it is too large: ' | ||
| '${e.message}', | ||
| ); |
There was a problem hiding this comment.
This clause likely never matches in the browser. The Invalid string length failure is a JS RangeError, which dart2js turns into an ArgumentError, and dart2wasm reports as a JS exception. Either way the export fails without the notification.
Either:
- Catch everything with a message that doesn't assume the cause (suggestion below), or
- Test a too-large export on both dart2js and dart2wasm, find the exact error types, and catch only those. Keep the "too large" wording in that case, since you'd know that's the cause.
Either way, send the failure to analytics with reportError from shared/framework/app_error_handling.dart. It calls ga.reportError and logs at severe level. Write the notification text by hand: reportError(notifyUser: true) would show the raw toString().
| } on RangeError catch (e) { | |
| // The data is too large to be encoded as a single JSON string (the | |
| // maximum string length in the browser is about 2^29 characters). | |
| notificationService.pushError( | |
| 'Unable to export this screen\'s data because it is too large: ' | |
| '${e.message}', | |
| ); | |
| } catch (e, st) { | |
| reportError(e, errorType: 'OfflineDataExportError', stack: st); | |
| notificationService.pushError( | |
| 'Unable to export this screen\'s data. Large recordings may exceed ' | |
| 'the maximum export size.', | |
| ); |
The suggestion covers the first option only, and you'll need to add the import of ../framework/app_error_handling.dart by hand.
This branch has not been deployed
4 participants
Fixes #10010
Saving Performance data wrote the Perfetto trace as a JSON array of numbers.
jsonEncodethen does roughly two string writes per trace byte, so a ~50 MB trace becomes a ~184 MB file and a very large number of string fragments in the browser, which is what runs the tab out of memory.This changes the export to:
traceBinaryBase64key (about 1.33x the trace size instead of about 3.7x).toJson()keeps aByteDataview and the existingtoEncodabledoes the base64 step, so "Review History" on disconnect still holds the trace without copying it.traceBinarylist format, so existing exported files still load. Older DevTools versions opening a new file will see no trace rather than throwing a type error, because the key is different.ByteDataEncodeDecodeto encode only the bytes in the view, not the whole backing buffer.Uint8ListRingBuffer.mergedonce at its exact size instead of lettingBytesBuilderround up to a power of two.Not changed: import still reads the whole file as one string, and traces over roughly 400 MB would need a different file format.
Tests: added round-trip tests for the new format, the legacy list and
Uint8Listinputs, an in-memoryByteData, a sub-view of a larger buffer, and a size check. Also updated the existingtoJsonassertion. I have not yet measured the memory difference in a browser against a real 2 minute recording.Pre-launch Checklist
General checklist
///).Issues checklist
Tests checklist
Feature-change checklist
packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.