Closed
Bug 691192
Opened 14 years ago
Closed 14 years ago
JSAutoEnterCompartment::enter shouldn't need to malloc()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: justin.lebar+bug, Unassigned)
References
Details
(Whiteboard: [inbound])
Attachments
(2 files, 2 obsolete files)
4.25 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
4.50 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
Summary: JSAutoEnterCompartment::enter shouldn't need to malloc(), should it? → JSAutoEnterCompartment::enter shouldn't need to malloc()
Reporter | ||
Comment 1•14 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•14 years ago
|
||
Reporter | ||
Comment 3•14 years ago
|
||
Non-empty patch this time.
Reporter | ||
Updated•14 years ago
|
Attachment #564326 -
Attachment is obsolete: true
Reporter | ||
Comment 4•14 years ago
|
||
Reporter | ||
Comment 5•14 years ago
|
||
As expected, the try push above burns on Windows because AutoCompartment has a different size there than on Linux...
Reporter | ||
Comment 6•14 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•14 years ago
|
||
Attachment #564547 -
Flags: review?(luke)
Reporter | ||
Updated•14 years ago
|
Attachment #564335 -
Attachment is obsolete: true
![]() |
||
Comment 8•14 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•14 years ago
|
||
Thanks for the review, Luke!
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/02bc0a560a43
Whiteboard: [inbound]
Reporter | ||
Comment 10•14 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•14 years ago
|
||
Ah, it's because sizeof(AutoCompartment) changed between then and now.
Reporter | ||
Comment 12•14 years ago
|
||
Er, no, it's because of
error: 'struct JSObject' has no member named 'getCompartment'
![]() |
||
Comment 14•14 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•14 years ago
|
||
Indeed!
Looks like third time was a charm. https://hg.mozilla.org/integration/mozilla-inbound/rev/64f1fe963b51
Whiteboard: [inbound]
Comment 16•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•