Bug 16537 - Make date code faster by removing redundant calls
Summary: Make date code faster by removing redundant calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-12-20 10:42 PST by Eric Seidel (no email)
Modified: 2007-12-21 09:15 PST (History)
0 users

See Also:


Attachments
Make msFrom1970ToYear human-readable (2.34 KB, patch)
2007-12-20 10:43 PST, Eric Seidel (no email)
ggaren: review+
Details | Formatted Diff | Diff
Make msToDayInMonth slightly more readable and avoid recalculating msSince1970ToAbsoluteYear (5.47 KB, patch)
2007-12-20 10:43 PST, Eric Seidel (no email)
ggaren: review-
Details | Formatted Diff | Diff
Remove use of isInLeapYear to avoid calling msSince1970ToAbsoluteYear (1.18 KB, patch)
2007-12-20 10:43 PST, Eric Seidel (no email)
ggaren: review+
Details | Formatted Diff | Diff
Remove dayInYear call by changing msToDayInMonth to dayInMonthFromDayInYear (2.74 KB, patch)
2007-12-20 10:43 PST, Eric Seidel (no email)
ggaren: review+
Details | Formatted Diff | Diff
Remove more duplicate calls to dayInYear and getUTCOffset for further speedup (4.79 KB, patch)
2007-12-20 10:43 PST, Eric Seidel (no email)
ggaren: review+
Details | Formatted Diff | Diff
Final combined patch w/ ggaren's suggested fixes (10.99 KB, patch)
2007-12-20 11:57 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-12-20 10:42:33 PST
Make date code faster by removing redundant calls

This is just one step in the right direction for the date code.  We're still executing way more than we need to each run through msToGregorianDateTime (which is called for basically every date function, since we store the date in ms since 1970 and have to convert to a GregorianDateTime struct each time before we can do anything interesting with the data).
Comment 1 Eric Seidel (no email) 2007-12-20 10:43:36 PST
Created attachment 18007 [details]
Make msFrom1970ToYear human-readable

 JavaScriptCore/kjs/DateMath.cpp |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)
Comment 2 Eric Seidel (no email) 2007-12-20 10:43:38 PST
Created attachment 18008 [details]
Make msToDayInMonth slightly more readable and avoid recalculating msSince1970ToAbsoluteYear

 JavaScriptCore/kjs/DateMath.cpp |   90 +++++++++++++++++++--------------------
 1 files changed, 44 insertions(+), 46 deletions(-)
Comment 3 Eric Seidel (no email) 2007-12-20 10:43:40 PST
Created attachment 18009 [details]
Remove use of isInLeapYear to avoid calling msSince1970ToAbsoluteYear

 JavaScriptCore/kjs/DateMath.cpp |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)
Comment 4 Eric Seidel (no email) 2007-12-20 10:43:42 PST
Created attachment 18010 [details]
Remove dayInYear call by changing msToDayInMonth to dayInMonthFromDayInYear

 JavaScriptCore/kjs/DateMath.cpp |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)
Comment 5 Eric Seidel (no email) 2007-12-20 10:43:44 PST
Created attachment 18011 [details]
Remove more duplicate calls to dayInYear and getUTCOffset for further speedup

 JavaScriptCore/kjs/DateMath.cpp |   40 +++++++++++++++++++++-----------------
 1 files changed, 22 insertions(+), 18 deletions(-)
Comment 6 Geoffrey Garen 2007-12-20 10:49:37 PST
Comment on attachment 18007 [details]
Make msFrom1970ToYear human-readable

r=me
Comment 7 Geoffrey Garen 2007-12-20 11:01:06 PST
Comment on attachment 18008 [details]
Make msToDayInMonth slightly more readable and avoid recalculating msSince1970ToAbsoluteYear

-static inline int msToYear(double ms)
+static inline int msSince1970ToAbsoluteYear(double msSince1970)

I don't really agree that this change is cleanup.

In the date code, "ms" always means "msSince1970." Substituting "msSince1970" makes the code read awkwardly for me. Also, I think it's a mistake to put "msSince1970" in only select places -- that makes it seem as if there's a difference between "ms" functions and "msSince1970" functions, when there's not.

Perhaps it would be better just to add a comment at the top of the file stating that "ms" means "ms since 1970."

I'm also not sure what you mean by the term "AbsoluteYear." What is an absolute year? (/me imagines an ad for Absolut Vodka on the back of the New Yorker.)

+    static int minYear = std::min(msSince1970ToAbsoluteYear(getCurrentUTCTime()), maximumYearForDST()-27) ;

Need a space around the "-" here.

The checkMonth change looks good. I wish the function had a name that hinted that it changed it arguments, but I can't think of a good one.

Not sure what to do with this patch. I guess I'll r- for the comments above.
Comment 8 Geoffrey Garen 2007-12-20 11:01:45 PST
Comment on attachment 18009 [details]
Remove use of isInLeapYear to avoid calling msSince1970ToAbsoluteYear

r=me
Comment 9 Geoffrey Garen 2007-12-20 11:02:33 PST
Comment on attachment 18010 [details]
Remove dayInYear call by changing msToDayInMonth to dayInMonthFromDayInYear

r=me
Comment 10 Geoffrey Garen 2007-12-20 11:03:11 PST
Comment on attachment 18011 [details]
Remove more duplicate calls to dayInYear and getUTCOffset for further speedup

r=me
Comment 11 Geoffrey Garen 2007-12-20 11:04:00 PST
I think you should run the JavaScriptCore tests with your machine set to interesting locales (on both sides of the international date line) before landing these patches.
Comment 12 Eric Seidel (no email) 2007-12-20 11:57:49 PST
Created attachment 18012 [details]
Final combined patch w/ ggaren's suggested fixes


        Reviewed by Geoff.

        Small reworking of Date code for 4% speedup on Date tests (0.2% overall)
        http://bugs.webkit.org/show_bug.cgi?id=16537

        Make msToYear human-readable
        Make msToDayInMonth slightly more readable and avoid recalculating msToYear
        Remove use of isInLeapYear to avoid calling msToYear
        Remove dayInYear call by changing msToDayInMonth to dayInMonthFromDayInYear
        Remove more duplicate calls to dayInYear and getUTCOffset for further speedup

        * kjs/DateMath.cpp:
        (KJS::daysFrom1970ToYear):
        (KJS::msToYear):
        (KJS::monthFromDayInYear):
        (KJS::checkMonth):
        (KJS::dayInMonthFromDayInYear):
        (KJS::dateToDayInYear):
        (KJS::getDSTOffsetSimple):
        (KJS::getDSTOffset):
        (KJS::gregorianDateTimeToMS):
        (KJS::msToGregorianDateTime):
---
 JavaScriptCore/ChangeLog        |   25 +++++++
 JavaScriptCore/kjs/DateMath.cpp |  146 ++++++++++++++++++++-------------------
 2 files changed, 99 insertions(+), 72 deletions(-)
Comment 13 David Kilzer (:ddkilzer) 2007-12-21 09:15:53 PST
Fixed in r28913.