Periodically expire JS timezone cache

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

({dev-doc-needed})

Trunk
mozilla18
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 666845 [details] [diff] [review]
Patch? v1

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 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
> (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 4

6 years ago
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+
(Assignee)

Comment 5

6 years ago
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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/92af89fcb638
(Assignee)

Updated

6 years ago
Assignee: general → justin.lebar+bug

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/92af89fcb638
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 7

5 years ago
for dev-doc-needed see this message (and related thread) https://mail.mozilla.org/pipermail/es-discuss/2013-July/032066.html
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.