Bug 155313 – [ES6] Make ToPropertyDescriptor spec compliant

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 273598 [details]
patch

[Mark Lam]

Comment on attachment 273598 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        This isn't valid according to the spec and it user observable

typo: it ==> it's

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326
> +        if (vm.exception())
>              return false;

Can remove this since we have an exception check immediately after.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:334
> +        if (vm.exception())
>              return false;

Ditto.  Not needed.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:343
> +        if (vm.exception())
>              return false;

Ditto.  Not needed.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:351
> +        if (vm.exception())
>              return false;

Ditto.

[Mark Lam]

Comment on attachment 273598 [details]
patch

r=me assuming there's no perf implications to this.  Can you confirm with benchmark run please?

[Saam Barati]

(In reply to comment #3)
> Comment on attachment 273598 [details]
> patch
> 
> r=me assuming there's no perf implications to this.  Can you confirm with
> benchmark run please?

Perf is OK.

VMs tested:
"og" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r197945)
"change" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r197945)

Collected 8 samples per benchmark/VM, with 8 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to
get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                                                    og                      change                                      
Octane:
   encrypt                                   0.21076+-0.00311    ?     0.21264+-0.00068       ?
   decrypt                                   3.79795+-0.05058    ?     3.81588+-0.04735       ?
   deltablue                        x2       0.17981+-0.00319          0.17929+-0.00302       
   earley                                    0.38653+-0.02030          0.37536+-0.00203         might be 1.0298x faster
   boyer                                     6.59598+-0.04493          6.57618+-0.02533       
   navier-stokes                    x2       6.52300+-0.00486    ?     6.52359+-0.00630       ?
   raytrace                         x2       1.16464+-0.03107    ?     1.18363+-0.03186       ? might be 1.0163x slower
   richards                         x2       0.10962+-0.00227          0.10883+-0.00103       
   splay                            x2       0.46213+-0.00191    ?     0.46575+-0.00646       ?
   regexp                           x2      25.99609+-0.46590    ?    26.03778+-0.28351       ?
   pdfjs                            x2      49.85612+-0.56007    ?    50.28483+-0.91745       ?
   mandreel                         x2      56.94099+-0.18768    ?    57.21665+-0.31544       ?
   gbemu                            x2      34.34817+-0.24418    ?    34.42325+-0.22974       ?
   closure                                   0.75193+-0.00376          0.75027+-0.00271       
   jquery                                    9.99679+-0.06307          9.94433+-0.07370       
   box2d                            x2      12.82058+-0.09195    ?    12.86507+-0.07055       ?
   zlib                             x2     485.29024+-12.12829   ?   491.49192+-3.04964       ? might be 1.0128x slower
   typescript                       x2     830.33755+-11.79711   ?   832.41931+-5.72747       ?

   <geometric>                               6.86439+-0.02715    ?     6.88181+-0.02244       ? might be 1.0025x slower

                                                    og                      change                                      
Kraken:
   ai-astar                                  114.786+-1.527      ?     115.641+-2.524         ?
   audio-beat-detection                       56.605+-0.377             56.344+-0.212         
   audio-dft                                 126.449+-0.622      ?     127.194+-0.356         ?
   audio-fft                                  42.420+-0.123             42.287+-0.117         
   audio-oscillator                           62.574+-0.304      ?      62.855+-0.341         ?
   imaging-darkroom                           76.973+-0.328      ?      77.087+-0.636         ?
   imaging-desaturate                         59.357+-1.117      ?      59.705+-2.017         ?
   imaging-gaussian-blur                      81.656+-5.394      ?      85.990+-5.076         ? might be 1.0531x slower
   json-parse-financial                       48.904+-0.380      ?      48.940+-0.510         ?
   json-stringify-tinderbox                   30.908+-0.618      ?      31.099+-0.449         ?
   stanford-crypto-aes                        53.128+-0.448      ?      53.232+-0.402         ?
   stanford-crypto-ccm                        46.954+-1.822      ?      47.051+-1.395         ?
   stanford-crypto-pbkdf2                    133.163+-0.537      ?     134.064+-1.485         ?
   stanford-crypto-sha256-iterative           51.134+-0.258             51.034+-0.165         

   <arithmetic>                               70.358+-0.379      ?      70.894+-0.508         ? might be 1.0076x slower

                                                    og                      change                                      
Geomean of preferred means:
   <scaled-result>                           21.9764+-0.0880     ?     22.0878+-0.0856        ? might be 1.0051x slower

[Mark Lam]

Comment on attachment 273598 [details]
patch

Cancelling commit for now.  I presume you didn't see my other comments.  Can you address them please?

[Saam Barati]

Comment on attachment 273598 [details]
patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326
>>              return false;
> 
> Can remove this since we have an exception check immediately after.

This code is bad. It should really read:
```
JSValue v = description->get(..)
if (vm.exception())
    retur false;
desc.setEnumerable(...).
```

[Mark Lam]

Comment on attachment 273598 [details]
patch

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

>>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326
>>>              return false;
>> 
>> Can remove this since we have an exception check immediately after.
> 
> This code is bad. It should really read:
> ```
> JSValue v = description->get(..)
> if (vm.exception())
>     retur false;
> desc.setEnumerable(...).
> ```

Should description->hasProperty() return true if it throws an exception?

[Saam Barati]

(In reply to comment #7)
> Comment on attachment 273598 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273598&action=review
> 
> >>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326
> >>>              return false;
> >> 
> >> Can remove this since we have an exception check immediately after.
> > 
> > This code is bad. It should really read:
> > ```
> > JSValue v = description->get(..)
> > if (vm.exception())
> >     retur false;
> > desc.setEnumerable(...).
> > ```
> 
> Should description->hasProperty() return true if it throws an exception?

Never. It should return false.

[Mark Lam]

Comment on attachment 273598 [details]
patch

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

>>>>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326
>>>>>              return false;
>>>> 
>>>> Can remove this since we have an exception check immediately after.
>>> 
>>> This code is bad. It should really read:
>>> ```
>>> JSValue v = description->get(..)
>>> if (vm.exception())
>>>     retur false;
>>> desc.setEnumerable(...).
>>> ```
>> 
>> Should description->hasProperty() return true if it throws an exception?
> 
> Never. It should return false.

Sorry, I missed the point that the exception check after get is needed so that we don't call setEnumerable() (and friends below).

[Saam Barati]

Created attachment 273631 [details]
patch for landing

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/197960