ES6: Add support for Map.prototype.forEach and Set.prototype.forEach

ES6: Add support for Map/Set forEach

This implements MapIterator and SetIterator which matches
the same constructs in the ES6 spec. However, these 2
iterators are not exposed to user code yet. They are only
used internally to implement Map.prototype.forEach and
Set.prototype.forEach.

Each iterator has a reference to the OrderedHashTable where
it directly accesses the hash table's entries.

The OrderedHashTable has a reference to the newest iterator
and each iterator has a reference to the next and previous
iterator, effectively creating a double linked list.

When the OrderedHashTable is mutated (or replaced) all the
iterators are updated.

When the iterator iterates passed the end of the data table
it closes itself. Closed iterators no longer have a
reference to the OrderedHashTable and they are removed from
the double linked list. In the case of Map/Set forEach, we
manually call Close on the iterator in case an exception was
thrown so that the iterator never reached the end.

At this point the OrderedHashTable keeps all the non finished
iterators alive but since the only thing we currently expose
is forEach there are no unfinished iterators outside a forEach
call. Once we expose the iterators to user code we will need
to make the references from the OrderedHashTable to the
iterators weak and have some mechanism to close an iterator
when it is garbage collected.

BUG=1793, 2323
LOG=Y
R=adamk@chromium.org, mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20781

[arv (Not doing code reviews), 2014-04-11 23:21:29]

Hi Adam, this is now ready for an initial review.

TODOs
 [ ] Extensive CL description
 [ ] Explain no MapIterator/SetIterator exposed to JS
 [ ] How GC will need to be done once we expose iterators to user code

[adamk, 2014-04-14 19:15:42]

Mostly looking good. I think there might be an edge case with the final element
in an iteration. And you've got some more handlification work. Other than that,
I like the look of it!

