Do exact rooting in jsdate.cpp

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(8 attachments)

Assignee

Description

7 years ago
In this bug I will do full (or close to full) exact rooting of jsdate.{h,cpp}.
Assignee

Comment 1

7 years ago
This patch removes some unnecessary |cx| parameters.  SetDateToNaN() is the
only tricky one -- it required changing cx->runtime->NaNValue.getDoubleRef()
to js_NaN.

Some browser code needed update as a result of these changes.
Attachment #663300 - Flags: review?(jwalden+bmo)
Assignee

Comment 2

7 years ago
A number of functions in jsdate.cpp are passed a |JSContext *| parameter but
they only access JSContext::dstOffsetCache.  This patch converts those
|JSContext *| parameters to |DSTOffsetCache *|.

(A bunch of the date_get*_impl functions look like they could be converted
as well, but they cannot because they are called via
CallNonGenericMethod().)

The motivation behind this is that it makes obvious which functions cannot
trigger GC (i.e. those that don't take a |JSContext *| arg).
Attachment #663302 - Flags: review?(jwalden+bmo)
Assignee

Comment 3

7 years ago
This patch converts all the existing Rooted<JSObject*> pointers to
- RawObject (in the date_get*_impl functions)
- RootedObject (in the date_set*_impl functions)
Attachment #663303 - Flags: review?(jwalden+bmo)
Assignee

Comment 4

7 years ago
This patch converts all the remaining |JSObject *| pointers in jsdate.cpp to
RootedObject or RawObject.  (Except for in js_InitDateClass(), which will
have to wait for bug 793086.)
Attachment #663305 - Flags: review?(jwalden+bmo)
Comment on attachment 663300 [details] [diff] [review]
(part 1) - Remove unnecessary |cx| parameters.

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

::: js/src/jsdate.cpp
@@ +1264,5 @@
>  /*
>   * Set UTC time to a given time and invalidate cached local time.
>   */
>  static JSBool
> +SetUTCTime(JSObject *obj, double t, Value *vp = NULL)

Followup to make this not return a value?

@@ +1281,5 @@
>      return true;
>  }
>  
>  static void
> +SetDateToNaN(JSObject *obj, Value *vp = NULL)

Followup to remove this function entirely and just make callers pass NaN explicitly?
Attachment #663300 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 663302 [details] [diff] [review]
(part 2) - In jsdate.cpp, convert |cx| parameters to |dstOffsetCache| where possible.

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

::: js/src/jsdate.cpp
@@ +420,5 @@
>  }
>  
>  /* ES5 15.9.1.8. */
>  static double
> +DaylightSavingTA(double t, DSTOffsetCache *dstOffsetCache)

