Bug 85120 - Add fast path for radix == 10 to numberProtoFuncToString
Summary: Add fast path for radix == 10 to numberProtoFuncToString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-27 19:27 PDT by Benjamin Poulain
Modified: 2012-07-31 23:41 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.94 KB, patch)
2012-04-27 20:20 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (31.73 KB, patch)
2012-04-28 15:09 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.00 MB, application/zip)
2012-04-28 15:54 PDT, WebKit Review Bot
no flags Details
Patch (2.09 KB, patch)
2012-04-28 17:41 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (32.43 KB, patch)
2012-04-28 17:50 PDT, Benjamin Poulain
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-04-27 19:27:52 PDT
Add fast patch for radix == 10 on numberProtoFuncToString
Comment 1 Benjamin Poulain 2012-04-27 20:20:00 PDT
Created attachment 139328 [details]
Patch
Comment 2 Benjamin Poulain 2012-04-27 20:21:52 PDT
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
Comment 3 Darin Adler 2012-04-28 08:37:59 PDT
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.
Comment 4 Benjamin Poulain 2012-04-28 15:09:25 PDT
Created attachment 139373 [details]
Patch
Comment 5 Benjamin Poulain 2012-04-28 15:13:10 PDT
> > 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.
Comment 6 WebKit Review Bot 2012-04-28 15:53:57 PDT
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
Comment 7 WebKit Review Bot 2012-04-28 15:54:02 PDT
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
Comment 8 Benjamin Poulain 2012-04-28 15:59:08 PDT
(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.
Comment 9 Benjamin Poulain 2012-04-28 17:41:09 PDT
Created attachment 139380 [details]
Patch
Comment 10 Benjamin Poulain 2012-04-28 17:42:05 PDT
Comment on attachment 139380 [details]
Patch

Wrong bug id
Comment 11 Benjamin Poulain 2012-04-28 17:50:21 PDT
Created attachment 139382 [details]
Patch
Comment 12 Benjamin Poulain 2012-04-30 12:01:45 PDT
Committed r115655: <http://trac.webkit.org/changeset/115655>
Comment 13 Ryosuke Niwa 2012-04-30 14:52:01 PDT
gcc build failure fixed in http://trac.webkit.org/changeset/115674.
Comment 14 Peter Kasting 2012-07-31 17:34:00 PDT
Benjamin, can this test be written in such a way that it passes on Chromium?  Should Chromium be rebaselined to expect failure here ( :( )?
Comment 15 Benjamin Poulain 2012-07-31 23:22:23 PDT
(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).
Comment 16 Peter Kasting 2012-07-31 23:41:06 PDT
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.