Closed Bug 1281450 Opened 4 years ago Closed 4 years ago

Speed up receiver wrapping in CrossCompartmentWrapper::{get, set}

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
While profiling bug 816159, I noticed we spend a fair amount of time wrapping the receiver in CrossCompartmentWrapper::get. Most of the time |receiver == wrapper|, so we can optimize this case by just unwrapping the CCW.

For the shell micro-benchmark below I get:

before: 672 ms
after:  430 ms

function f() {
    var g = newGlobal();
    g.eval("o = {x: 1}");
    var p = g.o;
    var res = 0;
    var t = new Date;
    for (var i=0; i<10000000; i++)
	res += p.x;
    print(new Date - t);
    return res;
}
f();
Attachment #8764225 - Flags: review?(efaustbmo)
Comment on attachment 8764225 [details] [diff] [review]
Patch

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

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ +176,5 @@
> +WrapReceiver(JSContext* cx, HandleObject wrapper, MutableHandleValue receiver)
> +{
> +    // Usually the receiver is the wrapper and we can just unwrap it.
> +    if (ObjectValue(*wrapper) == receiver) {
> +        receiver.setObject(*Wrapper::wrappedObject(wrapper));

No security concerns here, since we're in CCW::foo.
Attachment #8764225 - Flags: review?(efaustbmo) → review+
Attached patch Patch v2Splinter Review
Try uncovered a problem with the previous patch: if the unwrapped object is also a wrapper, UncheckedUnwrap called by compartment->wrap will unwrap it as well.

So to match that behavior, I added a check to not use the fast path if we get another wrapper.
Attachment #8764225 - Attachment is obsolete: true
Attachment #8765528 - Flags: review?(efaustbmo)
Comment on attachment 8765528 [details] [diff] [review]
Patch v2

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

Huh, I never considered how CCWs and Xray interacted until now. I guess that makes sense. I had just assumed that everything was flat. Nice find.
Attachment #8765528 - Flags: review?(efaustbmo) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a45e92e6ebe3
Speed up wrapping of the receiver object in CrossCompartmentWrapper::{get, set}. r=efaust
https://hg.mozilla.org/mozilla-central/rev/a45e92e6ebe3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.