638316 - remove parent

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

Every object has a parent in our engine, which not only wastes a ton of memory, but also creates a great way to leak or bloat. Hanging on to any object always automatically keeps the global object alive, which leaks the world.

With a series of patches I would like to remove parents, and replace them with a notion of scope, which can be queried only for functions and scope objects.

In particular, JS_GetParent and JS_SetParent have to go and will be replaced with JS_GetScope.

The rough outline of steps:
Step 1: Remove JS_GetParent and JS_SetParent and make the engine work (done).
Step 2: Make the browser work without JS_GetParent and JS_SetParent.
Step 3: Move parent into a slot and remove JSObject *parent from JSObject.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Brendan Eich [:brendan]]

Scope objects includes DOM event receivers.

/be

Comment 3

[Andreas Gal :gal]

Yeah, my plan as part of (2) was to see if we can wrap them into With objects.

Comment 4

[Brendan Eich [:brendan]]

No, do not follow Chrome/V8 down that path. The cost of a parent link in certain DOM event receiver classes is of no consequence. The cost of 'with' is. Instead, try something delegated to the non-native object implementation.

/be

Comment 5

[Andreas Gal :gal]

I would like to measure comment 4 before taking any more complexity then necessary. Once we are done with this, with objects will be 4 words. Even if a page has a thousand event handlers (I think a handful is more common), thats 4k. A single pointer would be 1k. So the difference is 3k for a totally unrealistically large event handler work load. I am happy to measure this once we get closer to this. There is a bunch of stuff to solve first anyway.

Comment 6

[Brendan Eich [:brendan]]

There's more to it than space. You have to enter and leave with in the event handler bodies. You have to avoid name collisions (e.g., on 'event', the name of the event handler's formal parameter). Do not assume your favored conclusion. Start with what we have, for these event receiver objects' classes.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

Need to sort out doGetObjectPrincipal too, if we do this.  I suppose we could hang the principal off the scope?

Comment 8

[Andreas Gal :gal]

compartments!

obj->compartment()->principals

Comment 9

[Boris Zbarsky [:bzbarsky]]

I wish.

Compartments are per-origin, but we use path information from principals in some places last I checked...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Now arguably we should stop doing that....

Comment 11

[Andreas Gal :gal]

Wow, alright, I will need a crash course on that by you or mrbkap on that, then.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Mostly I think (hope?) it's for referrers.  We should just fix that sanely.  I bet jlebar is interested!

There might be some file:// issues too, actually....

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that we also use the parent when creating a scope chain for onfoo attribute event handlers. I.e. for markup like <input onclick="x = codeHere;">

Generally this chain just element->globalObject, however for form elements the chain is element->form->globalObject. And for XUL elements it is the full DOM-parent chain IIRC.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Jonas, that's what comment 2 was about.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, I see. Note that if we are hell-bent on it, we can probably change the XUL behavior. It will cause breakage in existing code, but we can always fix our own code and ask addon authors to as well.

The HTML situation is different, where we're likely stuck with the crappy scope chain forever. All browsers implement it and it's likely depended on in many old unmaintained webapps out there.

Comment 16

[Luke Wagner [:luke]]

(In reply to comment #9)
> I wish.
> 
> Compartments are per-origin, but we use path information from principals in
> some places last I checked...

The same issue got in the way of Blake and I ripping out a bunch of crazy caps code (and was ultimately the hair that broke the camel's back for removing dummy frames in time for FF4) so I really hope "make {cx,obj}->compartment()->principals good enough" is the solution!

Comment 17

[Andreas Gal :gal]

Attachment: patch

Comment 18

[Andreas Gal :gal]

New patch, take a look.

Comment 19

[Nicholas Nethercote [inactive]]

What's the status of this?  Smaller JSObjects + fewer chances to leak = win.

Comment 20

[Luke Wagner [:luke]]

comment 10 pointed out that doing this is difficult without having compartment-per-global (which I marked as blocking this bug).

Comment 21

[Brian Hackett [Laid off!]]

Attachment: patch moving parent to BaseShape

Most objects associated with a given global have the same parent, so this patch moves JSObject::parent to its BaseShape (the actual links between objects are still there, so this doesn't do anything for the cycle collector).

Scope objects and functions can have many different parents within a global, so to avoid fragmentation these have already been restructured by dependent bugs.  There are still some ad hoc uses of parent which may also need restructuring (bound functions, some proxies, maybe others), I will be measuring BaseShape fragmentation after JSObject hits its final size to see if any more work is needed.

https://hg.mozilla.org/projects/jaegermonkey/rev/f852758f39d1

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 567003 [details] [diff] [review]
patch moving parent to BaseShape

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

::: js/src/jsobjinlines.h
@@ +348,5 @@
> +     * Unlike other scope objects, static blocks not nested in one another
> +     * do not have a scope chain.
> +     */
> +    JS_ASSERT(isStaticBlock());
> +    return getFixedSlot(0).isObject() ? &getFixedSlot(0).toObject() : NULL;

If you change JSObject::setScopeChain to
  setFixedSlot(0, js::ObjectOrNullValue(obj))
and changed Parser::letStatmement to remove the 'if (tc->blockChain())' test then this can be written
  return getFixedSlot(0).toObjectOrNull();

@@ +555,5 @@
> +     * Some information stored in shapes describes the object itself, and can
> +     * be changed via replaceLastProperty without converting to a dictionary.
> +     * Parent shapes in the property tree may not have this information set,
> +     * and we need to ensure when unwinding properties that the per-object
> +     * information is not accidentally reset.

This is an important aspect of Shape/BaseShape: could you move/expand this comment to the BaseShape comment and just reference it here?

::: js/src/jsscope.cpp
@@ +1117,5 @@
> +}
> +
> +/* static */ bool
> +Shape::setObjectParent(JSContext *cx, JSObject *parent, Shape **listp)
> +{

Can you
  JS_ASSERT(!inDictionaryMode());
  JS_ASSERT(parent->isGlobal());
?

@@ +1198,1 @@
>  {

Can you JS_ASSERT(parent->isGlobal()) ?

@@ +1256,2 @@
>      base.flags |= BaseShape::EXTENSIBLE_PARENTS;
>      BaseShape *nbase = BaseShape::lookup(cx, base);

This method is almost the exact same as setParent.  Since these are the only two clients of replaceLastProperty, perhaps you could just push everything from 'BaseShape *nbase = ...' downward into replaceLastProperty (by having it take a BaseShape).  Then replaceLastProperty could assert that the only variation in the given BaseShape is in base.parent and base.flags.

::: js/src/jsscopeinlines.h
@@ +171,5 @@
> +
> +    uint32 span = slotSpan();
> +    PropertyTable *table = &this->table();
> +
> +    *this = *static_cast<BaseShape *>(other);

static_cast not necessary, implicit '*this = *other' should work.