[webkit-reviews] review granted: [Bug 170007] [DFG][FTL] Efficiently execute number#toString() : [Attachment 319769] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 4 14:07:00 PDT 2017
Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 170007: [DFG][FTL] Efficiently execute number#toString()
https://bugs.webkit.org/show_bug.cgi?id=170007
Attachment 319769: Patch
https://bugs.webkit.org/attachment.cgi?id=319769&action=review
--- Comment #6 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 319769
--> https://bugs.webkit.org/attachment.cgi?id=319769
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.
More information about the webkit-reviews
mailing list