[builtins] move String.prototype[@@iterator] to C++ builtin

[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=bmeurer@chromium.org, adamk@chromium.org
TBR=hpayer@chromium.org

Committed: https://crrev.com/5784773feb92d06e76d4eb0fb93ed9496b9df30b
Cr-Commit-Position: refs/heads/master@{#39598}

[caitp, 2016-09-15 17:09:16]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 17:09:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 17:29:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 17:29:12]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Benedikt Meurer, 2016-09-15 17:33:47]

Description was changed from

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:???
R=rmcilroy@chromium.org, hpayer@chromium.org, bmeurer@chromium.org,
littledan@chromium.org, adamk@chromium.org
==========

to

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=rmcilroy@chromium.org, hpayer@chromium.org, bmeurer@chromium.org,
littledan@chromium.org, adamk@chromium.org
==========

[Benedikt Meurer, 2016-09-15 17:34:33]

Wow, very impressive and amazingly fast! :-)
First round of comments.

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1416: iterator->shared()->set_language_mode(STRICT);
Why do you need to set the language mode on a C++ builtin?

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1417: iterator->map()->set_is_constructor(false);
This mutates the map inplace, which is not what you want. You have to use the
appropriate map instead.

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:3398: PrototypeIterator
iter(native_context()->generator_object_prototype_map());
Can we make this more robust? I.e. have the iterator prototype as field in the
native context or the bootstrapper?

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:3405: JSObject::ForceSetPrototype(string_iterator_prototype,
iterator_prototype);
Not sure about this ForceSetPrototype. We seem to just use
JSObject::SetPrototype elsewehere in this file.

https://codereview.chromium.org/2348493003/diff/1/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2348493003/diff/1/src/builtins/builtins.h#new...
src/builtins/builtins.h:550: CPP(StringCreateIterator)                          
                        \
For consistency, please rename this to StringPrototypeIterator.

https://codereview.chromium.org/2348493003/diff/1/src/builtins/builtins.h#new...
src/builtins/builtins.h:553: CPP(StringIteratorNext)                            
                        \
And this to StringIteratorPrototypeNext

https://codereview.chromium.org/2348493003/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2348493003/diff/1/src/factory.cc#newcode746
src/factory.cc:746: iterator->set_string(*String::Flatten(string));
I'm not sure that GCmole will like this.

https://codereview.chromium.org/2348493003/diff/1/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2348493003/diff/1/src/factory.h#newcode179
src/factory.h:179: Handle<JSStringIterator> NewJSStringIterator();
Why do we need this factory method?

https://codereview.chromium.org/2348493003/diff/1/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2348493003/diff/1/src/objects-debug.cc#newcod...
src/objects-debug.cc:792: CHECK(index()->IsSmi() ||
index()->IsUndefined(isolate));
The index should always be a Smi.

https://codereview.chromium.org/2348493003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2348493003/diff/1/src/objects.cc#newcode19496
src/objects.cc:19496: void JSIteratorResult::Initialize(Handle<JSIteratorResult>
object,
This helper method seems a bit overkill; I'd propose to just inline the two
lines and remove the helper.

[caitp, 2016-09-15 18:48:57]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 18:49:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2016-09-15 19:02:40]

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1416: iterator->shared()->set_language_mode(STRICT);
On 2016/09/15 17:34:33, Benedikt Meurer wrote:
> Why do you need to set the language mode on a C++ builtin?

I guess the other builtins just use set_native(true), so I've done that. A
better fix would be to add a version of these helpers which accept symbol
properties, but that seemed like more effort

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:3398: PrototypeIterator
iter(native_context()->generator_object_prototype_map());
On 2016/09/15 17:34:33, Benedikt Meurer wrote:
> Can we make this more robust? I.e. have the iterator prototype as field in the
> native context or the bootstrapper?

Ok.

I assume it wasn't done originally to avoid growing the context more.

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:3405: JSObject::ForceSetPrototype(string_iterator_prototype,
iterator_prototype);
On 2016/09/15 17:34:33, Benedikt Meurer wrote:
> Not sure about this ForceSetPrototype. We seem to just use
> JSObject::SetPrototype elsewehere in this file.

was copy/pasted from one of the Map or Set iterator setup code. I'm not sure
this is actually the right place for this block to live at all, it seems out of
place.

