Skip to content

Avoid wrapping List<int>/Stream<List<int>> response bodies in a CastList/CastStream #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 1.1.1

* Avoid wrapping response bodies that already contained `List<int>` or
`Stream<List<int>>` in a `CastList`/`CastStream` to improve
performance.

## 1.1.0

* Change `Request.hijack` return type from `void` to `Never`. This may cause
Expand Down
9 changes: 8 additions & 1 deletion lib/src/body.dart
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,16 @@ class Body {
contentLength = encoded.length;
stream = Stream.fromIterable([encoded]);
}
} else if (body is List<int>) {
// Avoid performance overhead from an unnecessary cast.
contentLength = body.length;
stream = Stream.value(body);
} else if (body is List) {
contentLength = body.length;
stream = Stream.fromIterable([body.cast()]);
stream = Stream.value(body.cast());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else if (body is Stream<List<int>>) {
// Avoid performance overhead from an unnecessary cast.
stream = body;
} else if (body is Stream) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, while we are here should we assume that Stream.cast is likely to have similar overhead?

Suggested change
} else if (body is Stream) {
} else if (body is Stream<int>) {
stream = body;
} else if (body is Stream) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, seems worth the optimization

stream = body.cast();
} else {
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: shelf
version: 1.1.0
version: 1.1.1
description: >-
A model for web server middleware that encourages composition and easy reuse
repository: https://github.com/dart-lang/shelf
Expand Down
33 changes: 33 additions & 0 deletions test/response_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'dart:async';
import 'dart:convert';
import 'dart:typed_data';

import 'package:shelf/shelf.dart' hide Request;
import 'package:test/test.dart';
Expand All @@ -25,6 +26,38 @@ void main() {
});
});

test('supports a Uint8List body without copying', () async {
var bytes = Uint8List(10);
var response = Response.ok(bytes);

expect(response.contentLength, 10);
expect(await response.read().single, same(bytes));
});

test('supports a List<int> body without copying', () async {
var bytes = <int>[1, 2, 3, 4];
var response = Response.ok(bytes);

expect(response.contentLength, 4);
expect(await response.read().single, same(bytes));
});

test('supports a Stream<List<int>> body without copying', () async {
var bytes = Stream.value(<int>[1, 2, 3, 4]);
var response = Response.ok(bytes);

expect(response.read(), same(bytes));
});

test('Copies a dynamic list of int elements', () async {
var bytes = <dynamic>[1, 2, 3, 4];
var response = Response.ok(bytes);

expect(response.contentLength, 4);
expect(await response.read().single,
isA<List<int>>().having((values) => values, 'values', [1, 2, 3, 4]));
});

group('new Response.internalServerError without a body', () {
test('sets the body to "Internal Server Error"', () {
var response = Response.internalServerError();
Expand Down