Closed Bug 1256298 Opened 4 years ago Closed 4 years ago

Make DoCallFallback consume a bit less stack space


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: jandem, Assigned: jandem)




(1 file)

Attached patch PatchSplinter Review
Noticed this while looking into bug 1247496.

This patch changes DoCallFallback to call the Invoke() overload that takes CallArgs instead of separate arguments, to get rid of an unnecessary copy/call.

I also changed it to call ConstructFromStack instead of Construct. It's shorter and we can get rid of the ConstructArgs copy on the stack.
Attachment #8730181 - Flags: review?(jwalden+bmo)
Comment on attachment 8730181 [details] [diff] [review]

Review of attachment 8730181 [details] [diff] [review]:

This'll muck up two of my patches in bug 1259877.  Serves me right for taking so long to do this.

::: js/src/jit/BaselineIC.cpp
@@ +6134,5 @@
>      if (!stub->addMonitorStubForValue(cx, &info, res))
>          return false;
>      // If 'callee' is a potential Call_StringSplit, try to attach an
>      // optimized StringSplit stub.

Maybe worth noting |vp[0]| that was formerly the callee has been overwritten at this point.

::: js/src/vm/Interpreter.h
@@ +89,5 @@
>  Construct(JSContext* cx, HandleValue fval, const ConstructArgs& args, HandleValue newTarget,
>            MutableHandleObject objp);
> +extern bool
> +ConstructFromStack(JSContext* cx, const CallArgs& args);

Add a comment:

Check that in the given |args|, which must be |args.isConstructing()|, that |IsConstructor(args.callee())|.  If this is not the case, throw a TypeError.  Otherwise, the user must ensure that, additionally, |IsConstructor(args.newTarget())|.  (If |args| comes directly from the interpreter stack, as set up by JSOP_NEW, this comes for free.)  Then perform a Construct() operation using |args|.

This internal operation is intended only for use with arguments known to be on the JS stack, or at least in carefully-rooted memory.  The vast majority of potential users should instead use ConstructArgs in concert with Construct().
Attachment #8730181 - Flags: review?(jwalden+bmo) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1279853
You need to log in before you can comment on or make changes to this bug.