| Summary: | [ES6] Make ToPropertyDescriptor spec compliant | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Saam Barati
2016-03-10 11:56:50 PST
Created attachment 273598 [details]
patch
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. Comment on attachment 273598 [details]
patch
r=me assuming there's no perf implications to this. Can you confirm with benchmark run please?
(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 Comment on attachment 273598 [details]
patch
Cancelling commit for now. I presume you didn't see my other comments. Can you address them please?
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(...). ``` 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? (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. 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). Created attachment 273631 [details]
patch for landing
landed in: http://trac.webkit.org/changeset/197960 |