Make LineSplitter much faster for common case

Make LineSplitter much faster for common case

Avoiding the sync* function in the common case makes LineSplitter
50-100 times faster.

BUG=
R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/ab7eb4301349142d8c2e946aebf1bf77904342d7

[hausner, 2016-03-07 23:45:16]

Description was changed from

==========
Make LineSplitter much faster for common case

Avoiding the sync* function in the common case makes LineSplitter
50-100 times faster.


BUG=
==========

to

==========
Make LineSplitter much faster for common case

Avoiding the sync* function in the common case makes LineSplitter
50-100 times faster.

BUG=
==========

[hausner, 2016-03-07 23:45:16]

hausner@google.com changed reviewers:
+ floitsch@google.com

[kevmoo, 2016-03-08 00:51:25]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2016-03-08 00:51:26]

So sad the existing code is so much slower.

Do we know why? Is it the impl of `sync*`?

[Lasse Reichstein Nielsen, 2016-03-08 10:48:15]

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

[Lasse Reichstein Nielsen, 2016-03-08 10:48:16]

If the sync* code is so slow, then make it faster! Having a feature that sucks
so people are taught to avoid it is arguably worse than not having the feature
at all. Even if we make it fast, some people will keep avoiding it.
A 50-100x slowdown is reason enough to remove the feture, or make it a priority
to speed it up.

Also, the the split function is still slow, so maybe change that instead, and
keep the convert function as calling split. That way we won't duplicate code.
I have an old CL for that:
https://codereview.chromium.org/1302283002/diff/1/sdk/lib/convert/line_splitt...
I actually wanted to keep the sync* code because it's cleaner.

It's still creating an intermediate iterable, which this convert avoids, so you
can argue for having both, if the performance difference is measurable.

https://codereview.chromium.org/1775683002/diff/1/sdk/lib/convert/line_splitt...
File sdk/lib/convert/line_splitter.dart (right):

https://codereview.chromium.org/1775683002/diff/1/sdk/lib/convert/line_splitt...
sdk/lib/convert/line_splitter.dart:53: List lines = new List();
List<String> lines = <String>[];

https://codereview.chromium.org/1775683002/diff/1/sdk/lib/convert/line_splitt...
sdk/lib/convert/line_splitter.dart:71: lines.add(data.substring(sliceStart,
end));
remove "end", just:
  lines.add(data.substring(sliceStart));

[floitsch, 2016-03-08 12:36:36]

LGTM.

[hausner, 2016-03-08 18:35:08]

The _LineSplitter class in Lasse's change is another option. This change
duplicates a bit of code instead of introducing another class.

https://codereview.chromium.org/1775683002/diff/1/sdk/lib/convert/line_splitt...
File sdk/lib/convert/line_splitter.dart (right):

https://codereview.chromium.org/1775683002/diff/1/sdk/lib/convert/line_splitt...
sdk/lib/convert/line_splitter.dart:53: List lines = new List();
On 2016/03/08 10:48:16, Lasse Reichstein Nielsen wrote:
> List<String> lines = <String>[];

Done.

https://codereview.chromium.org/1775683002/diff/1/sdk/lib/convert/line_splitt...
sdk/lib/convert/line_splitter.dart:71: lines.add(data.substring(sliceStart,
end));
On 2016/03/08 10:48:16, Lasse Reichstein Nielsen wrote:
> remove "end", just:
>   lines.add(data.substring(sliceStart));

I like explicit parameters. Makes it clear(er) at the call site what's
happening.

[hausner, 2016-03-08 18:49:34]

Description was changed from

==========
Make LineSplitter much faster for common case

Avoiding the sync* function in the common case makes LineSplitter
50-100 times faster.

BUG=
==========

to

==========
Make LineSplitter much faster for common case

Avoiding the sync* function in the common case makes LineSplitter
50-100 times faster.

BUG=
R=floitsch@google.com

Committed:
https://github.com/dart-lang/sdk/commit/ab7eb4301349142d8c2e946aebf1bf77904342d7
==========

[hausner, 2016-03-08 18:49:36]

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