Closed Bug 815414 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Whiteboard: [js:t])

Attachments

(1 file)

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*.
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Whiteboard: [js:t][lang=c++][mentor=jwalden] → [js:t]
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.  :-)
Attachment #685864 - Flags: review?(jorendorff)
Is it OK to mark bug 804988 as a duplicate?
The WIP here is more better than mine.
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 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?
Attachment #685864 - Flags: review?(jorendorff) → review+
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
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/c4bb1f2098cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 804988
You need to log in before you can comment on or make changes to this bug.