Related to bug #813773, but this report is to decrease the delta between V8 and IM/JM+TI. Using the following pattern to create an object, every construction of a new object will have an enormous overhead in comparison to V8 (about 3x slower). var Class = { create: function() { return function() { this.initialize.apply(this, arguments); } } }; var Test = Class.create(); Test.prototype = { initialize : function() {} } new Test(); // Every call is now 3x slower than V8! If we could be smarter and transform this code to: var Test = function() { this.initialize(); } Test.prototype = { initialize : function() {} } new Test(); // Every call is now only 2x slower than V8! This would increase our score on v8-raytrace from 7612 to 9158 !!

(In reply to Hannes Verschore [:h4writer] from comment #0) > Related to bug #813773, but this report is to decrease the delta between V8 > and IM/JM+TI. > Using the following pattern to create an object, every construction of a new > object will have an enormous overhead in comparison to V8 (about 3x slower). > > var Class = { > create: function() { > return function() { > this.initialize.apply(this, arguments); > } > } > }; This pattern seems to be used in some Class libraries on the Web, it would make sense to detect them and inline the call of the targeted function. Which will prevent making 2 calls, including one which attempts at copying the arguments on the stack. The problem of inlining such function is that the snapshot still has to account for the lambda frame to restore the overflow of arguments after the fact, unless we want to break it, which might be fine for unnamed lambda.

It looks like V8's funapply optimization can take two paths: (1) The outer function is not being inlined. This is the only case we can handle, and like us they generate an ApplyArguments + Call instruction. (2) The outer function *is* being inlined, in which case they bypass ApplyArguments, push each individual argument, and then perform a normal invoke. They do not inline through the callee. Case (2) is what appears to happen in the above benchmark.

(In reply to David Anderson [:dvander] from comment #2) > Case (2) is what appears to happen in the above benchmark. Quick and dirty hack is promising: 7500 to 8500: 13% increase Still need to fix some issues and I hope I can post a patch tomorrow or Monday. Interesting part is that we can inline the outer function with minor extra work now! Haven't tested that yet, but I estimate it will give us the 9100 I reported in my first comment. I.e. another 6% increase on top of the first patch.

Has the same performance improvement on v8-raytrace as the quick and dirty hack. I.e. 11%-13% increase.

Comment on attachment 687956 [details] [diff] [review] Allow inlining of |arguments| in IM Review of attachment 687956 [details] [diff] [review]: ----------------------------------------------------------------- Try to avoid having similar code spread, either factor it or move it near other functions (makeCallHelper). I don't see any code for handling the recovery of arguments from snapshots. Check the recovery of the arguments vector either after a bailout or during the inlined frame activation. ::: js/src/ion/IonBuilder.cpp @@ +3724,5 @@ > passVp->replaceAllUsesWith(passVp->getArgument()); > passVp->block()->discard(passVp); > > + // When this script isn't inlined, use MApplyArgs, > + // to copy the arguments from the stack and call the function nit: remove trailing white space @@ +3759,5 @@ > + // When inlining we have the arguments the function gets called with > + // and can optimize even more, by just calling the functions with the args. > + JS_ASSERT(inliningDepth > 0); > + > + uint32 numActualArgs = inlinedArguments_.length(); nit: remove trailing white space @@ +3767,5 @@ > + // scripted. Native functions are passed an explicit 'argc' parameter. > + if (target && !target->isNative()) > + targetArgs = Max<uint32>(target->nargs, numActualArgs); > + > + MCall *call = MCall::New(target, targetArgs + 1, targetArgs, false); This is similar to MCallHelper and MCallBarrier. Try to share the logic with these functions, and at least move this into a function with similar names. ::: js/src/jit-test/tests/ion/bug813784.js @@ +1,5 @@ > +/* Test an inlined argument returns the arguments from the right function */ > +function get_arg_2() { return arguments[2]; } > +function test() { return get_arg_2(1,2,3); } > + > +for (var i=0; i < 20000; i++) { test cases are run with --no-jm which reduce the Ion trigger to ~40, you can use a loop to 100 instead. @@ +52,5 @@ > +function apply_fun1(a, b, c) { assertEq(c, undefined) } > +function funapply3() { return apply_fun1.apply({}, arguments); } > +function test7() { return funapply3(1,2) } > + > +for (var i=0; i < 100000; i++) { Why such a huge limit? @@ +59,5 @@ > + > +/* Test funapply when argument vector has less args than callee and callee unknown */ > +var fun; > +function apply_fun2(a, b, c) { assertEq(c, undefined) } > +function funapply4() { return fun.apply({}, arguments); } fun.apply ? @@ +65,5 @@ > + > +for (var i=0; i < 20000; i++) { > + fun = (i%2 == 0) ? apply_fun1 : apply_fun2; > + test8("a","b","c","d","e"); > +} Add a case where we bailout from the funapply() function and also where we dump the funapply function arguments: function dumpArgs(i) { if (i == 90) return funapply5.arguments; return [i]; } function funapply5() { return foo.apply({}, arguments); } function test9(i) { return funapply5(i); } var bailout = Proxy.createFunction({}, Math.sin); function bailing_arg_len() { if (i == 90) { bailout(); arg_len.apply({}, arguments); } return arguments.length; } function test10() { bailing_arg_len(1,2,3,4) } AssertEq(, test10("a","b").length == 4)

Comment on attachment 694297 [details] [diff] [review] Inlining |arguments| in IM Review of attachment 694297 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/bug813784.js @@ +86,5 @@ > + assertEq(arguments[1], 5); > + assertEq(arguments[2], 6); > +} > + > +function inline2(a) { return notinlined(4,5,6); Add closing bracket @@ +122,5 @@ > + > +////////////////// > + > +var bailout = Proxy.createFunction({}, Math.sin); > +function arg_len() { assertEq(arguments.length, 4); } s/arg_len/arg_len2/ @@ +126,5 @@ > +function arg_len() { assertEq(arguments.length, 4); } > +function bailing_arg_len(a) { > + if (a == 90) { > + bailout(); > + arg_len.apply({}, arguments); s/arg_len/arg_len2/

Comment on attachment 694297 [details] [diff] [review] Inlining |arguments| in IM Review of attachment 694297 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonFrameIterator-inl.h @@ +72,5 @@ > + SnapshotIterator s((++it).snapshotIterator()); > + > + // Skip over all slots untill we get to the arguments slots > + // the +2 is for [this] and [scopechain] > + JS_ASSERT(s.slots() > nactual - 2); This should be: JS_ASSERT(s.slots() >= actual + 2)

Comment on attachment 694297 [details] [diff] [review] Inlining |arguments| in IM Review of attachment 694297 [details] [diff] [review]: ----------------------------------------------------------------- This is a good way to go. I noted a few nits below. As discuss IRL, you need to take care of f.arguments, at least for a case where this is done within "f" like: function f(i) { if (i == 90) return f.arguments[0]; return i; } For both cases where f is inlined or not. Once f.arguments is executed f should invalidate it and never execute it into Ion as we forbid Ion compilation. ::: js/src/ion/IonBuilder.cpp @@ +443,5 @@ > + // Discard first argument, because it is |this| > + for (size_t i = 1; i < argv.length(); i++) { > + if (!inlinedArguments_.append(argv[i])) > + return false; > + } inlinedArguments_.append(argv.begin() + 1, argv.end()); @@ +4077,5 @@ > + JS_ASSERT(inliningDepth > 0); > + > + // TODO: add comment about the order of arguments on MakeCall > + Vector<MPassArg *> args(cx); > + for (int32_t i = 0; i < inlinedArguments_.length(); i++) { Add the following before the for loop: args.reserve(inlinedArguments_.length()); To prevent multiple temp re-allocations. @@ +4140,5 @@ > +IonBuilder::popFormals(uint32_t argc, MDefinition **fun, MPassArg **thisArg, > + Vector<MPassArg *> *args) > +{ > + // Get the arguments in the right order > + for (int32_t i = argc; i > 0; i--) Same thing, reserve the space before appending any elements. ::: js/src/ion/IonFrames.cpp @@ +894,5 @@ > + script_(NULL) > +{ > + if (frame_) { > + start_ = SnapshotIterator(*frame_); > + framesRead_ = iter->framesRead_ - 1; Add a comment to mention that -1 is needed because we want to settle on the same frame and findNextFrame will increment the number of frame read.

No warnings during execution anymore \0/. Somehow (++t).foo() on a InlineFrameIterator doesn't equal "++it; it.foo()", but "it.foo()", so we were looking at the wrong values. All tests passes. We now also invalidate when we see f.arguments and already have an IonScript. This was the intention, but this code went bogus over time and only disabled the script when there was no Ion script yet.

Comment on attachment 694765 [details] [diff] [review] Inlining |arguments| in IM Review of attachment 694765 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.cpp @@ +5752,5 @@ > bool > IonBuilder::jsop_arguments_getelem() > { > + if (inliningDepth != 0) > + return abort("NYI inlined get argument element"); Sadly, we have no way to check if the inlined function has this ahead of time. The analysis phase does not know if the object is a lazy argument object or not. We should open a follow-up bug to disable Ion inlining for such function without forbidding the Ion compilation if the function which is inlining this function. another way to do it, would be to detect this case in a previous compilation of the inlined script, like while compiling it in JM / Ion and flag this script as non-Ion-Inlinable. (already opened Bug 823884) ::: js/src/ion/IonFrameIterator-inl.h @@ +60,5 @@ > > JS_ASSERT(start <= end && end <= nactual); > > + if (more()) { > + // There is still a parent frame in this inlined frame. nit: s/in/of/ @@ +64,5 @@ > + // There is still a parent frame in this inlined frame. > + // That is the caller of the requested function. > + // Because the caller pushes all arguments, we want to take this > + // to reconstruct the arguments, instead of on the callee side, > + // where we don't have the overflown arguments. comment nit: Take arguments of the caller (parent inlined frame) it holds all actual arguments, needed in case of overflow, and because the analyze phase disable Ion inlining if the function redefine its arguments with JSOP_SETARG. notice: You should consider forbidding the inlining if the script has a JSOP_SETARG and if the frames need an argument object. If this does not have any benchmark impact, you should open a follow-up bug which depends on Bug 823884. @@ +69,4 @@ > > + // Take the next frame > + InlineFrameIterator it(this); > + it.findNextFrame(); why not using ++it here? ::: js/src/jsfun.cpp @@ +130,4 @@ > // fully recovered, so we try to mitigate observing this behavior by > // detecting its use early. > UnrootedScript script = iter.script(); > + ion::ForbidCompilation(cx, script); This sounds good to me to penalize people using “fun.arguments”. ::: js/src/jsscript.h @@ +566,5 @@ > return ion && ion != ION_DISABLED_SCRIPT && ion != ION_COMPILING_SCRIPT; > } > > bool canIonCompile() const { > + //printf("test script disabled: %d - %d\n", lineno, ion == ION_DISABLED_SCRIPT); nit: remove this comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/662f338798a9 (In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #12) > notice: You should consider forbidding the inlining if the script has a > JSOP_SETARG and if the frames need an argument object. If this does not > have any benchmark impact, you should open a follow-up bug which depends on > Bug 823884. As discussed on IRC, we already disabled inlining on JSOP_SETARG. In any case enabling it could prove to give a boost on earley-boyer. As it is used there. Atleast if we enable arguments_get_elem again.