Bug 154877

Summary: [ES6] Add support for Symbol.toPrimitive
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, mark.lam, msaboff, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 155033    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Benchmark results
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Patch for landing none

Description Keith Miller 2016-03-01 14:13:45 PST
[ES6] Add support for Symbol.toPrimitive
Comment 1 Keith Miller 2016-03-01 14:41:46 PST
Created attachment 272593 [details]
Patch
Comment 2 Keith Miller 2016-03-01 15:35:31 PST
Created attachment 272599 [details]
Patch
Comment 3 Keith Miller 2016-03-01 15:43:42 PST
Created attachment 272600 [details]
Patch
Comment 4 Keith Miller 2016-03-01 15:58:00 PST
Created attachment 272602 [details]
Benchmark results

Results of note:

   3d-raytrace                                      588.7549+-2.7082     !    599.1740+-2.7997        ! definitely 1.0177x slower
   string-base64                                    316.4520+-1.8219     !    320.9670+-2.1476        ! definitely 1.0143x slower
   ArrayBuffer-DataView-alloc-long-lived             12.1300+-0.3915     !     13.3037+-0.5720        ! definitely 1.0968x slower
   ftl-polymorphic-bitand                           109.1163+-5.1286     !    132.0995+-0.7221        ! definitely 1.2106x slower
   ftl-polymorphic-bitor                            107.0320+-2.9510     !    132.0258+-1.4583        ! definitely 1.2335x slower
   ftl-polymorphic-bitxor                           106.2510+-0.2048     !    131.9367+-0.9145        ! definitely 1.2417x slower
   ftl-polymorphic-div                              413.1315+-0.4384     !    442.6412+-4.5124        ! definitely 1.0714x slower
   ftl-polymorphic-lshift                           134.8433+-3.7186     !    158.9005+-1.3790        ! definitely 1.1784x slower
   ftl-polymorphic-mul                              203.1172+-0.3581     !    230.5443+-4.6064        ! definitely 1.1350x slower
   ftl-polymorphic-rshift                           134.6206+-1.3621     !    159.2634+-1.3534        ! definitely 1.1831x slower
   ftl-polymorphic-StringFromCharCode                39.4506+-0.5848     !     40.7247+-0.3355        ! definitely 1.0323x slower
   ftl-polymorphic-sub                              153.3777+-0.1528     !    179.1507+-0.6174        ! definitely 1.1680x slower
   ftl-polymorphic-urshift                          145.3800+-0.4638     !    170.9575+-1.9772        ! definitely 1.1759x slower

ArrayBuffer-DataView-alloc-long-lived is just noise. The other benchmarks are relevant but we are going to defer for now since we don't have the current tooling to make an easy fix for them yet. I have created a bugzilla, however, https://bugs.webkit.org/show_bug.cgi?id=154887
Comment 5 Build Bot 2016-03-01 16:32:58 PST
Comment on attachment 272600 [details]
Patch

Attachment 272600 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/908255

New failing tests:
http/tests/security/cross-frame-access-location-get.html
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html
Comment 6 Build Bot 2016-03-01 16:33:01 PST
Created attachment 272606 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-03-01 16:36:03 PST
Comment on attachment 272600 [details]
Patch

Attachment 272600 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/908257

New failing tests:
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html
Comment 8 Build Bot 2016-03-01 16:36:06 PST
Created attachment 272608 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 9 Keith Miller 2016-03-01 16:43:43 PST
I think the EWS failures are just a rebaseling of the tests now that we also attempt to lookup Symbol.toPrimitive. I'll double check before landing though.
Comment 10 Build Bot 2016-03-01 16:50:10 PST
Comment on attachment 272600 [details]
Patch

Attachment 272600 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/908289

New failing tests:
http/tests/security/cross-frame-access-location-get.html
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html
Comment 11 Build Bot 2016-03-01 16:50:13 PST
Created attachment 272610 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Saam Barati 2016-03-02 13:52:12 PST
Comment on attachment 272600 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:356
> +    vm, globalObject, headStructure, nullptr,

style: this should all be indented.

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:359
> +        generateCondition(vm, nullptr, object, uid, PropertyCondition::Absence);

this can go on one line

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:365
> +}

If you feel inclined, this and the function below only have one thing that are different.
It would be easy to create a common function for both that is templatized or takes an argument

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:631
> +inline PreferredPrimitiveType toPreferredPrimitiveType(ExecState* exec, JSValue value)

Style: indentation is off

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:638
> +        StringImpl* hintString = jsCast<JSString*>(value)->value(exec).impl();

This can throw.

> Source/JavaScriptCore/runtime/JSObject.cpp:1402
> +enum ThrowMode { TakesHint, DoesNotTakeHint };

Why is this called ThrowMode?
That name doesn't make sense to me.
I'd also make this an enum class

> Source/JavaScriptCore/runtime/JSObject.cpp:1443
>  // ECMA 8.6.2.6

It's worth updating this to section 7.1.1 of ES6.

> Source/JavaScriptCore/tests/stress/ropes-symbol-toprimitive.js:13
> +String.prototype[Symbol.toPrimitive] = function() { return "changed"; }

It's worth adding a test that compiles through the DFG and starts with String.prototype having this property
and ensuring we prevent the optimizations that we check using canOptimizeStringObjectAccess

> Source/JavaScriptCore/tests/stress/symbol-toprimitive.js:14
> +let foo = { }
> +foo[Symbol.toPrimitive] = function() { return {} };
> +
> +failed = true;
> +try {
> +    foo >= 1;
> +} catch (e) {
> +    if (e instanceof TypeError)
> +        failed = false;
> +}
> +
> +if (failed)
> +    throw "should have thrown on return of object";

it's probably worth adding a test involving snippets and the JITs
Comment 13 Keith Miller 2016-03-03 14:07:03 PST
Comment on attachment 272600 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:356
>> +    vm, globalObject, headStructure, nullptr,
> 
> style: this should all be indented.

Fixed.

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:359
>> +        generateCondition(vm, nullptr, object, uid, PropertyCondition::Absence);
> 
> this can go on one line

Fixed.

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:365
>> +}
> 
> If you feel inclined, this and the function below only have one thing that are different.
> It would be easy to create a common function for both that is templatized or takes an argument

I think it's probably fine, I think it would just end up making the code harder to read.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:631
>> +inline PreferredPrimitiveType toPreferredPrimitiveType(ExecState* exec, JSValue value)
> 
> Style: indentation is off

Fixed.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:638
>> +        StringImpl* hintString = jsCast<JSString*>(value)->value(exec).impl();
> 
> This can throw.

Good catch, thanks.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1402
>> +enum ThrowMode { TakesHint, DoesNotTakeHint };
> 
> Why is this called ThrowMode?
> That name doesn't make sense to me.
> I'd also make this an enum class

Yeah, that should be TypeHintMode. switched to class.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1443
>>  // ECMA 8.6.2.6
> 
> It's worth updating this to section 7.1.1 of ES6.

Fixed.

>> Source/JavaScriptCore/tests/stress/ropes-symbol-toprimitive.js:13
>> +String.prototype[Symbol.toPrimitive] = function() { return "changed"; }
> 
> It's worth adding a test that compiles through the DFG and starts with String.prototype having this property
> and ensuring we prevent the optimizations that we check using canOptimizeStringObjectAccess

Added.

>> Source/JavaScriptCore/tests/stress/symbol-toprimitive.js:14
>> +    throw "should have thrown on return of object";
> 
> it's probably worth adding a test involving snippets and the JITs

Snippets always call the slow path when passed an object and a primitive so that won't test anything yet. However, I put this function in a loop for good measure.
Comment 14 Keith Miller 2016-03-03 15:46:09 PST
Created attachment 272783 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2016-03-03 16:47:47 PST
Comment on attachment 272783 [details]
Patch for landing

Clearing flags on attachment: 272783

Committed r197531: <http://trac.webkit.org/changeset/197531>
Comment 16 WebKit Commit Bot 2016-03-03 16:47:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Joseph Pecoraro 2016-06-06 20:52:28 PDT
*** Bug 150462 has been marked as a duplicate of this bug. ***