1137497 - Remove shape_ from unboxed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

As described in bug 1137180, the shape_ field of unboxed objects can be repurposed to store information about extra properties that have been added to the object.  The attached patch removes JSObject::shape_, and adds it to the various direct subclasses of JSObject besides UnboxedPlainObject.  UnboxedPlainObject has a dummy field in its place, which will be filled in by bug 1137180.

In the future, other non-native objects could potentially also lose their shape_ field, though doing this depends on removing the generic uses of object shapes in place (after this patch, those places that call maybeShape() or ensureShape()).

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8570181 [details] [diff] [review]
patch

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

Looks good.

::: js/src/gc/Nursery.cpp
@@ +673,5 @@
>          tenuredSize += moveElementsToTenured(ndst, nsrc, dstKind);
> +
> +        /* The shape's list head may point into the old object. */
> +        if (&nsrc->shape_ == ndst->shape_->listp)
> +            ndst->shape_->listp = &ndst->shape_;

Shouldn't we also do this for InlineTypedObjects? If not, add a comment explaining why.

::: js/src/jsfriendapi.h
@@ +558,5 @@
>  };
>  
> +// This layout is shared by all native objects. For non-native objects, the
> +// group may always be accessed safely, and other members may be as well,
> +// depending on the object's specific layout.

GetObjectParent and GetObjectCompartment access obj->shape->base. Can we make shape() an accessor function that asserts group->class is not an unboxed object class? Then we can give GetObjectCompartment an unlined path just for unboxed objects.

::: js/src/vm/NativeObject.h
@@ +380,5 @@
>                        "inconsistent maximum object size");
>      }
>  
>    public:
> +    Shape * lastProperty() const {

Nit: Shape *lastProperty

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afda1ff329bf

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afda1ff329bf

Comment 4

[Daniel Holbert [:dholbert]]

Is it conceivable that this could've impacted the performance of long-running mochitests? (maybe by making GCs take longer under certain conditions, or get flushed more eagerly, or something)

In bug 1140502, I'm trying to see why some mochitests that used to take ~4 minutes are now frequently taking ~5 minutes (and causing test failures, since 5 minutes happens to be the default "mochitest timed out" threshold).  My retriggers are currently suggesting that this bug here is the main candidate for causing this change.

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Is it conceivable that this could've impacted the performance of
> long-running mochitests? (maybe by making GCs take longer under certain
> conditions, or get flushed more eagerly, or something)
> 
> In bug 1140502, I'm trying to see why some mochitests that used to take ~4
> minutes are now frequently taking ~5 minutes (and causing test failures,
> since 5 minutes happens to be the default "mochitest timed out" threshold). 
> My retriggers are currently suggesting that this bug here is the main
> candidate for causing this change.

This bug changes the layout of some objects that are part of a feature that is off by default (and not able to be turned on at all in the browser).  As part of this it changed the tracing paths for other objects somewhat, but not in a way that changes what gets traced by an object or the amount of time tracing should take.  So I don't think there is a legitimate reason this should have changed the amount of time a given mochitest takes, though with our testing infrastructure who really knows.  What happens with the affected tests when you test locally before/after this patch?

Comment 6

[Daniel Holbert [:dholbert]]

Thanks for the response, & sorry for troubling you spam -- more test-triggers have revealed that this was innocent after all. (The push before this one had a large number of retriggers without hitting the timeout that I'm investigating, but then more retriggers ended up hitting it eventually.)

I'm tagging comment 4 and 5 as obsolete, as well as this comment here, so they're autohidden & don't get in the way of the actual discussion (& any subsequent discussion) on this bug.