Summary: | [INTL] Change Intl prototypes to plain objects | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy VanWagoner <andy> | ||||||
Component: | JavaScriptCore | Assignee: | Andy VanWagoner <andy> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, commit-queue, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam, sukolsak | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Andy VanWagoner
2017-02-11 14:32:31 PST
Created attachment 301274 [details]
Patch
Comment on attachment 301274 [details] Patch Attachment 301274 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3148910 New failing tests: cdjs-tests.yaml/main.js.ftl-no-cjit cdjs-tests.yaml/main.js.ftl-eager-no-cjit Comment on attachment 301274 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301274&action=review r=me > Source/JavaScriptCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Could you add tests to ensure that the prototypes become plain objects correctly? (In reply to comment #4) > Comment on attachment 301274 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301274&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:6 > > + Reviewed by NOBODY (OOPS!). > > Could you add tests to ensure that the prototypes become plain objects > correctly? Do you have any suggestions on how to do this? The following are true before this change. shouldBe("Object.getPrototypeOf(Intl.Collator.prototype)", "Object.prototype"); shouldBe("Object.prototype.toString.call(Intl.Collator.prototype)", "'[object Object]'"); There are already tests in this patch that throw when trying to use the prototype object with compare, format, and resolvedOptions. (In reply to comment #3) > Comment on attachment 301274 [details] > Patch > > Attachment 301274 [details] did not pass jsc-ews (mac): > Output: http://webkit-queues.webkit.org/results/3148910 > > New failing tests: > cdjs-tests.yaml/main.js.ftl-no-cjit > cdjs-tests.yaml/main.js.ftl-eager-no-cjit How do I go about finding the specific cause of the performance regression? My gut feeling is the removal of the invalidated optimization in String#localeCompare. Created attachment 302762 [details]
Patch
Comment on attachment 302762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302762&action=review > Source/JavaScriptCore/builtins/StringPrototype.js:240 > + Normally I'd just have a simple variable, but I'm not confident I can construct a Collator yet, and I've only ever seen functions at the top level of a builtins file. var defaultCollator = new @Collator(); > LayoutTests/js/script-tests/intl-collator.js:213 > +shouldBe("Object.prototype.toString.call(Intl.Collator.prototype)", "'[object Object]'"); Here's the check to observe the new behavior. Only the constructor property would have failed before this patch, though. > LayoutTests/js/script-tests/intl-collator.js:235 > +shouldThrow("Intl.Collator.prototype.compare", "'TypeError: Intl.Collator.prototype.compare called on value that\\'s not an object initialized as a Collator'"); Here is the real change in behavior. Trying to get the bound compare function, or resolvedOptions on the prototype directly will throw now. > LayoutTests/js/script-tests/intl-numberformat.js:283 > +shouldBe("Intl.NumberFormat('fr').format(1234.567)", "'1\\xA0234,567'"); I changed this to an escaped code because I couldn't manage to get my text editor to insert the right space character. Comment on attachment 302762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302762&action=review r=me >> LayoutTests/js/script-tests/intl-numberformat.js:283 >> +shouldBe("Intl.NumberFormat('fr').format(1234.567)", "'1\\xA0234,567'"); > > I changed this to an escaped code because I couldn't manage to get my text editor to insert the right space character. One might say your editor broke the non-breaking spaces. (In reply to JF Bastien from comment #9) > Comment on attachment 302762 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302762&action=review > > r=me > > >> LayoutTests/js/script-tests/intl-numberformat.js:283 > >> +shouldBe("Intl.NumberFormat('fr').format(1234.567)", "'1\\xA0234,567'"); > > > > I changed this to an escaped code because I couldn't manage to get my text editor to insert the right space character. > > One might say your editor broke the non-breaking spaces. :) I'm not a committer, so if this is good to go, can you add the commit-queue+ flag? Comment on attachment 302762 [details]
Patch
cq+
Comment on attachment 302762 [details] Patch Clearing flags on attachment: 302762 Committed r215349: <http://trac.webkit.org/changeset/215349> All reviewed patches have been landed. Closing bug. |