1240283 - Slim down JSDate instances by not caching cheaply-computed values

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Till Schneidereit [:till]]

JSDate instances are 224 bytes, because they have 10 reserved slots, so get allocated as OBJECT12. At least three of these values, local hours/minutes/seconds, are trivial to computed if we instead just store the number of seconds in the current year. This gets us down to 8 slots and thus OBJECT8, 160 bytes. Still a ridiculous size, but saving 64 bytes seems pretty good to me.

Comment 1

[Till Schneidereit [:till]]

Attachment: Slim down JSDate instances by not caching cheaply-computed values

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8708676 [details] [diff] [review]
Slim down JSDate instances by not caching cheaply-computed values

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

Nice idea.

::: js/src/jsdate.cpp
@@ +1540,5 @@
>      DateObject* dateObj = &args.thisv().toObject().as<DateObject>();
>      dateObj->fillLocalTimeSlots();
>  
> +    Value yearSeconds = dateObj->getReservedSlot(YEAR_SECONDS_SLOT);
> +    if (yearSeconds.isDouble()) {

You're assuming yearSeconds is double ^ int32, but

    /*
     * Cached slots holding local properties of the date.
     * These are undefined until the first actual lookup occurs
     * and are reset to undefined whenever the date's time is modified.
     */

so something like

  var d = new Date();
  d.setUTCTime(8675309);
  d.getHours();

should assert/do the wrong thing, right?  Add a test for this (and all the other methods you've modified with the same bug) if we don't have one already.

@@ +1541,5 @@
>      dateObj->fillLocalTimeSlots();
>  
> +    Value yearSeconds = dateObj->getReservedSlot(YEAR_SECONDS_SLOT);
> +    if (yearSeconds.isDouble()) {
> +        MOZ_ASSERT(!IsFinite(yearSeconds.toDouble()));

I think you can refine this even further to IsNaN, because

  setFixedSlot(UTC_TIME_SLOT, TimeValue(t));

says UTC_TIME_SLOT is the result of TimeValue,

inline Value
TimeValue(ClippedTime time)
{
    return DoubleValue(JS::CanonicalizeNaN(time.toDouble()));
}

says TimeValue fills in NaN,

    double utcTime = UTCTime().toNumber();

and

    const js::Value& UTCTime() const {
        return getFixedSlot(UTC_TIME_SLOT);
    }

says utcTime is NaN, and

    if (!IsFinite(utcTime)) {
        for (size_t ind = COMPONENTS_START_SLOT; ind < RESERVED_SLOTS; ind++)
            setReservedSlot(ind, DoubleValue(utcTime));
        return;
    }

says all these reserved slots will be NaN-filled.

@@ +1544,5 @@
> +    if (yearSeconds.isDouble()) {
> +        MOZ_ASSERT(!IsFinite(yearSeconds.toDouble()));
> +        args.rval().set(yearSeconds);
> +    } else {
> +        args.rval().setInt32((yearSeconds.toInt32() / (60 * 60)) % 24);

Use vm/DateTime.h's SecondsPerHour and HoursPerDay.

@@ +1585,5 @@
> +    if (yearSeconds.isDouble()) {
> +        MOZ_ASSERT(!IsFinite(yearSeconds.toDouble()));
> +        args.rval().set(yearSeconds);
> +    } else {
> +        args.rval().setInt32((yearSeconds.toInt32() / 60) % 60);

SecondsPerMinute, MinutesPerHour.  (And double-check I didn't flip the meanings/assignment of those, since they're the same value.)

@@ +1619,1 @@
>  /* Date.getSeconds is mapped to getUTCSeconds */

Expand this, because I was dumb and not grasping this on seeing it just now:


/*
 * Date.getSeconds is mapped to getUTCSeconds.  This is okay, notwithstanding
 * that ES6's algorithms for the two methods are textually different, as long
 * as two conditions hold.
 *
 * First, no time zone adjustment can have a fractional-minute component.
 * (Fortunately no tinpot dictator currently achieves notoriety this way.)
 *
 * Second, we must continue to implement ES5's braindead semantics of applying
 * the current time zone to all dates and times into the past and future.
 * No modern LocalTZA has a fractional-minute component, but sadly Dublin Mean
 * Time used UTC−00:25:21 from ~1800-1916.
 * https://en.wikipedia.org/wiki/UTC%E2%88%9200:25:21  If we ever support this
 * historical time zone, we'll have to split these methods in two.
 */

Also update the getMilliseconds-maps-to-getUTCMilliseconds comment to refer to this comment:

/*
 * Date.getMilliseconds is mapped to getUTCMilliseconds for the same reasons
 * that getSeconds is mapped to getUTCSeconds (see above).  No known LocalTZA
 * has *ever* included a fractional-second component, however, so we can keep
 * this simplification even if we stop implementing ES5 local-time computation
 * semantics.
 */

@@ +1628,5 @@
> +    if (yearSeconds.isDouble()) {
> +        MOZ_ASSERT(!IsFinite(yearSeconds.toDouble()));
> +        args.rval().set(yearSeconds);
> +    } else {
> +        args.rval().setInt32(yearSeconds.toInt32() % 60);

SecondsPerMinute.

::: js/src/vm/DateObject.h
@@ +30,5 @@
>      static const uint32_t LOCAL_YEAR_SLOT    = COMPONENTS_START_SLOT + 1;
>      static const uint32_t LOCAL_MONTH_SLOT   = COMPONENTS_START_SLOT + 2;
>      static const uint32_t LOCAL_DATE_SLOT    = COMPONENTS_START_SLOT + 3;
>      static const uint32_t LOCAL_DAY_SLOT     = COMPONENTS_START_SLOT + 4;
> +    static const uint32_t YEAR_SECONDS_SLOT  = COMPONENTS_START_SLOT + 5;

Separate this slot from the others by a blank line and an explanatory comment.  And actually, this probably should have a better name as well.  What about LOCAL_SECONDS_INTO_YEAR_SLOT?  Then we could have:

"""
Unlike the above slots that hold LocalTZA-adjusted component values, LOCAL_SECONDS_INTO_YEAR_SLOT holds a composite value that can be used to compute LocalTZA-adjusted hours, minutes, and seconds values.  Specifically, LOCAL_SECONDS_INTO_YEAR_SLOT holds the number of LocalTZA-adjusted seconds into the year.  Unix timestamps ignore leap seconds, so recovering hours/minutes/seconds requires only trivial division/modulus operations.
"""

Comment 3

[Till Schneidereit [:till]]

Attachment: Slim down JSDate instances by not caching cheaply-computed values. v2

> >      DateObject* dateObj = &args.thisv().toObject().as<DateObject>();
> >      dateObj->fillLocalTimeSlots();
> >  
> > +    Value yearSeconds = dateObj->getReservedSlot(YEAR_SECONDS_SLOT);
> > +    if (yearSeconds.isDouble()) {
> 
> You're assuming yearSeconds is double ^ int32

I'm pretty sure I assume correctly: the dateObj->fillLocalTimeSlots() three lines above makes it so. I added
// Note: LOCAL_SECONDS_INTO_YEAR_SLOT is guaranteed to contain an
// int32 or NaN after the call to fillLocalTimeSlots.

> 
> @@ +1541,5 @@
> >      dateObj->fillLocalTimeSlots();
> >  
> > +    Value yearSeconds = dateObj->getReservedSlot(YEAR_SECONDS_SLOT);
> > +    if (yearSeconds.isDouble()) {
> > +        MOZ_ASSERT(!IsFinite(yearSeconds.toDouble()));
> 
> I think you can refine this even further to IsNaN, because
> [...]
> says all these reserved slots will be NaN-filled.

True. I just copied over the preexisting asserts, but you're right, I should improve them while I'm here.

> @@ +1619,1 @@
> >  /* Date.getSeconds is mapped to getUTCSeconds */
> 
> Expand this, because I was dumb and not grasping this on seeing it just now:
> 
> 
> /*
>  * Date.getSeconds is mapped to getUTCSeconds.  This is okay, notwithstanding
>  * that ES6's algorithms for the two methods are textually different, as long
>  * as two conditions hold.
>  * [...] we'll have to split these methods in two.
>  */

I added a more concise, slightly less editorializing version of this comment.

> /*
>  * Date.getMilliseconds is mapped to getUTCMilliseconds for the same reasons
>  * that getSeconds is mapped to getUTCSeconds (see above).  No known LocalTZA
>  * has *ever* included a fractional-second component, however, so we can keep
>  * this simplification even if we stop implementing ES5 local-time
> computation
>  * semantics.
>  */

This, I took verbatim.

> > +    static const uint32_t YEAR_SECONDS_SLOT  = COMPONENTS_START_SLOT + 5;
> 
> Separate this slot from the others by a blank line and an explanatory
> comment.  And actually, this probably should have a better name as well. 
> What about LOCAL_SECONDS_INTO_YEAR_SLOT?

Good suggestion, done.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8709687 [details] [diff] [review]
Slim down JSDate instances by not caching cheaply-computed values. v2

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

(In reply to Till Schneidereit [:till] from comment #3)
> I'm pretty sure I assume correctly: the dateObj->fillLocalTimeSlots() three
> lines above makes it so.

Oh, right.  That should have been obvious, shouldn't it.  I'd be on the fence about adding the comment, given that, but certainly doesn't hurt having it.

> I added a more concise, slightly less editorializing version of this comment.

Boooooooooooooooooooo you're no fun.  :-)

> This, I took verbatim.

Twist the knife in me, why don't you.  :-P

::: js/src/jsdate.cpp
@@ +1338,5 @@
>  
>      int weekday = WeekDay(localTime);
>      setReservedSlot(LOCAL_DAY_SLOT, Int32Value(weekday));
>  
> +    setReservedSlot(LOCAL_SECONDS_INTO_YEAR_SLOT, Int32Value(yearSeconds));

We might do well converting all these (on Date objects) to setFixedSlot, for maybe slightly more efficiency but more importantly to use more precise verbs that make that much clearer what we expect/are doing.  Shouldn't be in this patch/bug, of course.

@@ +1626,5 @@
> + * specifications aren't observable.
> + *
> + * We'll have to split the implementations once we implement ES6's
> + * 20.3.1.7 Local Time Zone Adjustment (because time zones with adjustments
> + * like that did historically exist), or if a new time zone with a

I would state that at least Dublin Mean Time had a UTC-00:25:21 offset, so that a specific counterexample is provided.  Plus, it's trivia that will brighten the reader's day.  :-)

Comment 5

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/276bf29a46c45596ca99e1c0283aa1d6a3707a50
Bug 1240283 - Slim down JSDate instances by not caching cheaply-computed values. r=Waldo

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/276bf29a46c4