700228 - JSObject::shrinkSlots wastes space

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Many objects have some fixed slots and some dynamic slots.  In JSObject::shrinkSlots we do this:

    Value *tmpslots = (Value*) cx->realloc_(slots, newcap * sizeof(Value));

then:

    capacity = newcap;

I'm pretty sure this is a bug, because we actually now have space for newcap+numFixedSlots(), so the last numFixedSlots() slots in the dynamic slots array will never be used.  (It's really unclear if shrinkSlots() is meant to be able to replace the dynamic slots with NULL or not, BTW, because of this confusion.)

This is sympomatic of a larger problem -- looking through allocSlots(), growSlots() and shrinkSlots(), I'm seeing lots of confusion over the term "capacity" -- it now means the number of fixed lots plus the number of dynamic slots, but that wasn't the case in the past, and lots of the occurrences seem to mean just the number of dynamic slots.  Another example:  SLOT_CAPACITY_MIN should be called DYNAMIC_SLOT_CAPACITY_MIN if you trust the comment above its definition.

These three functions, and maybe some others, need an overhaul to make the variable names and comments consistent, and to catch any other problems like the shrinkSlot capacity problem.

I've attached a patch where I started trying to fix this, in case it's useful, but I wasn't quite sure what the intent was in various places (e.g. in growSlots() are we trying to double the number of slots or the number of dynamic slots?) so I didn't finish it.  bhackett, are you able to fix this problem?

Comment 1

[Nicholas Nethercote [inactive]]

BTW, the shrinkSlot() bug is moderately frequent, here is a histogram of occurrences opening Gmail and TechCrunch:

( 1)   1226 (53.0%, 53.0%): Shrink: newcap=8, numFixedSlots=8
( 2)    105 ( 4.5%, 57.5%): Shrink: newcap=8, numFixedSlots=2
( 3)     82 ( 3.5%, 61.0%): Shrink: newcap=11, numFixedSlots=2
( 4)     43 ( 1.9%, 62.9%): Shrink: newcap=131, numFixedSlots=16
( 5)     43 ( 1.9%, 64.8%): Shrink: newcap=9, numFixedSlots=2
( 6)     41 ( 1.8%, 66.5%): Shrink: newcap=12, numFixedSlots=2
( 7)     38 ( 1.6%, 68.2%): Shrink: newcap=9, numFixedSlots=8
( 8)     28 ( 1.2%, 69.4%): Shrink: newcap=20, numFixedSlots=4
( 9)     26 ( 1.1%, 70.5%): Shrink: newcap=21, numFixedSlots=4
(10)     23 ( 1.0%, 71.5%): Shrink: newcap=10, numFixedSlots=8

In the most common case, we have 8 fixed slots, realloc 8 dynamic slots, but set capacity to 8 so the dynamic slots are never used.

Comment 2

[Brian Hackett [Laid off!]]

This stuff has all been rewritten in the JM branch.  The underlying problem is that capacity is used in the above sense for normal objects, but is overloaded to specify the exact number of slots for dense arrays.  allocSlots/growSlots/shrinkSlots are used for both types of objects.  The object shrinking stuff fixes this by keeping the capacities separate: for normal objects the capacity is determined as a function of the slot span, and for dense arrays it is a member of the object's array header.  Bugs like this should be fixed in m-c, but I don't think the code should be overhauled as it will be replaced soon.  I'm not sure what this bug amounts to in practice, it seems like it will incur unnecessary allocation calls rather than actually waste space (we allocate the desired amount of memory, but don't mark all of it as being usable).

Comment 3

[Nicholas Nethercote [inactive]]

Great!  I don't think this is important enough to bother fixing in m-c, we can wait for objshrink to be merged.  Thanks.