[JSC] Add strength reduction for NumberToStringWithRadix
Created attachment 305193 [details] Patch WIP
Created attachment 319759 [details] Patch
Created attachment 319760 [details] Patch
Created attachment 319769 [details] Patch
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.
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.
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.
Committed r221601: <http://trac.webkit.org/changeset/221601>
https://perf.webkit.org/v3/#/charts?since=1504003447301&paneList=((18-906)) Wow! It improves Speedometer 2.0 React-Redux 12%.
This bug or https://bugs.webkit.org/show_bug.cgi?id=176222 may have regressed ARES-6 by 3%. Can you look into it?
<rdar://problem/34694176>
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.
(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.
(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.
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.
(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.
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.
(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
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.
(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.