Will change it to SetPrototype and see if that breaks anything, anyway.
(actually, after uploading the patch, I may have forgotten to change it to
SetPrototype, but I'll wait for the tryjob to see if there are other failures
before changing it)

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:664: Handle<JSFunction> iterator_prototype_iterator =
SimpleCreateFunction(
The full %IteratorPrototype% is now set up here, so this allows deleting
src/js/iterator-prototype.js

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1434: JSObject::ForceSetPrototype(string_iterator_prototype,
iterator_prototype);
TODO: use JSObject::SetPrototype()

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1446: isolate,
factory->NewStringFromAsciiChecked("StringIterator"),
I assume this string is actually used for something, but it's not clear what.

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2511: Handle<JSObject> iterator_prototype(
This stuff is still used by collections iterators

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-i...
File src/builtins/builtins-iterator.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-iterator.cc:13:
assembler->Return(assembler->Parameter(0));
this is just here so that IteratorPrototype can be completely set up in the
bootstrapper, without needing extra JS stuff. since it's so little code, I don't
think it's that big of a deal.

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:650: Handle<String> value =
isolate->factory()->NewProperSubString(
I was expecting NewProperSubString to create SlicedStrings, but it looks like it
doesn't. There might be a smarter/faster way to do this instead.

https://codereview.chromium.org/2348493003/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/factory.cc#newcode610
src/factory.cc:610: DCHECK(begin > 0 || end <= str->length());
this seems to be necessary to use this for the last two code units in the
string, but I'm not positive.

https://codereview.chromium.org/2348493003/diff/20001/src/js/string-iterator.js
File src/js/string-iterator.js (left):

https://codereview.chromium.org/2348493003/diff/20001/src/js/string-iterator....
src/js/string-iterator.js:86: %FunctionSetInstanceClassName(StringIterator,
'String Iterator');
not doing this might cause layout test failures, but it seems unlikely --- will
re-add it if it's needed

[commit-bot: I haz the power, 2016-09-15 19:06:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 19:06:35]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[caitp, 2016-09-15 19:14:57]

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1434: JSObject::ForceSetPrototype(string_iterator_prototype,
iterator_prototype);
On 2016/09/15 19:02:39, caitp wrote:
> TODO: use JSObject::SetPrototype()

JSObject::SetPrototype actually doesn't seem like the better option, it looks
like ForceSetObject will copy and migrate the map as expected, without any of
the extra functionality that the API or JS needs. Most of the callers in
bootstrapper use ForceSetMap for this reason, I guess.

[caitp, 2016-09-15 20:09:35]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 20:09:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 20:52:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 20:53:00]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12816)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[Benedikt Meurer, 2016-09-16 03:16:37]

GCmole complains about the Handle return being immediately dereferenced.

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2348493003/diff/1/src/bootstrapper.cc#newcode...
src/bootstrapper.cc:1416: iterator->shared()->set_language_mode(STRICT);
Yes, we really need a version that can deal with names in general, but Yang
and/or Andreas expressed some concerns regarding the number of helpers...

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1434: JSObject::ForceSetPrototype(string_iterator_prototype,
iterator_prototype);
On 2016/09/15 19:14:57, caitp wrote:
> On 2016/09/15 19:02:39, caitp wrote:
> > TODO: use JSObject::SetPrototype()
> 
> JSObject::SetPrototype actually doesn't seem like the better option, it looks
> like ForceSetObject will copy and migrate the map as expected, without any of
> the extra functionality that the API or JS needs. Most of the callers in
> bootstrapper use ForceSetMap for this reason, I guess.

Acknowledged.

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-i...
File src/builtins/builtins-iterator.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-i...
src/builtins/builtins-iterator.cc:13:
assembler->Return(assembler->Parameter(0));
On 2016/09/15 19:02:39, caitp wrote:
> this is just here so that IteratorPrototype can be completely set up in the
> bootstrapper, without needing extra JS stuff. since it's so little code, I
don't
> think it's that big of a deal.

Acknowledged.

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:650: Handle<String> value =
isolate->factory()->NewProperSubString(
Sliced strings would be really a waste of time/space for single char strings.
You should just look up the single char string in the cache and/or create one;
the CodeStubAssembler (once we go to TurboFan) has a StringFromCharCode that
does that.

https://codereview.chromium.org/2348493003/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2348493003/diff/20001/src/factory.cc#newcode610
src/factory.cc:610: DCHECK(begin > 0 || end <= str->length());
No that'd definitely be a bug, you'd read out of bounds for the string.

https://codereview.chromium.org/2348493003/diff/20001/src/js/string-iterator.js
File src/js/string-iterator.js (left):

https://codereview.chromium.org/2348493003/diff/20001/src/js/string-iterator....
src/js/string-iterator.js:86: %FunctionSetInstanceClassName(StringIterator,
'String Iterator');
On 2016/09/15 19:02:39, caitp wrote:
> not doing this might cause layout test failures, but it seems unlikely ---
will
> re-add it if it's needed

Acknowledged.

[Benedikt Meurer, 2016-09-16 03:18:18]

franzih: FYI

[caitp, 2016-09-16 03:18:20]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 03:18:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 04:00:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 04:00:45]

Dry run: This issue passed the CQ dry run.

[caitp, 2016-09-16 17:23:34]

Okay, PTAL --- I think this is ready for a proper look, whenever folks have
time.

[Benedikt Meurer, 2016-09-19 04:05:18]

Really nice. Next round of comments (getting close).

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:635: if (*string !=
isolate->heap()->empty_string()) {
You don't need this check, as the empty string will fail the position < length
below. That's the idea behind using the empty string instead of undefined.

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:648: Handle<String> value =
isolate->factory()->NewProperSubString(
I'd prefer to not use NewProperSubString here, as we know anyways that we have
to create a two byte string with 2 characters for the surrogate pair. Maybe we
can have a simple NewSurrogatePairString function here?

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:657: string, position, position + 1);
Please use LookupSingleCharacterStringFromCode directly here.

https://codereview.chromium.org/2348493003/diff/80001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/factory.cc#newcode610
src/factory.cc:610: DCHECK(begin > 0 || end <= str->length());
This still looks wrong to me. Please undo this change (and maybe introduce a
helper NewSurrogatePairString for your use case). See comments on the builtins.

https://codereview.chromium.org/2348493003/diff/80001/src/factory.cc#newcode738
src/factory.cc:738: Handle<Map>
map(isolate()->native_context()->string_iterator_map());
Nit: Handle<Map> map(isolate()->native_context()->string_iterator_map(),
                     isolate());

https://codereview.chromium.org/2348493003/diff/80001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2348493003/diff/80001/src/factory.h#newcode179
src/factory.h:179: Handle<JSStringIterator> NewJSStringIterator();
Remove this dead declaration.

https://codereview.chromium.org/2348493003/diff/80001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:791: CHECK(index_object()->IsSmi());
Replace this with:

CHECK_LE(0, index());
CHECK_LE(index(), String::kMaxLength);

https://codereview.chromium.org/2348493003/diff/80001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2348493003/diff/80001/src/objects-inl.h#newco...
src/objects-inl.h:8251: ACCESSORS(JSStringIterator, index_object, Object,
kNextIndexOffset)
Remove this accessor.

https://codereview.chromium.org/2348493003/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2348493003/diff/80001/src/objects.h#newcode10271
src/objects.h:10271: DECL_ACCESSORS(index_object, Object)
You don't need this, the index() accessor does Smi checking internally. See
comment in objects-debug.cc

[caitp, 2016-09-19 16:03:05]

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:635: if (*string !=
isolate->heap()->empty_string()) {
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> You don't need this check, as the empty string will fail the position < length
> below. That's the idea behind using the empty string instead of undefined.

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:648: Handle<String> value =
isolate->factory()->NewProperSubString(
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> I'd prefer to not use NewProperSubString here, as we know anyways that we have
> to create a two byte string with 2 characters for the surrogate pair. Maybe we
> can have a simple NewSurrogatePairString function here?

Done --- Should the new factory method try to lookup a match in the interned
string table? I thought it might not be worth it to spend time doing the hash
lookup for (probably) short lived small strings.

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:657: string, position, position + 1);
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> Please use LookupSingleCharacterStringFromCode directly here.

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/factory.cc#newcode610
src/factory.cc:610: DCHECK(begin > 0 || end <= str->length());
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> This still looks wrong to me. Please undo this change (and maybe introduce a
> helper NewSurrogatePairString for your use case). See comments on the
builtins.

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/factory.cc#newcode738
src/factory.cc:738: Handle<Map>
map(isolate()->native_context()->string_iterator_map());
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> Nit: Handle<Map> map(isolate()->native_context()->string_iterator_map(),
>                      isolate());

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2348493003/diff/80001/src/factory.h#newcode179
src/factory.h:179: Handle<JSStringIterator> NewJSStringIterator();
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> Remove this dead declaration.

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:791: CHECK(index_object()->IsSmi());
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> Replace this with:
> 
> CHECK_LE(0, index());
> CHECK_LE(index(), String::kMaxLength);

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2348493003/diff/80001/src/objects-inl.h#newco...
src/objects-inl.h:8251: ACCESSORS(JSStringIterator, index_object, Object,
kNextIndexOffset)
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> Remove this accessor.

Done.

https://codereview.chromium.org/2348493003/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2348493003/diff/80001/src/objects.h#newcode10271
src/objects.h:10271: DECL_ACCESSORS(index_object, Object)
On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> You don't need this, the index() accessor does Smi checking internally. See
> comment in objects-debug.cc

Done.

[caitp, 2016-09-19 16:03:20]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 16:03:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 16:31:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 16:31:20]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-19 17:44:50]

Overall looks great to me (will defer to Benedikt's review); just one nit.

https://codereview.chromium.org/2348493003/diff/90001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2348493003/diff/90001/src/objects.h#newcode61
src/objects.h:61: //         - JSMapIterator
Please add JSStringIterator here

[Benedikt Meurer, 2016-09-20 04:57:53]

Awesome, thanks. LGTM!

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:648: Handle<String> value =
isolate->factory()->NewProperSubString(
On 2016/09/19 16:03:05, caitp wrote:
> On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> > I'd prefer to not use NewProperSubString here, as we know anyways that we
have
> > to create a two byte string with 2 characters for the surrogate pair. Maybe
we
> > can have a simple NewSurrogatePairString function here?
> 
> Done --- Should the new factory method try to lookup a match in the interned
> string table? I thought it might not be worth it to spend time doing the hash
> lookup for (probably) short lived small strings.

Nope, that doesn't make sense. Especially since the stub/inlined code should
just create a two char string and be done with it.

[caitp, 2016-09-20 13:28:15]

Alright, thanks. If there are no other comments, will update with Adam's nit
addressed and cq

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2348493003/diff/80001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:648: Handle<String> value =
isolate->factory()->NewProperSubString(
On 2016/09/20 04:57:53, Benedikt Meurer wrote:
> On 2016/09/19 16:03:05, caitp wrote:
> > On 2016/09/19 04:05:18, Benedikt Meurer wrote:
> > > I'd prefer to not use NewProperSubString here, as we know anyways that we
> have
> > > to create a two byte string with 2 characters for the surrogate pair.
Maybe
> we
> > > can have a simple NewSurrogatePairString function here?
> > 
> > Done --- Should the new factory method try to lookup a match in the interned
> > string table? I thought it might not be worth it to spend time doing the
hash
> > lookup for (probably) short lived small strings.
> 
> Nope, that doesn't make sense. Especially since the stub/inlined code should
> just create a two char string and be done with it.

Acknowledged.

https://codereview.chromium.org/2348493003/diff/90001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2348493003/diff/90001/src/objects.h#newcode61
src/objects.h:61: //         - JSMapIterator
On 2016/09/19 17:44:50, adamk wrote:
> Please add JSStringIterator here

Done.

[caitp, 2016-09-20 13:41:01]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 13:41:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 14:23:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 14:23:04]

Dry run: This issue passed the CQ dry run.

[caitp, 2016-09-20 15:39:23]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-09-20 15:39:25]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2348493003/#ps110001
(title: "V7 (Adam's nit)")

[commit-bot: I haz the power, 2016-09-20 15:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 15:50:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 15:50:40]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24512)

[caitp, 2016-09-20 16:40:16]

guess  I'm blocked on a src/heap LGTM

[caitp, 2016-09-21 13:23:26]

Description was changed from

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=rmcilroy@chromium.org, hpayer@chromium.org, bmeurer@chromium.org,
littledan@chromium.org, adamk@chromium.org
==========

to

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=bmeurer@chromium.org, adamk@chromium.org
TBR=hpayer@chromium.org
==========

[caitp, 2016-09-21 13:23:37]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2016-09-21 13:23:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 13:27:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 13:27:14]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24611)

