WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
170007
[DFG][FTL] Efficiently execute number#toString()
https://bugs.webkit.org/show_bug.cgi?id=170007
Summary
[DFG][FTL] Efficiently execute number#toString()
Yusuke Suzuki
Reported
2017-03-23 07:36:39 PDT
[JSC] Add strength reduction for NumberToStringWithRadix
Attachments
Patch
(18.39 KB, patch)
2017-03-23 07:37 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(40.92 KB, patch)
2017-09-02 21:28 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(41.01 KB, patch)
2017-09-02 21:29 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(42.00 KB, patch)
2017-09-03 03:06 PDT
,
Yusuke Suzuki
keith_miller
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2017-03-23 07:37:00 PDT
Created
attachment 305193
[details]
Patch WIP
Yusuke Suzuki
Comment 2
2017-09-02 21:28:11 PDT
Created
attachment 319759
[details]
Patch
Yusuke Suzuki
Comment 3
2017-09-02 21:29:16 PDT
Created
attachment 319760
[details]
Patch
Yusuke Suzuki
Comment 4
2017-09-03 03:06:52 PDT
Created
attachment 319769
[details]
Patch
Darin Adler
Comment 5
2017-09-03 16:24:21 PDT
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 6
2017-09-04 14:07:00 PDT
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 7
2017-09-04 20:01:50 PDT
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
Comment 8
2017-09-04 20:03:17 PDT
Committed
r221601
: <
http://trac.webkit.org/changeset/221601
>
Yusuke Suzuki
Comment 9
2017-09-05 04:25:43 PDT
https://perf.webkit.org/v3/#/charts?since=1504003447301&paneList=((18-906
)) Wow! It improves Speedometer 2.0 React-Redux 12%.
Saam Barati
Comment 10
2017-09-14 15:49:19 PDT
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
Comment 11
2017-09-27 12:52:13 PDT
<
rdar://problem/34694176
>
Keith Miller
Comment 12
2018-01-26 14:26:01 PST
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
Comment 13
2018-01-27 01:46:10 PST
(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
Comment 14
2018-01-27 04:24:18 PST
(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
Comment 15
2018-01-27 04:24:54 PST
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
Comment 16
2018-01-27 04:48:06 PST
(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 17
2018-01-27 04:52:04 PST
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
Comment 18
2018-01-27 05:31:12 PST
(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 19
2018-02-06 11:48:56 PST
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
Comment 20
2018-02-06 23:59:15 PST
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug