We need use @putByValDirect instead array.prototyp.@push. @push is observable because it uses Set operation
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
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"
(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.
Created attachment 411062 [details] Patch
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 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 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.
Created attachment 411364 [details] Patch Bring back @shift, cache length to a variable, introduce @arrayPush intrinsic and utilize it in WebCore.
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 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.
Created attachment 411371 [details] Patch for landing
(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!
Committed r268489: <https://trac.webkit.org/changeset/268489> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411371 [details].
<rdar://problem/70309207>
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