Consider this benchmark: var date = new Date("1/1/2007 1:11:11"); for (var i = 0; i < 1000000; ++i) date.getYear(); By subtracting out the empty loop time, I found that the time for one call to date.getYear is: JSC: 0.2 us SM: 1.2 us This difference adds up to a 3 ms difference on date-format-xparb.

SS calls the date functions over and over reusing the same dates, e.g. out of 51949 calls to YearFromTime, for 45949 the date is the same as the previous call. Using a braindead cache here gets 3ms on SS, will put together a patch with similar caches for other functions and more general perf work, which we seem to spend a lot of time on in date-format-*.

This patch speeds up the computation of DST offsets, modifies the DST cache to handle the pattern seen in date-format-tofte, alternating between two sets of dates several years apart, and caches the result of YearFromTime. Saves 10ms for me between date-format-tofte and date-format-xparb, out of a total time of 14ms in Date-related functions. Even with this, in microbenchmarks the basic Date functions are slow compared to JSC when there are many Date objects floating around. This could be improved by caching the year/month/day/etc. of the date in extra slots on the object. Probably only worth 2ms or so on SS.

(In reply to comment #2) > Even with this, in microbenchmarks the basic Date functions are slow compared > to JSC when there are many Date objects floating around. This could be > improved by caching the year/month/day/etc. of the date in extra slots on the > object. Probably only worth 2ms or so on SS. Thinking on this some more, if the date properties are at known consistent slots (possibly holes) then the getYear etc. methods could be inlined as in charAt/charCodeAt without needing to make a call, saving more time. There are I think about 50k calls this would apply to.

They aren't at known, consistent slots, because the methods/properties are all configurable. Still, you might be able to do some sort of if-as-expected-then-fastpath hack that would speed things up so long as Date.prototype isn't modified (and if you did you'd be back on a slower-but-still-correct path). Not certain what that'd look like, exactly.

(In reply to comment #4) > They aren't at known, consistent slots, because the methods/properties are all > configurable. Still, you might be able to do some sort of > if-as-expected-then-fastpath hack that would speed things up so long as > Date.prototype isn't modified (and if you did you'd be back on a > slower-but-still-correct path). Not certain what that'd look like, exactly. I don't think we'll have much trouble making it work using PICs for calls--shape check on Date.prototype, then fast path. It would be very similar to the PICs for callprop on string primitives, which we already have.

(In reply to comment #4) > They aren't at known, consistent slots, because the methods/properties are all > configurable. Still, you might be able to do some sort of > if-as-expected-then-fastpath hack that would speed things up so long as > Date.prototype isn't modified (and if you did you'd be back on a > slower-but-still-correct path). Not certain what that'd look like, exactly. The idea would be to add fixed, non-property slots to store the year/month etc. of any value of the Date class, in the same way as the LOCAL_TIME slot currently there. These would be filled all at the same time (it seems to me substantially cheaper to do it this way than filling things in separate calls, but haven't measured), either when the Date is created/modified or at the first lookup after such an event. At the call site, the IC would behave similar to what you describe: when a callsite to e.g. the particular getYear native is found, that callsite will be patched with guards such that if the native is getYear and the object is a Date then the LOCAL_YEAR slot is looked up directly (with a hole check if needed), otherwise getYear is called which will do the right thing. This is modelled on the access patterns in tofte and xparb, which create a total of 5000 dates (either unique Date objects or setTimes on a single object), but do 10 or so lookups on each date created.

Use reserved slots to cache a date's local (not UTC) year/month/date/day/hour/minute/second. On top of the previous optimizations this gets 1.5ms (11.5ms saved total), but the getters will be easily inlinable to avoid 50k calls in SS. For this microbenchmark: function foo() { var d = new Date(); for (var i = 0; i < 1000000; i++) d.getYear(); } foo(); JSC: 18ms V8: 36ms JM (old): 126ms JM (new): 32ms For this microbenchmark, which defeats simple caching: function foo() { var a = Array(); var start = (new Date()).getTime(); for (var i = 0; i < 100; i++) a[i] = new Date(start + (i * 1000 * 60 * 60 * 24 * 365)); for (var i = 0; i < 10000; i++) { for (var j = 0; j < 100; j++) a[j].getYear(); } } foo(); I get these times: JSC: 19ms V8: 1296ms (uses simple caching apparently) JM (old): 127ms JM (new): 37ms

Who should review this?

Comment on attachment 461450 [details] [diff] [review] inlinable date getters >diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp > /* > * Set UTC time to a given time and invalidate cached local time. > */ > static JSBool > SetUTCTime(JSContext *cx, JSObject *obj, jsdouble t, Value *vp = NULL) > { > JS_ASSERT(obj->getClass() == &js_DateClass); JS_ASSERT(obj->isDate()) while you're here. >+static void >+FillLocalTimes(JSContext *cx, JSObject *obj) >+{ >+ jsdouble utcTime = obj->getSlot(JSObject::JSSLOT_DATE_UTC_TIME).toNumber(); Put a JS_ASSERT(obj->isDate()) here to catch bad callers that might show up in the future. >+ /* Make sure there are slots to store the cached information. */ >+ if (!obj->dslots) >+ obj->growSlots(cx, JSObject::DATE_FIXED_RESERVED_SLOTS); This could fail, couldn't it? Looks to me like FillLocalTimes needs to be fallible. Pity we don't have custom-length objects (yet, perhaps). >+ if (!JSDOUBLE_IS_FINITE(utcTime)) { >+ for (size_t ind = JSObject::JSSLOT_DATE_UTC_TIME + 1; >+ ind < JSObject::DATE_FIXED_RESERVED_SLOTS; ind++) >+ obj->setSlot(ind, DoubleValue(utcTime)); >+ return; >+ } The for-loop here should be braced, because its head occupies more than one line. Please add a constant for the starting boundary and use it here (could do the ending boundary too for completeness, but not strictly necessary): something like JSSLOT_DATE_COMPONENTS_START_SLOT, maybe. >+ jsint year = (jsint) floor(localTime /(msPerDay*365.2425)) + 1970; I expect your constant-multiplication shortcut will lose precision (mega bonus points if you were to track down such a failing value and write a test for it, but not required). Do it per spec with two successive divisions, please. Or... >+ jsdouble yearStartTime = (jsdouble) TimeFromYear(year); >+ >+ jsint yearDays; >+ if (yearStartTime > localTime) { >+ year--; >+ yearStartTime -= (msPerDay * DaysInYear(year)); >+ yearDays = DaysInYear(year); >+ } else { >+ yearDays = DaysInYear(year); >+ jsdouble nextStart = yearStartTime + (msPerDay * yearDays); >+ if (nextStart <= localTime) { >+ year++; >+ yearStartTime = nextStart; >+ yearDays = DaysInYear(year); >+ } >+ } Is this code supposed to catch inaccuracies from floating point imprecision, including that caused by the collapsed divisions above? Please add a comment to that effect if you think this is the case (and disregard the order-of-operations quibble), maybe check with some other people to get some consensus on correctness as I doubt the original author, if trackable, is likely to remember. (The original code that did this should have had such a comment, but since you're touching it, it's your problem to deal with now. :-) ) >+ const jsint SECONDS_PER_DAY = 60 * 60 * 24; Might be worth moving this into a more central header file... >+ jsint day = yearSeconds / SECONDS_PER_DAY; >+ >+ jsint step = -1, next = 30; >+ jsint month; >+ >+#define SET_MONTH(N) do { month = N; goto monthDone; } while (0) >+ >+ if (day <= next) >+ SET_MONTH(0); >+ step = next; >+ next += ((yearDays == 366) ? 29 : 28); >+ if (day <= next) >+ SET_MONTH(1); >+ step = next; >+ if (day <= (next += 31)) >+ SET_MONTH(2); >+ step = next; >+ if (day <= (next += 30)) >+ SET_MONTH(3); >+ step = next; >+ if (day <= (next += 31)) >+ SET_MONTH(4); >+ step = next; >+ if (day <= (next += 30)) >+ SET_MONTH(5); >+ step = next; >+ if (day <= (next += 31)) >+ SET_MONTH(6); >+ step = next; >+ if (day <= (next += 31)) >+ SET_MONTH(7); >+ step = next; >+ if (day <= (next += 30)) >+ SET_MONTH(8); >+ step = next; >+ if (day <= (next += 31)) >+ SET_MONTH(9); >+ step = next; >+ if (day <= (next += 30)) >+ SET_MONTH(10); >+ step = next; >+ SET_MONTH(11); >+ >+#undef SET_MONTH The obscuring macro usage here seems gratuitous. Use a do-while(false) loop with non-macroized assignments/breaks. > date_getYear(JSContext *cx, uintN argc, Value *vp) > { >- return GetYear(cx, JS_FALSE, vp); >+ JSObject *obj = ComputeThisFromVp(cx, vp); >+ if (!GetAndCacheLocalTime(cx, obj, vp)) >+ return JS_FALSE; >+ >+ Value yearVal = obj->getSlot(JSObject::JSSLOT_DATE_LOCAL_YEAR); >+ if (yearVal.isInt32()) { >+ /* Follow ECMA-262 to the letter, contrary to IE JScript. */ >+ jsint year = yearVal.getInt32Ref() - 1900; >+ vp->setInt32(year); Please use toInt32(), no need for the ref version here. >- if (!GetAndCacheLocalTime(cx, ComputeThisFromVp(cx, vp), vp, &result)) >+ JSObject *obj = ComputeThisFromVp(cx, vp); >+ if (!GetAndCacheLocalTime(cx, obj, vp)) > return JS_FALSE; Tangential, but I'm not much of a fan of not error-checking ComputeThisFromVp, because it might report an error, and then GetAndCacheLocalTime (via InstanceOf) would report a second error atop it. Mind filing a bug on error-checking all ComputeThisFromVp calls in jsdate.cpp? >diff --git a/js/src/jsobj.h b/js/src/jsobj.h >+ /* >+ * Cached slots holding local properties of the date. >+ * These are undefined until the first actual lookup occurs. >+ */ > static const uint32 JSSLOT_DATE_LOCAL_TIME = JSSLOT_PRIVATE + 1; >+ static const uint32 JSSLOT_DATE_LOCAL_YEAR = JSSLOT_PRIVATE + 2; >+ static const uint32 JSSLOT_DATE_LOCAL_MONTH = JSSLOT_PRIVATE + 3; >+ static const uint32 JSSLOT_DATE_LOCAL_DATE = JSSLOT_PRIVATE + 4; >+ static const uint32 JSSLOT_DATE_LOCAL_DAY = JSSLOT_PRIVATE + 5; >+ static const uint32 JSSLOT_DATE_LOCAL_HOURS = JSSLOT_PRIVATE + 6; >+ static const uint32 JSSLOT_DATE_LOCAL_MINUTES = JSSLOT_PRIVATE + 7; >+ static const uint32 JSSLOT_DATE_LOCAL_SECONDS = JSSLOT_PRIVATE + 8; Please also explain that these slots are set to undefined to clear the cache when this date is modified. >diff --git a/js/src/prmjtime.cpp b/js/src/prmjtime.cpp The offset-caching changes look good. The rest of the changes may or may not be good (but: use of localtime seems suspect, see the other callers that try to use localtime_r when possible; you've removed the only uses of a few variables/methods and thus introduced warnings), but they seem unrelated and unnecessary for this bug. Can you split them into a separate patch/bug, please? (Ask someone else to review them, C date/time APIs are not my proficiency.) Looks broadly good, would just like to see these loose ends cleared up before giving a final stamp of approval.

Re: year computation, this code was adapted from YearFromTime (YearFromTime is not simply called because it makes and discards temporaries which FillLocalTimes needs later). I've added comments to both sections of code describing what's going on. The issue isn't floating point imprecision, but with using the average length of a year --- the computation will be off by several hours for the first ms of most years, regardless of the precision of the division. Re: localtime, I've changed this to use localtime_r as in other places in prmjtime.cpp. From the docs, for this purpose the only difference should be that localtime_r is thread safe (important though!). Using localtime instead of mktime is actually the biggest single perf gain from this patch (at least on OSX).

> static const uint32 DATE_FIXED_RESERVED_SLOTS = 9; How does this work? There are only 3 fslots. I haven't yet worked out if this subsumes bug 585866 or not.

There can be more reserved slots than fixed slots (er, well, at least things don't crash), the reserved slots should just determine where the first scripted properties start. I think this patch should get rid of all/most of the YearFromTimes you are seeing in 585866 --- MonthFromTime is still called from getUTCMonth (which SS never calls), but for getMonth the cached version is used which does YearFromTime only once.

(In reply to comment #12) > There can be more reserved slots than fixed slots (er, well, at least things > don't crash), the reserved slots should just determine where the first scripted > properties start. I guess the _FIXED_ is a misnomer then. > I think this patch should get rid of all/most of the YearFromTimes you are > seeing in 585866 --- MonthFromTime is still called from getUTCMonth (which SS > never calls), but for getMonth the cached version is used which does > YearFromTime only once. Ok. YearFromTime() is still called from DateFromTime() a lot. And the patch in bug 585866 is also a readability win, so probably still worth doing.

Nick: how about DATE_CLASS_RESERVED_SLOTS to avoid overloading the f-word? Same elsewhere (sorry for the overloading). /be

Comment on attachment 464491 [details] [diff] [review] patch fixing issues in comment 9 >diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp > Class js_DateClass = { > js_Date_str, > JSCLASS_HAS_RESERVED_SLOTS(JSObject::DATE_FIXED_RESERVED_SLOTS) | >- JSCLASS_HAS_CACHED_PROTO(JSProto_Date) | >- JSCLASS_FAST_CONSTRUCTOR, >+ JSCLASS_HAS_CACHED_PROTO(JSProto_Date), I suspect you didn't mean to do this (ditto for the un-fastifying bits later), please revert. :-) >+ if (!JSDOUBLE_IS_FINITE(utcTime)) { >+ for (size_t ind = JSObject::JSSLOT_DATE_COMPONENTS_START; >+ ind < JSObject::DATE_FIXED_RESERVED_SLOTS; ind++) { >+ obj->setSlot(ind, DoubleValue(utcTime)); >+ } Nit: if you have to wrap the head of a for loop, each part of the head should be on its own line. (I updated the style guide with this example, thanks for it. :-) ) >+ /* Adjust the year in case the approximation was wrong, as in YearFromTime. */ Ah, I should have seen this myself (maybe I even did but forgot the why, memory's playing tricks on me). > JSBool >-js_Date(JSContext *cx, uintN argc, Value *vp) >+js_Date(JSContext *cx, JSObject *obj, uintN argc, Value *argv, Value *rval) > { > /* Date called as function. */ >- if (!vp[1].isMagic(JS_FAST_CONSTRUCTOR)) >- return date_format(cx, NowAsMillis(), FORMATSPEC_FULL, vp); >+ if (!JS_IsConstructing(cx)) >+ return date_format(cx, NowAsMillis(), FORMATSPEC_FULL, rval); > > /* Date called as constructor. */ > jsdouble d; > if (argc == 0) { > d = NowAsMillis(); > } else if (argc == 1) { >- if (!vp[2].isString()) { >+ if (!argv[0].isString()) { > /* the argument is a millisecond number */ >- if (!ValueToNumber(cx, vp[2], &d)) >+ if (!ValueToNumber(cx, argv[0], &d)) > return JS_FALSE; > d = TIMECLIP(d); > } else { > /* the argument is a string; parse it. */ >- JSString *str = js_ValueToString(cx, vp[2]); >+ JSString *str = js_ValueToString(cx, argv[0]); > if (!str) > return JS_FALSE; >- vp[2].setString(str); >+ argv[0].setString(str); > > if (!date_parseString(str, &d, cx)) > d = js_NaN; > else > d = TIMECLIP(d); > } > } else { > jsdouble msec_time; >- if (!date_msecFromArgs(cx, argc, vp + 2, &msec_time)) >+ if (!date_msecFromArgs(cx, argc, argv, &msec_time)) > return JS_FALSE; > > if (JSDOUBLE_IS_FINITE(msec_time)) { > msec_time = UTC(msec_time, cx); > msec_time = TIMECLIP(msec_time); > } > d = msec_time; > } >- >- JSObject *obj = js_NewDateObjectMsec(cx, d); >- if (!obj) >- return JS_FALSE; >- vp->setObject(*obj); >- >- return JS_TRUE; >+ return SetUTCTime(cx, obj, d); > } > > JSObject * > js_InitDateClass(JSContext *cx, JSObject *obj) > { > /* set static LocalTZA */ > LocalTZA = -(PRMJ_LocalGMTDifference() * msPerSecond); >- JSObject *proto = js_InitClass(cx, obj, NULL, &js_DateClass, (Native) js_Date, MAXARGS, >+ JSObject *proto = js_InitClass(cx, obj, NULL, &js_DateClass, js_Date, MAXARGS, > NULL, date_methods, NULL, date_static_methods); ...the remainder of the probably unintended changes to be reverted. >diff --git a/js/src/jsobj.h b/js/src/jsobj.h >+ /* >+ * Cached slots holding local properties of the date. >+ * These are undefined until the first actual lookup occurs, >+ * and reset to undefined whenever the date's time is modified. >+ */ Run-on! Take a hatchet to that comma, and add an "are" before "reset" for proper parallel structure. >+ static const uint32 DATE_FIXED_RESERVED_SLOTS = 9; Brendan's suggestion for a better name sounds good to me. The slowness of localtime_r versus localtime on OS X is...unfortunate, but I (think? hope?) we care about being correct over being sometimes completely wrong but fast. I would approve of a semi-lightly-worded comment noting this unfortunate behavior, if you felt like adding one. :-) r=me with all those changes made.

(In reply to comment #15) > The slowness of localtime_r versus localtime on OS X is...unfortunate, but I > (think? hope?) we care about being correct over being sometimes completely > wrong but fast. I would approve of a semi-lightly-worded comment noting this > unfortunate behavior, if you felt like adding one. :-) Actually, the slowness is for mktime, not localtime_r (which is used by the most recent patch). I just microbenchmarked these three on my macbook: localtime_r is about the same as localtime, and more than 10x faster than mktime. Each call to mktime is about 3us (almost as much as a page fault).

(In reply to comment #16) > Actually, the slowness is for mktime, not localtime_r (which is used by the > most recent patch). I just microbenchmarked these three on my macbook: > localtime_r is about the same as localtime, and more than 10x faster than > mktime. Each call to mktime is about 3us (almost as much as a page fault). \o/

(In reply to comment #10) > Created attachment 464491 [details] [diff] [review] > patch fixing issues in comment 9 http://hg.mozilla.org/tracemonkey/rev/acb2dd3a0d5a This looks like 16ms for TM on AWFY, should be about the same when merged into JM.

(In reply to comment #18) > > This looks like 16ms for TM on AWFY, should be about the same when merged into > JM. Not bad for a "puny" win :)