1271929 - Don't use ReadBarrieredObject for PendingMetadata

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

When allocating arguments objects, the post barrier fired under setObjectPendingMetadata adds significant overhead (we just allocated the object so most of the time it require a store buffer edge).

Because we mark this object as root (in JSCompartment::traceRoots and AutoSetNewObjectMetadata::trace - can we remove one of these?), I think it's fine to use a raw JSObject* instead of ReadBarrieredObject.

For the Foo1 microbenchmark in bug 1271473:

before: 152 ms
after:   95 ms

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8751208 [details] [diff] [review]
Patch

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

Thanks! Impressive results.

It might be easier to make PendingMetadata a PersistentRooted<JSObject*>, and then we could remove all the hand-rolled (duplicate!?) tracing and use the standard GC infrastructure. If putting the rooted inside the variant doesn't work due to various ctors not being present, then we could make a GCPolicy specialization for the whole of NewObjectMetadataState and change the compartment's pending metadata to be a PersistentRooted<NewObjectMetadataState>. Up to you if you feel like it is worth it or not, but this patch could definitely land as-is (barring comment about updating pointers) too.

::: js/src/jscompartment.cpp
@@ +703,5 @@
>      if (objectMetadataState.is<PendingMetadata>()) {
>          // We should never finalize an object before it gets its metadata! That
>          // would mean we aren't calling the object metadata callback for every
>          // object!
> +        MOZ_ASSERT(!IsAboutToBeFinalizedUnbarriered(&objectMetadataState.as<PendingMetadata>()));

This needs to be MOZ_ALWAYS_TRUE so that it always runs and updates the pointer if it moved, right?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #1)
> > +        MOZ_ASSERT(!IsAboutToBeFinalizedUnbarriered(&objectMetadataState.as<PendingMetadata>()));
> 
> This needs to be MOZ_ALWAYS_TRUE so that it always runs and updates the
> pointer if it moved, right?

Hm good point. Why do we need this code though? We mark this object as root and AFAIK that's enough to make sure the object is moved?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #1)
> > > +        MOZ_ASSERT(!IsAboutToBeFinalizedUnbarriered(&objectMetadataState.as<PendingMetadata>()));
> > 
> > This needs to be MOZ_ALWAYS_TRUE so that it always runs and updates the
> > pointer if it moved, right?
> 
> Hm good point. Why do we need this code though? We mark this object as root
> and AFAIK that's enough to make sure the object is moved?

Ah, yes. That makes sense. This is why I would prefer using the standard GCPolicy + PersistentRooted infrastructure longer term...

Comment 4

[Jan de Mooij [:jandem]]

Thanks for the fast review btw.

I like the PersistentRooted suggestion, but I think I'll leave that alone for now - a bunch of other things on my plate atm :)

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/764fd30b53e1

Comment 6

[Guilherme Lima]

This patch improved a few tests from Sunspider and Kraken on AWFY!

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/764fd30b53e1