815414 - Move the DST offset cache and LocalTZA out of JSContext and into JSRuntime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

We want JSContext to go away, and JSRuntime is the sensible place to move this stuff.  LocalTZA is even worse, actually, because it's a static that's potentially accessed across threads.  (!)

This isn't a completely trivial good first bug, but it seems like it should be reasonably so.  I'm happy to answer any questions people have about how to make this work.  Note that you'll have to change internal method signatures in various places to do this, because not all the current APIs take a JSContext* or a JSRuntime*.

Comment 1

[Jeff Walden [:Waldo]]

Bug 804988 has me realizing that this is a real issue for code correctness and cleanliness, so I'm going to take this back out of the pool to fix now.  Patch shortly.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch, passes tests

It's possible in a rivertrail world this should really live in PerThreadData or something, but doing that supposedly-fully-correctly with locks or mutexes or whatever requires a great deal more pain for very little gain -- let's just take this improvement and run with it.

More could obviously be done here to remove PRMJTime and salt the ground from whence it came, but this is already pretty large for mostly-code-motion.  :-)

Comment 3

[Benjamin Chen [:bechen]]

Is it OK to mark bug 804988 as a duplicate?
The WIP here is more better than mine.

Comment 4

[Jeff Walden [:Waldo]]

If you want -- but assuming you want this fixed in b2g in the short term, before this could ride the trains into releases, it doesn't hurt to have this bug clearly trunk-focused, and that one clearly branch/b2g-focused.  Up to you, doesn't much bother me either way.  Do note that there's no way I'm ever going to push for this particular patch to get uplifted for b2g's sake, tho -- I'd very likely argue against it, even.  :-)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 685864 [details] [diff] [review]
Patch, passes tests

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

Looks good. Bonus points if you rename jsdate.{h,cpp} to builtins/Date.{h,cpp} while you're in here. :)

::: js/src/jscompartment.cpp
@@ +109,5 @@
>       * shouldn't interfere with benchmarks which create tons of date objects
>       * (unless they also create tons of iframes, which seems unlikely).
>       */
> +    if (cx)
> +        cx->runtime->dateTimeInfo.updateTimeZoneAdjustment();

Just noticed that RegExpCompartment::init has a bug in the case where cx is NULL and it hits OOM. Just a one liner; fix it in passing?

Comment 6

[Jeff Walden [:Waldo]]

I have a gut feeling there's more in that file that needs moving around before builtins/Date can happen, but I could be wrong.  I'd rather investigate that before moving forward with that, and that feels too dilatory for me to do it now.  :-)  (Particularly since this was already itself a digression from other important tasks.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bb1f2098cd

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4bb1f2098cd