Closed Bug 491126 Opened 15 years ago Closed 15 years ago

Sharing object map for fast array instances

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

(Blocks 1 open bug, )

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

Currently JSObjectMap uses reference counting to account all its users. This is not necessary since the GC can safely remove the map when finalizing the object that owns the map. This should allow not only to remove the nrefs field from JSObjectMap but also to make the fast array map shared between all array instances.
Is map->freeslot unused by dense arrays? Yow.

We really want to get rid of map as a mandatory object member but that's for a different bug. This will be a big help soon (1.9.1 even) -- see bug 486321.

/be
Blocks: 486321
Flags: wanted1.9.1?
(In reply to comment #1)
> Is map->freeslot unused by dense arrays? Yow.

Both dense array and live connect objects do not use map->freeslot.
This is any easy fix to needless dense array memory bloat that we should try to get in soon. I'm probably preaching to the choir, but the other nice thing about this bug is that the fix should be safe and easy to analyze.

/be
Attached patch v1 (work in progreess) (obsolete) — Splinter Review
The patch compiles and survives some testing but currently it leaks JSScope instances.
I morph this bug to focus explicitly on sharing object map between array instances. For that it is sufficient to move both freeslot and nrefs from JSObjectMap into JSScope. The nrefs can be eliminated later in a separated bug minimizing the amount of changes here.
Summary: Eliminating JSObjectMap.nrefs → Sharing object map for fast array instances
Attached patch v2 (obsolete) — Splinter Review
The new patch makes fast array and liveconnect (all non-native objects) to always share single map instance. I will ask for a review if it passes the try server.
Attachment #375479 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
The patch moves both freeslot and nrefs members of JSObjectMap into JSScope and makes the map for non-native objects always shared. The shared instance is given by JSObjectOps.objectMap. For natives Ops the member must be null.

For synthetic benchmarks allocating and GC-ing empty array instances the patch speedups the execution by 20-25% both for JIT and non JIT-modes.
Attachment #375688 - Attachment is obsolete: true
Attachment #376561 - Flags: review?(brendan)
I'll review today -- any ./bench.sh speedup results?

We want to get rid of mandatory map in object, but we need (evolved) ops pointer. This is close to that but still requires double indirection to reach ops. For 3.5 we could do with a smaller patch that leaves newObjectMap in JSObjectOps. Beyond that we could get rid of the indirection, speeding up lots of trace-JITted code.

So is it worth splitting the patch in two, one minimal for 3.5, the other going beyond the const objectMap pointer to separate map and ops?

/be
(In reply to comment #8)
> I'll review today -- any ./bench.sh speedup results?

There are no timing changes. It is not surprising since the benchmarks do not exercise the array object creation and GC. 

> So is it worth splitting the patch in two, one minimal for 3.5, the other going
> beyond the const objectMap pointer to separate map and ops?

The patch is already a minimal one. Most of the noise comes from moving nrefs and especially freeslot from JSObjectMap into JSScope. The non-trivial parts are in jsobj.cpp and jslock.cpp that comes from the map initialization.
(In reply to comment #8)
> We want to get rid of mandatory map in object, but we need (evolved) ops
> pointer.

The most effective thing is to move JSOjectOps into JSClass as an opaque reserved for future use data structure so only single vtable would present in the object.
(In reply to comment #10)
> (In reply to comment #8)
> > We want to get rid of mandatory map in object, but we need (evolved) ops
> > pointer.
> 
> The most effective thing is to move JSOjectOps into JSClass as an opaque
> reserved for future use data structure so only single vtable would present in
> the object.

This all sounds good, except for the redundancy between JSClass and JSObjectOps, and the bulk of specialization using JSClass only, and the API constraints on JSClass. An alternative in the form of a syllogism:

1. JSClass is public API, JSObjectOps is no longer.
2. Flat single struct of ops, internal to the engine, best for future-proofing.
3. Therefore JSClass should be a "recipe" for filling in the internal vtable.

This was the thinking Jason and I agreed to in the JSObjectOps make-over bug (bug 408416). What do you think?

In any case, that is for the 1.9.2 future. If we want this bug fixed in 1.9.1 (we do, IMHO) then we need a minimal patch that does not scare release drivers. I wonder if there isn't a smaller patch that leaves newObjectMap in JSObjectOps for now.

/be
(In reply to comment #11)
> I wonder if there isn't a smaller patch that leaves newObjectMap in JSObjectOps
> for now.

With sufficient efforts the patch could be made smaller. On the other hand, if it would not be sufficiently disruptive, then the compiler can no be used to find reliably all the cases where the changes must propagate. Plus the patch changes are on hot paths repeatedly executed over and over and, even more importantly, they are local and do not affect the GC. 

In this regard the v1 was really bad. By eliminating nrefs it subtly changed some GC and cycle collector interactions leading to leaks.
Attached patch v4 (obsolete) — Splinter Review
The new version brings better assert coverage and optimize js_NewObjectWithGivenProto to minimize unnecessary rooting.
Attachment #376561 - Attachment is obsolete: true
Attachment #376736 - Flags: review?(brendan)
Attachment #376561 - Flags: review?(brendan)
I will review tonight.

/be
Comment on attachment 376736 [details] [diff] [review]
v4

>@@ -396,30 +396,31 @@ js_Arguments(JSContext* cx)
>     return NULL;
> }
> JS_DEFINE_CALLINFO_1(extern, OBJECT, js_Arguments, CONTEXT, 0, 0)
> 
> JSObject* FASTCALL
> js_NewNullClosure(JSContext* cx, JSObject* funobj, JSObject* proto, JSObject *parent)

Argh, what happened to the placement of that last *? Feel free to fix.

>@@ -2450,18 +2453,18 @@ js_PutBlockObject(JSContext *cx, JSBool 
>     obj = fp->scopeChain;
>     JS_ASSERT(OBJ_GET_CLASS(cx, obj) == &js_BlockClass);
>     JS_ASSERT(OBJ_GET_PRIVATE(cx, obj) == cx->fp);
>     JS_ASSERT(OBJ_IS_CLONED_BLOCK(obj));
> 
>     /*
>      * Block objects should never be exposed to scripts. Thus the clone should
>      * not own the property map and rather always share it with the prototype
>-     * object. This allows to skip updating OBJ_SCOPE(obj)->map.freeslot after
>-     * we copy the stack slots into reserved slots.
>+     * object. This allows to skip updating OBJ_SCOPE(obj)->freeslot after we

s/allows/allows us/

>+++ tm/js/src/jsobj.h    2009-05-11 17:01:37.000000000 +0200
>@@ -51,19 +51,24 @@
> #include "jshash.h" /* Added by JSIFY */
> #include "jsprvtd.h"
> #include "jspubtd.h"
> 
> JS_BEGIN_EXTERN_C
> 
> /* For detailed comments on these function pointer types, see jsprvtd.h. */
> struct JSObjectOps {
>+    /*
>+     * Custom shared object map for non-native objects. For native objects
>+     * this should be null indicating that JSObject.map is an instance of

Nit: comma after indicating.

>@@ -201,18 +204,18 @@ struct JSObject {
> /*
>  * Maximum net gross capacity of the obj->dslots vector, excluding the additional
>  * hidden slot used to store the length of the vector.
>  */
> #define MAX_DSLOTS_LENGTH   (JS_MAX(~(uint32)0, ~(size_t)0) / sizeof(jsval))
> 
> /*
>  * STOBJ prefix means Single Threaded Object. Use the following fast macros to
>- * directly manipulate slots in obj when only one thread can access obj and
>- * when obj->map->freeslot can be inconsistent with slots.
>+ * directly manipulate slots in obj when only one thread can access obj or

Nit: comma before or.

>@@ -363,22 +366,24 @@ STOBJ_GET_CLASS(const JSObject* obj)
> 
> /*
>  * Class is invariant and comes from the fixed clasp member. Thus no locking
>  * is necessary to read it. Same for the private slot.
>  */
> #define OBJ_GET_CLASS(cx,obj)           STOBJ_GET_CLASS(obj)
> #define OBJ_GET_PRIVATE(cx,obj)         STOBJ_GET_PRIVATE(obj)
> 
>+/*
>+ * Test whether the object is native. FIXME: consider how it would affect the
>+ * performance to do just the !ops->objectMap check.
>+ */
>+#define OPS_IS_NATIVE(ops)                                                    \
>+    JS_LIKELY((ops) == &js_ObjectOps || !(ops)->objectMap)

This is worth checking.

There's another FIXME we want according to the make-over plan: separate ops and map in JSObject. We could make map concrete at that point: JSScope *scope. It would be optional, so null for non-natives. It would take another word, but we could save one by getting rid of classword, folding class into ops one way or another.

The make-over bug proposes to use class as a recipe to fill in an ops vector, which raises a few design issues around allocation, lifetime, etc. If ops structs are GC-allocated then we can share them easily. However the many class identity checks would have to test something else, as efficiently if not moreso.

This may be grist for the make-over bug (bug 408416) but I wanted to throw it out here. The double-indirection remaining, obj->map->ops, hurts enough that it seems worth thinking twice about short paths to eliminate it. On the other hand, I don't see a way to go other than the make-over plan that actually wins perf.

r=me with nits picked, thanks for this -- good patch.

/be
Attachment #376736 - Flags: review?(brendan) → review+
Blocks: 492938
Attached patch v5Splinter Review
Besides addressing the nits the patch also adds the OBJ_IS_NATIVE assert to OBJ_SCOPE().
Attachment #376736 - Attachment is obsolete: true
Attachment #377388 - Flags: review+
(In reply to comment #15)

> We could make map concrete at that point: JSScope *scope. It
would be optional, so null for non-natives.

Non-natives (read array instances) should be free to use the word for their own needs like storing array elements.

> but we could save one by getting rid of classword, folding class into ops one way or another.

I think for 1.9.2 we should consider merging JSObjectOps and JSClass into one entity. Those members can still live in the JSObjectOps struct to emphasis the non-public-and-free-to-change status.

If JSObjectOps would be added at the end of JSClass and all its members would default to NULL, it would be even source-compatible with 1.9.1 code defining JSClass instances.
http://hg.mozilla.org/tracemonkey/rev/dead45971ca2
Whiteboard: fixed-in-tracemonkey
(In reply to comment #18)
> http://hg.mozilla.org/tracemonkey/rev/dead45971ca2

It is funny when the revision number starts with "dead". It has probability one 1 in 2**16. I guess I should have played more with cards during that All Hands party ;)
(In reply to comment #17)
> (In reply to comment #15)
> 
> > We could make map concrete at that point: JSScope *scope. It
> would be optional, so null for non-natives.
> 
> Non-natives (read array instances) should be free to use the word for their own
> needs like storing array elements.

Agreed.
 
> > but we could save one by getting rid of classword, folding class into ops one way or another.
> 
> I think for 1.9.2 we should consider merging JSObjectOps and JSClass into one
> entity. Those members can still live in the JSObjectOps struct to emphasis the
> non-public-and-free-to-change status.

We have some name conflicts (getProperty, etc.). The origins and design targets differ too, JSClass was lower-level with common hashtable machinery above it in the original design; JSObjectOps was the higher-level "switch" by which the Java to JS bridge could bypass the hashtable stuff.

Nevertheless, what you propose is expedient.

> If JSObjectOps would be added at the end of JSClass and all its members would
> default to NULL, it would be even source-compatible with 1.9.1 code defining
> JSClass instances.

On the other hand this re-exposes JSObjectOps or its embedded suffix of JSClass in the public API again.

The public API may be subject to more breaking changes over time from now on. We certainly want to break some APIs. JSClass itself, I'm not sure about -- it has not been directly targeted. JSExtendedClass, for sure.

The idea of using JSClass as a mostly-frozen descriptor for generating internal JSObjectOps that implement the specified JSClass "recipe" would have to involve either saving a JSClass pointer in such generated ops vectors, or else doing exactly what you propose here, copying the method pointers (along with name and class) into one superset structure.

We should take this up in the wiki and the make-over bug, but I wanted to write a reply to your comment here. This is a good discussion. Indeed we might want to first move to m.d.t.js-engine, since embedders to read that group and may have insights into their public-API dependencies that we don't have, along with embedding-specific wishlists.

If anyone reading this starts a thread, please leave a link here. Thanks.

/be
http://hg.mozilla.org/mozilla-central/rev/dead45971ca2
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
No longer blocks: 486321
Flags: wanted1.9.1? → wanted1.9.1+
No longer depends on: 494208
You need to log in before you can comment on or make changes to this bug.