Closed Bug 558263 Opened 14 years ago Closed 13 years ago

Make the value in dslots[-1] consistent


(Core :: JavaScript Engine, defect)

Not set





(Reporter: n.nethercote, Assigned: n.nethercote)



For dense arrays, dslots[-1] holds the dslots capacity.  For all other objects, it holds the dslots capacity + JS_INITIAL_SLOTS.  This is really confusing and error-prone (js_GetObjectTotalSize() gets it wrong, for example).

I propose to change it so that dslots[-1] always holds the dslots capacity.  This will require some more adds/subs in some places and fewer in others, so I expect it'll make no difference, performance-wise.

This bug depends on bug 558262, which will encapsulate all dslots accesses.  That will make this change much easier.
Blocks: 552837
+1 to making this more consistent, I experimented with encapsulating dslots in bug 553681 and must have gotten the capacity wrong in every way possible.
Another possibility: do something vaguely (or so I hear) like what JSC does, and keep fslots/dslots in a union, storing the capacity of dslots adjacent to the dslots pointer.  The nice thing about that is that the dslots storage wouldn't hit reallocation edge cases as often -- if you have an array mapping, say, ASCII character codes to something, you aren't going to be allocating the next larger power-of-two just to hold dslots[-1].  See the comment in jsarray.cpp!EnsureCapacity; currently that 256-element array would require dslots with space for 512 elements.
Er, make that dslots with space for 511 elements plus 1 for dslots[-1].
(In reply to comment #2)
> Another possibility: do something vaguely (or so I hear) like what JSC does,
> and keep fslots/dslots in a union, storing the capacity of dslots adjacent to
> the dslots pointer.

That's not this bug. It's bug 555128. Let's move this bug forward first.

Even for big objects, tracing can constant-fold the first few properties' slot address computation and load from obj, avoiding a dependent load of dslots or its future equivalent. This needs measurement before jumping on the JSC bandwagon. It could be ok if L1 hit rate is high enough but that's a big if, esp. with our GC!

Fine, whatever, pretend I didn't even mention JSC, just that I suggested storing dslots length in objects directly -- I only made the comment here because people in the office said, when I mentioned the bad off-by-one-at-power-of-two behavior with arrays, that this was where the comment should go.
Ok, pretend I didn't say "dependent load" :-/.

This is a righteous bug to fix. Gregor and I were confused by what the heck dslots[-1], and when it changes meaning.

One of the aims of all this encapsulation I'm doing is to make it easier to do experiments with the layout -- such as storing dslots capacity directly.  But the encapsulation has to come first!  (Admittedly, this bug isn't so much about encapsulation although it does help slightly, because we won't have to change the capacity when a dense array becomes slow.)
Arrays no longer use dslots[-1];  the capacity is now stored in an fslot.  

So the question is now whether it's worth leaving dslots[-1] as holding the total number of slots (fslots + dslots) for non-arrays, or whether dslots[-1] should just hold the number of dslots.  I lean towards the latter, since we always know how many fslots there are, so including it in dslots[-1] is redundant.  Then again, bhackett is talking about variable-length objects...
To predict optimal fslots and avoid a branch, bhackett is making dslots point to fslots. This moves private out to a data member wasted on instances of classes that lack privates; more winningly, moves capacity from dslots[-1] to another data member of JSObject. The tracer can still constant fold its way to direct fslot addressing for the first N properties.

That doesn't leave much value in this bug unless the approach Brian is pursuing does not pan out.

Bug 584917 fixed this by getting rid of dslots[-1] altogether.  Hooray!
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.