691192 - JSAutoEnterCompartment::enter shouldn't need to malloc()

Status: RESOLVED FIXED

Description

[Justin Lebar (not reading bugmail)]

According to my measurements, JSAutoEnterCompartment::enter() is responsible for about 1% of malloc calls in the browser.

Do we actually need to malloc an AutoCompartment here?  Can we just make space for the AutoCompartment inline in the JSAutoEnterCompartment and create the AutoCompartment with placement new?

Comment 1

[Justin Lebar (not reading bugmail)]

This is tricky because of header issues.  I wonder if there's a less-than-very-ugly way to do this.

Comment 2

[Justin Lebar (not reading bugmail)]

Created attachment 564326 [details] [diff] [review]
Patch? v1

Comment 3

[Justin Lebar (not reading bugmail)]

Created attachment 564335 [details] [diff] [review]
Patch? v2

Non-empty patch this time.

Comment 4

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=c8b634e73ca7

Comment 5

[Justin Lebar (not reading bugmail)]

As expected, the try push above burns on Windows because AutoCompartment has a different size there than on Linux...

Comment 6

[Justin Lebar (not reading bugmail)]

Seems like sizeof(AutoCompartment) == 13 * sizeof(void*) everywhere but 32-bit Windows, where it's sizeof(AutoCompartment) == 16 * sizeof(void*).

Comment 7

[Justin Lebar (not reading bugmail)]

Created attachment 564547 [details] [diff] [review]
Patch v3

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 564547 [details] [diff] [review]
Patch v3

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

Nice.  This punched me in the face when I first saw it; I didn't know whether it was worth optimizing at the time, I'm glad you looked :)

::: js/src/jsapi.cpp
@@ +1349,5 @@
> +{
> +    if (state == STATE_OTHER_COMPARTMENT) {
> +        AutoCompartment* ac = reinterpret_cast<AutoCompartment*>(bytes);
> +        CHECK_REQUEST(ac->context);
> +        ac->leave();

Although this is equivalent (actually a branch faster), I think it would be more future-proof to replace ac->leave() with ac->~AutoCompartment().

@@ +1354,5 @@
> +    }
> +}
> +
> +void
> +JSAutoEnterCompartment::swap(JSAutoEnterCompartment &other)

I just built a browser with swap removed, so I think you can remove it in this patch.

::: js/src/jsapi.h
@@ +2215,5 @@
>  class JS_PUBLIC_API(JSAutoEnterCompartment)
>  {
> +    // This is a poor man's Maybe<AutoCompartment>, because we don't have
> +    // access to the AutoCompartment definition here.  AutoCompartment is 13
> +    // words eveywhere except 32-bit Windows, where it's 16 words.  (We

I think you can leave out the second sentence; the code says it just fine.  The "We statically assert..." part is all we need.

Also, the comment style here and below should be:

/*
 * Multiline
 */

/* Single line */

Comment 9

[Justin Lebar (not reading bugmail)]

Thanks for the review, Luke!

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/02bc0a560a43

Comment 10

[Justin Lebar (not reading bugmail)]

Hm, the static assert failed on MacOS on inbound.  Which is strange, since it worked on Try!

Backed out for now.

Comment 11

[Justin Lebar (not reading bugmail)]

Ah, it's because sizeof(AutoCompartment) changed between then and now.

Comment 12

[Justin Lebar (not reading bugmail)]

Er, no, it's because of

error: 'struct JSObject' has no member named 'getCompartment'

Comment 13

[Justin Lebar (not reading bugmail)]

Created attachment 564692 [details] [diff] [review]
Patch v4

Un-burn Android.

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 564692 [details] [diff] [review]
Patch v4

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

::: js/src/jsapi.cpp
@@ +1320,5 @@
>      Foreground::delete_(realcall);
>  }
>  
> +template<int X>
> +struct x { int x[X]; };

I'm assuming this was meant to be removed :)

Comment 15

[Justin Lebar (not reading bugmail)]

Indeed!

Looks like third time was a charm.  https://hg.mozilla.org/integration/mozilla-inbound/rev/64f1fe963b51

Comment 16

[Marco Bonardo [::mak]]

https://hg.mozilla.org/mozilla-central/rev/64f1fe963b51