Bug 170007 – [DFG][FTL] Efficiently execute number#toString()

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Add strength reduction for NumberToStringWithRadix

[Yusuke Suzuki]

Created attachment 305193 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 319759 [details]
Patch

[Yusuke Suzuki]

Created attachment 319760 [details]
Patch

[Yusuke Suzuki]

Created attachment 319769 [details]
Patch

[Darin Adler]

Comment on attachment 319769 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGNode.h:698
> +        ASSERT(radix >= 2 && radix <= 36);

General WebKit style comment:

We write two asserts instead of one assert with an &&, that way we know which of the two failed.

[Keith Miller]

Comment on attachment 319769 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:9
> +        However, our IC only cares cells. If the base value is a number, it always goes to the slow path.

Typo: cares => cares about.

> Source/JavaScriptCore/ChangeLog:19
> +        conservatively use read(World)/write(Heap). But in reality, `number.toString` is well called with the constant

Maybe "`number.toString` is well called" could be "`number.toString` is mostly called"?

> Source/JavaScriptCore/ChangeLog:31
> +        number-to-string-with-radix-cse           43.8312+-1.3017     ^      7.4930+-0.5105        ^ definitely 5.8496x faster
> +        number-to-string-with-radix-10             7.2775+-0.5225     ^      2.1906+-0.1864        ^ definitely 3.3222x faster
> +        number-to-string-with-radix               39.7378+-1.4921     ^     16.6137+-0.7776        ^ definitely 2.3919x faster
> +        number-to-string-strength-reduction       94.9667+-2.7157     ^      9.3060+-0.7202        ^ definitely 10.2049x faster

