Bug 175432 - Use @putByValDirect instead of Array.prototype.@push in built-ins
Summary: Use @putByValDirect instead of Array.prototype.@push in built-ins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Trivial
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-10 11:02 PDT by GSkachkov
Modified: 2020-10-14 16:31 PDT (History)
14 users (show)

See Also:


Attachments
Patch (8.49 KB, patch)
2020-10-11 13:20 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2020-10-14 12:24 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch for landing (19.59 KB, patch)
2020-10-14 14:12 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2017-08-10 11:02:54 PDT
We need use @putByValDirect instead array.prototyp.@push. @push is observable because it uses Set operation
Comment 1 GSkachkov 2017-08-14 12:36:46 PDT
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
Comment 2 Saam Barati 2017-08-14 15:56:56 PDT
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"
Comment 3 GSkachkov 2017-08-16 00:13:45 PDT
(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.
Comment 4 Alexey Shvayka 2020-10-11 13:20:11 PDT
Created attachment 411062 [details]
Patch
Comment 5 Darin Adler 2020-10-11 19:32:10 PDT
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?
Comment 6 youenn fablet 2020-10-12 03:00:22 PDT
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
Comment 7 Keith Miller 2020-10-12 11:55:38 PDT
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.
Comment 8 Alexey Shvayka 2020-10-14 12:24:12 PDT
Created attachment 411364 [details]
Patch

Bring back @shift, cache length to a variable, introduce @arrayPush intrinsic and utilize it in WebCore.
Comment 9 Yusuke Suzuki 2020-10-14 12:33:00 PDT
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.
Comment 10 Darin Adler 2020-10-14 12:54:37 PDT
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.
Comment 11 Alexey Shvayka 2020-10-14 14:12:58 PDT
Created attachment 411371 [details]
Patch for landing
Comment 12 Alexey Shvayka 2020-10-14 14:23:12 PDT
(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!
Comment 13 EWS 2020-10-14 14:48:55 PDT
Committed r268489: <https://trac.webkit.org/changeset/268489>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411371 [details].
Comment 14 Radar WebKit Bug Importer 2020-10-14 14:49:18 PDT
<rdar://problem/70309207>
Comment 15 Ryan Haddad 2020-10-14 16:31:00 PDT
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