Bug 85120 – Add fast path for radix == 10 to numberProtoFuncToString

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Benjamin Poulain]

Add fast patch for radix == 10 on numberProtoFuncToString

[Benjamin Poulain]

Created attachment 139328 [details]
Patch

[Benjamin Poulain]

In a follow up, I'll move toStringWithRadix() and toUStringWithRadix() to be a feature of UString.
And in a follow-follow-up, I'll move all that crap to a itoa file in WTF. It is silly UString::number() and String::number() differ so much.

But for now, I'd enjoy some careful eyes on this patch :-D

[Darin Adler]

Comment on attachment 139328 [details]
Patch

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

review- because I believe toUStringWithRadix will not work correctly for MIN_INT. Otherwise this patch seems great!

> Source/JavaScriptCore/ChangeLog:12
> +        This patch shortcuts the creation of a JSValue and use NumericStrings directly.

Should say “uses” instead of “use”.

> Source/JavaScriptCore/ChangeLog:19
> +        integers that do not fall in the two previous optimization gets 32% faster.

Should say “optimizations” instead of “optimization”.

> Source/JavaScriptCore/ChangeLog:23
> +        (JSC):

Please remove this bogus line from the change log.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:344
> +    LChar buf[1 + 32]; // Worst case is radix == 2, which gives us 32 positions + sign.

32 digits is better wording than 32 positions

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:351
> +        number = -number;

When you do this to MIN_INT, it just gives you MIN_INT again. I’m not sure the code below will work in that case. Did you test that edge case to see that it works correctly?

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:455
> +static int32_t extractRadixFromArgs(ExecState* exec)

I think you want this inlined so probably best to mark it inline.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:469
> +static EncodedJSValue integerValueToString(ExecState* exec, int32_t radix, int32_t value)

Do you want this inlined? I think it probably should be marked inline.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:483
> +        JSString* string;
> +        string = jsString(globalData, globalData->numericStrings.add(value));
> +        return JSValue::encode(string);

Should initialize the local variable named string on the same line it’s defined on. Or, better, just do it all in one expression:

    return JSValue::encode(jsString(globalData, globalData->numericStrings.add(value)));

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:507
> +        JSString* string = jsString(globalData, globalData->numericStrings.add(doubleValue));
> +        return JSValue::encode(string);

Not sure why you did this in two lines instead of one.

[Benjamin Poulain]

Created attachment 139373 [details]
Patch

[Benjamin Poulain]

> > Source/JavaScriptCore/runtime/NumberPrototype.cpp:351
> > +        number = -number;

This was actually working out of dumb luck.

The modulo always return positive number so "number % radix" was doing the right thing. I modified the patch to make it not rely on this behavior and make this code easier to read.

I also added some tests.

[WebKit Review Bot]

Comment on attachment 139373 [details]
Patch

Attachment 139373 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12557738

New failing tests:
fast/js/toString-number.html

[WebKit Review Bot]

Created attachment 139374 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Benjamin Poulain]

(In reply to comment #7)
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Looks like Chromium rounds differently on radix == 36. I'll need different result for Chromium.

[Benjamin Poulain]

Created attachment 139380 [details]
Patch

[Benjamin Poulain]

Comment on attachment 139380 [details]
Patch

Wrong bug id

[Benjamin Poulain]

Created attachment 139382 [details]
Patch

[Benjamin Poulain]

Committed r115655: <http://trac.webkit.org/changeset/115655>

[Ryosuke Niwa]

gcc build failure fixed in http://trac.webkit.org/changeset/115674.

[Peter Kasting]

Benjamin, can this test be written in such a way that it passes on Chromium?  Should Chromium be rebaselined to expect failure here ( :( )?

[Benjamin Poulain]

(In reply to comment #14)
> Benjamin, can this test be written in such a way that it passes on Chromium?  Should Chromium be rebaselined to expect failure here ( :( )?

I can rewrite the test to accept a rounding delta, but in my opinion it is a bad idea. We want to ensure the output is exactly what we expect.

Rebaseline Chromium with failure is a fair solution. The results from Chromium were not incorrect if I remember correctly.

Ideally, you could modify V8's Number.prototype.string to follow JSC. It is also the values you would get from Firefox (and from Opera to some extent).

[Peter Kasting]

If other engines are consistent, then making V8 match them seems like a reasonable idea.  Since you understand what's going on here better than I, would you mind writing something up at http://code.google.com/p/v8/issues/entry ?  I'll try and ensure the right folks take a look at it if so.