579957 - parent as a field in JSObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently the parent link is stored in the first fixed slot of JSObject *. With fat values on 32-bit CPU this results in bloating the object size by 8 bytes, 4 unused bytes to store the parent object pointer in a value slot and 4 bytes to pad fixed slots so they starts on 8-bytes boundary.

The attached patch avoids that by storing the parent as JSObject* field.

Comment 1

[Igor Bukanov]

Attachment: v1

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 458390 [details] [diff] [review]
v1

Awesome!  This also makes setting/getting the parent faster (on both x86 and x64)!  I don't know why I assumed that JSObject*-ifying JSSLOT_PARENT would be hard... superstitious belief that somehow code wanted it to be an fslot or something...

I would be interested to see the effect on SS/V8.

Comment 3

[Brendan Eich [:brendan]]

Could you hold off until bug 558451 is rebased at least? I will repack JSObject as a "last patch in mq" step there.

Also, this seems to go against the bug 508357 comment 1 idea (which may have its own bug, I couldn't find it) of making the parent fslot optional (by class).

/be

Comment 4

[Luke Wagner [:luke]]

I was curious, so I measured.  SS showed no change, but V8 showed:

  1.010x as fast    5289.0ms +/- 0.4%   5237.4ms +/- 0.4%     significant

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> Could you hold off until bug 558451 is rebased at least? I will repack JSObject
> as a "last patch in mq" step there.

I will wait.

> Also, this seems to go against the bug 508357 comment 1 idea (which may have
> its own bug, I couldn't find it) of making the parent fslot optional (by
> class).

This is orthogonal to this bug which which is about quick recovering of 8 wasted byets per object on 32 bit. Elimination of the parent can be done both for its jsval and JSObject* forms.

Comment 6

[Igor Bukanov]

Brendan - is it OK to land this now?

Comment 7

[Brendan Eich [:brendan]]

I'm close to landing bug 558451 modulo review. I can rebase if you land this, though. But apart from 32-bit packing, why is this good?

If we want to make parent optional but declared, we'll need C++ subtypes -- that is for a later bug but I wonder if it would work out. Would it be the case that every scope object and all the DOM objects would derive from the new JSObjectWithParent? How would the API for this look?

I suspect it would be too complicated to be worth the benefits. If we use an fslot instead, we can decide based on JSCLASS_HAS_PARENT (and assert that as needed). We need runtime type information anyway, not just C++ type info (_sans_ RTTI).

/be

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Decoding the parent becomes easier on x64 but evidence suggests this doesn't matter.

Comment 9

[Andreas Gal :gal]

I think we should land this. I will probably make a patch to obsolete parent in most objects, but until then this is a strict improvement.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> I think we should land this. I will probably make a patch to obsolete parent in
> most objects, but until then this is a strict improvement.

Please respond to comment 7.

/be

Comment 11

[Andreas Gal :gal]

In an ideal world, only a very small set of objects needs a parent. The vast majority of them are functions, which have extra storage we can move the parent into. Aside from that we just need parent in With and a few other oddballs, which we can make work. I don't think we need a common case for "stuff with parents". There are very few, and they are not touched in related places so we don't save anything by having a common interface.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> In an ideal world, only a very small set of objects needs a parent. The vast
> majority of them are functions, which have extra storage we can move the parent
> into. Aside from that we just need parent in With and a few other oddballs,
> which we can make work.

Wrong. It's not "a few", it is the DOM elements that can have event handlers, which is all of them.

> I don't think we need a common case for "stuff with
> parents". There are very few, and they are not touched in related places so we
> don't save anything by having a common interface.

I repeat: how are you going to factor JSObject *parent; so it is in a C++ type used by all of the DOM types as well as the JS core-language scope objects? As the JS API allows objects to be on the scope chain precisely so the DOM can put its elements there, how will you tell at runtime that you have a parent-full object? Both TM and JM need to know, and we can't use RTTI.

/be

Comment 13

[Andreas Gal :gal]

(In reply to comment #12)
> (In reply to comment #11)
> > In an ideal world, only a very small set of objects needs a parent. The vast
> > majority of them are functions, which have extra storage we can move the parent
> > into. Aside from that we just need parent in With and a few other oddballs,
> > which we can make work.
> 
> Wrong. It's not "a few", it is the DOM elements that can have event handlers,
> which is all of them.

We currently (ab)use parent of DOM elements for this purpose. In my reworked experimental proxy-based DOM bindings DOM elements will be non-native, and very different rules apply. I can store the parent wherever I want.

> 
> > I don't think we need a common case for "stuff with
> > parents". There are very few, and they are not touched in related places so we
> > don't save anything by having a common interface.
> 
> I repeat: how are you going to factor JSObject *parent; so it is in a C++ type
> used by all of the DOM types as well as the JS core-language scope objects? As
> the JS API allows objects to be on the scope chain precisely so the DOM can put
> its elements there, how will you tell at runtime that you have a parent-full
> object? Both TM and JM need to know, and we can't use RTTI.

As indicated above I think in the engine there are only a few things that have parents, and DOM objects might become non-native and special potentially.

> 
> /be

Comment 14

[Brendan Eich [:brendan]]

Ok, but the JS API would need changes. Igor just landing a JSObject *parent; change is one step along a path. Is it important to do just this step right now instead of doing the work to make parent rare?

/be

Comment 15

[Brendan Eich [:brendan]]

I can take the rebase (I've had worse lately), so that is not the main issue. It is doing something to pack JSObject on 32 bits that I'll repack anyway soon after rebasing, and not doing the work that matters: factoring parent out of JSObject.

/be

Comment 16

[Andreas Gal :gal]

This is a really small patch. I have no strong preferences. Your call.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 458390 [details] [diff] [review]
v1

>     void traceProtoAndParent(JSTracer *trc) const {
>         if (JSObject *proto = getProto())
>-            JS_CALL_OBJECT_TRACER(trc, proto, "__proto__");
>+            JS_CALL_OBJECT_TRACER(trc, proto, "proto");
>         if (JSObject *parent = getParent())
>             JS_CALL_OBJECT_TRACER(trc, parent, "parent");
>     }

Good patch, let's get it in -- but since you are touching one line here, why not get rid of the temporaries and getProto()/getParent() layering and just test proto and parent?

/be

Comment 18

[Brendan Eich [:brendan]]

Sorry, I should have looked harder. The patch is a strict improvement over the fslot approach.

/be

Comment 19

[Igor Bukanov]

Attachment: v2

This version removes traceProtoAndParent and just inlines the corresponding code in jsgc.cpp.

Comment 20

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/d8bbb2ef3038

Comment 21

[Robert Sayre]

I backed this out because it appeared to cause a crash in crashtest on all platforms.

Comment 22

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/549c12569cb8 - relanded after fixing the bug 583404.

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d8bbb2ef3038

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/549c12569cb8