Nice!

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1956
> +            if (radix >= 2 && radix <= 36) {

Nit: I prefer to have my range checking code as "if (2 <= radix && radix <= 36)" since that makes it slightly easier to read that radix is between 2 and 36.

>> Source/JavaScriptCore/dfg/DFGNode.h:698
>> +        ASSERT(radix >= 2 && radix <= 36);
> 
> General WebKit style comment:
> 
> We write two asserts instead of one assert with an &&, that way we know which of the two failed.

Interesting, I usually use one assert for range checking assertions since it feels like the assertions take up a lot of space otherwise. I also rarely care about which way the assertion fails since it could usually fail the other way just as easily.

[Yusuke Suzuki]

Comment on attachment 319769 [details]
Patch

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

Thanks for your reviews!

>> Source/JavaScriptCore/ChangeLog:9
>> +        However, our IC only cares cells. If the base value is a number, it always goes to the slow path.
> 
> Typo: cares => cares about.

Fixed.

>> Source/JavaScriptCore/ChangeLog:19
>> +        conservatively use read(World)/write(Heap). But in reality, `number.toString` is well called with the constant
> 
> Maybe "`number.toString` is well called" could be "`number.toString` is mostly called"?

Oops, fixed.

>> Source/JavaScriptCore/ChangeLog:31
>> +        number-to-string-strength-reduction       94.9667+-2.7157     ^      9.3060+-0.7202        ^ definitely 10.2049x faster
> 
> Nice!

:D

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1956
>> +            if (radix >= 2 && radix <= 36) {
> 
> Nit: I prefer to have my range checking code as "if (2 <= radix && radix <= 36)" since that makes it slightly easier to read that radix is between 2 and 36.

Looks very nice. Fixed.

>>> Source/JavaScriptCore/dfg/DFGNode.h:698
>>> +        ASSERT(radix >= 2 && radix <= 36);
>> 
>> General WebKit style comment:
>> 
>> We write two asserts instead of one assert with an &&, that way we know which of the two failed.
> 
> Interesting, I usually use one assert for range checking assertions since it feels like the assertions take up a lot of space otherwise. I also rarely care about which way the assertion fails since it could usually fail the other way just as easily.

I think `ASSERT(2 <= radix && radix <= 36);` is nice (a bit tweaked version of the assertion). This assert would like to check the radix value is in range of [2, 36]. So, this is readable I think.

[Yusuke Suzuki]

Committed r221601: <http://trac.webkit.org/changeset/221601>

[Yusuke Suzuki]

https://perf.webkit.org/v3/#/charts?since=1504003447301&paneList=((18-906)) Wow! It improves Speedometer 2.0 React-Redux 12%.

[Saam Barati]

This bug or https://bugs.webkit.org/show_bug.cgi?id=176222 may have regressed ARES-6 by 3%. Can you look into it?

[Radar WebKit Bug Importer]

<rdar://problem/34694176>

[Keith Miller]

Hmm, it looks like this breaks twitch.tv chat.

Do you have time to look into this Yusuke?

I think you can reproduce this by doing:
1. Navigate to a Twitch channel with numerous viewers and an active chat (e.g. 15,000+ viewers).
2. Spam scroll up and down at the bottom of the chat view and the layout will typically break within a minute.

[Yusuke Suzuki]

(In reply to Keith Miller from comment #12)
> Hmm, it looks like this breaks twitch.tv chat.
> 
> Do you have time to look into this Yusuke?
> 
> I think you can reproduce this by doing:
> 1. Navigate to a Twitch channel with numerous viewers and an active chat
> (e.g. 15,000+ viewers).
> 2. Spam scroll up and down at the bottom of the chat view and the layout
> will typically break within a minute.

I'll check it. I cannot reproduce it with Epiphany.
I'll open it in STP and check the behavior.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #13)
> (In reply to Keith Miller from comment #12)
> > Hmm, it looks like this breaks twitch.tv chat.
> > 
> > Do you have time to look into this Yusuke?
> > 
> > I think you can reproduce this by doing:
> > 1. Navigate to a Twitch channel with numerous viewers and an active chat
> > (e.g. 15,000+ viewers).
> > 2. Spam scroll up and down at the bottom of the chat view and the layout
> > will typically break within a minute.
> 
> I'll check it. I cannot reproduce it with Epiphany.
> I'll open it in STP and check the behavior.

I reproduced it with STP 47. I can't reproduce it with Debug build Minibrowser yet.

[Yusuke Suzuki]

O(In reply to Yusuke Suzuki from comment #14)
> (In reply to Yusuke Suzuki from comment #13)
> > (In reply to Keith Miller from comment #12)
> > > Hmm, it looks like this breaks twitch.tv chat.
> > > 
> > > Do you have time to look into this Yusuke?
> > > 
> > > I think you can reproduce this by doing:
> > > 1. Navigate to a Twitch channel with numerous viewers and an active chat
> > > (e.g. 15,000+ viewers).
> > > 2. Spam scroll up and down at the bottom of the chat view and the layout
> > > will typically break within a minute.
> > 
> > I'll check it. I cannot reproduce it with Epiphany.
> > I'll open it in STP and check the behavior.
> 
> I reproduced it with STP 47. I can't reproduce it with Debug build
> Minibrowser yet.

OK, I've reproduced it with Debug build Minibrowser too.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #15)
> O(In reply to Yusuke Suzuki from comment #14)
> > (In reply to Yusuke Suzuki from comment #13)
> > > (In reply to Keith Miller from comment #12)
> > > > Hmm, it looks like this breaks twitch.tv chat.
> > > > 
> > > > Do you have time to look into this Yusuke?
> > > > 
> > > > I think you can reproduce this by doing:
> > > > 1. Navigate to a Twitch channel with numerous viewers and an active chat
> > > > (e.g. 15,000+ viewers).
> > > > 2. Spam scroll up and down at the bottom of the chat view and the layout
> > > > will typically break within a minute.
> > > 
> > > I'll check it. I cannot reproduce it with Epiphany.
> > > I'll open it in STP and check the behavior.
> > 
> > I reproduced it with STP 47. I can't reproduce it with Debug build
> > Minibrowser yet.
> 
> OK, I've reproduced it with Debug build Minibrowser too.

OK, I've found the bug. I'll create the patch for this.

[Yusuke Suzuki]

Comment on attachment 319769 [details]
Patch

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

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:360
> +static String toStringWithRadixInternal(int32_t number, unsigned radix)

This function does not accept 0 correctly since this function is originally used if `(unsigned)number >= (unsigned)radix` condition met.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #17)
> Comment on attachment 319769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319769&action=review
> 
> > Source/JavaScriptCore/runtime/NumberPrototype.cpp:360
> > +static String toStringWithRadixInternal(int32_t number, unsigned radix)
> 
> This function does not accept 0 correctly since this function is originally
> used if `(unsigned)number >= (unsigned)radix` condition met.

Opened the issue and uploaded the patch. https://bugs.webkit.org/show_bug.cgi?id=182213

[Saam Barati]

Comment on attachment 319769 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:654
> +                            node->setOpAndDefaultFlags(ToString);

This makes this node MustGenerate even though it shouldn't be. I'll fix.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #19)
> Comment on attachment 319769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319769&action=review
> 
> > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:654
> > +                            node->setOpAndDefaultFlags(ToString);
> 
> This makes this node MustGenerate even though it shouldn't be. I'll fix.

Oops, nice.