Bug 16643

Summary: number_object.cpp needs a bath
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on:    
Bug Blocks: 16640    
Attachments:
Description Flags
Apply wkstyle/astyle and fix placement of *
sam: review+
Break out callAsFunction implmentations into static functions
sam: review+
More small attempts to make number code readable
sam: review+
More small cleanups to toPrecision
sam: review+
More changes to make number code readable
sam: review+
Move to modern multi-class function prototype system sam: review-

Description Eric Seidel (no email) 2007-12-28 11:12:36 PST
date_object.cpp needs a bath

I went to fix toFixed and toPrecision this evening, but before I can do so number_object.cpp needs to be made readable.

That's what these patches do (I'm happy to attach them in whitespace ignoring form as well).
Comment 1 Eric Seidel (no email) 2007-12-28 11:14:44 PST
Created attachment 18145 [details]
Apply wkstyle/astyle and fix placement of *

 JavaScriptCore/kjs/number_object.cpp |  695 +++++++++++++++++-----------------
 JavaScriptCore/kjs/object.cpp        |    3 +-
 2 files changed, 339 insertions(+), 359 deletions(-)
Comment 2 Eric Seidel (no email) 2007-12-28 11:14:47 PST
Created attachment 18146 [details]
Break out callAsFunction implmentations into static functions

 JavaScriptCore/kjs/number_object.cpp |  476 +++++++++++++++++-----------------
 1 files changed, 244 insertions(+), 232 deletions(-)
Comment 3 Eric Seidel (no email) 2007-12-28 11:14:51 PST
Created attachment 18147 [details]
More small attempts to make number code readable

 JavaScriptCore/kjs/number_object.cpp |   98 ++++++++++++++++++----------------
 1 files changed, 51 insertions(+), 47 deletions(-)
Comment 4 Eric Seidel (no email) 2007-12-28 11:14:53 PST
Created attachment 18148 [details]
More small cleanups to toPrecision

 JavaScriptCore/kjs/number_object.cpp |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Comment 5 Eric Seidel (no email) 2007-12-28 11:14:55 PST
Created attachment 18149 [details]
More changes to make number code readable

 JavaScriptCore/kjs/number_object.cpp |   44 +++++++++++++++++----------------
 1 files changed, 23 insertions(+), 21 deletions(-)
Comment 6 Eric Seidel (no email) 2007-12-28 15:19:10 PST
I meant number_object.cpp not date_object!
Comment 7 Sam Weinig 2007-12-28 15:23:07 PST
Comment on attachment 18146 [details]
Break out callAsFunction implmentations into static functions

It seems odd to me to use static functions here and not follow the lead of all the other classes and put each function in it's own class with its own callAsFunction. r-.
Comment 8 Sam Weinig 2007-12-28 15:23:18 PST
Comment on attachment 18145 [details]
Apply wkstyle/astyle and fix placement of *

Needs a changelog.
Comment 9 Sam Weinig 2007-12-28 15:25:35 PST
Comment on attachment 18147 [details]
More small attempts to make number code readable

Looks good, but again needs a changelog. r=me
Comment 10 Sam Weinig 2007-12-28 15:30:03 PST
Comment on attachment 18149 [details]
More changes to make number code readable

Looks good.  I think this line should be a FIXME.

+    if (decimalPoint == 999) // ? 9999 is the magical "result is Inf or NaN" value.  what's 999??
Comment 11 Sam Weinig 2007-12-28 15:31:00 PST
Comment on attachment 18146 [details]
Break out callAsFunction implmentations into static functions

After talking with Eric on IRC, he said he would provide a follow up patch to convert the static functions into their own classes to avoid the indirection.
Comment 12 Eric Seidel (no email) 2007-12-28 17:30:54 PST
Created attachment 18162 [details]
Move to modern multi-class function prototype system

 JavaScriptCore/kjs/JSGlobalObject.cpp |    2 +-
 JavaScriptCore/kjs/number_object.cpp  |   79 ++++++++++++++++-----------------
 JavaScriptCore/kjs/number_object.h    |   20 ++++-----
 3 files changed, 49 insertions(+), 52 deletions(-)
Comment 13 Eric Seidel (no email) 2007-12-28 17:32:01 PST
Comment on attachment 18162 [details]
Move to modern multi-class function prototype system

Sam should look at this.  Particularly the use of the "length" parameter in the prototype constructors.  I'm not really sure I did that part right.
Comment 14 Eric Seidel (no email) 2007-12-28 17:42:17 PST
SunSpider reports that all of the patches add up to a slowdown:

** TOTAL **:           *1.004x as slow*  3875.1ms +/- 0.2%   3889.5ms +/- 0.2%     significant

I've not tested individually yet to see which patch(s) cause a slowdown.
Comment 15 Eric Seidel (no email) 2007-12-28 17:47:06 PST
The slowdown seems to come entirely from the final patch.  the "multi-class" prototype system.

** TOTAL **:           -                 3875.1ms +/- 0.2%   3871.7ms +/- 0.2%

is the number w/o that patch.
Comment 16 Eric Seidel (no email) 2007-12-28 17:50:56 PST
Running sunspider again, shows the total patch as a wash:
** TOTAL **:           -                 3875.1ms +/- 0.2%   3878.3ms +/- 0.2% 

So I think this is OK, even with the final "many class" patch.
Comment 17 Sam Weinig 2007-12-28 19:07:47 PST
Comment on attachment 18162 [details]
Move to modern multi-class function prototype system

There are two things that worry me about this patch.  The first is that a different functionPrototype object can be set since you are no longer passing in d()->functionPrototype but using the one off the execState (note, these may be the same thing).  I am not sure this is an issue in practice, but it is a concern.  The second issue is that by using the macro to make the class declarations for the functions, an unused create function is also being created.  I am going to think about these two issues a little and do a more conclusive review later on tonight.
Comment 18 Eric Seidel (no email) 2007-12-28 20:20:06 PST
I've landed the reviewed patches.  When sam decides on the last one I'll land or not land that.
Comment 19 Sam Weinig 2007-12-29 12:15:16 PST
Comment on attachment 18162 [details]
Move to modern multi-class function prototype system

I don't see any point in landing this change if it is not a performance improvement.  It will just add to the code size for no particular progression.  r-
Comment 20 Eric Seidel (no email) 2007-12-29 15:49:25 PST
Sounds fine to me.