555429 - TM: encapsulate JSObject slots better

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Conceptually, JSObject holds a variable-sized array of slots.  This is implemented as a fixed-size (5 elements) array called fslots[] and a pointer to an optional variable-sized array called dslots.  When dslots is present, the size of fslots and dslots is stored in dslots[-1].  Dense arrays are an exception;  for them dslots holds the array elements and dslots[-1] holds the size of dslots.

This is done for performance reasons (see bug 331966), which is all well and good, but it's non-obvious and easy to get wrong.  There is some encapsulation already, eg. OBJ_GET_SLOT, but plenty of places still manipulate fslots and dslots directly.

The goal of this bug is to improve the encapsulation without hurting performance.  This will make the code more maintainable and also make it easier to experiment with different slot arrangements (see bug 555128).

Comment 1

[Nicholas Nethercote [inactive]]

In case it's not clear, my goal here is to completely hide the fact that
slots are split into fslots and dslots.  In other words, JSObject's
interface will present the slots conceptually as a single resizable array.

I've done some preliminary hacking on this.  Some notes:

- Encapsulating fslots is pretty easy.  My plan is to make all the JSSLOT_*
  values static const fields of JSObject, and then have getters/setters for
  each one.  Eg. getArrayLength(), setArrayLength().  There are few enough
  pre-defined slots of this nature that this is quite feasible.  
  
  The JSSLOT_* fields should be able to be made private once all these
  getters/setters are in place.  The one tricky bit is that jstracer.cpp
  needs to know about the internal layout of JSObject in order to generate
  code that accesses slots.  I think making TraceRecorder a friend of
  JSObject will be necessary.

- dslots are trickier.  First, I think it'll make things easier if
  dslots[-1] is changed to always hold the size of dslots -- currently
  it holds the size of dslots for dense arrays, but the size of both dslots
  *and* fslots for other objects.

  For dense arrays, some interface changes will be required for array
  element slots.  The dense array slot arrangement can conceptually be
  presented as "the first few are for attributes like length, size, and then
  all the ones after that are the actual array elements".  Eg. it'll need
  'jsval* JSObject::getElemSlots()'.  This shouldn't be a problem.  I think
  Arguments will need similar treatment.

  After that, the GETDSLOT and CALLDSLOT ops are a problem.  I don't even
  understand what they do yet, but they obviously punch right through the
  abstraction I'm trying to create.

  jstracer.cpp again is tricky.  I suspect some of its current dslot needs can
  be hidden within JSObject methods, but sometimes it'll need to access them
  directly.  Hopefully not too often.

  I also guess that JaegerMonkey will have similar requirements to
  jstracer.cpp.

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> - Encapsulating fslots is pretty easy.  My plan is to make all the JSSLOT_*
>   values static const fields of JSObject, and then have getters/setters for
>   each one.  Eg. getArrayLength(), setArrayLength().  There are few enough
>   pre-defined slots of this nature that this is quite feasible.  

Sure -- this is already done for the first few (getProto(), etc.).

>   The JSSLOT_* fields should be able to be made private once all these
>   getters/setters are in place.  The one tricky bit is that jstracer.cpp
>   needs to know about the internal layout of JSObject in order to generate
>   code that accesses slots.  I think making TraceRecorder a friend of
>   JSObject will be necessary.

Friend is good.

> - dslots are trickier.  First, I think it'll make things easier if
>   dslots[-1] is changed to always hold the size of dslots -- currently
>   it holds the size of dslots for dense arrays, but the size of both dslots
>   *and* fslots for other objects.

Could this actually save cycles? I'm still interested in how the trade-off shifts as measured, which you are good at measuring ;-).

>   For dense arrays, some interface changes will be required for array
>   element slots.  The dense array slot arrangement can conceptually be
>   presented as "the first few are for attributes like length, size, and then
>   all the ones after that are the actual array elements".  Eg. it'll need
>   'jsval* JSObject::getElemSlots()'.  This shouldn't be a problem.  I think
>   Arguments will need similar treatment.

