The default bug view has changed. See this FAQ.

Move JSObject::newType and TypeObject::emptyShapes to a hashtable

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
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).
Crash Signature: 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 wh…
Blocks: 637931
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
Assignee: general → bhackett1024
Attachment #565701 - Flags: review?(luke)

Comment 3

6 years ago
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/
Attachment #565701 - Flags: review?(luke) → review+
(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.
Depends on: 698899
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.