[webkit-reviews] review granted: [Bug 170007] [DFG][FTL] Efficiently execute number#toString() : [Attachment 319769] Patch

[bugzilla-daemon at webkit.org]

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.