Bug 162887 – String.prototype.toLowerCase should be a DFG/FTL intrinsic

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

This is around a 7% speedup on ES6SampleBench/Basic. I also suspect it might help other things like Dromaeo.

[Saam Barati]

Created attachment 290563 [details]
WIP

This is the gist of the patch, but needs some cleanup and possibly some better rules for Clobberize. I think it's probably valid to CSE toLowerCase().

[Saam Barati]

Created attachment 290662 [details]
patch

[Saam Barati]

Created attachment 290666 [details]
patch

rebased

[JF Bastien]

Comment on attachment 290662 [details]
patch

What happens when code does: String.prototype.toLowerCase = function() { return this; };

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 290662 [details]
> patch
> 
> What happens when code does: String.prototype.toLowerCase = function() {
> return this; };

The intrinsic is tied to the primordial function we instantiate for this property, not the property path "String.prototype.toLowerCase".

[Filip Pizlo]

Comment on attachment 290666 [details]
patch

so awesome.

[Yusuke Suzuki]

Comment on attachment 290666 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290666&action=review

r=me too.

> Source/JavaScriptCore/ChangeLog:8
> +        This patch makes ToLowerCase an intrinsic in the DFG/FTL. On the fast

It is nice if we can handle the similar intrinsic in toUpperCase() in the separated patch.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1010
> +        break;

OK, target is already filtered with SpecString. So it does not clobber.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1011
> +    }

And we have a chance to emit LazyJSValue if the given child is constant-folded in StrengthReduction phase.

> Source/JavaScriptCore/dfg/DFGClobberize.h:1297
> +        return;

Nice.

> Source/WTF/wtf/text/StringImpl.h:685
> +    WTF_EXPORT_STRING_API Ref<StringImpl> convertToLowercaseWithoutLocaleStartingAtFailingIndex(unsigned);

Let's include some word indicating that this function is only effective for 8bit. Maybe, "8Bit" ?

[Saam Barati]

Comment on attachment 290666 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290666&action=review

>> Source/WTF/wtf/text/StringImpl.h:685
>> +    WTF_EXPORT_STRING_API Ref<StringImpl> convertToLowercaseWithoutLocaleStartingAtFailingIndex(unsigned);
> 
> Let's include some word indicating that this function is only effective for 8bit. Maybe, "8Bit" ?

Sounds good. I'll make it: convertToLowercaseWithoutLocaleStartingAtFailingIndex8Bit

[Saam Barati]

Created attachment 290676 [details]
patch for landing

[WebKit Commit Bot]

Attachment 290676 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 290689 [details]
patch for landing v2

Fix changelog.

[WebKit Commit Bot]

Comment on attachment 290689 [details]
patch for landing v2

Clearing flags on attachment: 290689

Committed r206804: <http://trac.webkit.org/changeset/206804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.