Add `peek` and `lookAhead` to `StreamQueue`.

Add `peek` and `lookAhead` to `StreamQueue`.

These allow users to look at events (similar to `next` and `take`)
without consuming them. For simple cases, they can be used instead of
`startTransaction` to decide what to do before doing it.

R=nweiz@google.com

Committed: https://github.com/dart-lang/async/commit/420d0840ae6f84f8be12aa39837b8499dbded0a6

[Lasse Reichstein Nielsen, 2017-01-24 08:29:42]

lrn@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2017-01-24 22:37:59]

https://codereview.chromium.org/2649033006/diff/1/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/2649033006/diff/1/CHANGELOG.md#newcode7
CHANGELOG.md:7: These allow users to look at events without consuming them.
I released 1.12 so I could start using it in other packages, so these should go
in 1.13 now.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:126: /// Look at the next [count] data events without
consuming them.
"Look at" -> "Returns"

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:132: if (count < 0) throw new RangeError.range(count,
0, null, "count");
`RangeError.checkNotNegative()`?

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:139: }
Why not just implement these in terms of transactions and take()/next? It would
be a lot simpler.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:164: /// Looks at the next (yet unrequested) event
from the stream.
"Looks at" -> "Returns"

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:377: /// may be called again.
Is it really necessary to explicitly list all the requests? It seems difficult
to maintain, and we're already saying that no more requests can be added.

[Lasse Reichstein Nielsen, 2017-01-25 07:58:03]

https://codereview.chromium.org/2649033006/diff/1/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/2649033006/diff/1/CHANGELOG.md#newcode7
CHANGELOG.md:7: These allow users to look at events without consuming them.
On 2017/01/24 22:37:59, nweiz wrote:
> I released 1.12 so I could start using it in other packages, so these should
go
> in 1.13 now.

Done.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:126: /// Look at the next [count] data events without
consuming them.
On 2017/01/24 22:37:59, nweiz wrote:
> "Look at" -> "Returns"

Done.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:132: if (count < 0) throw new RangeError.range(count,
0, null, "count");
On 2017/01/24 22:37:59, nweiz wrote:
> `RangeError.checkNotNegative()`?

Done.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:139: }
Because it would also be a lot more expensive.
I prefer not to implement less complex operations in terms of more complex ones.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:164: /// Looks at the next (yet unrequested) event
from the stream.
On 2017/01/24 22:37:59, nweiz wrote:
> "Looks at" -> "Returns"

Done.

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:377: /// may be called again.
It seemed like a great idea at the time ... but it is getting ridiculous.
I'll reword it to only use examples, not an exhausting list.

[nweiz, 2017-01-25 22:41:18]

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:139: }
On 2017/01/25 07:58:03, Lasse Reichstein Nielsen wrote:
> Because it would also be a lot more expensive.
> I prefer not to implement less complex operations in terms of more complex
ones.

Do you have benchmarks where the extra processing produces a real-world
difference? Do you have a strong expectation that these APIs will be used in
performance-critical core loops? If not, I think we should strongly favor
cleaner, simpler implementations in terms of APIs that are already
thoroughly-tested. It may be a little slower, but it will make maintenance and
external contribution much easier.

[Lasse Reichstein Nielsen, 2017-01-26 12:13:43]

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/2649033006/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:139: }
I've added a benchmark below. It shows that `return sq.peek` is 4-6 times as
fast as `var t = sq.startTransaction();return
t.newQueue().next().whenComplete(t.reject)` (which is the fastest version I
found that handles errors correctly).

My expectation of how it will be used probably isn't really useful, since
history shows that prediction is hard - this is a general purpose library, so I
cannot predict how it will be used. Libraries, platform or otherwise, should
always assume that they are being used in performance critical places, because
the users of the libraries are not in a (realistic) position to change them.

