Closed Bug 796523 Opened 12 years ago Closed 12 years ago

Periodically expire JS timezone cache


(Core :: JavaScript Engine, defect)

Not set





(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)



(1 file)

Have we considered periodically expiring the JS timezone cache?

The browser's internal timezone is actually quite visible to the user.  For example, Gmail shows dates and times according to the browser's timezone.

For people who travel, I don't think it's actually too uncommon to change your machine's timezone without restarting your browser.

Can we just expire our timezone cache after N minutes?  Or maybe even more clever, could we expire the timezone cache every time a new compartment is created?  That would ensure that refreshing a page always gets us the latest timezone, and likely wouldn't affect benchmarks.

Ideally, of course, we'd write code for each of our platforms which notices when the system timezone changes, but that seems much harder.
Attached patch Patch? v1Splinter Review
Picking a reviewer at random here, please forward to someone else if I didn't
choose well.

I'm not sure how to do a performance test of this (I've never written a JS
patch) to ensure that it doesn't affect benchmark.  Should I run SS locally, or
is Talos sufficient?  If I need to run something locally, should I do it in the
JS shell, or use the browser?  If I need to use the JS shell, are there
instructions somewhere?
Attachment #666845 - Flags: review?(luke)
Comment on attachment 666845 [details] [diff] [review]
Patch? v1

Waldo added this cache so I think he should be the reviewer.

My own 2 cents is that, while compartment creation is infrequent, it seems hacky to dump in something completely unrelated.  (Our classic dumping ground for "somewhat frequently but not *too* frequently" actions is GC :)
Attachment #666845 - Flags: review?(luke) → review?(jwalden+bmo)
> (Our classic dumping ground for "somewhat frequently but not *too* frequently" actions 
> is GC :)

I considered that, but I didn't think it was quite right, since I want to ensure that if I refresh a page or navigate to a new one, the date cache is up-to-date.

I'd be OK dumping the cache both on GC and on compartment creation...
Comment on attachment 666845 [details] [diff] [review]
Patch? v1

Review of attachment 666845 [details] [diff] [review]:

I still want that ideal situation where the OS notifies us and we don't even have to have an API to allow embedders to affirmatively clear date caches.  :-(  So I am basically indifferent to whether GC or compartment-creation time or both is a good time to do this; if you want an actual opinion about either or both being better, find someone else.

::: js/src/jscompartment.cpp
@@ +98,5 @@
> +     * compartment.  This ensures that the cache is always relatively fresh, but
> +     * shouldn't interfere with benchmarks which create tons of date objects
> +     * (unless they also create tons of iframes, which seems unlikely).
> +     */
> +    JS_ClearDateCaches(cx);

Use js_ClearDateCaches() here, please, not the public API.
Attachment #666845 - Flags: review?(jwalden+bmo) → review+
Since nobody seems particularly concerned about the performance implications of this, I went ahead and pushed it.  Anyway it's a lot easier to find perf regressions on inbound than on try.
Assignee: general → justin.lebar+bug
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
for dev-doc-needed see this message (and related thread)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.