We really shouldn't need to pass around DSTOffsetCache like this.  :-\  Maybe, now that runtime is single-threaded, we should move DSTOffsetCache into JSRuntime, and then these methods could use TLS to get the cache when they need it?  This would much better comport with the spec algorithms these methods implement, which don't take anything like a DST cache (naturally, as it's a total benchmarketing hack).

But a followup, of course.

@@ +1292,5 @@
>   * If UTC time is not finite (e.g., NaN), the local time
>   * slots will be set to the UTC time without conversion.
>   */
>  static bool
> +CacheLocalTime(DSTOffsetCache *dstOffsetCache, JSObject *obj)

...so this isn't fallible, then?  Super-fast followup to make this void, then?

I did a huge double-take on seeing every call site of this being checked for returning false, then that return-false being propagated outward with no possible way for an exception to have been properly set.  (And obviously all those cases couldn't possibly have been meant to be silent script termination.)  People used to the cx-means-fallible idiom are going to be very confused by this intermediate state, if it's not fixed very quickly -- please do so.

@@ +1429,5 @@
>      return true;
>  }
>  
>  inline bool
> +GetCachedLocalTime(DSTOffsetCache *dstOffsetCache, JSObject *obj, double *time)

When CacheLocalTime's signature is changed, the only case where this returns anything but true is if !obj.  Looking at the callers, it looks like this only happens with the friend APIs to extract components of a Date object.  But this is nonsense -- it really should just be an error (an assertion, that is) to ask for the minutes component, say, of NULL.  Another followup to make this an infallible (object -> double) method, please?  (Conveniently, it looks like all callers already ensure the provided object is non-null, and a Date, so no caller changes should be needed.)

@@ +3217,1 @@
>      return obj;

Might as well get rid of the |obj| temporary while you're here, and define and initialize msec_time at a single spot.
Attachment #663302 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 663303 [details] [diff] [review]
(part 3) - In jsdate.cpp, convert Rooted<JSObject*> pointers to RawObject or RootedObject.

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

Frankly, I don't see a reason to use RawObject here at all.  We have no evidence that using raw pointers is measurably more performant on any benchmarks, and raw pointers require more care and trouble to use correctly, versus rooted pointers that are just safe by default, the way we use them.  But I'm not particularly interested in arguing the point right now, either.  :-\

::: js/src/jsdate.cpp
@@ +1861,5 @@
>  date_setMilliseconds_impl(JSContext *cx, CallArgs args)
>  {
>      JS_ASSERT(IsDate(args.thisv()));
>  
> +    RootedObject thisObj(cx, &args.thisv().toObject());

Booooooooooo, Rooted<JSObject*> was better.  :-(
Attachment #663303 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 663305 [details] [diff] [review]
(part 4) - In jsdate.cpp, convert remaining JSObject* pointers in jsdate.cpp to RawObject or RootedObject.

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

::: js/src/jsdate.cpp
@@ +3141,5 @@
> +        RawObject obj = js_NewDateObjectMsec(cx, d);
> +        if (!obj)
> +            return false;
> +        args.rval().setObject(*obj);
> +    }

Erm, why a new scope here?  I can't see any reason for this...

I want an answer to this question before this patch lands, even if I r+ it (as seems likely right now given how much I've reviewed of it already).

@@ +3151,2 @@
>  {
> +    RootedObject obj(cx, objArg);

js_InitDateClass is called by sufficiently few callers (two, I think?) that really you should just change them to pass a rooted object, and make this method take a handle.
Attachment #663305 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 9

7 years ago
> Frankly, I don't see a reason to use RawObject here at all.

Fair enough.  I've changed them.
Assignee

Comment 10

7 years ago
> ::: js/src/jsdate.cpp
> @@ +3141,5 @@
> > +        RawObject obj = js_NewDateObjectMsec(cx, d);
> > +        if (!obj)
> > +            return false;
> > +        args.rval().setObject(*obj);
> > +    }
> 
> Erm, why a new scope here?  I can't see any reason for this...

I sometimes do this to indicate the scope of a RawThing.  In this case it was overzealous, given that the following line returns :)  I'll remove it.
Assignee

Comment 11

7 years ago
> js_InitDateClass is called by sufficiently few callers (two, I think?) that
> really you should just change them to pass a rooted object, and make this
> method take a handle.

Turns out I already did this in bug 793086.
Assignee

Comment 12

7 years ago
As requested.
Attachment #668303 - Flags: review?(jwalden+bmo)
Assignee

Comment 13

7 years ago
As requested.
Attachment #668305 - Flags: review?(jwalden+bmo)
Assignee

Comment 15

7 years ago
As requested... is this what you meant?
Attachment #668309 - Flags: review?(jwalden+bmo)
Attachment #668303 - Flags: review?(jwalden+bmo) → review+
Attachment #668305 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 668307 [details] [diff] [review]
(part 7) - Change CacheLocalTimes()'s return type from bool to void.

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

::: js/src/jsdate.cpp
@@ +1526,5 @@
>  {
>      JS_ASSERT(IsDate(args.thisv()));
>  
>      RootedObject thisObj(cx, &args.thisv().toObject());
> +    CacheLocalTime(&cx->dstOffsetCache, thisObj);

Hmm.  I'm not particularly liking the "CacheLocalTime" name any more.  It's really populating the object's cached local time slots, possibly using the DST offset cache to do so.  Maybe FillLocalTimeSlots?  ComputeLocalTimeSlots?  ComputeCachedLocalTime?  I can't think of a name I'm absolutely happy with.  Maybe someone on IRC will have an idea, or maybe you have one?
Attachment #668307 - Flags: review?(jwalden+bmo) → review+
Attachment #668309 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 17

7 years ago
> Hmm.  I'm not particularly liking the "CacheLocalTime" name any more.  It's
> really populating the object's cached local time slots, possibly using the
> DST offset cache to do so.  Maybe FillLocalTimeSlots? 

FillLocalTimeSlots sounds fine, I'll do that.
Blocks: 799067
You need to log in before you can comment on or make changes to this bug.