https://codereview.chromium.org/236143002/diff/1/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode130
src/collection.js:130: if (typeof f !== 'function') {
I think you want IS_SPEC_FUNCTION(f) here, which is a macro expanding to:

%_ClassOf(arg) === 'Function'

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode138
src/collection.js:138: %_CallFunction(receiver, entry.value, entry.value, this,
f);
Can you add a test that throws in its forEach function (both for set and maps)?

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode243
src/collection.js:243: throw MakeTypeError('incompatible_method_receiver',
Can you add tests for this and the other error cases?

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode247
src/collection.js:247: if (typeof f !== 'function') {
IS_SPEC_FUNCTION

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

https://codereview.chromium.org/236143002/diff/1/src/factory.cc#newcode2005
src/factory.cc:2005: InternalizeOneByteString(STATIC_ASCII_VECTOR("value")),
"value" and "done" are already available in the heap, and you can get a handle
for them from the factory by calling value_string() and done_string() methods on
this.

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

https://codereview.chromium.org/236143002/diff/1/src/objects-debug.cc#newcode710
src/objects-debug.cc:710: CHECK(table()->IsOrderedHashTable() ||
table()->IsUndefined());
Seems like it would be nice to add some validation to the table itself...maybe
put a TODO here?

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

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15772
src/objects.cc:15772: table->set_iterators(table->GetHeap()->undefined_value());
Nit: s/table->GetHeap()/isolate->heap()/ (as above)

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15804
src/objects.cc:15804: int new_capacity = 4;
This magic number probably means kMinCapacity in Allocate() should be moved up
to be a constant on the class.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15810
src/objects.cc:15810: for (Object* object = table->iterators();
Putting a DisallowHeapAllocation above this loop would be a good idea, since you
use a raw Object* to handle the iteration.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15854
src/objects.cc:15854: for (Object* object = table->iterators();
Same as above, DisallowHeapAllocation

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15903
src/objects.cc:15903: for (Object* object = iterators();
And one more DisallowHeapAllocation.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15916
src/objects.cc:15916: new_iterator->set_table(this);
You triggered an allocation above, but are referring to a bare |this| here,
which is unsafe. CreateIterator has to be a static, and take the table it's to
operate on as a a handlified argument.

Would be nice to make this a lint error in V8...

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15926
src/objects.cc:15926:
new_iterator->set_next_iterator(GetHeap()->undefined_value());
Nit: whitespace off here (extra space)

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16046
src/objects.cc:16046: table->NumberOfElements() +
table->NumberOfDeletedElements();
I see this a lot, maybe OrderedHashTable should expose an accessor that returns
the result of this expression.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value, false);
Method should be static since it does allocation.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value, false);
I notice you pass false here unconditionally, even if the table is already
Closed() at this point. Is that per spec, or is it up to the implementation?

Also makes me wonder, what if the forEach function adds an entry to the table
during its final callback? It looks like that might cause the forEach to
terminate without hitting that last entry, and yet I think the spec would say
that the newly-added entry should also be iterated over...

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16112
src/objects.cc:16112: ASSERT(kind == kKindValues || kind == kKindEntries);
I suppose it's arbitrary, but I was surprised at that kKindValues here refers to
the set's keys....

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16121
src/objects.cc:16121: Handle<FixedArray> array = factory->NewFixedArray(2);
This method should be static since it does allocation.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16160
src/objects.cc:16160: return factory->NewJSArrayWithElements(array);
Same as above, should be static.

https://codereview.chromium.org/236143002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4279
src/objects.h:4279: //   [3]: live iterators (double linked list)
Nit: s/double linked/doubly-linked/

https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4323
src/objects.h:4323: Handle<Iterator> CreateIterator(int kind);
This should at least have a comment referring to the enum it's supposed to
conform to.

Also, needs to be static.

https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4326
src/objects.h:4326: return get(kIteratorsIndex);
Nit: if you can fit these on one line, please do so.

https://codereview.chromium.org/236143002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/236143002/diff/1/src/runtime.cc#newcode1580
src/runtime.cc:1580: || kind == JSSetIterator::kKindEntries);
Only one of these is used at the moment, I assume these are for when we expose
the iterator creation to JS? It would be nice to at least cover these with
tests, especially since I the kKindEntries versions are currently (as written)
buggy due to not being properly handlified.

[arv (Not doing code reviews), 2014-04-14 19:51:48]

I'll look into changing things to use static next

https://codereview.chromium.org/236143002/diff/1/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode130
src/collection.js:130: if (typeof f !== 'function') {
On 2014/04/14 19:15:42, adamk wrote:
> I think you want IS_SPEC_FUNCTION(f) here, which is a macro expanding to:
> 
> %_ClassOf(arg) === 'Function'

IS_SPEC_FUNCTION is not correct any more. It tests that the instance is a true
function. What we really care about is Callable. Most of the IS_SPEC function
have been removed in the spec and now the spec checks for Callable instead.

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode138
src/collection.js:138: %_CallFunction(receiver, entry.value, entry.value, this,
f);
On 2014/04/14 19:15:42, adamk wrote:
> Can you add a test that throws in its forEach function (both for set and
maps)?

Done.

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode243
src/collection.js:243: throw MakeTypeError('incompatible_method_receiver',
On 2014/04/14 19:15:42, adamk wrote:
> Can you add tests for this and the other error cases?

Done.

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

https://codereview.chromium.org/236143002/diff/1/src/factory.cc#newcode2005
src/factory.cc:2005: InternalizeOneByteString(STATIC_ASCII_VECTOR("value")),
On 2014/04/14 19:15:42, adamk wrote:
> "value" and "done" are already available in the heap, and you can get a handle
> for them from the factory by calling value_string() and done_string() methods
on
> this.

Done.

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

https://codereview.chromium.org/236143002/diff/1/src/objects-debug.cc#newcode710
src/objects-debug.cc:710: CHECK(table()->IsOrderedHashTable() ||
table()->IsUndefined());
On 2014/04/14 19:15:42, adamk wrote:
> Seems like it would be nice to add some validation to the table itself...maybe
> put a TODO here?

Done.

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

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15772
src/objects.cc:15772: table->set_iterators(table->GetHeap()->undefined_value());
On 2014/04/14 19:15:42, adamk wrote:
> Nit: s/table->GetHeap()/isolate->heap()/ (as above)

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15804
src/objects.cc:15804: int new_capacity = 4;
On 2014/04/14 19:15:42, adamk wrote:
> This magic number probably means kMinCapacity in Allocate() should be moved up
> to be a constant on the class.

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15810
src/objects.cc:15810: for (Object* object = table->iterators();
On 2014/04/14 19:15:42, adamk wrote:
> Putting a DisallowHeapAllocation above this loop would be a good idea, since
you
> use a raw Object* to handle the iteration.

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15854
src/objects.cc:15854: for (Object* object = table->iterators();
On 2014/04/14 19:15:42, adamk wrote:
> Same as above, DisallowHeapAllocation

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15903
src/objects.cc:15903: for (Object* object = iterators();
On 2014/04/14 19:15:42, adamk wrote:
> And one more DisallowHeapAllocation.

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15926
src/objects.cc:15926:
new_iterator->set_next_iterator(GetHeap()->undefined_value());
On 2014/04/14 19:15:42, adamk wrote:
> Nit: whitespace off here (extra space)

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16046
src/objects.cc:16046: table->NumberOfElements() +
table->NumberOfDeletedElements();
On 2014/04/14 19:15:42, adamk wrote:
> I see this a lot, maybe OrderedHashTable should expose an accessor that
returns
> the result of this expression.

Any suggestion for a name?

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value, false);
On 2014/04/14 19:15:42, adamk wrote:
> I notice you pass false here unconditionally, even if the table is already
> Closed() at this point. Is that per spec, or is it up to the implementation?
> 
> Also makes me wonder, what if the forEach function adds an entry to the table
> during its final callback? It looks like that might cause the forEach to
> terminate without hitting that last entry, and yet I think the spec would say
> that the newly-added entry should also be iterated over...

I think you are right. I'll review the spec and add tests.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16112
src/objects.cc:16112: ASSERT(kind == kKindValues || kind == kKindEntries);
On 2014/04/14 19:15:42, adamk wrote:
> I suppose it's arbitrary, but I was surprised at that kKindValues here refers
to
> the set's keys....

This follows from the spec. Set.prototype has values but not keys. We could
internally only use keys but either way it is pretty arbitrary.

I added a comment.

https://codereview.chromium.org/236143002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4279
src/objects.h:4279: //   [3]: live iterators (double linked list)
On 2014/04/14 19:15:42, adamk wrote:
> Nit: s/double linked/doubly-linked/

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4323
src/objects.h:4323: Handle<Iterator> CreateIterator(int kind);
On 2014/04/14 19:15:42, adamk wrote:
> This should at least have a comment referring to the enum it's supposed to
> conform to.
> 
> Also, needs to be static.

This is an instance method. The iterator depends on the OrderedHashTable.

I added comment clarifying this. Maybe we should provide some other API to make
this more explicit?

https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4326
src/objects.h:4326: return get(kIteratorsIndex);
On 2014/04/14 19:15:42, adamk wrote:
> Nit: if you can fit these on one line, please do so.

Done.

https://codereview.chromium.org/236143002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/236143002/diff/1/src/runtime.cc#newcode1580
src/runtime.cc:1580: || kind == JSSetIterator::kKindEntries);
On 2014/04/14 19:15:42, adamk wrote:
> Only one of these is used at the moment, I assume these are for when we expose
> the iterator creation to JS? It would be nice to at least cover these with
> tests, especially since I the kKindEntries versions are currently (as written)
> buggy due to not being properly handlified.

Entries is being tested for Map. I can expand the cctest?

[arv (Not doing code reviews), 2014-04-14 20:30:10]

Fix issue with closing the iterator too early

[arv (Not doing code reviews), 2014-04-14 20:59:05]

static-ify

[adamk, 2014-04-14 21:25:53]

Needs moar Handles.

Basically, in a function that does allocation, it's a Bad Idea to not use
Handles, because any of your raw Object* stored on the stack might get
relocated.

That's why I asked you add DisallowHeapAllocation to the places where you loop
over the linked list. Another option in those callsites is just to use a Handle
to store your pointer into the list (though that's a little slower).

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

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16046
src/objects.cc:16046: table->NumberOfElements() +
table->NumberOfDeletedElements();
On 2014/04/14 19:51:49, arv wrote:
> On 2014/04/14 19:15:42, adamk wrote:
> > I see this a lot, maybe OrderedHashTable should expose an accessor that
> returns
> > the result of this expression.
> 
> Any suggestion for a name?

FilledCapacity()? UsedCapacity()?

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode15918
src/objects.cc:15918: Object* undefined = table->GetHeap()->undefined_value();
Please use a Handle here (available from isolate->factory()->undefined_value()).

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode15926
src/objects.cc:15926: Object* old_iterator = table->iterators();
Style-wise I'd also handl-ify this one (in case more code gets added below that
happens to cause allocations).

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16076
src/objects.cc:16076: Object* object = iterator->table();
Need to use a Handle here.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16115
src/objects.cc:16115: OrderedHashSet* table =
OrderedHashSet::cast(iterator->table());
Needs to be a Handle too.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16145
src/objects.cc:16145: OrderedHashMap* table =
OrderedHashMap::cast(iterator->table());
And this one too.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16156
src/objects.cc:16156: Object* value = table->get(entry_index + 1);
These both need to be Handles.

[adamk, 2014-04-14 21:33:01]

https://codereview.chromium.org/236143002/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/236143002/diff/60001/src/factory.cc#newcode2003
src/factory.cc:2003: JSObject::SetProperty(result, value_string(), value, NONE,
SLOPPY).Assert();
I think you can skip all the JSObject machinery here and set these properties
directly into the object. I believe the syntax is:

result->InObjectPropertyAtPut(
    JSGeneratorObject::kResultValuePropertyIndex, value);
result->InObjectPropertyAtPut(
    JSGeneratorObject::kResultDonePropertyIndex, ToBoolean(done));

Seems like we might want to move those constants out of JSGeneratorObject and
into another class...not sure what the v8 folks would want.

[arv (Not doing code reviews), 2014-04-14 21:41:45]

Store map in native context and use InObjectPropertyAtPut

[arv (Not doing code reviews), 2014-04-14 21:41:55]

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

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value, false);
On 2014/04/14 19:51:49, arv wrote:
> On 2014/04/14 19:15:42, adamk wrote:
> > I notice you pass false here unconditionally, even if the table is already
> > Closed() at this point. Is that per spec, or is it up to the implementation?
> > 
> > Also makes me wonder, what if the forEach function adds an entry to the
table
> > during its final callback? It looks like that might cause the forEach to
> > terminate without hitting that last entry, and yet I think the spec would
say
> > that the newly-added entry should also be iterated over...
> 
> I think you are right. I'll review the spec and add tests.

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16121
src/objects.cc:16121: Handle<FixedArray> array = factory->NewFixedArray(2);
On 2014/04/14 19:15:42, adamk wrote:
> This method should be static since it does allocation.

Done.

https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16160
src/objects.cc:16160: return factory->NewJSArrayWithElements(array);
On 2014/04/14 19:15:42, adamk wrote:
> Same as above, should be static.

Done.

https://codereview.chromium.org/236143002/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/236143002/diff/60001/src/factory.cc#newcode2003
src/factory.cc:2003: JSObject::SetProperty(result, value_string(), value, NONE,
SLOPPY).Assert();
On 2014/04/14 21:33:01, adamk wrote:
> I think you can skip all the JSObject machinery here and set these properties
> directly into the object. I believe the syntax is:
> 
> result->InObjectPropertyAtPut(
>     JSGeneratorObject::kResultValuePropertyIndex, value);
> result->InObjectPropertyAtPut(
>     JSGeneratorObject::kResultDonePropertyIndex, ToBoolean(done));
> 
> Seems like we might want to move those constants out of JSGeneratorObject and
> into another class...not sure what the v8 folks would want.

Done.

[arv (Not doing code reviews), 2014-04-14 22:13:04]

Handle-ify and UsedCapacity

[arv (Not doing code reviews), 2014-04-14 22:13:26]

PTAL

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode15918
src/objects.cc:15918: Object* undefined = table->GetHeap()->undefined_value();
On 2014/04/14 21:25:54, adamk wrote:
> Please use a Handle here (available from
isolate->factory()->undefined_value()).

Done.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode15926
src/objects.cc:15926: Object* old_iterator = table->iterators();
On 2014/04/14 21:25:54, adamk wrote:
> Style-wise I'd also handl-ify this one (in case more code gets added below
that
> happens to cause allocations).

Done.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16076
src/objects.cc:16076: Object* object = iterator->table();
On 2014/04/14 21:25:54, adamk wrote:
> Need to use a Handle here.

Done.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16115
src/objects.cc:16115: OrderedHashSet* table =
OrderedHashSet::cast(iterator->table());
On 2014/04/14 21:25:54, adamk wrote:
> Needs to be a Handle too.

Done.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16145
src/objects.cc:16145: OrderedHashMap* table =
OrderedHashMap::cast(iterator->table());
On 2014/04/14 21:25:54, adamk wrote:
> And this one too.

Done.

https://codereview.chromium.org/236143002/diff/60001/src/objects.cc#newcode16156
src/objects.cc:16156: Object* value = table->get(entry_index + 1);
On 2014/04/14 21:25:54, adamk wrote:
> These both need to be Handles.

Done.

[arv (Not doing code reviews), 2014-04-14 22:39:04]

Michael, Andreas,

I've been working with Adam on this over the last week. I think it is ready for
review now.

Thanks.

[adamk, 2014-04-14 22:46:38]

https://codereview.chromium.org/236143002/diff/90001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/236143002/diff/90001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1295: if (FLAG_harmony_collections) {
Can you move those map constructions up to here?

https://codereview.chromium.org/236143002/diff/90001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1400: Handle<Map> map = isolate()->factory()->NewMap(
Please move these up to their associated collections.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16023
src/objects.cc:16023: Handle<Object> undefined =
isolate->factory()->undefined_value();
Why did these get handlified? I don't see any allocation. Can this function just
get a DisallowHeapAllocation at the top?

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16056
src/objects.cc:16056: Handle<Object> object(this->table(), isolate);
Ditto down here, why all the handles?

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16107
src/objects.cc:16107: Handle<Map>
map(isolate->context()->native_context()->set_iterator_map());
You should be able to just say

isolate->set_iterator_map()

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16122
src/objects.cc:16122: Handle<Object> object(iterator->table(), isolate);
I don't see this used anywhere, you should be able to collapse it into the line
below.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16139
src/objects.cc:16139: Handle<Map>
map(isolate->context()->native_context()->map_iterator_map());
isolate->map_iterator_map()

[arv (Not doing code reviews), 2014-04-15 00:23:39]

less handless

[arv (Not doing code reviews), 2014-04-15 00:29:16]

https://codereview.chromium.org/236143002/diff/90001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/236143002/diff/90001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1295: if (FLAG_harmony_collections) {
On 2014/04/14 22:46:38, adamk wrote:
> Can you move those map constructions up to here?

Done.

https://codereview.chromium.org/236143002/diff/90001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1400: Handle<Map> map = isolate()->factory()->NewMap(
On 2014/04/14 22:46:38, adamk wrote:
> Please move these up to their associated collections.

Done.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16023
src/objects.cc:16023: Handle<Object> undefined =
isolate->factory()->undefined_value();
On 2014/04/14 22:46:38, adamk wrote:
> Why did these get handlified? I don't see any allocation. Can this function
just
> get a DisallowHeapAllocation at the top?

Done.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16056
src/objects.cc:16056: Handle<Object> object(this->table(), isolate);
On 2014/04/14 22:46:38, adamk wrote:
> Ditto down here, why all the handles?

Done.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16107
src/objects.cc:16107: Handle<Map>
map(isolate->context()->native_context()->set_iterator_map());
On 2014/04/14 22:46:38, adamk wrote:
> You should be able to just say
> 
> isolate->set_iterator_map()

Done.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16122
src/objects.cc:16122: Handle<Object> object(iterator->table(), isolate);
On 2014/04/14 22:46:38, adamk wrote:
> I don't see this used anywhere, you should be able to collapse it into the
line
> below.

It is used on the next line.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16139
src/objects.cc:16139: Handle<Map>
map(isolate->context()->native_context()->map_iterator_map());
On 2014/04/14 22:46:38, adamk wrote:
> isolate->map_iterator_map()

Done.

[adamk, 2014-04-15 01:12:41]

lgtm from my end, but of course I'm no v8 OWNER; looking forward to what Michael
and/or Andreas have to say.

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/236143002/diff/90001/src/objects.cc#newcode16122
src/objects.cc:16122: Handle<Object> object(iterator->table(), isolate);
On 2014/04/15 00:29:17, arv wrote:
> On 2014/04/14 22:46:38, adamk wrote:
> > I don't see this used anywhere, you should be able to collapse it into the
> line
> > below.
> 
> It is used on the next line.

Sorry, I mis-spoke, I meant to say the next line could instead read:

Handle<OrderedHashSet> table(OrderedHashSet::cast(iterator->table()), isolate);

[arv (Not doing code reviews), 2014-04-15 02:44:19]

Refactor to pass table into Create and remove CreateIterator

[Michael Starzinger, 2014-04-15 10:47:23]

One high-level comment: Is there a particular reason why SetIterator and
MapIterator are JSObjects (with mutable properties and elements)? Will they ever
be exposed to user-land JavaScript? If not then I would vote for making them
internal objects (ideally a subclass of v8::internal::Struct). That should
simplify the implementation (e.g. no context-dependent map for these objects,
support code is automatically generated, fixed instance size).

[rossberg, 2014-04-15 10:49:29]

https://codereview.chromium.org/236143002/diff/1/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode130
src/collection.js:130: if (typeof f !== 'function') {
On 2014/04/14 19:51:49, arv wrote:
> On 2014/04/14 19:15:42, adamk wrote:
> > I think you want IS_SPEC_FUNCTION(f) here, which is a macro expanding to:
> > 
> > %_ClassOf(arg) === 'Function'
> 
> IS_SPEC_FUNCTION is not correct any more. It tests that the instance is a true
> function. What we really care about is Callable. Most of the IS_SPEC function
> have been removed in the spec and now the spec checks for Callable instead.

They are testing the same thing (whether the instance type is either JS_FUNCTION
or JS_FUNCTION_PROXY -- there are no other callables in V8). However,
IS_SPEC_FUNCTION does not require a runtime call, so is preferable.

Somebody should fix the misleading comment in macros.py :)

[arv (Not doing code reviews), 2014-04-15 12:56:56]

On 2014/04/15 10:47:23, Michael Starzinger wrote:
> One high-level comment: Is there a particular reason why SetIterator and
> MapIterator are JSObjects (with mutable properties and elements)? Will they
ever
> be exposed to user-land JavaScript? If not then I would vote for making them
> internal objects (ideally a subclass of v8::internal::Struct). That should
> simplify the implementation (e.g. no context-dependent map for these objects,
> support code is automatically generated, fixed instance size).

MapIterator is returned by Map.prototype.{keys,values,entries} and SetIterator
is returned by Set.prototype.{values,entries}.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-createmapiterator
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-createsetiterator

These are not yet exposed but they will need to be eventually.

[arv (Not doing code reviews), 2014-04-15 13:05:23]

On 2014/04/15 10:49:29, rossberg wrote:
> https://codereview.chromium.org/236143002/diff/1/src/collection.js
> File src/collection.js (right):
> 
> https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode130
> src/collection.js:130: if (typeof f !== 'function') {
> On 2014/04/14 19:51:49, arv wrote:
> > On 2014/04/14 19:15:42, adamk wrote:
> > > I think you want IS_SPEC_FUNCTION(f) here, which is a macro expanding to:
> > > 
> > > %_ClassOf(arg) === 'Function'
> > 
> > IS_SPEC_FUNCTION is not correct any more. It tests that the instance is a
true
> > function. What we really care about is Callable. Most of the IS_SPEC
function
> > have been removed in the spec and now the spec checks for Callable instead.
> 
> They are testing the same thing (whether the instance type is either
JS_FUNCTION
> or JS_FUNCTION_PROXY -- there are no other callables in V8). However,
> IS_SPEC_FUNCTION does not require a runtime call, so is preferable.
> 
> Somebody should fix the misleading comment in macros.py :)

How are the callables in DOM represented? We use
ObjectTemplate::SetCallAsFunctionHandler for these

https://code.google.com/p/chromium/codesearch#search/&q=LegacyCallAsFunction%...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

However, all of these return "object" for typeof.

I'll switch to IS_SPEC_OBJECT for consistency.

[rossberg, 2014-04-15 13:21:58]

On 2014/04/15 13:05:23, arv wrote:
> How are the callables in DOM represented? We use
> ObjectTemplate::SetCallAsFunctionHandler for these
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=LegacyCallAsFunction%...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Hm, good question. I honestly don't know.

[arv (Not doing code reviews), 2014-04-15 15:12:04]

Use IS_SPEC_FUNCTION instead

[Michael Starzinger, 2014-04-15 16:13:56]

Looking good. One round of comments, mostly nits.

https://codereview.chromium.org/236143002/diff/120001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/236143002/diff/120001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1370:
ASSERT(object_function->initial_map()->inobject_properties() == 0);
nit: This assert belongs to the code creating the "iterator_result_map" I think.
It ensures that the "Object" map can be used as a base for result object map.
Can we move it down?

https://codereview.chromium.org/236143002/diff/120001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:121: var SET_ITERATOR_KIND_VALUES = 2;
nit: Can we move this constant, together with the comment into macros.py?

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:130: if (typeof f !== 'function') {
I think this should use IS_SPEC_FUNCTION for consistency with the rest of the
code. I'll defer to Andreas' comment about this.

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:238: var MAP_ITERATOR_KIND_ENTRIES = 3;
nit: Likewise.

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:247: if (typeof f !== 'function') {
Likewise.

https://codereview.chromium.org/236143002/diff/120001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/236143002/diff/120001/src/factory.cc#newcode2000
src/factory.cc:2000: Handle<JSObject>
Factory::CreateIteratorResultObject(Handle<Object> value,
nit: Likewise, can we move this up to the other allocation functions?

https://codereview.chromium.org/236143002/diff/120001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/236143002/diff/120001/src/factory.h#newcode596
src/factory.h:596: Handle<JSObject> CreateIteratorResultObject(Handle<Object>
value, bool done);
nit: Can we call this "NewIteratorResultObject" instead and move it up to where
the other allocation functions are?

https://codereview.chromium.org/236143002/diff/120001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/236143002/diff/120001/src/objects-printer.cc#...
src/objects-printer.cc:775: PrintF(out, " - count = ");
nit: I think there are couple of newline characters missing here. The
ShortPrint() will not print a trailing newline IIRC.

https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h#...
src/objects-visiting.h:101: V(JSMapIterator)            \
These definitions should not be needed, the two objects behave like normal
JSObjects and don't need any special casing in the visitor.

https://codereview.chromium.org/236143002/diff/140001/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/236143002/diff/140001/src/objects-visiting.cc...
src/objects-visiting.cc:114: return kVisitJSSetIterator;
We should be able to use a generic visitor for JSObjects here.

[Michael Starzinger, 2014-04-15 16:18:13]

One more comment.

https://codereview.chromium.org/236143002/diff/140001/test/mjsunit/harmony/co...
File test/mjsunit/harmony/collections.js (right):

https://codereview.chromium.org/236143002/diff/140001/test/mjsunit/harmony/co...
test/mjsunit/harmony/collections.js:820: })();
Also can we have a test-case that triggers a GC (i.e. by calling the gc()
function) while an iterator is active. Because I have the feeling that this is
not covered ATM.

[arv (Not doing code reviews), 2014-04-15 16:56:03]

Michael's code review

[arv (Not doing code reviews), 2014-04-15 17:06:14]

Add test that calls gc()

[arv (Not doing code reviews), 2014-04-15 17:07:22]

PTAL

Michael, I'm not sure what your concern about GC is? At this point the
references are all strong refs.

I added a test but I'm not sure what you had in mind?

https://codereview.chromium.org/236143002/diff/120001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/236143002/diff/120001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1370:
ASSERT(object_function->initial_map()->inobject_properties() == 0);
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> nit: This assert belongs to the code creating the "iterator_result_map" I
think.
> It ensures that the "Object" map can be used as a base for result object map.
> Can we move it down?

Done.

https://codereview.chromium.org/236143002/diff/120001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:121: var SET_ITERATOR_KIND_VALUES = 2;
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> nit: Can we move this constant, together with the comment into macros.py?

I put ITERATOR_KIND_* in macros and replaced *_ITERATOR_KIND_* with this.

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:130: if (typeof f !== 'function') {
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> I think this should use IS_SPEC_FUNCTION for consistency with the rest of the
> code. I'll defer to Andreas' comment about this.

Done.

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:238: var MAP_ITERATOR_KIND_ENTRIES = 3;
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newco...
src/collection.js:247: if (typeof f !== 'function') {
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/236143002/diff/120001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/236143002/diff/120001/src/factory.cc#newcode2000
src/factory.cc:2000: Handle<JSObject>
Factory::CreateIteratorResultObject(Handle<Object> value,
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> nit: Likewise, can we move this up to the other allocation functions?

Done.

https://codereview.chromium.org/236143002/diff/120001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/236143002/diff/120001/src/factory.h#newcode596
src/factory.h:596: Handle<JSObject> CreateIteratorResultObject(Handle<Object>
value, bool done);
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> nit: Can we call this "NewIteratorResultObject" instead and move it up to
where
> the other allocation functions are?

Done.

https://codereview.chromium.org/236143002/diff/120001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/236143002/diff/120001/src/objects-printer.cc#...
src/objects-printer.cc:775: PrintF(out, " - count = ");
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> nit: I think there are couple of newline characters missing here. The
> ShortPrint() will not print a trailing newline IIRC.

Done.

I also missed printing the map().

Fixed JSProxyPrint and JSFunctionProxyPrint too.

https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h#...
src/objects-visiting.h:101: V(JSMapIterator)            \
On 2014/04/15 16:13:56, Michael Starzinger wrote:
> These definitions should not be needed, the two objects behave like normal
> JSObjects and don't need any special casing in the visitor.

Done.

I'll have to re-add them when I expose the iterators to user code.

[Michael Starzinger, 2014-04-15 22:02:30]

LGTM.

https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h#...
src/objects-visiting.h:101: V(JSMapIterator)            \
On 2014/04/15 17:07:23, arv wrote:
> On 2014/04/15 16:13:56, Michael Starzinger wrote:
> > These definitions should not be needed, the two objects behave like normal
> > JSObjects and don't need any special casing in the visitor.
> 
> Done.
> 
> I'll have to re-add them when I expose the iterators to user code.

These visitor ids are the main reason I asked for a GC test case. Because with
the old version I reviewed (i.e. patch set #8 and #9) the iterator objects got
separate visitor ids assigned but none of the visitors actually registered any
callbacks for these ids. So although I didn't try to run it, I was under the
impression that e.g. the static marking visitor would not have been able to
handle those new visitor ids. I am still pretty convinced that running the new
test against patch set #9 will crash, because it will dispatch to a
non-registered callback.

[arv (Not doing code reviews), 2014-04-15 22:23:41]

On 2014/04/15 22:02:30, Michael Starzinger wrote:
> LGTM.

Thanks for the review. Can you land this for me?
 
> https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h
> File src/objects-visiting.h (right):
> 
>
https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h#...
> src/objects-visiting.h:101: V(JSMapIterator)            \
> On 2014/04/15 17:07:23, arv wrote:
> > On 2014/04/15 16:13:56, Michael Starzinger wrote:
> > > These definitions should not be needed, the two objects behave like normal
> > > JSObjects and don't need any special casing in the visitor.
> > 
> > Done.
> > 
> > I'll have to re-add them when I expose the iterators to user code.
> 
> These visitor ids are the main reason I asked for a GC test case. Because with
> the old version I reviewed (i.e. patch set #8 and #9) the iterator objects got
> separate visitor ids assigned but none of the visitors actually registered any
> callbacks for these ids. So although I didn't try to run it, I was under the
> impression that e.g. the static marking visitor would not have been able to
> handle those new visitor ids. I am still pretty convinced that running the new
> test against patch set #9 will crash, because it will dispatch to a
> non-registered callback.

You are right. I just checked out the old version with the gc test and it did
crash.

[adamk, 2014-04-16 00:40:18]

Committed patchset #11 manually as r20781 (presubmit successful).

[adamk, 2014-04-16 01:06:45]

On 2014/04/16 00:40:18, adamk wrote:
> Committed patchset #11 manually as r20781 (presubmit successful).

This got reverted in https://code.google.com/p/v8/source/detail?r=20782 for
breaking the Win32 build due to some unresolved symbols:

http://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug%20build...

Needs to be guarded with DECLARE_PRINTER():

3>e:\b\build\slave\win-dbg-b\build\v8\src\objects.h(10115): warning C4661: 'void
v8::internal::OrderedHashTableIterator<Derived,TableType>::OrderedHashTableIteratorPrint(FILE
*)' : no suitable definition provided for explicit template instantiation
request

Needs forced instantiations:

10>test-dictionary.obj :error LNK2019: unresolved external symbol "public: int
__thiscall v8::internal::OrderedHashTable<class
v8::internal::OrderedHashMap,class
v8::internal::JSMapIterator,2>::FindEntry(class v8::internal::Object *)"
(?FindEntry@?$OrderedHashTable@VOrderedHashMap@internal@v8@@VJSMapIterator@23@$01@internal@v8@@QAEHPAVObject@23@@Z)
referenced in function "void __cdecl `anonymous namespace'::TestHashMap<class
v8::internal::OrderedHashMap>(class v8::internal::Handle<class
v8::internal::OrderedHashMap>)"
(??$TestHashMap@VOrderedHashMap@internal@v8@@@?A0xf5b4ac9d@@YAXV?$Handle@VOrderedHashMap@internal@v8@@@internal@v8@@@Z)

10>test-ordered-hash-table.obj :error LNK2019: unresolved external symbol
"public: static class v8::internal::Handle<class v8::internal::JSObject> __cdecl
v8::internal::OrderedHashTableIterator<class v8::internal::JSSetIterator,class
v8::internal::OrderedHashSet>::Next(class v8::internal::Handle<class
v8::internal::JSSetIterator>)"
(?Next@?$OrderedHashTableIterator@VJSSetIterator@internal@v8@@VOrderedHashSet@23@@internal@v8@@SA?AV?$Handle@VJSObject@internal@v8@@@23@V?$Handle@VJSSetIterator@internal@v8@@@23@@Z)
referenced in function "void __cdecl `anonymous namespace'::TestSet(void)"
(?TestSet@?A0x835bb4df@@YAXXZ)

10>test-ordered-hash-table.obj :error LNK2019: unresolved external symbol
"public: static class v8::internal::Handle<class v8::internal::JSObject> __cdecl
v8::internal::OrderedHashTableIterator<class v8::internal::JSMapIterator,class
v8::internal::OrderedHashMap>::Next(class v8::internal::Handle<class
v8::internal::JSMapIterator>)"
(?Next@?$OrderedHashTableIterator@VJSMapIterator@internal@v8@@VOrderedHashMap@23@@internal@v8@@SA?AV?$Handle@VJSObject@internal@v8@@@23@V?$Handle@VJSMapIterator@internal@v8@@@23@@Z)
referenced in function "void __cdecl `anonymous namespace'::TestMap(void)"
(?TestMap@?A0x835bb4df@@YAXXZ)


I don't understand:

10>test-ordered-hash-table.obj :error LNK2019: unresolved external symbol
"public: static class v8::internal::Handle<class v8::internal::JSSetIterator>
__cdecl v8::internal::JSSetIterator::Create(class v8::internal::Handle<class
v8::internal::OrderedHashSet>,int)"
(?Create@JSSetIterator@internal@v8@@SA?AV?$Handle@VJSSetIterator@internal@v8@@@23@V?$Handle@VOrderedHashSet@internal@v8@@@23@H@Z)
referenced in function "void __cdecl `anonymous namespace'::TestSet(void)"
(?TestSet@?A0x835bb4df@@YAXXZ)

10>test-ordered-hash-table.obj :error LNK2019: unresolved external symbol
"public: static class v8::internal::Handle<class v8::internal::JSMapIterator>
__cdecl v8::internal::JSMapIterator::Create(class v8::internal::Handle<class
v8::internal::OrderedHashMap>,int)"
(?Create@JSMapIterator@internal@v8@@SA?AV?$Handle@VJSMapIterator@internal@v8@@@23@V?$Handle@VOrderedHashMap@internal@v8@@@23@H@Z)
referenced in function "void __cdecl `anonymous namespace'::TestMap(void)"
(?TestMap@?A0x835bb4df@@YAXXZ)