[caitp, 2016-09-21 13:46:15]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-09-21 13:46:16]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2348493003/#ps130001
(title: "V8 (rebase + fix bytecode expectations)")

[commit-bot: I haz the power, 2016-09-21 13:46:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 14:17:48]

Description was changed from

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=bmeurer@chromium.org, adamk@chromium.org
TBR=hpayer@chromium.org
==========

to

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=bmeurer@chromium.org, adamk@chromium.org
TBR=hpayer@chromium.org
==========

[commit-bot: I haz the power, 2016-09-21 14:17:50]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-09-21 14:18:08]

Description was changed from

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=bmeurer@chromium.org, adamk@chromium.org
TBR=hpayer@chromium.org
==========

to

==========
[builtins] move String.prototype[@@iterator] to C++ builtin

BUG=v8:5388
R=bmeurer@chromium.org, adamk@chromium.org
TBR=hpayer@chromium.org

Committed: https://crrev.com/5784773feb92d06e76d4eb0fb93ed9496b9df30b
Cr-Commit-Position: refs/heads/master@{#39598}
==========

[commit-bot: I haz the power, 2016-09-21 14:18:10]

Patchset 8 (id:??) landed as
https://crrev.com/5784773feb92d06e76d4eb0fb93ed9496b9df30b
Cr-Commit-Position: refs/heads/master@{#39598}