Indeed I almost named GetArgsSlot GetArgsElem, and of course I wished it could be a method on JSObject or some ArgumentsObject subtype, not an inline function.

>   After that, the GETDSLOT and CALLDSLOT ops are a problem.  I don't even
>   understand what they do yet, but they obviously punch right through the
>   abstraction I'm trying to create.

Abstractions are our servants, but you have a point: punching servants is not nice. These can simply be renamed GETFLATUPVAR and CALLFLATUPVAR or some such. I chose the more concrete names at the time to avoid overlong names and premature abstraction.

>   jstracer.cpp again is tricky.  I suspect some of its current dslot needs can
>   be hidden within JSObject methods, but sometimes it'll need to access them
>   directly.  Hopefully not too often.

Since abstractions are servants and we're in charge, we can do a little creative servant management here. No punching of course. But representation has to matter in a high-performance JIT. Abstraction is a tool for hiding inessential differences, not all differences and not the ones most important to performance.

'Scuse my sermonizing. Good to see this going forward.

/be

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> > - dslots are trickier.  First, I think it'll make things easier if
> >   dslots[-1] is changed to always hold the size of dslots -- currently
> >   it holds the size of dslots for dense arrays, but the size of both dslots
> >   *and* fslots for other objects.
> 
> Could this actually save cycles? I'm still interested in how the trade-off
> shifts as measured, which you are good at measuring ;-).

I'll give it a go.  I'll be surprised if it makes much difference, but I'd prefer the change to occur because I find the current arrangement confusing.

> Indeed I almost named GetArgsSlot GetArgsElem, and of course I wished it could
> be a method on JSObject or some ArgumentsObject subtype, not an inline
> function.

I don't see why it can't be a method on JSObject?

Brendan, I said to you that I wanted to do a big patch before breaking this into little patches, to make sure it would work.  I'm now leaning away from that because it's a bit of a pain for me.  And I don't see any real showstoppers with the whole plan (and even if there were, having 90% of fslots/dslots accesses encapsulated still seems better than the status quo).

So unless there are objections I'll start doing it more incrementally, filing bugs and posting patches for what should be non-controversial things, particularly the getters/setters for fslots.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > > - dslots are trickier.  First, I think it'll make things easier if
> > >   dslots[-1] is changed to always hold the size of dslots -- currently
> > >   it holds the size of dslots for dense arrays, but the size of both dslots
> > >   *and* fslots for other objects.
> > 
> > Could this actually save cycles? I'm still interested in how the trade-off
> > shifts as measured, which you are good at measuring ;-).
> 
> I'll give it a go.  I'll be surprised if it makes much difference, but I'd
> prefer the change to occur because I find the current arrangement confusing.

Gregor can testify that he and I both were confused when he first stated to measure object size populations in bug 502736.

> > Indeed I almost named GetArgsSlot GetArgsElem, and of course I wished it could
> > be a method on JSObject or some ArgumentsObject subtype, not an inline
> > function.
> 
> I don't see why it can't be a method on JSObject?

Could be the best place. Waldo with his MIT training and I with some fear of unintended consequences are thinking of lightweight inline-only "view subtypes" that add class-specific methods to objects but can assert that |this| is of only one or two classes.

> So unless there are objections I'll start doing it more incrementally, filing
> bugs and posting patches for what should be non-controversial things,
> particularly the getters/setters for fslots.

Sure, I am pretty easy about this, probably more tolerant of mega-patches than others. "If it were done when 'tis done, then 'twere well It were done quickly".

(In reply to comment #1)
> - dslots are trickier.  First, I think it'll make things easier if
>   dslots[-1] is changed to always hold the size of dslots -- currently
>   it holds the size of dslots for dense arrays, but the size of both dslots
>   *and* fslots for other objects.

Not that anyone is confused, but in case someone down the road might be, house style uses "size" in sizeof terms, i.e. bytes, and "length" for the number of elements in an array.

/be

Comment 5

[Nicholas Nethercote [inactive]]

Bug 673451 made JSObject::slots private, so there's nothing left to do here.