The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Unassigned)

Tracking

(Depends on: 1 bug)

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Summary: JSAutoEnterCompartment::enter shouldn't need to malloc(), should it? → JSAutoEnterCompartment::enter shouldn't need to malloc()
(Reporter)

Comment 1

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

Comment 2

6 years ago
Created attachment 564326 [details] [diff] [review]
Patch? v1
(Reporter)

Comment 3

6 years ago
Created attachment 564335 [details] [diff] [review]
Patch? v2

Non-empty patch this time.
(Reporter)

Updated

6 years ago
Attachment #564326 - Attachment is obsolete: true
(Reporter)

Comment 4

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c8b634e73ca7
(Reporter)

Comment 5

6 years ago
As expected, the try push above burns on Windows because AutoCompartment has a different size there than on Linux...
(Reporter)

Comment 6

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

Comment 7

6 years ago
Created attachment 564547 [details] [diff] [review]
Patch v3
Attachment #564547 - Flags: review?(luke)
(Reporter)

Updated

6 years ago
Attachment #564335 - Attachment is obsolete: true

Comment 8

6 years ago
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 */
Attachment #564547 - Flags: review?(luke) → review+
(Reporter)

Comment 9

6 years ago
Thanks for the review, Luke!

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/02bc0a560a43
Whiteboard: [inbound]
(Reporter)

Comment 10

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

Backed out for now.
Whiteboard: [inbound]
(Reporter)

Comment 11

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

Comment 12

6 years ago
Er, no, it's because of

error: 'struct JSObject' has no member named 'getCompartment'
(Reporter)

Comment 13

6 years ago
Created attachment 564692 [details] [diff] [review]
Patch v4

Un-burn Android.
Attachment #564692 - Flags: review?(luke)

Comment 14

6 years ago
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 :)
Attachment #564692 - Flags: review?(luke) → review+
(Reporter)

Comment 15

6 years ago
Indeed!

Looks like third time was a charm.  https://hg.mozilla.org/integration/mozilla-inbound/rev/64f1fe963b51
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/64f1fe963b51
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Reporter)

Updated

6 years ago
Blocks: 693393
You need to log in before you can comment on or make changes to this bug.