Bug 217928 – %TypedArray%#sort helper functions should be globalPrivate

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

[Ross Kirsling]

%TypedArray%#sort helper functions should be globalPrivate

[Ross Kirsling]

Created attachment 411810 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 411810 [details]
Patch

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

r=me

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:212
> +    var buffer = [];
> +    buffer.length = valueCount;

Use `var buffer = @newArrayWithSize(valueCount)`.

[Ross Kirsling]

Created attachment 411812 [details]
Patch for landing

[Alexey Shvayka]

Comment on attachment 411810 [details]
Patch

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

> Source/JavaScriptCore/builtins/GlobalOperations.js:132
> +function sortMin(a, b)

In future, we may consider replacing this (and maxWithPositives() of ArrayPrototype.js) with exposed Math.{min,max} built-ins as they have fast path for 2 arguments.

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:227
> +        for(var i = 0; i < valueCount; i++)

nit: space before a parentheses and using pre-increment instead of post-increment (based on https://bugs.webkit.org/show_bug.cgi?id=202582#c12).

[Ross Kirsling]

Created attachment 411821 [details]
Patch for landing

[Ross Kirsling]

Thanks for the double review!

[Alexey Shvayka]

Comment on attachment 411821 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1231
> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), mathProtoFuncMin, TruncIntrinsic));

If there isn't a "race condition" during built-in objects init, we can reuse this JSFunction in MathObject::finishCreation() via `globalObject->linkTimeConstant(LinkTimeConstant::min);` to avoid creating the second one?

[Ross Kirsling]

(In reply to Alexey Shvayka from comment #7)
> Comment on attachment 411821 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411821&action=review
> 
> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1231
> > +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), mathProtoFuncMin, TruncIntrinsic));
> 
> If there isn't a "race condition" during built-in objects init, we can reuse
> this JSFunction in MathObject::finishCreation() via
> `globalObject->linkTimeConstant(LinkTimeConstant::min);` to avoid creating
> the second one?

Is there a precedent for that? I was just mimicking mathProtoFuncTrunc here.

[Alexey Shvayka]

Comment on attachment 411821 [details]
Patch for landing

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

>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1231
>>> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), mathProtoFuncMin, TruncIntrinsic));
>> 
>> If there isn't a "race condition" during built-in objects init, we can reuse this JSFunction in MathObject::finishCreation() via `globalObject->linkTimeConstant(LinkTimeConstant::min);` to avoid creating the second one?
> 
> Is there a precedent for that? I was just mimicking mathProtoFuncTrunc here.

Not that I can find; it might be better to do this optimization in a separate patch, for all link-time-constants possible.
Oh, and please set the correct intrinsic (TruncIntrinsic => MinIntrinsic), as it's what DFG uses to emit the fast path.

[Ross Kirsling]

(In reply to Alexey Shvayka from comment #9)
> Oh, and please set the correct intrinsic (TruncIntrinsic => MinIntrinsic),
> as it's what DFG uses to emit the fast path.

Oh geez, what a silly mistake. Thanks.

[Ross Kirsling]

Created attachment 411834 [details]
Patch for landing

[EWS]

Committed r268715: <https://trac.webkit.org/changeset/268715>

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

[Radar WebKit Bug Importer]

<rdar://problem/70472747>