[ObjShrink] Get all new object empty shapes via a hash lookup

NEW
Assigned to

Status

()

Core
JavaScript Engine
7 years ago
7 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
With the current object shrink stuff in place, empty shapes for new objects can come from three places: a per-compartment hashtable for non-native objects and certain builtins (String/RegExp), empty shapes associated with the default 'new' types of prototypes, and one-off empty shapes created for objects with a class or parent mismatch with the prototype.  The latter two cases should be removed (removing TypeObject::emptyShapes), and all empty shape lookups should happen via a hash table.  This potentially adds another hashtable lookup to object creation paths, but will not slow down cases which hit the new object cache.

This is mostly to kill the last category of empty shapes, which can bloat the number of shapes created and inhibit ICs and other shape-based caching in the VM.  On a CSS query test based on one of the dromaeo tests, I hit the one-off shape case 45,000 times, with only 5,000 possible combinations of clasp/parent/proto.
(Assignee)

Comment 1

7 years ago
Created attachment 573713 [details] [diff] [review]
patch

https://hg.mozilla.org/projects/jaegermonkey/rev/8b9d7d9b2325
Assignee: general → bhackett1024
Attachment #573713 - Flags: review?(luke)

Comment 2

7 years ago
Comment on attachment 573713 [details] [diff] [review]
patch

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

You may want to grep const_cast to pick up any stragglers.

::: js/src/jsscope.cpp
@@ +1252,2 @@
>  /* static */ UnownedBaseShape *
>  BaseShape::lookup(JSContext *cx, const BaseShape &base)

Could you s/BaseShape::lookup/BaseShape::get/ and s/EmptyShape::lookupInitialShape/EmptyShape::getInitialShape/ in line with the previous-mentioned create/lookup/get naming distinction?
Attachment #573713 - Flags: review?(luke) → review+
(Assignee)

Comment 3

7 years ago
Created attachment 575061 [details] [diff] [review]
followup

Followup to fix an issue where we were not reusing initial shapes when changing attributes of the last property of an empty object --- a new shape was being created instead.  It is not uncommon to change the object flags on an object immediately after creating it (e.g. bug 703047 does this), and without this fix such objects essentially became de facto dictionaries.

https://hg.mozilla.org/projects/jaegermonkey/rev/fbfab0e75a63
Attachment #575061 - Flags: review?(luke)

Comment 4

7 years ago
Comment on attachment 575061 [details] [diff] [review]
followup

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

::: js/src/jsscope.cpp
@@ +1317,5 @@
>  
>      BaseShape base(*shape->base()->unowned());
>      base.flags |= BaseShape::EXTENSIBLE_PARENTS;
>  
> +    return replaceLastProperty(cx, base, NULL, listp);

Perhaps explicitly comment that "This is only called on bindings, which are the shapes of static block objects which have a null prototype".

@@ +1333,5 @@
>  Bindings::setParent(JSContext *cx, JSObject *obj)
>  {
>      if (!ensureShape(cx))
>          return false;
> +    return Shape::setObjectParent(cx, obj, NULL, &lastBinding);

and here "block objects have a null prototype"
Attachment #575061 - Flags: review?(luke) → review+
You need to log in before you can comment on or make changes to this bug.