Bug 155294 – [ES6] Reflect.set with receiver

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 273754 [details]
Patch

WIP part1

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 273969 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 274095 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 274102 [details]
Patch

[Yusuke Suzuki]

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.

[Saam Barati]

can you post performance numbers?

[Yusuke Suzuki]

(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.

[Saam Barati]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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().

[Yusuke Suzuki]

Created attachment 274186 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 274186 [details]
Patch

oops, accidentally uploaded with r?.

[Yusuke Suzuki]

Committed r198270: <http://trac.webkit.org/changeset/198270>

[Yusuke Suzuki]

I'm looking at fast/css/getPropertyValue-webkit-marquee.html failure.

[Yusuke Suzuki]

Ah, not related. This failure occurs before this patch.