| Summary: | [JSC] Computed function properties compute their keys twice | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||
| Component: | JavaScriptCore | Assignee: | Ross Kirsling <ross.kirsling> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=170934 | ||||||||
| Attachments: |
|
||||||||
Created attachment 390185 [details]
Patch
Comment on attachment 390185 [details]
Patch
The current approach is very simple but incurs a ToPropertyKey instruction even for non-function computed properties.
I suppose it would be better to split emitSetFunctionNameIfNeeded into needsSetFunctionName and emitSetFunctionName so that this can be avoided.
Created attachment 390291 [details]
Patch
Ping? Comment on attachment 390291 [details]
Patch
r=me.
Comment on attachment 390291 [details] Patch Clearing flags on attachment: 390291 Committed r256846: <https://trac.webkit.org/changeset/256846> All reviewed patches have been landed. Closing bug. |
Basic repro: ``` let count1 = 0; let key1 = { toString() { count1++; return 'foo'; } }; ({ [key1]: 'bar' }); let count2 = 0; let key2 = { toString() { count2++; return 'foo'; } }; ({ [key2]() { return 'bar'; } }); print(count1, count2); ``` λ eshost -s test.js #### ch, sm, v8, xs 1 1 #### jsc 1 2 Notes: - This also reproes if: - we write the function property as `[key]: function () { return 'bar'; }` or `[key]: () => 'bar'` instead - we set toString to null and use valueOf instead - Relevant test262 failures: https://github.com/tc39/test262/blob/master/test/language/computed-property-names/to-name-side-effects/class.js https://github.com/tc39/test262/blob/master/test/language/computed-property-names/to-name-side-effects/numbers-class.js https://github.com/tc39/test262/blob/master/test/language/computed-property-names/to-name-side-effects/object.js - Bug 170934 seems related but separate.