Bug 175432 – Use @putByValDirect instead of Array.prototype.@push in built-ins

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

[GSkachkov]

We need use @putByValDirect instead array.prototyp.@push. @push is observable because it uses Set operation

[GSkachkov]

It seems that this task will require anther approach, than just replace @push by @putByValDirect. For instance Object.entries, that is implemented in builtins\ObjectConstructor.js, used [Set] operation somewhere in getOwnPropertyNames, tested by following code:
```
Object.defineProperty(Array.prototype, '0', { 
    set : function (x) { throw new Error('Should not be raised:' + x); }
});
```
Possible we need some internal data structure that closed to Array and implement push, slice and access to index item, but did not open its API

[Saam Barati]

It seems like we should only replace this when the spec doesn't call Set(...), e.g, when we're using the Array for internal bookkeeping. If the spec exposes that we call Set(...), then we can keep using @push or push depending on what the spec says for property access of "push"

[GSkachkov]

(In reply to Saam Barati from comment #2)
> It seems like we should only replace this when the spec doesn't call
> Set(...), e.g, when we're using the Array for internal bookkeeping. If the
> spec exposes that we call Set(...), then we can keep using @push or push
> depending on what the spec says for property access of "push"

Ok. I see, I'll check spec where @push is used.

[Alexey Shvayka]

Created attachment 411062 [details]
Patch

[Darin Adler]

Comment on attachment 411062 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        since [[Set]] is performed on indexed properies [2].

typo: properties

> Source/JavaScriptCore/builtins/RegExpPrototype.js:135
> +        @putByValDirect(resultList, resultList.length, resultString);

Is this a less efficient idiom than @push? Seems sort of unpleasant to have to invoke length. Is the call to length observable?

[youenn fablet]

Comment on attachment 411062 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Use @putByValDirect instead of Array.prototype.@push in built-ins

Should we envision doing so for WebCore built-ins as well?
See StreamInternals.js and WritableStreamInternals.js as placed where we use push/shift

[Keith Miller]

Comment on attachment 411062 [details]
Patch

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

> Source/JavaScriptCore/builtins/ObjectConstructor.js:35
> +    for (var i = 0; i < names.length; ++i) {

names.length can be moved into a local variable, which should be a bit faster than getting it on each iteration.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:135
>> +        @putByValDirect(resultList, resultList.length, resultString);
> 
> Is this a less efficient idiom than @push? Seems sort of unpleasant to have to invoke length. Is the call to length observable?

Length shouldn't be observable here since resultList is guaranteed to be an Array. I don't think is more efficient since we will IC the length access here, whereas the push call is polymorphic and not IC'd (it's implemented in C++).

Although, we should move the length access into a local variable at the top of the loop.

[Alexey Shvayka]

Created attachment 411364 [details]
Patch

Bring back @shift, cache length to a variable, introduce @arrayPush intrinsic and utilize it in WebCore.

[Yusuke Suzuki]

Comment on attachment 411364 [details]
Patch

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

r=me with one comment.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:132
>          if (!resultString.length)

Can you change the other code like that to @arrayPush too?

`@putByValDirect(xxx, xxx.length, yyy)`

For example, RegExpPrototype.js includes some code like this.

@putByValDirect(result, result.length, subStr);

ArrayPrototype.js also has similar one.

[Darin Adler]

Comment on attachment 411364 [details]
Patch

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

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:132
>>          if (!resultString.length)
> 
> Can you change the other code like that to @arrayPush too?
> 
> `@putByValDirect(xxx, xxx.length, yyy)`
> 
> For example, RegExpPrototype.js includes some code like this.
> 
> @putByValDirect(result, result.length, subStr);
> 
> ArrayPrototype.js also has similar one.

I did a grep for putByValDirect.*length and those are the only two source files that have hits: 1 in ArrayPrototype.js and 3 in RegExpPrototype.js.

[Alexey Shvayka]

Created attachment 411371 [details]
Patch for landing

[Alexey Shvayka]

(In reply to youenn fablet from comment #6)
> Should we envision doing so for WebCore built-ins as well?
> See StreamInternals.js and WritableStreamInternals.js as placed where we use
> push/shift

I've updated the patch to replace Array.prototype.@push => @arrayPush() in WebCore as well.
Array.prototype.@shift is unobservable for non-sparse arrays and is also non-trivial to replace, so let's keep it.

(In reply to Yusuke Suzuki from comment #9)
> r=me with one comment.

Thank you for review!

[EWS]

Committed r268489: <https://trac.webkit.org/changeset/268489>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411371 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70309207>

[Ryan Haddad]

This caused test/built-ins/Object/entries/order-after-define-property.js to fail on test262 bots. Details in https://bugs.webkit.org/show_bug.cgi?id=217738