Bug 96330

Summary: Add String::numberToStringFixedWidth()
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: Web Template FrameworkAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96264    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
buildbot: commit-queue-
Patch
none
Patch none

Description Patrick R. Gansterer 2012-09-10 15:15:39 PDT
Add String::numberToFixedPrecisionString()
Comment 1 Patrick R. Gansterer 2012-09-10 15:26:36 PDT
Created attachment 163230 [details]
Patch
Comment 2 Patrick R. Gansterer 2012-09-14 14:15:21 PDT
Created attachment 164225 [details]
Patch
Comment 3 Benjamin Poulain 2012-09-14 14:26:17 PDT
Please benchmark JSC's NumberPrototype.

It is important code. From a similar change I did in String, I expect this to have visible perf improvement.
Comment 4 Patrick R. Gansterer 2012-09-14 14:30:02 PDT
(In reply to comment #3)
> Please benchmark JSC's NumberPrototype.
> 
> It is important code. From a similar change I did in String, I expect this to have visible perf improvement.

IMHO it won't have any visible perf improvement until i remove the strlen() calls in bug 96132, but I'll check...
Comment 5 Benjamin Poulain 2012-09-14 14:46:14 PDT
> IMHO it won't have any visible perf improvement until i remove the strlen() calls in bug 96132, but I'll check...

You make two important changes for performance:
1) remove one branch
2) remove one parameter

It does not look like much, but that code is in JSC's runtime. This kind of code is frequently exercised in benchmarks.
Comment 6 Build Bot 2012-09-14 16:47:10 PDT
Comment on attachment 164225 [details]
Patch

Attachment 164225 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13855598
Comment 7 Patrick R. Gansterer 2012-09-18 13:54:44 PDT
Created attachment 164616 [details]
Patch
Comment 8 Patrick R. Gansterer 2012-09-18 14:00:00 PDT
(In reply to comment #5)
> > IMHO it won't have any visible perf improvement until i remove the strlen() calls in bug 96132, but I'll check...
> 
> You make two important changes for performance:
> 1) remove one branch
> 2) remove one parameter
> 
> It does not look like much, but that code is in JSC's runtime. This kind of code is frequently exercised in benchmarks.

Changing the NumberPrototype.cpp to use the String::number() showed a (small) performance regression in a micro micro benchmark. I don't think this will be measurable in usual benchmark suites, but I removed the change from this patch. I will improve the String::number() function in a next step (e.g. bug 96132) first, before changing NumberPrototype. But I still see value in this patch, since it introduces the final API already.
Comment 9 Patrick R. Gansterer 2012-09-18 14:04:05 PDT
Created attachment 164618 [details]
Patch
Comment 10 Build Bot 2012-09-18 14:42:32 PDT
Comment on attachment 164618 [details]
Patch

Attachment 164618 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13892323
Comment 11 Patrick R. Gansterer 2012-09-19 04:35:13 PDT
Created attachment 164710 [details]
Patch
Comment 12 Patrick R. Gansterer 2012-09-19 13:40:54 PDT
Created attachment 164772 [details]
Patch
Comment 13 WebKit Review Bot 2012-09-20 14:03:34 PDT
Comment on attachment 164772 [details]
Patch

Clearing flags on attachment: 164772

Committed r129165: <http://trac.webkit.org/changeset/129165>
Comment 14 WebKit Review Bot 2012-09-20 14:03:39 PDT
All reviewed patches have been landed.  Closing bug.