340992 - JS Date() performance slow compared to IE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ashley Bischoff (blog at handcoding.com)]

To start things off, here's my BuildID: 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060530 BonEcho/2.0a3 ID:2006053003

I ran across an article that compares the performance of the Date() object in Firefox and IE (http://www.cybergrain.com/archives/2006/02/ajax_perf_javas.html). And, in a test of various Date-related operations over 10,000 iterations, Firefox was several times slower than IE. 

=========

The author links to his testcases (http://www.cybergrain.com/tech/snippets/dateperf.html), which I also tried on my own box, a 2.1 GHz Pentium M with 2 GB RAM running WinXP Pro. Here's an excerpt of how those turned out:

IE6 - Date() Constructor: 78 ms
Firefox - Date() Constructor: 360 ms

IE6 - getMonth: 15 ms
Firefox - getMonth: 360 ms

IE6 - getYear: 16 ms
Firefox - getYear 359 ms

(BTW, if you run the testcases yourself, you'll get an output like "360/62 (test 0) Constructor" -- the second number of that "360/62" pairing represents the time if an object literal is used to store date-related information; basically, you can ignore that second number.)

Anyhow, I'm generally very pleased with Firefox's JavaScript performance -- I just thought this may be an area for improvement.

Comment 1

[Bob Clary [:bc] (inactive)]

On a WinXP Pentium M 1.7Ghz

browser   Const- get  get   get  get get  get    get
          ructor Year Month Date Day Hour Minute Second
bone cho  441    431  430   430  431 431  420    10
minefield 480    470  431   430  431 421  440     0
shell     361    350  351   351  350 351  360    10
msie6     110     10   20    20   20  20   20    20
opera854   40     30   40    30   20  10   20    10

Why is getSecond so fast in Bone Cho/Minefield compared to the other methods?

Comment 2

[Feng Qian (Google)]

GProf on Linux showed most of time samples were spent on daylight savings adjustment, call chain: DaylightSavingTA -> PRMJ_DSTOffset -> PRMJ_basetime. 

Comment 3

[Feng Qian (Google)]

(In reply to comment #1)
> On a WinXP Pentium M 1.7Ghz
> 
> browser   Const- get  get   get  get get  get    get
>           ructor Year Month Date Day Hour Minute Second
> bone cho  441    431  430   430  431 431  420    10
> minefield 480    470  431   430  431 421  440     0
> shell     361    350  351   351  350 351  360    10
> msie6     110     10   20    20   20  20   20    20
> opera854   40     30   40    30   20  10   20    10
> 
> Why is getSecond so fast in Bone Cho/Minefield compared to the other methods?

JS date_getSeconds maps to date_getUTCSeconds which doesn't call UTC, I think that's reason why it is so fast.

> 

Comment 4

[Feng Qian (Google)]

I just confirmed the theory by changing get[FullYear, Date, ...] to getUTC[...], and performance is dramatically fast. 

16/33 (test 0) Constructor
7/4 (test 1) getUTCFullYear
9/4 (test 2) getUTCMonth

10/5 (test 3) getUTCDate
5/5 (test 4) getUTCDay
6/4 (test 5) getUTCHour
6/5 (test 6) getUTCMinute
6/4 (test 7) getUTCSecond

What's a fix? Probably we can cache adjusted result of UTC() in a date object.

Comment 5

[Feng Qian (Google)]

Attachment: cache local time in a Date object (initial version)

Use a reserved slot to cache local time computed from LocalTime call. The fix gives big speedup on the benchmark reported here.

8/4 (test 0) Constructor
7/5 (test 1) getYear
9/4 (test 2) getMonth

11/4 (test 3) getDate
6/6 (test 4) getDay
6/5 (test 5) getHour
7/5 (test 6) getMinute
6/4 (test 7) getSecond

Didn't run regression tests, so I don't know if it breaks something.

Comment 6

[Feng Qian (Google)]

(In reply to comment #5)
> Created an attachment (id=225357) [edit]
> cache local time in a Date object (initial version)
> 
> Use a reserved slot to cache local time computed from LocalTime call. The fix
> gives big speedup on the benchmark reported here.
> 
> 8/4 (test 0) Constructor
> 7/5 (test 1) getYear
> 9/4 (test 2) getMonth
> 
> 11/4 (test 3) getDate
> 6/6 (test 4) getDay
> 6/5 (test 5) getHour
> 7/5 (test 6) getMinute
> 6/4 (test 7) getSecond
> 
> Didn't run regression tests, so I don't know if it breaks something.
> 

I am not sure why constructor gets so fast. 

Comment 7

[Feng Qian (Google)]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=225357) [edit]
> > cache local time in a Date object (initial version)
> > 
> > Use a reserved slot to cache local time computed from LocalTime call. The fix
> > gives big speedup on the benchmark reported here.
> > 
> > 8/4 (test 0) Constructor
> > 7/5 (test 1) getYear
> > 9/4 (test 2) getMonth
> > 
> > 11/4 (test 3) getDate
> > 6/6 (test 4) getDay
> > 6/5 (test 5) getHour
> > 7/5 (test 6) getMinute
> > 6/4 (test 7) getSecond
> > 
> > Didn't run regression tests, so I don't know if it breaks something.
> > 
> 
> I am not sure why constructor gets so fast. 

I got it, I commented out code for testing constructor. Here is right number for constructor:

270/31 (test 0) Constructor

It is a lot slower than IE because of calling UTC(), which eventually calls PRMJ_basetime.

> 

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 225357 [details] [diff] [review]
cache local time in a Date object (initial version)

This patch as a whole looks fine. My only questions are more stylistic:

-- date_getProlog seems slightly misnamed now, IMO. I wonder if it should just be called GetUTCTime now.

-- Why did you use PRIVATE and 1 reserved slot instead of 2 reserved slots? IMO, once you've switched to using slots, it's better to be consistent (and we wouldn't have to call OBJ_SET_SLOT manually anymore).

-- Please look at the other declarations of JSClasses that have cached protos for how to indent (or not indent) them.

-- See the style guide at http://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style for most of the other comments that I'd have. I know it's not fun and doesn't seem critical, but it really helps with the legibility of the engine.

Comment 9

[Feng Qian (Google)]

(In reply to comment #8)
> (From update of attachment 225357 [details] [diff] [review] [edit])
> This patch as a whole looks fine. My only questions are more stylistic:
> 
> -- date_getProlog seems slightly misnamed now, IMO. I wonder if it should just
> be called GetUTCTime now.

GetUTCTime isn't a proper name here, date_getProlog returns the address of the slot, and is used for setting the time too. I think we can either live with the current name, or change it to something explicit, e.g., date_getUTCTimeSlotAddr.
What do you think?

> 
> -- Why did you use PRIVATE and 1 reserved slot instead of 2 reserved slots?
> IMO, once you've switched to using slots, it's better to be consistent (and we
> wouldn't have to call OBJ_SET_SLOT manually anymore).

I had two considerations, correct me if I was wrong:
1) PRIVATE slot is always there, so if I use 2 reserved slots, it will waste the space for the private slot;
2) PRIVATE slot always has data, and the reserved slot is used as a cache, it has valid data only when local time is used, in that sense, both are not the same. 

> -- Please look at the other declarations of JSClasses that have cached protos
> for how to indent (or not indent) them.
> 
> -- See the style guide at
> http://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style for most of the
> other comments that I'd have. I know it's not fun and doesn't seem critical,
> but it really helps with the legibility of the engine.

Thanks for reminding me coding style, I knew it is important. I will try my best to follow the rule.
 

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

> date_getUTCTimeSlotAddr.

Hmm, how about just date_getUTCTimeSlot.

> I had two considerations, correct me if I was wrong:
> 1) PRIVATE slot is always there, so if I use 2 reserved slots, it will waste
> the space for the private slot;

Currently, SpiderMonkey always allocates JS_INITIAL_NSLOTS, which is 5. If the class is marked as JSCLASS_HAS_PRIVATE, then the reserved slots start at index 4 and the private data goes in index 3. If there is no private data, then the reserved slots start at 3 instead (see JSSLOT_START in jsobj.h).

Comment 11

[Feng Qian (Google)]

(In reply to comment #10)
> > date_getUTCTimeSlotAddr.
> 
> Hmm, how about just date_getUTCTimeSlot.
> 
> > I had two considerations, correct me if I was wrong:
> > 1) PRIVATE slot is always there, so if I use 2 reserved slots, it will waste
> > the space for the private slot;
> 
> Currently, SpiderMonkey always allocates JS_INITIAL_NSLOTS, which is 5. If the
> class is marked as JSCLASS_HAS_PRIVATE, then the reserved slots start at index
> 4 and the private data goes in index 3. If there is no private data, then the
> reserved slots start at 3 instead (see JSSLOT_START in jsobj.h).

Ok, I can change it to use two reserved slots if you think it is necessary. Please let me know if you want me to do that.

> 

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> Ok, I can change it to use two reserved slots if you think it is necessary.
> Please let me know if you want me to do that.

Yeah, let's do this.

Comment 13

[Feng Qian (Google)]

Attachment: revised patch, using reserved slots for both UTC and local time

Switched to use reserved slots for both UTC and local time. Get rid of date_getProlog. This patch touches many places. I runned tests under js/tests/ecma/Date, js/tests/ecma_3/Date, and js1_5/Date. It seems fine.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 227326 [details] [diff] [review]
revised patch, using reserved slots for both UTC and local time

There are a couple of missing braces that I'll fix before checking this in. I'm assuming that much of the latter part of the patch was mainly mechanical.

Comment 15

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #14)
> (From update of attachment 227326 [details] [diff] [review] [edit])
> There are a couple of missing braces that I'll fix before checking this in. I'm
> assuming that much of the latter part of the patch was mainly mechanical.

Does that mean that the patch is checked in?

Comment 16

[Feng Qian (Google)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 227326 [details] [diff] [review] [edit] [edit])
> > There are a couple of missing braces that I'll fix before checking this in. I'm
> > assuming that much of the latter part of the patch was mainly mechanical.
> 
> Does that mean that the patch is checked in?

Not yet, can someone help me checkin this patch? 

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I just checked this in. Sorry for the delay.