Let Uint16List.setRange detect a String.codeUnits object, and copy directly from the string.

Let Uint16List.setRange detect a String.codeUnits object, and copy directly from the string.

R=vegorov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43327

[Lasse Reichstein Nielsen, 2015-01-29 12:17:14]

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

[Lasse Reichstein Nielsen, 2015-01-29 12:17:15]

A simple benchmark that just does repeated:
  list.setRange(0, string.length, string.codeUnits);
  r = new String.fromCharCodes(list);
improves significantly.

Before:
Latin-1/Uint8 short: 123616.000 char/ms
Latin-1/Uint16 short: 113536.000 char/ms
Unicode/Uint16 short: 99552.000 char/ms
Latin-1/Uint8 long: 154671.984 char/ms
Latin-1/Uint16 long: 145117.764 char/ms
Unicode/Uint16 long: 160958.084 char/ms

After:
Latin-1/Uint8 short: 185376.000 char/ms
Latin-1/Uint16 short: 132512.000 char/ms
Unicode/Uint16 short: 125600.000 char/ms
Latin-1/Uint8 long: 290525.475 char/ms
Latin-1/Uint16 long: 164864.000 char/ms
Unicode/Uint16 long: 216871.129 char/ms

[Vyacheslav Egorov (Google), 2015-01-29 13:04:54]

lgtm

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/internal_pat...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/internal_pat...
runtime/lib/internal_patch.dart:16: static final int cid = ClassID.getID(new
CodeUnits(""));
Maybe implement ClassID.getClassId(Type) and use that instead?

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:1059: _setCodeUnits(iterable, start, end,
skipCount);
Maybe we should also have an optimization for Int8Array/Uint8Array and one byte
string in this CL?

It's also unclear to me why Int16Array is not covered by this optimization?

https://codereview.chromium.org/856053002/diff/40001/sdk/lib/internal/interna...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/856053002/diff/40001/sdk/lib/internal/interna...
sdk/lib/internal/internal.dart:54: String _string;
can this be final?

[Lasse Reichstein Nielsen, 2015-01-29 13:13:28]

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/internal_pat...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/internal_pat...
runtime/lib/internal_patch.dart:16: static final int cid = ClassID.getID(new
CodeUnits(""));
I think that's a great idea, but not something I think I'll be able to pull off.
Not for this CL for sure.

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/856053002/diff/40001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:1059: _setCodeUnits(iterable, start, end,
skipCount);
On 2015/01/29 13:04:54, Vyacheslav Egorov (Google) wrote:
> Maybe we should also have an optimization for Int8Array/Uint8Array and one
byte
> string in this CL?

I was planning to do that in a later CL.

> It's also unclear to me why Int16Array is not covered by this optimization?

Good point. Writing to an Int16Array and an Uint16Array is completely equivalent
(it's the same bits getting written in either case, it's only reading them back
that differs).

I'll add the setRange to Int16Array/View too.

https://codereview.chromium.org/856053002/diff/40001/sdk/lib/internal/interna...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/856053002/diff/40001/sdk/lib/internal/interna...
sdk/lib/internal/internal.dart:54: String _string;
Absolutely.

[srdjan, 2015-01-29 17:35:15]

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

[srdjan, 2015-01-29 17:35:16]

DBC

https://codereview.chromium.org/856053002/diff/60001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/856053002/diff/60001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:1061: if (ClassID.getID(iterable) != CodeUnits.cid)
{
Positive tests are more readable:
if (ClassID.getID(iterable) == CodeUnits.cid) {
} else {
}

https://codereview.chromium.org/856053002/diff/60001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:2675: if (ClassID.getID(iterable) != CodeUnits.cid)
{
ditto

https://codereview.chromium.org/856053002/diff/60001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:2733: if (ClassID.getID(iterable) != CodeUnits.cid)
{
ditto

https://codereview.chromium.org/856053002/diff/60001/sdk/lib/internal/interna...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/856053002/diff/60001/sdk/lib/internal/interna...
sdk/lib/internal/internal.dart:56: CodeUnits(this._string);
Can this be a const constructor?

[sra1, 2015-01-29 22:42:12]

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

[sra1, 2015-01-29 22:42:13]

Do we have a Golem benchmark that covers this?
I'm curious where it would be used.

[Lasse Reichstein Nielsen, 2015-01-30 09:42:54]

There are no current benchmarks that use this code.
It's meant to address requests like http://dartbug.com/16376 by being able to
recommend doing:
  unit16List.setRange(4, 4 + string.length, string.codeUnits);
to copy code units from a string to a typed data list.

By having this CL (+ uint8List support for one-byte strings, probably refactored
to be more orthogonal), we should be able to store and retrieve strings from
type-data lists by doing:
  typedData.setRange(a, b. string.codeUnits)
and 
  new String.fromCodeUnits(typedDat, a, b)
as efficiently as possible (well, at least if we eventually optimize the inner
loop to a memcpy).

https://codereview.chromium.org/856053002/diff/60001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/856053002/diff/60001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:1061: if (ClassID.getID(iterable) != CodeUnits.cid)
{
Done, in all four places.

https://codereview.chromium.org/856053002/diff/60001/sdk/lib/internal/interna...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/856053002/diff/60001/sdk/lib/internal/interna...
sdk/lib/internal/internal.dart:56: CodeUnits(this._string);
Sadly not. The UnmodifiableListBase super-class does not have a const
constructor.

I don't think it would buy much anyway - the constructor is only used by
String.codeUnits where it's used with "new".

[Lasse Reichstein Nielsen, 2015-01-30 09:46:35]

Committed patchset #5 (id:80001) manually as 43327 (presubmit successful).