Last Comment Bug 638316 - remove parent
: remove parent
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on: cpg 693754 694247 700799 710492
Blocks: ObjectShrink
  Show dependency treegraph
 
Reported: 2011-03-02 18:04 PST by Andreas Gal :gal
Modified: 2013-07-24 02:47 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.83 KB, patch)
2011-03-02 18:05 PST, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (41.05 KB, patch)
2011-03-15 15:40 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch moving parent to BaseShape (109.30 KB, patch)
2011-10-13 20:37 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-03-02 18:04:50 PST
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 2011-03-02 18:05:12 PST
Created attachment 516473 [details] [diff] [review]
patch
Comment 2 Brendan Eich [:brendan] 2011-03-02 19:06:09 PST
Scope objects includes DOM event receivers.

/be
Comment 3 Andreas Gal :gal 2011-03-02 19:27:46 PST
Yeah, my plan as part of (2) was to see if we can wrap them into With objects.
Comment 4 Brendan Eich [:brendan] 2011-03-02 19:33:37 PST
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 2011-03-02 19:38:35 PST
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] 2011-03-02 19:41:47 PST
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 [:bz] 2011-03-02 20:30:35 PST
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 2011-03-02 20:32:55 PST
compartments!

obj->compartment()->principals
Comment 9 Boris Zbarsky [:bz] 2011-03-02 20:40:41 PST
I wish.

Compartments are per-origin, but we use path information from principals in some places last I checked...
Comment 10 Boris Zbarsky [:bz] 2011-03-02 20:41:24 PST
Now arguably we should stop doing that....
Comment 11 Andreas Gal :gal 2011-03-02 20:43:31 PST
Wow, alright, I will need a crash course on that by you or mrbkap on that, then.
Comment 12 Boris Zbarsky [:bz] 2011-03-02 20:56:12 PST
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) PTO Until July 5th 2011-03-03 00:27:35 PST
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 [:bz] 2011-03-03 07:22:36 PST
Jonas, that's what comment 2 was about.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2011-03-03 10:21:42 PST
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] 2011-03-03 11:30:54 PST
(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 2011-03-15 15:40:18 PDT
Created attachment 519534 [details] [diff] [review]
patch
Comment 18 Andreas Gal :gal 2011-03-15 15:40:45 PDT
New patch, take a look.
Comment 19 Nicholas Nethercote [:njn] 2011-06-21 16:29:04 PDT
What's the status of this?  Smaller JSObjects + fewer chances to leak = win.
Comment 20 Luke Wagner [:luke] 2011-06-21 16:32:18 PDT
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 (:bhackett) 2011-10-13 20:37:06 PDT
Created attachment 567003 [details] [diff] [review]
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] 2011-11-08 16:14:43 PST
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.

Note You need to log in before you can comment on or make changes to this bug.