Last Comment Bug 691192 - JSAutoEnterCompartment::enter shouldn't need to malloc()
: JSAutoEnterCompartment::enter shouldn't need to malloc()
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: general
:
Mentors:
Depends on: 688979
Blocks: 693393
  Show dependency treegraph
 
Reported: 2011-10-02 13:32 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-10-10 12:53 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch? v1 (264 bytes, patch)
2011-10-03 14:17 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch? v2 (3.62 KB, patch)
2011-10-03 14:27 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v3 (4.25 KB, patch)
2011-10-04 07:36 PDT, Justin Lebar (not reading bugmail)
luke: review+
Details | Diff | Splinter Review
Patch v4 (4.50 KB, patch)
2011-10-04 15:46 PDT, Justin Lebar (not reading bugmail)
luke: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-10-02 13:32:10 PDT
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) 2011-10-03 12:58:03 PDT
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) 2011-10-03 14:17:54 PDT
Created attachment 564326 [details] [diff] [review]
Patch? v1
Comment 3 Justin Lebar (not reading bugmail) 2011-10-03 14:27:50 PDT
Created attachment 564335 [details] [diff] [review]
Patch? v2

Non-empty patch this time.
Comment 4 Justin Lebar (not reading bugmail) 2011-10-03 14:28:37 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c8b634e73ca7
Comment 5 Justin Lebar (not reading bugmail) 2011-10-03 14:36:29 PDT
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) 2011-10-04 07:19:49 PDT
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) 2011-10-04 07:36:18 PDT
Created attachment 564547 [details] [diff] [review]
Patch v3
Comment 8 Luke Wagner [:luke] 2011-10-04 10:44:41 PDT
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) 2011-10-04 12:28:16 PDT
Thanks for the review, Luke!

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/02bc0a560a43
Comment 10 Justin Lebar (not reading bugmail) 2011-10-04 12:42:44 PDT
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) 2011-10-04 12:49:01 PDT
Ah, it's because sizeof(AutoCompartment) changed between then and now.
Comment 12 Justin Lebar (not reading bugmail) 2011-10-04 12:55:30 PDT
Er, no, it's because of

error: 'struct JSObject' has no member named 'getCompartment'
Comment 13 Justin Lebar (not reading bugmail) 2011-10-04 15:46:02 PDT
Created attachment 564692 [details] [diff] [review]
Patch v4

Un-burn Android.
Comment 14 Luke Wagner [:luke] 2011-10-04 15:48:28 PDT
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) 2011-10-04 18:31:39 PDT
Indeed!

Looks like third time was a charm.  https://hg.mozilla.org/integration/mozilla-inbound/rev/64f1fe963b51
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-05 05:09:42 PDT
https://hg.mozilla.org/mozilla-central/rev/64f1fe963b51

Note You need to log in before you can comment on or make changes to this bug.