Closed Bug 793076 Opened 8 years ago Closed 8 years ago

Do exact rooting in jsdate.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(8 files)

In this bug I will do full (or close to full) exact rooting of jsdate.{h,cpp}.
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)
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)
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)
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+
> Frankly, I don't see a reason to use RawObject here at all.

Fair enough.  I've changed them.
> ::: 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.
> 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.
As requested.
Attachment #668303 - Flags: review?(jwalden+bmo)
As requested.
Attachment #668305 - Flags: review?(jwalden+bmo)
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+
> 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.