Bug 204043 – [JSC] Introduce JSArrayIterator

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Using JSInternalObjectImpl. This is perfect target since we do not expose ArrayIterator constructor to users.

[Yusuke Suzuki]

Created attachment 383204 [details]
Patch

[Devin Rousso]

Comment on attachment 383204 [details]
Patch

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:394
> +            kindString = jsNontrivialString(vm, "key+value"_s);

Should we just call this `entries`?

[Yusuke Suzuki]

Comment on attachment 383204 [details]
Patch

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

Nice comment! I'll change it. And now I'm implementing full-fledged object-allocation-sinking for ArrayIterator.

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:394
>> +            kindString = jsNontrivialString(vm, "key+value"_s);
> 
> Should we just call this `entries`?

I think either is fine :)

1. "entries" is derived from the ECMAScript's `Array#entries` function name. It is really intuitive.
2. "key+value" is derived from the ECMAScript's ArrayIteratorKind definition https://tc39.es/ecma262/#sec-array.prototype.entries, it is also fine.

Maybe, "entries" seems better since "entries" is developer-facing interface's name (Array#entries). Changing.

[Yusuke Suzuki]

Created attachment 383209 [details]
Patch

[Yusuke Suzuki]

Created attachment 383210 [details]
Patch

[Yusuke Suzuki]

Created attachment 383211 [details]
Patch

[Yusuke Suzuki]

Created attachment 383212 [details]
Patch

[EWS Watchlist]

Comment on attachment 383212 [details]
Patch

Attachment 383212 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13230360

New failing tests:
stress/super-getter-reset-ic.js.ftl-eager-no-cjit

[EWS Watchlist]

Comment on attachment 383212 [details]
Patch

Attachment 383212 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13242709

New failing tests:
fetch/shadowing-forEach.html
fast/text/unicode-range-javascript.html
fast/dom/nodeListIterator.html
fetch/header-constructor-overriden.html
fetch/header-constructor-subclass.html
css3/filters/blur-various-radii.html
http/tests/security/cross-frame-access-enumeration.html
editing/inserting/insert-paragraph-in-designmode-document.html
requestidlecallback/requestidlecallback-document-gc.html
fast/dom/domTokenListIterator.html
fast/images/exif-orientation-canvas.html
fast/dom/DOMURL/searchparams-iterable.html
resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.html
resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-alive.html
fast/dom/NodeList/nodelist-iterable.html
resize-observer/element-leak.html

[EWS Watchlist]

Created attachment 383357 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Yusuke Suzuki]

Created attachment 383399 [details]
Patch

[Yusuke Suzuki]

Created attachment 383423 [details]
Patch

[EWS Watchlist]

Comment on attachment 383423 [details]
Patch

Attachment 383423 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13246218

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 383448 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Yusuke Suzuki]

Created attachment 383734 [details]
Patch

[Keith Miller]

Created attachment 386931 [details]
Patch

[Keith Miller]

Created attachment 386933 [details]
Patch

[Keith Miller]

Created attachment 386934 [details]
Patch

[Keith Miller]

Created attachment 387016 [details]
Patch

[Keith Miller]

Created attachment 387026 [details]
Patch

[Keith Miller]

Created attachment 387034 [details]
Patch

[Keith Miller]

Created attachment 387039 [details]
Patch

[Keith Miller]

Created attachment 387057 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 387057 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1462
> +                        [&] (RegisteredStructure structure) {

WebKit coding style now requires no-space between [] and ().

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:888
> +        case NewArrayIterator: {
> +            target = handleInternalFieldClass<JSArrayIterator>(node, writes);
> +            break;
> +        }

Can you add/file FIXME about supporting other types of internal field objects? (JSGenerator etc.)

> Source/JavaScriptCore/runtime/JSArrayIterator.cpp:68
> +    Base::finishCreation(vm);

We should call Base::finishCreation(vm) first to finish "initializingObject" phase. (JSCell::finishCreation).

[Devin Rousso]

Comment on attachment 387057 [details]
Patch

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

r=me for the inspector parts.  Looks great!

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:381
> +                return jsNontrivialString(vm, "entries"_s);

I think this is better.  Although "key+value" is the spec name for this, I think that developers won't know what that is, and `entries` will be more familiar to them.  Thanks for also changing Map/Set too :)

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:383
> +            return jsNontrivialString(vm, "unknown"_s);

Rather than show "unknown", could we just omit the `kind` altogether?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:385
> +        };
> +        if (auto* arrayIterator = jsDynamicCast<JSArrayIterator*>(vm, iteratorObject)) {

Style: please add an extra newline

> LayoutTests/ChangeLog:10
> +        * inspector/model/remote-object/iterator-expected.txt:

Just to be safe, it's probably a good idea to run all the tests under 'inspector/model' and 'inspector/runtime'.

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:528
> +EXPRESSION: map = new Map; map.set(1, 2); map.set("keys", "values"); map.values()

This should not have changed.  "key" and "value" are actual JS values, not internal properties/labels.

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:569
> +          "_description": "values",

Ditto (528)

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:622
> +          "_description": "keys",

Ditto (528)

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:697
> +              "_value": "keys"

Ditto (528)

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:702
> +              "_value": "values"

Ditto (528)

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:859
> +              "_value": "keys"

Ditto (inspector/model/remote-object/iterator-expected.txt:528)

> LayoutTests/inspector/model/remote-object/iterator-expected.txt:864
> +              "_value": "values"

Ditto (inspector/model/remote-object/iterator-expected.txt:528)

> LayoutTests/inspector/model/remote-object/iterators-mutated-expected.txt:167
> +              "_value": "keys"

Ditto (528)

> LayoutTests/inspector/model/remote-object/iterators-mutated-expected.txt:172
> +              "_value": "values"

Ditto (528)

[Keith Miller]

Whoops, forgot to add CheckArray to prediction propagation.

[Keith Miller]

Created attachment 387176 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 387176 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3361
> +        if (node->op() == CheckArray && node->arrayMode().alreadyChecked(m_graph, node, forNode(node->child1()))) {

What's the purpose of `node->op() == CheckArray`? If we would like to take the following part in CheckNeutered too, I think we need to put FALLTHROUGH instead of break in CheckNeutered.

[Keith Miller]

Comment on attachment 387176 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3361
>> +        if (node->op() == CheckArray && node->arrayMode().alreadyChecked(m_graph, node, forNode(node->child1()))) {
> 
> What's the purpose of `node->op() == CheckArray`? If we would like to take the following part in CheckNeutered too, I think we need to put FALLTHROUGH instead of break in CheckNeutered.

Whoops, that's leftover from the first iteration of CheckNeutered that didn't rely on CheckArray doing the array check. Will remove.

[Yusuke Suzuki]

Comment on attachment 387176 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3361
>>> +        if (node->op() == CheckArray && node->arrayMode().alreadyChecked(m_graph, node, forNode(node->child1()))) {
>> 
>> What's the purpose of `node->op() == CheckArray`? If we would like to take the following part in CheckNeutered too, I think we need to put FALLTHROUGH instead of break in CheckNeutered.
> 
> Whoops, that's leftover from the first iteration of CheckNeutered that didn't rely on CheckArray doing the array check. Will remove.

OK, make sense.

[Keith Miller]

Created attachment 387187 [details]
Patch for landing

[Keith Miller]

Created attachment 387188 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387188 [details]
Patch for landing

Clearing flags on attachment: 387188

Committed r254252: <https://trac.webkit.org/changeset/254252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58433315>

[THONG NGO]

Created attachment 469104 [details]
Rentokil Initial plc

NGO QUANG THONG