Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 1.1.1

* Avoid wraping response bodies that already contained `List<int>` in a `CastList`.
This will improve the performance of large response bodies

## 1.1.0

* Change `Request.hijack` return type from `void` to `Never`. This may cause
Expand Down
6 changes: 5 additions & 1 deletion lib/src/body.dart
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,13 @@ 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) {
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
28 changes: 28 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,33 @@ 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('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((List<int> values) => values, 'values', [1, 2, 3, 4]));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.having((List<int> values) => values, 'values', [1, 2, 3, 4]));
.having((values) => values, 'values', [1, 2, 3, 4]));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

});

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