Bug 12867 - REGRESSION: BenchJS test 7 (dates) is 220% slower than in Safari 2.0.4
Summary: REGRESSION: BenchJS test 7 (dates) is 220% slower than in Safari 2.0.4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2007-02-23 00:42 PST by Maciej Stachowiak
Modified: 2007-03-02 09:56 PST (History)
3 users (show)

See Also:


Attachments
patch to restore time zone caching (11.97 KB, patch)
2007-02-27 07:06 PST, Darin Adler
kmccullough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2007-02-23 00:42:14 PST
The 24Fun benchmark's flying date test (test 7) is 220% slower in current
WebKit than Safari 2.0.4.

I suspect this was due to adopting Mozilla's date computation code.
Comment 1 Maciej Stachowiak 2007-02-23 00:44:48 PST
<rdar://problem/5018674>
Comment 2 David Kilzer (:ddkilzer) 2007-02-23 05:38:49 PST
May be related to Bug 12070 (r18584) or Bug 12057 (r18514) which were follow-on patches to the Mozilla date code.

http://trac.webkit.org/projects/webkit/changeset/18584
http://trac.webkit.org/projects/webkit/changeset/18514

Comment 3 David Kilzer (:ddkilzer) 2007-02-23 05:55:20 PST
Bug 12070 is the more likely culprit since it removed a caching mechanism.  If this is the cause, we probably need to determine how to cache the timezone value so that the UTC offset is only recomputed when needed.

Comment 4 Darin Adler 2007-02-27 07:06:04 PST
Created attachment 13397 [details]
patch to restore time zone caching

If the regression is really due to the fact that the time zone isn't cached, then this patch might help. It restores caching of the time zone, using the same "time zone change" notification we have historically used on Mac OS X. It will still be slow on other platforms, though.
Comment 5 Kevin McCullough 2007-02-27 18:09:48 PST
Comment on attachment 13397 [details]
patch to restore time zone caching

do we have any quantification on performance improvement?
Comment 6 Maciej Stachowiak 2007-02-27 19:18:59 PST
We need to measure to see if this patch is sufficient. I have a fresh release build so I can try that.
Comment 7 Maciej Stachowiak 2007-02-27 22:38:20 PST
Safari 2.0.4: 0.149s
TOT w/o patch: 0.479
TOT with patch: .13

It looks like Darin's patch gets us to faster than Safari 2.0.4. However, because JS execution speed has improved so much generally, and we replaced the date code wholesale, I think we should still compare profiles to see if anything leaps out as potentiall speed-upabble before closing this out.
Comment 8 David Kilzer (:ddkilzer) 2007-02-28 06:40:48 PST
(In reply to comment #7)
> [...] I think we should still compare profiles to see if
> anything leaps out as potentiall speed-upabble before closing this out.

Another potentially "speed-uppable" section of code is that code which calculates the DST offset:  getDSTOffsetSimple() and getDSTOffset().  As I recall it never had any caching mechanism, although that code *appears* to be a hot path since it's called from a lot of places.  (I have not profiled the code to know whether it's a hot path or not.)

Does Mac OS X generate a timezone change when DST takes effect, or is there an equivalent notification for DST changes that may be used?

Comment 9 Darin Adler 2007-03-02 09:55:49 PST
(In reply to comment #8)
> Does Mac OS X generate a timezone change when DST takes effect, or is there an
> equivalent notification for DST changes that may be used?

I don't think that's a relevant question, because that function doesn't depend on whether DST is in effect at the time it's called. It asks if DST is in effect at a given time that may be in the past or future.
Comment 10 Darin Adler 2007-03-02 09:56:06 PST
Committed revision 19943.