Last Comment Bug 684410 - Move JSObject::newType and TypeObject::emptyShapes to a hashtable
: Move JSObject::newType and TypeObject::emptyShapes to a hashtable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 698899
Blocks: ObjectShrink
  Show dependency treegraph
 
Reported: 2011-09-02 17:13 PDT by Brian Hackett (:bhackett)
Modified: 2011-12-07 16:46 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (38.69 KB, patch)
2011-10-07 20:10 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-09-02 17:13:04 PDT

    
Comment 1 Brian Hackett (:bhackett) 2011-09-02 17:15:41 PDT
JSObject::newType stores the default 'new' type object for a JS object, and that new type's TypeObject::emptyShapes stores the empty shapes of objects with the type.  These fields are only used for JSObjects which are prototypes of other objects, which are very sparse.  Both fields can be shaved by moving this state into a per-compartment hashtable, and shouldn't hurt GC (the table would hold weak references as with type inference stuff and would need to be traversed on GC, but we wouldn't need to inspect every single object's newType while marking).
Comment 2 Brian Hackett (:bhackett) 2011-10-07 20:10:53 PDT
Created attachment 565701 [details] [diff] [review]
patch

Move JSObject::newType to a hashtable.  Leaves TypeObject::emptyShapes alone (for now).

https://hg.mozilla.org/projects/jaegermonkey/rev/40f829990c82
Comment 3 Luke Wagner [:luke] 2011-10-11 09:35:19 PDT
Comment on attachment 565701 [details] [diff] [review]
patch

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

Looks generally good.  Obviously, this could hurt perf for objects created from C++ via the NewObject non-WithType path, but I assume you've already made all the hot paths use the -WithType path?

::: js/src/jscompartment.h
@@ +523,5 @@
> +    NewTypeObjectSet             newTypeObjects;
> +
> +    void sweepNewTypeObjectTable(JSContext *cx);
> +
> +    js::types::TypeObject        *emptyTypeObject;

Can you make emptyTypeObject private?

@@ +526,5 @@
> +
> +    js::types::TypeObject        *emptyTypeObject;
> +
> +    /* Get the default 'new' type for objects with a NULL prototype. */
> +    inline js::types::TypeObject *getEmptyType(JSContext *cx);

It seems like this could be made infallible by eagerly giving JSCompartment an emptyTypeObject.  Then JSObject::clearType could also be infallible.

::: js/src/jsinfer.cpp
@@ +5679,5 @@
> +
> +    if (!table.initialized())
> +        return false;
> +
> +    JSCompartment::NewTypeObjectSet::AddPtr p = table.lookupForAdd(this);

s/AddPtr/Ptr/ and s/lookupForAdd/lookup/
Comment 4 Brian Hackett (:bhackett) 2011-10-11 10:16:12 PDT
(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 565701 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 565701 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Looks generally good.  Obviously, this could hurt perf for objects created
> from C++ via the NewObject non-WithType path, but I assume you've already
> made all the hot paths use the -WithType path?

Most C++ object creation still passes the prototype rather than the type when creating objects.  As followup I'll be looking into whether hot creation sites need to be changed, particularly for JSON.  The hashtable lookup should cost a good deal less than FindClassPrototype, and places which cache prototypes to avoid FindClassPrototype can cache the type instead.

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