And I do believe, very strongly, that defining simple operations in terms of
more complex ones is likely to give you unexpected dependencies and make
maintenance harder in the long run (but then, that's a prediction too).

Benchmark:

import "dart:async";
import "package:async/async.dart";

bench(n, f, t) async {
  var c = 0;
  var e = 0;
  var s = new Stopwatch()..start();
  do {
    var st = new Stream.fromIterable([1,2,3,4,5,6]);
    var sq = new StreamQueue(st);
    while (await sq.hasNext) {
      var p = await f(sq);
      var a = await sq.next;
      if (!identical(p, a)) throw "wat";
    }
    e = s.elapsedMilliseconds;
    c += 6;
  } while (e < t);
  s.stop();
  return c/e;
}

main() async {
  await bench("peek", peek, 0);
  await bench("startTransaction", transact, 0);
  print("peek: ${await bench("peek", peek, 2000)} /ms");
  print("startTransaction: ${await bench("peek", transact, 2000)} /ms");
  // Do the important computation more than once, to emphasize it relative
  // to the overhead around it.
  print("peek*2: ${await bench("peek", twice(peek), 2000)} /ms");
  print("startTransaction*2: ${await bench("peek", twice(transact), 2000)}
/ms");
  print("peek*3: ${await bench("peek", thrice(peek), 2000)} /ms");
  print("startTransaction*3: ${await bench("peek", thrice(transact), 2000)}
/ms");
}

peek(sq) => sq.peek;
transact(sq) {
  var t = sq.startTransaction();
  return t.newQueue().next.whenComplete(t.reject);
}

twice(f) => (x) => f(x).then((_)=>f(x));
thrice(f) => (x) {
  var again = (_) => f(x);
  return f(x).then(again).then(again);
};

[nweiz, 2017-01-26 22:51:26]

> Libraries, platform or otherwise, should always assume that they are being
> used in performance critical places, because the users of the libraries are
> not in a (realistic) position to change them.

I strongly disagree. This philosophy leads to extremely complex implementations
that take longer to write and are very difficult to maintain or expand upon,
especially once ownership shifts to people other than the original author. It's
premature optimization. When you write simple code first and drive your
optimization energy based on actual user need, you're able to produce more code
that runs more reliably.

> And I do believe, very strongly, that defining simple operations in terms of
> more complex ones is likely to give you unexpected dependencies and make
> maintenance harder in the long run (but then, that's a prediction too).

I also disagree with this. We create abstractions to make computations simpler
to express and reason about, and that benefit applies regardless of how complex
the implementation of the abstraction is.

If you still feel like the efficiency here is worth the maintenance and
readability hit, I won't push further, but I want to lay out my reasoning.

[Lasse Reichstein Nielsen, 2017-01-27 11:46:25]

On 2017/01/26 22:51:26, nweiz wrote:
> > Libraries, platform or otherwise, should always assume that they are being
> > used in performance critical places, because the users of the libraries are
> > not in a (realistic) position to change them.
> 
> I strongly disagree. This philosophy leads to extremely complex
implementations
> that take longer to write and are very difficult to maintain or expand upon,
> especially once ownership shifts to people other than the original author.
It's
> premature optimization. When you write simple code first and drive your
> optimization energy based on actual user need, you're able to produce more
code
> that runs more reliably.

That is a point, and at some abstraction level I think I agree. However, I do
feel very strongly that at the lowest abstraction levels, you should be
efficient from the start, so things built on top of those abstractions are not
slowed down too.

It's exactly the problem we have with `await for` - it's supposed to be a
low-level primitive, but it's not efficient enough, so we write around it
instead of optimizing it.
Little inefficiencies at low level accumulate to larger inefficiencies in the
final applications, and the only way to be truly efficient, is to be it all the
way down.

Optimizing only when users meet a pain threshold (which is probably higher than
the annoyance threshold) will mean that you will always be just below the pain
threshold, and never really fast.
I do want platform libraries, and our corresponding packages, to be fast, so
people won't avoid using them.

> > And I do believe, very strongly, that defining simple operations in terms of
> > more complex ones is likely to give you unexpected dependencies and make
> > maintenance harder in the long run (but then, that's a prediction too).
> 
> I also disagree with this. We create abstractions to make computations simpler
> to express and reason about, and that benefit applies regardless of how
complex
> the implementation of the abstraction is.
> 
> If you still feel like the efficiency here is worth the maintenance and
> readability hit, I won't push further, but I want to lay out my reasoning.

I can understand that. 
I do think it is worth it here - also because the complexity of the two
operations is pretty low.

[nweiz, 2017-01-27 21:45:52]

lgtm

[Lasse Reichstein Nielsen, 2017-01-31 11:40:33]

Description was changed from

==========
Add `peek` and `lookAhead` to `StreamQueue`.

These allow users to look at events (similar to `next` and `take`)
without consuming them. For simple cases, they can be used instead of
`startTransaction` to decide what to do before doing it.
==========

to

==========
Add `peek` and `lookAhead` to `StreamQueue`.

These allow users to look at events (similar to `next` and `take`)
without consuming them. For simple cases, they can be used instead of
`startTransaction` to decide what to do before doing it.

R=nweiz@google.com

Committed:
https://github.com/dart-lang/async/commit/420d0840ae6f84f8be12aa39837b8499dbd...
==========

[Lasse Reichstein Nielsen, 2017-01-31 11:40:35]

Committed patchset #2 (id:20001) manually as
420d0840ae6f84f8be12aa39837b8499dbded0a6 (presubmit successful).