[ES6] Add support for Symbol.toPrimitive
Created attachment 272593 [details] Patch
Created attachment 272599 [details] Patch
Created attachment 272600 [details] Patch
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 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
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 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
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
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 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
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 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 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.
Created attachment 272783 [details] Patch for landing
Comment on attachment 272783 [details] Patch for landing Clearing flags on attachment: 272783 Committed r197531: <http://trac.webkit.org/changeset/197531>
All reviewed patches have been landed. Closing bug.
*** Bug 150462 has been marked as a duplicate of this bug. ***