...
Created attachment 273754 [details] Patch WIP part1
Comment on attachment 273754 [details] Patch Attachment 273754 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/962282 Number of test failures exceeded the failure limit.
Created attachment 273760 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 273754 [details] Patch Attachment 273754 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/962283 Number of test failures exceeded the failure limit.
Created attachment 273761 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 273754 [details] Patch Attachment 273754 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/962285 Number of test failures exceeded the failure limit.
Created attachment 273762 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 273754 [details] Patch Attachment 273754 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/962279 Number of test failures exceeded the failure limit.
Created attachment 273764 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 273969 [details] Patch WIP
Created attachment 274095 [details] Patch WIP
Created attachment 274102 [details] Patch
Comment on attachment 274102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274102&action=review Added some comments. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:314 > + // 9.4.5.5-2-b-i Return ? IntegerIndexedElementSet(O, numericIndex, V). Here is very special case. IntegerIndexedElementSet ignores the receiver. > Source/JavaScriptCore/tests/es6.yaml:1024 > + cmd: runES6 :normal Yeah, they are fixed.
can you post performance numbers?
(In reply to comment #14) > can you post performance numbers? Yup, I'll take the performance in my OSX laptop. I'll post it when I get it.
Comment on attachment 274102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274102&action=review r=me > Source/JavaScriptCore/ChangeLog:21 > + We need not to change any JIT part, because the JS code cannot alter the receiver without Reflect.set. JS code can have a different receiver and base through the use of ProxyObject. > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1012 > + JSProxy* jsProxy = jsDynamicCast<JSProxy*>(thisValue); Is there a better way to determine if something is JSProxy than through jsDynamicCast? > Source/JavaScriptCore/runtime/JSObject.cpp:514 > + // 9.1.9.1-8 Perform ? Call(setter, Receiver, « V »). style: I would write this without the unicode characters. Call(setter, Receiver, <V>) > Source/JavaScriptCore/tests/stress/proxy-set.js:93 > + set: function(theTarget, propName, value, receiver) { I forgot how to spell ;-) > Source/JavaScriptCore/tests/stress/proxy-set.js:514 > + //assert(receiver === obj); I'm not sure why I added a commented out assert. Lets make it valid or remove it. > Source/JavaScriptCore/tests/stress/proxy-set.js:550 > + //assert(receiver === obj); ditto > Source/JavaScriptCore/tests/stress/reflect-set-proxy-set.js:6 > +function shouldThrow(func, message) { not used. > Source/JavaScriptCore/tests/stress/reflect-set-proxy-set.js:542 > + //assert(receiver === obj); remove or make valid > Source/JavaScriptCore/tests/stress/reflect-set-proxy-set.js:578 > + //assert(receiver === obj); ditto > Source/JavaScriptCore/tests/stress/reflect-set-receiver-proxy-set.js:6 > +function shouldThrow(func, message) { not used > Source/JavaScriptCore/tests/stress/reflect-set-with-global-proxy.js:6 > +function shouldThrow(func, message) { not used > Source/JavaScriptCore/tests/stress/reflect-set.js:11 > error = e; I didn't read through every test case, but it would be nice to have at one test for every exception that can be thrown from OrdinarySet. It may seem like overkill now, but it will prevent regressions.
Created attachment 274120 [details] Performance results Attached the full perf results. Seems performance neutral. SunSpider: 3d-cube 6.5677+-0.6867 6.4046+-0.6634 might be 1.0255x faster 3d-morph 6.1957+-0.2018 ? 6.2047+-0.2463 ? 3d-raytrace 7.5704+-0.2695 7.4280+-0.3502 might be 1.0192x faster access-binary-trees 2.9183+-0.9461 2.6565+-0.1635 might be 1.0985x faster access-fannkuch 7.5958+-1.0448 7.5466+-0.9002 access-nbody 3.3962+-0.1910 3.3019+-0.0915 might be 1.0286x faster access-nsieve 3.4871+-0.2067 3.4482+-0.3801 might be 1.0113x faster bitops-3bit-bits-in-byte 1.5870+-0.0481 1.5690+-0.0385 might be 1.0115x faster bitops-bits-in-byte 3.4130+-0.2969 3.4083+-0.1571 bitops-bitwise-and 2.3727+-0.4383 ? 2.5131+-0.2736 ? might be 1.0591x slower bitops-nsieve-bits 3.8406+-0.0598 3.8090+-0.0300 controlflow-recursive 2.9078+-0.1297 2.8478+-0.0181 might be 1.0211x faster crypto-aes 5.0846+-0.3115 4.9697+-0.0210 might be 1.0231x faster crypto-md5 3.1804+-0.0497 ? 3.2078+-0.0728 ? crypto-sha1 2.9185+-0.0895 ? 2.9477+-0.2603 ? might be 1.0100x slower date-format-tofte 10.7265+-0.0952 10.6842+-0.6291 date-format-xparb 6.1201+-0.4927 5.9937+-0.2458 might be 1.0211x faster math-cordic 3.6545+-0.2196 3.5285+-0.0700 might be 1.0357x faster math-partial-sums 6.0555+-0.0786 ? 6.0659+-0.0880 ? math-spectral-norm 2.7208+-0.1937 2.6859+-0.1051 might be 1.0130x faster regexp-dna 7.8043+-1.0235 ? 7.8580+-1.1827 ? string-base64 5.4235+-0.1838 ? 6.4940+-2.3075 ? might be 1.1974x slower string-fasta 7.1857+-0.3570 ? 7.2751+-0.2340 ? might be 1.0124x slower string-tagcloud 10.4770+-2.7568 ? 10.8365+-1.9697 ? might be 1.0343x slower string-unpack-code 21.8265+-0.2940 21.5762+-0.8236 might be 1.0116x faster string-validate-input 5.0407+-0.0558 ? 5.0956+-0.2344 ? might be 1.0109x slower <arithmetic> 5.7720+-0.1505 ? 5.7829+-0.1271 ? might be 1.0019x slower Baseline Mine LongSpider: 3d-cube 1069.2930+-276.0238 1025.2760+-30.8526 might be 1.0429x faster 3d-morph 714.4610+-6.6025 713.1762+-9.8049 3d-raytrace 794.1237+-13.7553 792.8201+-10.9314 access-binary-trees 1090.8655+-11.3712 ? 1097.8271+-31.7392 ? access-fannkuch 303.6585+-8.4011 303.5850+-7.8593 access-nbody 629.3448+-5.1973 ? 629.5256+-9.5904 ? access-nsieve 470.0767+-7.8940 464.1314+-8.7283 might be 1.0128x faster bitops-3bit-bits-in-byte 45.0070+-5.1883 43.1230+-0.9169 might be 1.0437x faster bitops-bits-in-byte 133.6899+-3.8284 132.1310+-2.5370 might be 1.0118x faster bitops-nsieve-bits 430.1555+-7.0874 424.6672+-7.9728 might be 1.0129x faster controlflow-recursive 562.6976+-13.0445 558.0458+-8.6820 crypto-aes 805.8644+-23.9579 801.6342+-29.5374 crypto-md5 693.3176+-5.1434 ? 694.8063+-9.8189 ? crypto-sha1 807.0845+-16.2783 ? 817.7719+-16.3772 ? might be 1.0132x slower date-format-tofte 911.5757+-42.7865 907.5555+-22.2791 date-format-xparb 887.0358+-20.9681 ? 911.9042+-35.7397 ? might be 1.0280x slower hash-map 187.9840+-3.7170 ? 188.6201+-7.6537 ? math-cordic 568.2661+-15.6167 ? 569.3737+-11.6329 ? math-partial-sums 567.7679+-10.5478 564.9844+-10.4529 math-spectral-norm 903.2505+-1.4445 ? 905.7955+-5.0476 ? string-base64 480.1818+-15.4929 475.1559+-14.9551 might be 1.0106x faster string-fasta 433.4783+-9.4118 ? 438.0720+-11.5703 ? might be 1.0106x slower string-tagcloud 208.7055+-5.4354 ? 214.2831+-4.6632 ? might be 1.0267x slower <geometric> 489.8093+-5.6077 488.6515+-1.6800 might be 1.0024x faster Baseline Mine V8Spider: crypto 48.3240+-0.5912 48.2603+-0.4591 deltablue 62.8879+-2.8139 ? 64.8147+-3.1006 ? might be 1.0306x slower earley-boyer 52.4504+-2.8782 49.8222+-0.9212 might be 1.0528x faster raytrace 33.5526+-1.2466 33.5205+-3.1629 regexp 72.7196+-0.8944 72.6405+-5.5284 richards 52.7487+-3.9995 52.4179+-1.8408 splay 42.0688+-4.3587 41.5193+-4.2111 might be 1.0132x faster <geometric> 50.6923+-0.6903 50.3688+-0.6431 might be 1.0064x faster Baseline Mine Octane: encrypt 0.21850+-0.00274 0.21465+-0.00315 might be 1.0179x faster decrypt 3.79411+-0.00940 ? 3.81833+-0.06029 ? deltablue x2 0.19965+-0.00123 0.19844+-0.00469 earley 0.41199+-0.01020 ? 0.41320+-0.01449 ? boyer 6.39525+-0.20523 6.38330+-0.02843 navier-stokes x2 5.39196+-0.05769 5.35928+-0.05524 raytrace x2 1.28173+-0.03875 ? 1.30512+-0.03539 ? might be 1.0183x slower richards x2 0.10541+-0.00109 ? 0.10550+-0.00134 ? splay x2 0.45348+-0.00665 0.45232+-0.00369 regexp x2 24.34807+-0.26823 ? 24.62600+-0.22142 ? might be 1.0114x slower pdfjs x2 49.87285+-1.64077 49.85491+-1.47615 mandreel x2 53.63629+-0.32785 53.61327+-0.40509 gbemu x2 32.55159+-0.26847 ? 32.59965+-0.90422 ? closure 0.74999+-0.00839 0.74879+-0.00830 jquery 9.58344+-0.09196 ? 9.63366+-0.13441 ? box2d x2 11.94537+-0.17728 11.87965+-0.22833 zlib x2 435.17017+-3.79508 420.14256+-33.41801 might be 1.0358x faster typescript x2 846.98914+-8.87864 846.28363+-9.83530 <geometric> 6.69578+-0.01634 6.68258+-0.03175 might be 1.0020x faster Baseline Mine Kraken: ai-astar 99.702+-5.285 96.485+-1.010 might be 1.0333x faster audio-beat-detection 57.321+-1.535 56.848+-0.540 audio-dft 111.051+-4.873 ? 112.328+-3.147 ? might be 1.0115x slower audio-fft 45.854+-0.559 45.828+-0.313 audio-oscillator 57.817+-0.315 ? 58.579+-1.141 ? might be 1.0132x slower imaging-darkroom 73.362+-0.497 ? 73.854+-0.638 ? imaging-desaturate 67.525+-4.784 ? 68.002+-5.480 ? imaging-gaussian-blur 81.729+-0.658 ? 82.751+-6.423 ? might be 1.0125x slower json-parse-financial 49.312+-0.804 ? 49.369+-0.870 ? json-stringify-tinderbox 28.511+-0.412 ? 29.151+-1.621 ? might be 1.0225x slower stanford-crypto-aes 45.948+-0.389 ? 46.381+-0.767 ? stanford-crypto-ccm 50.644+-7.209 43.608+-5.651 might be 1.1613x faster stanford-crypto-pbkdf2 115.327+-4.544 114.793+-5.022 stanford-crypto-sha256-iterative 45.694+-0.294 45.435+-0.290 <arithmetic> 66.414+-1.172 65.958+-0.398 might be 1.0069x faster
Comment on attachment 274102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274102&action=review Thanks for your review. >> Source/JavaScriptCore/ChangeLog:21 >> + We need not to change any JIT part, because the JS code cannot alter the receiver without Reflect.set. > > JS code can have a different receiver and base through the use of ProxyObject. Yeah, I'll describe the precise text here. ProxyObject can alter the base values, but its put is never cached. To make safer, I'll explicitly disable caching in ProxyObject::put(...) similar to ProxyObject::getOwnPropertySlotCommon's slot.disableCaching(). >> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1012 >> + JSProxy* jsProxy = jsDynamicCast<JSProxy*>(thisValue); > > Is there a better way to determine if something is JSProxy than through jsDynamicCast? If JSProxy is never inherited, we can check this guard by looking classInfo(). And JSProxy should not be inherited (JSProxy is not following ordinary JSObject inheritance system, it redirects to the target instead of calling Base::xxx()), I'll note the comment in JSProxy.h. >> Source/JavaScriptCore/runtime/JSObject.cpp:514 >> + // 9.1.9.1-8 Perform ? Call(setter, Receiver, « V »). > > style: I would write this without the unicode characters. > Call(setter, Receiver, <V>) Fixed. >> Source/JavaScriptCore/tests/stress/proxy-set.js:514 >> + //assert(receiver === obj); > > I'm not sure why I added a commented out assert. Lets make it valid or remove it. Ah, right. It's valid. So I'll fix it. >> Source/JavaScriptCore/tests/stress/proxy-set.js:550 >> + //assert(receiver === obj); > > ditto Fixed. >> Source/JavaScriptCore/tests/stress/reflect-set-proxy-set.js:6 >> +function shouldThrow(func, message) { > > not used. Dropped. >> Source/JavaScriptCore/tests/stress/reflect-set-proxy-set.js:542 >> + //assert(receiver === obj); > > remove or make valid Made valid. >> Source/JavaScriptCore/tests/stress/reflect-set-proxy-set.js:578 >> + //assert(receiver === obj); > > ditto Fixed. >> Source/JavaScriptCore/tests/stress/reflect-set-receiver-proxy-set.js:6 >> +function shouldThrow(func, message) { > > not used Dropped. >> Source/JavaScriptCore/tests/stress/reflect-set-with-global-proxy.js:6 >> +function shouldThrow(func, message) { > > not used Dropped. >> Source/JavaScriptCore/tests/stress/reflect-set.js:11 >> error = e; > > I didn't read through every test case, but it would be nice to have > at one test for every exception that can be thrown from OrdinarySet. > It may seem like overkill now, but it will prevent regressions. Make sense. I'll add such test cases.
Comment on attachment 274102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274102&action=review >>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:1012 >>> + JSProxy* jsProxy = jsDynamicCast<JSProxy*>(thisValue); >> >> Is there a better way to determine if something is JSProxy than through jsDynamicCast? > > If JSProxy is never inherited, we can check this guard by looking classInfo(). > And JSProxy should not be inherited (JSProxy is not following ordinary JSObject inheritance system, it redirects to the target instead of calling Base::xxx()), I'll note the comment in JSProxy.h. After searching, I've found that WindowShell inherits JSProxy. But we can change this to strucuture()->isProxy().
Created attachment 274186 [details] Patch
Comment on attachment 274186 [details] Patch oops, accidentally uploaded with r?.
Committed r198270: <http://trac.webkit.org/changeset/198270>
I'm looking at fast/css/getPropertyValue-webkit-marquee.html failure.
Ah, not related. This failure occurs before this patch.