Create string efficiently from Uint16List/View.

Create string efficiently from Uint16List/View.

R=asiva@google.com, fschneider@google.com

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

[Lasse Reichstein Nielsen, 2015-01-20 10:10:46]

lrn@google.com changed reviewers:
+ asiva@google.com, fschneider@google.com

[siva, 2015-01-20 23:50:27]

The Cl description says "Create string efficiently from Uint16List/View" but
there is no benchmark to substantiate that or a result stating how much an
existing benchmark improved with this change.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:279: ASSERT(length >= 0);
Not sure why this assertion is needed here considering we have a strong check
above for length which results in a throw.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:293: }
Maybe you could abstract this code out into a static helper function
static void CheckTypeAndLength(bool type_check, bool length_check) {
  if (!type_ok) {
    const Array& args = Array::Handle(Array::New(1));
    args.SetAt(0, list);
    Exceptions::ThrowByType(Exceptions::kArgument, args);
  }
  if (!length_ok) {
    const Array& args = Array::Handle(Array::New(1));
    args.SetAt(0, end_obj);
    Exceptions::ThrowByType(Exceptions::kArgument, args);
  }
}

and use it as
if (list.IsTypedData()) {
  const TypedData& array = TypedData::Cast(list);
  CheckTypeAndLength((array.ElementType() == kUint16ArrayElement),
                     (end <= array.LengthInBytes());
  return TwoByteString::New(array, start * sizeof(uint16_t), length, space);
} else if (....) {
  ....
  CheckTypeAndLength((array.ElementType() == kUint16ArrayElement),
                     (end <= array.LengthInBytes());
  ....
}

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:338: String& string =
String::Handle(TwoByteString::New(length, space));
Since 'isolate' is available to you in the native call use it when creating
handles, e.g:
const String& string = String::Handle(isolate, TwoByteString::New(length,
space));

Also missing 'const'

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:352: String& string =
String::Handle(TwoByteString::New(length, space));
Ditto.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:63: * [codePoints].
The parameter name is not documented correctly, the function has charCodes and
here it is documented as [codePoints].

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:66: * It's `null` if unknown.
Since limit is documented maybe we should also document start and end.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:94: }
the (limit < 0) should be pulled outside the if (limit == null) block to protect
against a negative limit value being passed in to this function.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:98: }
Since you are checking for 'limit' not being < 0 above the optimized path could
also potentially work if charCodes is a Int16List?

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:102: return _createFromCodePoints(charCodes,
start, end);
The same TODO applies to this call of createFromCodePoints too as limit has
already been computed and can be passed in thus avoiding the need to scan again.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:147: } else {
why not move the end < start check here to avoid having to test if end is null
multiple times.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:161: if (bits < 0) throw new
ArgumentError(charCodes);
This style of not using a '{' ... '}' seems inconsistent with other parts of
code in this file.

https://codereview.chromium.org/864463002/diff/1/tests/corelib/string_fromcha...
File tests/corelib/string_fromcharcodes_test.dart (right):

https://codereview.chromium.org/864463002/diff/1/tests/corelib/string_fromcha...
tests/corelib/string_fromcharcodes_test.dart:243: }
Does this also test the ExternalTypedData and Uint16ListView cases?

[Lasse Reichstein Nielsen, 2015-01-22 07:16:33]

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:279: ASSERT(length >= 0);
ACK, I added the explicit check later, and forgot to remove the assert.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:293: }
The function also needs the list and end_obj values, so calls get a little
bigger.
I'll start out by using Exception::ThrowArgumentError instead of these larger
blocks, which should make the code more manageable.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:338: String& string =
String::Handle(TwoByteString::New(length, space));
On 2015/01/20 23:50:26, siva wrote:
> Since 'isolate' is available to you in the native call use it when creating
> handles, e.g:
> const String& string = String::Handle(isolate, TwoByteString::New(length,
> space));
> 
> Also missing 'const'

Done.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string.cc#newcod...
runtime/lib/string.cc:352: String& string =
String::Handle(TwoByteString::New(length, space));
On 2015/01/20 23:50:26, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:63: * [codePoints].
On 2015/01/20 23:50:26, siva wrote:
> The parameter name is not documented correctly, the function has charCodes and
> here it is documented as [codePoints].

Done.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:66: * It's `null` if unknown.
On 2015/01/20 23:50:26, siva wrote:
> Since limit is documented maybe we should also document start and end.

Done.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:94: }
While that shouldn't happen (we check that elsewhere), I guess it's better to be
safe.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:98: }
True. I'll consider if it's worth it for another CL.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:102: return _createFromCodePoints(charCodes,
start, end);
If we get here, then limit > 0xffff, so there is at least one code point that
need to be converted to a surrogate pair. That means that the next code will
need to scan to count these to find the string length anyway (and see if any
value is not a valid code point, which the limit doesn't show), so the saving
isn't expected to be great - but still, it might be useful, so adding a TODO.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:147: } else {
On 2015/01/20 23:50:26, siva wrote:
> why not move the end < start check here to avoid having to test if end is null
> multiple times.

Done.

https://codereview.chromium.org/864463002/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:161: if (bits < 0) throw new
ArgumentError(charCodes);
I use it when it fits on a line, but not consistently. Probably better to be
consistent (and not hide control flow late on a line).

https://codereview.chromium.org/864463002/diff/1/tests/corelib/string_fromcha...
File tests/corelib/string_fromcharcodes_test.dart (right):

https://codereview.chromium.org/864463002/diff/1/tests/corelib/string_fromcha...
tests/corelib/string_fromcharcodes_test.dart:243: }
No. I'll add Uint16ListViev. Is there a way to create an ExtrenalTypedData from
Dart code?

[siva, 2015-01-23 01:06:34]

lgtm

https://codereview.chromium.org/864463002/diff/1/tests/corelib/string_fromcha...
File tests/corelib/string_fromcharcodes_test.dart (right):

https://codereview.chromium.org/864463002/diff/1/tests/corelib/string_fromcha...
tests/corelib/string_fromcharcodes_test.dart:243: }
On 2015/01/22 07:16:33, Lasse Reichstein Nielsen wrote:
> No. I'll add Uint16ListViev. Is there a way to create an ExtrenalTypedData
from
> Dart code? 

You would have to create it in native code and pass it back to dart, there are
some examples in dart_api_impl_test.cc (e.g: ExternalByteDataAccess test)

[Lasse Reichstein Nielsen, 2015-01-28 11:38:12]

Added test for external typed lists and views on same. 
Florian, PTAL.

[Florian Schneider, 2015-01-28 12:00:06]

lgtm

[Lasse Reichstein Nielsen, 2015-01-28 12:44:54]

Committed patchset #3 (id:40001) manually as 43235 (presubmit successful).