Closed Bug 1146165 Opened 5 years ago Closed 5 years ago

Stop calling Proxy::set directly from Ion IC stub

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

Bug 1142794 makes this use another register, and there aren't any more registers.

So it's time for this to go; instead of calling Proxy::set we'll call a tiny and very nice little wrapper around Proxy::set.
Will this wrapper be able to inline Proxy::set in it?  Because function calls aren't free.  :(
Well -- that's *possible*: I can put the wrapper in proxy/Proxy.cpp and make it share an implementation with Proxy::set.

I have little enough confidence in my own reasoning about performance, but my gut says surely this function call doesn't matter. I'll try and measure a difference using cross-compartment wrappers (which I can test in the shell). If there's some other kind of wrapper I should be microbenchmarking instead (the outer window?) let me know.
Hmm.  I was going to say [n] on a DOM list, but this is just for _set_.

Given that, chances are the function call overhead is in fact not relevant.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
I will check the performance before landing this. Optimistically putting it into Eric's queue.
Something's wrong with the patch, but it won't repro for me locally. I have split the patch up into 7 parts for try-server "bisecting" (hepta-secting? bi-sept-ing?), but try is closed atm.
Jan found a big scary WARNING in the code I was touching. Turns out it was relevant.

diff --git a/js/src/jit/IonCaches.cpp b/js/src/jit/IonCaches.cpp
--- a/js/src/jit/IonCaches.cpp
+++ b/js/src/jit/IonCaches.cpp
@@ -2256,13 +2256,12 @@ EmitCallProxySet(JSContext *cx, MacroAss
     // Push stubCode for marking.
     attacher.pushStubCodePointer(masm);
 
+    masm.move32(Imm32(strict), argStrictReg);
+
     // Push args on stack so we can take pointers to make handles.
-    // Push value before touching any other registers (see WARNING above).
     masm.Push(value);
     masm.movePtr(StackPointer, argVpReg);
 
-    masm.move32(Imm32(strict), argStrictReg);
-
     masm.Push(propId, scratch);
     masm.movePtr(StackPointer, argIdReg);
Comment on attachment 8583437 [details] [diff] [review]
Stop calling Proxy::set directly from Ion IC stub. EmitObjectOpResultCheck is retained because GenerateCallSetter still uses it in the JSSetterOp case. try: -b d -p linux64 -u none -t none

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

Yeah, this is nicer than a whole host of jitcode to do the same thing as one line of C++. r=me, assuming it doesn't make awfy scream. I don't expect it will.
Attachment #8583437 - Flags: review?(efaustbmo) → review+
var g = newGlobal();
var objw = g.evaluate("({x: 13})");

function test(proxy) {
    var t0 = Date.now();
    for (var i = 0; i < 20000000; i++)
        proxy.x = 7;
    print(Date.now() - t0);
}

test(objw);

----

Without the patch:  1346 1334 1349 1360 1366 1345 1399 1405 1384 1398 => avg 1368.6ms
With the patch:     1373 1368 1378 1371 1347 1369 1450 1387 1440 1352 => avg 1383.5ms

With the patch is 1% slower on average, signifying nothing. I ran this earlier with
a slightly different script and with-the-patch was 1% faster.
https://hg.mozilla.org/mozilla-central/rev/18bdc63a2ca0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.