WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
154877
[ES6] Add support for Symbol.toPrimitive
https://bugs.webkit.org/show_bug.cgi?id=154877
Summary
[ES6] Add support for Symbol.toPrimitive
Keith Miller
Reported
2016-03-01 14:13:45 PST
[ES6] Add support for Symbol.toPrimitive
Attachments
Patch
(29.44 KB, patch)
2016-03-01 14:41 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(29.41 KB, patch)
2016-03-01 15:35 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(30.04 KB, patch)
2016-03-01 15:43 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Benchmark results
(65.64 KB, text/plain)
2016-03-01 15:58 PST
,
Keith Miller
no flags
Details
Archive of layout-test-results from ews100 for mac-yosemite
(768.84 KB, application/zip)
2016-03-01 16:33 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2
(814.36 KB, application/zip)
2016-03-01 16:36 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews114 for mac-yosemite
(836.64 KB, application/zip)
2016-03-01 16:50 PST
,
Build Bot
no flags
Details
Patch for landing
(34.58 KB, patch)
2016-03-03 15:46 PST
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2016-03-01 14:41:46 PST
Created
attachment 272593
[details]
Patch
Keith Miller
Comment 2
2016-03-01 15:35:31 PST
Created
attachment 272599
[details]
Patch
Keith Miller
Comment 3
2016-03-01 15:43:42 PST
Created
attachment 272600
[details]
Patch
Keith Miller
Comment 4
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
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Build Bot
Comment 7
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
Build Bot
Comment 8
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
Keith Miller
Comment 9
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.
Build Bot
Comment 10
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
Build Bot
Comment 11
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
Saam Barati
Comment 12
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
Keith Miller
Comment 13
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.
Keith Miller
Comment 14
2016-03-03 15:46:09 PST
Created
attachment 272783
[details]
Patch for landing
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2016-03-03 16:47:51 PST
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 17
2016-06-06 20:52:28 PDT
***
Bug 150462
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug