Bug 147607

<rdar://problem/15455101>

<rdar://problem/22459074>

Created attachment 267691 [details] Patch

Comment on attachment 267691 [details] Patch Attachment 267691 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/580889 New failing tests: sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.9_String.prototype.localeCompare/S15.5.4.9_3.html js/kde/StringObject.html js/dom/string-prototype-properties.html

Created attachment 267692 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 267691 [details] Patch Attachment 267691 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/580893 New failing tests: sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.9_String.prototype.localeCompare/S15.5.4.9_3.html js/kde/StringObject.html js/dom/string-prototype-properties.html

Created attachment 267694 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Created attachment 267695 [details] Patch

Comment on attachment 267695 [details] Patch Attachment 267695 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/580999 New failing tests: js/dom/string-prototype-properties.html

Created attachment 267697 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 267695 [details] Patch Attachment 267695 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/581055 New failing tests: js/dom/string-prototype-properties.html

Created attachment 267698 [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

Comment on attachment 267695 [details] Patch Attachment 267695 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/581074 New failing tests: js/dom/string-prototype-properties.html

Created attachment 267699 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 267700 [details] Patch

Comment on attachment 267700 [details] Patch Clearing flags on attachment: 267700 Committed r194328: <http://trac.webkit.org/changeset/194328>

All reviewed patches have been landed. Closing bug.

The following JSC tests are failing after this change: ** The following JSC stress test failures have been introduced: cdjs-tests.yaml/main.js.default cdjs-tests.yaml/main.js.ftl cdjs-tests.yaml/main.js.ftl-eager-no-cjit cdjs-tests.yaml/main.js.ftl-no-cjit <https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20JSC%20%28Tests%29/builds/4406> <https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/7398> <https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/1101> <https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/943>

Reverted r194328 for reason: This change appears to have caused failures in JSC tests Committed r194332: <http://trac.webkit.org/changeset/194332>

Created attachment 267843 [details] Patch

Comment on attachment 267843 [details] Patch Clearing flags on attachment: 267843 Committed r194394: <http://trac.webkit.org/changeset/194394>

All reviewed patches have been landed. Closing bug.

This patch is a 2x slow-down on CDjs. Did you run any performance tests before landing this patch? We usually have a policy that nothing lands without performance evaluation, especially if it's in code that's important to benchmarks.

Comment on attachment 267843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267843&action=review > Source/JavaScriptCore/ChangeLog:11 > + Add localeCompare in builtin JavaScript that delegates comparing to Intl.Collator. > + Keep existing native implementation for use if INTL flag is disabled. > + For the common case where no locale or options are specified, avoid creating > + a new collator and just use the prototype which is initialized with the defaults. Your changelog implies that you have not run performance, and yet this patch affects the behavior and performance of a critical piece of VM functionality. Please make sure that you run comprehensive performance tests whenever making changes to important things in the VM. It's customary to note in the changelog which benchmarks were run and what the results were. > Source/JavaScriptCore/builtins/StringPrototype.js:28 > +function localeCompare(that/*, locales, options */) Please make "locales" and "options" be actual arguments to this function. > Source/JavaScriptCore/builtins/StringPrototype.js:40 > + if (this === null) > + throw new @TypeError("String.prototype.localeCompare requires that |this| not be null"); > + > + if (this === undefined) > + throw new @TypeError("String.prototype.localeCompare requires that |this| not be undefined"); This should be one branch rather than two. For example: // Catch both null and undefined. if (this == null) { if (this === null) throw new @TypeError("String.prototype.localeCompare requires that |this| not be null"); throw new @TypeError("String.prototype.localeCompare requires that |this| not be undefined"); } > Source/JavaScriptCore/builtins/StringPrototype.js:51 > + if (arguments[1] === undefined && arguments[2] === undefined) It's really bad that this is using the "arguments" object. Please instead use the actual "locales" and "options" arguments by name. That avoids having to allocate the arguments object for no good reason. > Source/JavaScriptCore/builtins/StringPrototype.js:52 > + return @Collator.prototype.compare(thisString, thatString); This is probably incorrect behavior. The spec probably means the collator's original prototype, not the prototype it happens to have now. > Source/JavaScriptCore/builtins/StringPrototype.js:59 > + // 6. Let collator be Construct(%Collator%, «locales, options»). > + // 7. ReturnIfAbrupt(collator). > + var collator = new @Collator(arguments[1], arguments[2]); > + > + // 8. Return CompareStrings(collator, S, That). > + return collator.compare(thisString, thatString); It's really bad that you're allocating a new Collator object every time. This suggest that this should never have been a builtin. It should be a native function instead.

Rolled out in http://trac.webkit.org/changeset/198171 because of huge performance regressions. I think that these behavioral changes can probably be implemented without performance regressions. I'm happy to help with this, whenever someone decides to land this again.

I'll look at relanding this after I'm done with some other regressions...

Created attachment 277170 [details] work in progress This is a combination of Andy's patch and some work to try to reduce the cost of the localeCompare() builtin. I think I know how to do it.

Created attachment 277176 [details] the patch

Comment on attachment 277176 [details] the patch Attachment 277176 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1210666 New failing tests: js/regress/locale-compare.html

Created attachment 277181 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 277182 [details] the patch Fixes some DFG validation failures.

Landed in http://trac.webkit.org/changeset/199967

(In reply to comment #24) > Comment on attachment 267843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267843&action=review > > > Source/JavaScriptCore/ChangeLog:11 > > + Add localeCompare in builtin JavaScript that delegates comparing to Intl.Collator. > > + Keep existing native implementation for use if INTL flag is disabled. > > + For the common case where no locale or options are specified, avoid creating > > + a new collator and just use the prototype which is initialized with the defaults. > > Your changelog implies that you have not run performance, and yet this patch > affects the behavior and performance of a critical piece of VM functionality. > > Please make sure that you run comprehensive performance tests whenever > making changes to important things in the VM. It's customary to note in the > changelog which benchmarks were run and what the results were. > > > Source/JavaScriptCore/builtins/StringPrototype.js:28 > > +function localeCompare(that/*, locales, options */) > > Please make "locales" and "options" be actual arguments to this function. Turns out we probably don't want to do that, at least not without more builtins parsing support to be able to specify that the .length property reports "1" even though we have three arguments. > > > Source/JavaScriptCore/builtins/StringPrototype.js:40 > > + if (this === null) > > + throw new @TypeError("String.prototype.localeCompare requires that |this| not be null"); > > + > > + if (this === undefined) > > + throw new @TypeError("String.prototype.localeCompare requires that |this| not be undefined"); > > This should be one branch rather than two. For example: > > // Catch both null and undefined. > if (this == null) { > if (this === null) > throw new @TypeError("String.prototype.localeCompare requires that > |this| not be null"); > throw new @TypeError("String.prototype.localeCompare requires that > |this| not be undefined"); > } This is still a good idea, but I haven't done it yet. > > > Source/JavaScriptCore/builtins/StringPrototype.js:51 > > + if (arguments[1] === undefined && arguments[2] === undefined) > > It's really bad that this is using the "arguments" object. Please instead > use the actual "locales" and "options" arguments by name. That avoids > having to allocate the arguments object for no good reason. I fixed some FTL bugs that were preventing escape analysis from removing the allocation. But I think that in future, we should find a way to write such code without using 'arguments', since 'arguments' object optimizations are gated on a lot of stuff. > > > Source/JavaScriptCore/builtins/StringPrototype.js:52 > > + return @Collator.prototype.compare(thisString, thatString); > > This is probably incorrect behavior. The spec probably means the collator's > original prototype, not the prototype it happens to have now. > > > Source/JavaScriptCore/builtins/StringPrototype.js:59 > > + // 6. Let collator be Construct(%Collator%, «locales, options»). > > + // 7. ReturnIfAbrupt(collator). > > + var collator = new @Collator(arguments[1], arguments[2]); > > + > > + // 8. Return CompareStrings(collator, S, That). > > + return collator.compare(thisString, thatString); > > It's really bad that you're allocating a new Collator object every time. > This suggest that this should never have been a builtin. It should be a > native function instead. I don't know what to do about this yet. I think that this has to be a builtin so long as the "Collator.prototype.compare()" call is there, since this needs a lot of caching to be fast. I still want to figure out how to avoid the @Collator allocation on this path, but I don't have a benchmark to measure that path yet.