849785 - DST regressed sunspider date tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Our SS score on AWFY regressed by 7% over the weekend, due to DST in the US and Canada. date-format-xparb is responsible for most of the slowdown, but also a small date-format-tofte regression.

For the micro-benchmark below I get:

- Without DST:  293 ms
- with DST:    1221 ms

I tracked it down to UTCToLocalStandardOffsetSeconds, there's an extra mktime call with DST:

    currentNoDST = mktime(&local);

Jeff, is this a regression from bug 830257? I hope so because it means beta and release are not affected. Can you take a look?

----

var t = new Date;
var date = new Date("1/1/2007 1:11:11");
for (i = 0; i < 400000; ++i) {
    date.getFullYear();
    date.setTime(date.getTime() + 84266956);
}
print(new Date - t);

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> Jeff, is this a regression from bug 830257? I hope so because it means beta
> and release are not affected.

I can't reproduce on mozilla-beta.

Comment 2

[Jeff Walden [:Waldo]]

Probably yeah, a regression from that.

Given the way the DST cache works, there's a chance this delta will go away in a month or so.  It would be interesting for someone to test with system time adjusted to May or so to see if the problem shows up then.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

This patch fixes the regression for me.  The function with the mktime (which is known from bug 578259 to be horrifically slow on at least some platforms) is explicitly computing the difference vs. UTC without any DST effects, so its result will always be the same within a given time zone.  The patch just caches this difference, and refreshes it at the same time as the time zone adjustment.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 723697 [details] [diff] [review]
patch

Review of attachment 723697 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8948998650e1

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 723697 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 830257
User impact if declined: worse sunspider score
Testing completed (on m-c, etc.): on trunk
Risk to taking this patch (and alternatives if risky): low

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8948998650e1

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This is going to need some love to land on b2g18.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a64ef5e8030c

Comment 10

[Brian Hackett [Laid off!]]

Attachment: b2g patch

This is different enough to need a re-review.

Comment 11

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/af9270e8f205

Comment 12

[Julien Wajsberg [:julienw]]

should this land on b2g 1.0.1 too ?

Comment 13

[Julien Wajsberg [:julienw]]

According to Bug 830257 comment 47 we should uplift this to v1.0.1 too.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e07929f2fcb5