Bug 16643 – number_object.cpp needs a bath

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

Created attachment 18146 [details]
Break out callAsFunction implmentations into static functions

 JavaScriptCore/kjs/number_object.cpp |  476 +++++++++++++++++-----------------
 1 files changed, 244 insertions(+), 232 deletions(-)

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

Created attachment 18148 [details]
More small cleanups to toPrecision

 JavaScriptCore/kjs/number_object.cpp |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

[Eric Seidel (no email)]

Created attachment 18149 [details]
More changes to make number code readable

 JavaScriptCore/kjs/number_object.cpp |   44 +++++++++++++++++----------------
 1 files changed, 23 insertions(+), 21 deletions(-)

[Eric Seidel (no email)]

I meant number_object.cpp not date_object!

[Sam Weinig]

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-.

[Sam Weinig]

Comment on attachment 18145 [details]
Apply wkstyle/astyle and fix placement of *

Needs a changelog.

[Sam Weinig]

Comment on attachment 18147 [details]
More small attempts to make number code readable

Looks good, but again needs a changelog. r=me

[Sam Weinig]

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??

[Sam Weinig]

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.

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Sam Weinig]

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.

[Eric Seidel (no email)]

I've landed the reviewed patches.  When sam decides on the last one I'll land or not land that.

[Sam Weinig]

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-

[Eric Seidel (no email)]

Sounds fine to me.