Bug 221526 – [JSC] C++ iteration should support fast iterator protocol

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] C++ iteration should support fast iterator protocol

[Yusuke Suzuki]

Created attachment 419531 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 419531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419531&action=review

> Source/JavaScriptCore/runtime/IteratorOperations.h:120
> +                JSArrayIterator* iterator = JSArrayIterator::create(vm, globalObject.arrayIteratorStructure(), array, IterationKind::Values);
> +                iterator->internalField(JSArrayIterator::Field::Index).setWithoutWriteBarrier(jsNumber(index + 1));
> +                iteratorClose(&globalObject, iterator);
> +                return;

We need to have an array iterator only when we need to catch an error. So, when we found an error, we create an array iterator with appropriate state.

[Yusuke Suzuki]

Created attachment 419532 [details]
Patch

[Yusuke Suzuki]

Created attachment 419533 [details]
Patch

[Yusuke Suzuki]

Created attachment 419534 [details]
Patch

[Yusuke Suzuki]

Created attachment 419535 [details]
Patch

[Yusuke Suzuki]

Created attachment 419536 [details]
Patch

[Alexey Shvayka]

Comment on attachment 419536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419536&action=review

Well done! r=me with a comment on test and a few nits.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:867
> +    if (prepareForFastArrayIteration(vm, globalObject, iterable, symbolIterator) == IterationMode::FastArray) {

nit: "prepare" in the name suggests the method is performing some side-effects, yet it's pure. Maybe it's worth renaming it to something like "getIterationMode"?

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:249
> +    // We don't want to allocate the values function just to check if it's the same as our function at so we use the concurrent accessor.

typo: extra "at" after "function"

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:264
> +    // FIXME: We want to support broader JSArrays so long as array[@@iterator] is not defined.

typo: *as* long as

> Source/JavaScriptCore/runtime/IteratorOperations.h:70
> +            JSValue nextValue = array->getIndex(globalObject, static_cast<unsigned>(index));

nit: Do we really need the static_cast<unsigned> here given that `index` is unsigned and getIndex() has only one overload.

> Source/JavaScriptCore/runtime/IteratorOperations.h:76
> +                iterator->internalField(JSArrayIterator::Field::Index).setWithoutWriteBarrier(jsNumber(index + 1));

This is very tricky, great catch! Could you please expand stress/map-constructor-hole-throw.js to cover this line?
Since we get a hold of iterator whose [[IteratedObject]] is still defined, we can test its [[ArrayIteratorNextIndex]] value by calling next(), right?

> Source/JavaScriptCore/runtime/IteratorOperations.h:112
> +            JSValue nextValue = array->getIndex(&globalObject, static_cast<unsigned>(index));

Ditto on static_cast<unsigned>.

[Yusuke Suzuki]

Comment on attachment 419536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419536&action=review

Thanks!

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:867
>> +    if (prepareForFastArrayIteration(vm, globalObject, iterable, symbolIterator) == IterationMode::FastArray) {
> 
> nit: "prepare" in the name suggests the method is performing some side-effects, yet it's pure. Maybe it's worth renaming it to something like "getIterationMode"?

Sounds good! Fixed.

>> Source/JavaScriptCore/runtime/IteratorOperations.cpp:249
>> +    // We don't want to allocate the values function just to check if it's the same as our function at so we use the concurrent accessor.
> 
> typo: extra "at" after "function"

Fixed.

>> Source/JavaScriptCore/runtime/IteratorOperations.cpp:264
>> +    // FIXME: We want to support broader JSArrays so long as array[@@iterator] is not defined.
> 
> typo: *as* long as

Fixed.

>> Source/JavaScriptCore/runtime/IteratorOperations.h:70
>> +            JSValue nextValue = array->getIndex(globalObject, static_cast<unsigned>(index));
> 
> nit: Do we really need the static_cast<unsigned> here given that `index` is unsigned and getIndex() has only one overload.

Fixed.

>> Source/JavaScriptCore/runtime/IteratorOperations.h:76
>> +                iterator->internalField(JSArrayIterator::Field::Index).setWithoutWriteBarrier(jsNumber(index + 1));
> 
> This is very tricky, great catch! Could you please expand stress/map-constructor-hole-throw.js to cover this line?
> Since we get a hold of iterator whose [[IteratedObject]] is still defined, we can test its [[ArrayIteratorNextIndex]] value by calling next(), right?

Right! Added that test :)

>> Source/JavaScriptCore/runtime/IteratorOperations.h:112
>> +            JSValue nextValue = array->getIndex(&globalObject, static_cast<unsigned>(index));
> 
> Ditto on static_cast<unsigned>.

Fixed.

[Yusuke Suzuki]

Committed r272638: <https://trac.webkit.org/changeset/272638>

[Radar WebKit Bug Importer]

<rdar://problem/74175114>