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-

Eric Seidel (no email)
Reported 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).
Attachments
Apply wkstyle/astyle and fix placement of * (25.99 KB, patch)
2007-12-28 11:14 PST, Eric Seidel (no email)
sam: review+
Break out callAsFunction implmentations into static functions (16.59 KB, patch)
2007-12-28 11:14 PST, Eric Seidel (no email)
sam: review+
More small attempts to make number code readable (6.20 KB, patch)
2007-12-28 11:14 PST, Eric Seidel (no email)
sam: review+
More small cleanups to toPrecision (1.01 KB, patch)
2007-12-28 11:14 PST, Eric Seidel (no email)
sam: review+
More changes to make number code readable (3.10 KB, patch)
2007-12-28 11:14 PST, Eric Seidel (no email)
sam: review+
Move to modern multi-class function prototype system (8.86 KB, patch)
2007-12-28 17:30 PST, Eric Seidel (no email)
sam: review-
Eric Seidel (no email)
Comment 1 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(-)
Eric Seidel (no email)
Comment 2 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(-)
Eric Seidel (no email)
Comment 3 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(-)
Eric Seidel (no email)
Comment 4 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(-)
Eric Seidel (no email)
Comment 5 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(-)
Eric Seidel (no email)
Comment 6 2007-12-28 15:19:10 PST
I meant number_object.cpp not date_object!
Sam Weinig
Comment 7 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-.
Sam Weinig
Comment 8 2007-12-28 15:23:18 PST
Comment on attachment 18145 [details] Apply wkstyle/astyle and fix placement of * Needs a changelog.
Sam Weinig
Comment 9 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
Sam Weinig
Comment 10 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??
Sam Weinig
Comment 11 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.
Eric Seidel (no email)
Comment 12 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(-)
Eric Seidel (no email)
Comment 13 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.
Eric Seidel (no email)
Comment 14 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.
Eric Seidel (no email)
Comment 15 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.
Eric Seidel (no email)
Comment 16 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.
Sam Weinig
Comment 17 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.
Eric Seidel (no email)
Comment 18 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.
Sam Weinig
Comment 19 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-
Eric Seidel (no email)
Comment 20 2007-12-29 15:49:25 PST
Sounds fine to me.
Note You need to log in before you can comment on or make changes to this bug.