Closed Bug 691192 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

(Whiteboard: [inbound])

Attachments

(2 files, 2 obsolete files)

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?
Summary: JSAutoEnterCompartment::enter shouldn't need to malloc(), should it? → JSAutoEnterCompartment::enter shouldn't need to malloc()
This is tricky because of header issues. I wonder if there's a less-than-very-ugly way to do this.
Attached patch Patch? v1 (obsolete) — Splinter Review
Attached patch Patch? v2 (obsolete) — Splinter Review
Non-empty patch this time.
Attachment #564326 - Attachment is obsolete: true
As expected, the try push above burns on Windows because AutoCompartment has a different size there than on Linux...
Seems like sizeof(AutoCompartment) == 13 * sizeof(void*) everywhere but 32-bit Windows, where it's sizeof(AutoCompartment) == 16 * sizeof(void*).
Attached patch Patch v3Splinter Review
Attachment #564547 - Flags: review?(luke)
Attachment #564335 - Attachment is obsolete: true
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+
Whiteboard: [inbound]
Hm, the static assert failed on MacOS on inbound. Which is strange, since it worked on Try! Backed out for now.
Whiteboard: [inbound]
Ah, it's because sizeof(AutoCompartment) changed between then and now.
Er, no, it's because of error: 'struct JSObject' has no member named 'getCompartment'
Attached patch Patch v4Splinter Review
Un-burn Android.
Attachment #564692 - Flags: review?(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 :)
Attachment #564692 - Flags: review?(luke) → review+
Whiteboard: [inbound]
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Blocks: 693393
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: