Bug 147607 – [INTL] Implement String.prototype.localeCompare in ECMA-402

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andy VanWagoner]

Implement ECMA-402 2.0 13.1.1 String.prototype.localeCompare (that [, locales [, options ]])
http://ecma-international.org/publications/standards/Ecma-402.htm

This definition supersedes the definition provided in ES2015, 21.1.3.10.

[Timothy Hatcher]

<rdar://problem/15455101>

[Radar WebKit Bug Importer]

<rdar://problem/22459074>

[Andy VanWagoner]

Created attachment 267691 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Andy VanWagoner]

Created attachment 267695 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Andy VanWagoner]

Created attachment 267700 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267700 [details]
Patch

Clearing flags on attachment: 267700

Committed r194328: <http://trac.webkit.org/changeset/194328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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>

[Ryan Haddad]

Reverted r194328 for reason:

This change appears to have caused failures in JSC tests

Committed r194332: <http://trac.webkit.org/changeset/194332>

[Andy VanWagoner]

Created attachment 267843 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267843 [details]
Patch

Clearing flags on attachment: 267843

Committed r194394: <http://trac.webkit.org/changeset/194394>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 277176 [details]
the patch

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Created attachment 277182 [details]
the patch

Fixes some DFG validation failures.

[Filip Pizlo]

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

[Filip Pizlo]

(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.