Avoid wrapping List<int>/Stream<List<int>> response bodies in a CastList/CastStream #178
Conversation
lib/src/body.dart
Outdated
| @@ -52,6 +54,13 @@ class Body { | |||
| contentLength = encoded.length; | |||
| stream = Stream.fromIterable([encoded]); | |||
| } | |||
| } else if (body is Uint8List) { | |||
There was a problem hiding this comment.
Or could this just be is List<int>?
There was a problem hiding this comment.
Ahh yeah, changed!
|
1.1.1 ? |
jonahwilliams
changed the title
| } else if (body is List) { | ||
| contentLength = body.length; | ||
| stream = Stream.fromIterable([body.cast()]); | ||
| stream = Stream.value(body.cast()); | ||
| } else if (body is Stream) { |
There was a problem hiding this comment.
I wonder, while we are here should we assume that Stream.cast is likely to have similar overhead?
| } else if (body is Stream) { | |
| } else if (body is Stream<int>) { | |
| stream = body; | |
| } else if (body is Stream) { |
There was a problem hiding this comment.
Definitely, seems worth the optimization
test/response_test.dart
Outdated
| expect( | ||
| await response.read().single, | ||
| isA<List<int>>() | ||
| .having((List<int> values) => values, 'values', [1, 2, 3, 4])); |
There was a problem hiding this comment.
| .having((List<int> values) => values, 'values', [1, 2, 3, 4])); | |
| .having((values) => values, 'values', [1, 2, 3, 4])); |
jonahwilliams
changed the title
|
I don't have any merge permissions btw 😄 |
| } else if (body is List) { | ||
| contentLength = body.length; | ||
| stream = Stream.fromIterable([body.cast()]); | ||
| stream = Stream.value(body.cast()); |
There was a problem hiding this comment.
If this is likely to be iterated over multiple times, you may be better to eagerly copy the list with List.from instead of using cast.
There was a problem hiding this comment.
My hunch is this isn't a problem. I don't expect multiple iterations in most cases. Usually when we would need this cast it would be a Response which will be passed to dart:io which I expect iterates once. Otherwise, when this stream is getting passed to user code (where it may be iterated more than once) I'd expect the optimizations in this PR should kick in since it will most often be a Stream<List<int>> coming from dart:io to start with.
|
This is published |
(Allows flutter/flutter#81010 to be fixed, though we need to change some code on the flutter side too)
TLDR: when writing data to a socket in dart, anything besides a Uint8List or Int8List will be necessarily copied (c.f. https://github.com/dart-lang/sdk/blob/3f0ad61daaac68e591bf01d0a3096926c30c1d42/sdk/lib/io/common.dart#L92)
shelf will unnecessarily wrap all Lists in a CastList which will perform runtime checks for every single element.
The combination of these behaviors causes requests for large files in the flutter run web server to be incredibly slow, observed to be on the order of multiple minutes locally. With the combination of this change and a patch to the tool to avoid creating Uint8List views, the time to serve the app drops to a few seconds.
I took a CPU profile starting from when chrome opened until the app's first frame. I could upload the actual timeline data too, but a picture something something a thousand words
Before:
After: