Last Comment Bug 784568 - IonMonkey: Simplify call paths
: IonMonkey: Simplify call paths
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-21 18:46 PDT by Sean Stangl [:sstangl]
Modified: 2012-08-22 17:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Sanitize calls. (29.66 KB, patch)
2012-08-22 15:45 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2012-08-21 18:46:03 PDT
The implementation of visitCallGeneric() is a huge, tangled mess. We can fix that:

1. Separate the hasSingleTarget() paths of visitCallGeneric() into a visitCallKnown() of some LCallKnown.

2. Attach a function pointer to each JSScript that is always Ion-callable. If no IonScript is attached, the pointer defaults to the equivalent of InvokeFunction() assuming an IonFramePrefix above it.

This eliminates 3 movePtr()s and 1 branchPtr() from the hot path in the known-case; calling a function becomes at most 6 branch-free instructions, 3 of which construct the IonFramePrefix. Additionally, the entire InvokeFunction path disappears. This should also be more semantically pleasing: functions are always called in the same way regardless of compilation state.

The generic case still must handle the ArgumentsRectifier, but that's only a single compare and branch on the hot path.
Comment 1 Sean Stangl [:sstangl] 2012-08-22 15:45:59 PDT
Created attachment 654396 [details] [diff] [review]
Sanitize calls.

Cleans up calling code. Hopefully this addresses yesterday's valid complaints that visitCallGeneric() is an unholy mess of evolutionary code. Although this patch duplicates some functionality for the sake of sanity, it deletes more lines than it adds.

This patch:
- Factors common LCallFoo code into an LFormalCallInstructionHelper, for all LIR derived from MCall.
- Adds an LCallKnown, so visitCallGeneric() doesn't have stupid branches all over the place.
- Makes visitCall() readable.
- Refactors visitCallGeneric() to finally only use a single safepoint.
Comment 2 David Anderson [:dvander] 2012-08-22 16:34:00 PDT
Comment on attachment 654396 [details] [diff] [review]
Sanitize calls.

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

This indeed looks simpler. When we talked offline I was imagining something like:

  struct JSScript {
      ....
      ion::IonCode *invoke;
  
Where |invoke| points to either a trampoline or ionScript->method()->raw(), which would remove a bunch of loads and a NULL check. I don't know how hard that is but it could simplify things even further.

::: js/src/ion/CodeGenerator.cpp
@@ +737,5 @@
> +
> +    // Argument fixed needed. Load the ArgumentsRectifier.
> +    JS_ASSERT(ArgumentsRectifierReg != objreg);
> +    masm.bind(&thunk);
> +    masm.movePtr(ImmGCPtr(argumentsRectifier), objreg); // Necessary for GC marking.

To make it clear this is a separate path, I like this pattern:

    masm.bind(&thunk);
    {
        ....
    }

::: js/src/ion/LIR-Common.h
@@ +442,5 @@
>  };
>  
> +// Common code for LIR descended from MCall.
> +template <size_t Defs, size_t Operands, size_t Temps>
> +class LFormalCallInstructionHelper : public LCallInstructionHelper<Defs, Operands, Temps>

s/Formal/JS/
Comment 3 Sean Stangl [:sstangl] 2012-08-22 16:40:12 PDT
(In reply to David Anderson [:dvander] from comment #2)
> This indeed looks simpler. When we talked offline I was imagining something
> like:
> 
>   struct JSScript {
>       ....
>       ion::IonCode *invoke;
>   
> Where |invoke| points to either a trampoline or ionScript->method()->raw(),
> which would remove a bunch of loads and a NULL check. I don't know how hard
> that is but it could simplify things even further.

Yeah, that's #2 on the list in Comment 0. It's way more complicated than this minor refactoring patch.
Comment 4 Sean Stangl [:sstangl] 2012-08-22 16:58:46 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/df9b62214347
I'll open a separate bug for part 2 in deference to policy.
Comment 5 Sean Stangl [:sstangl] 2012-08-22 17:55:59 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/ecf3af6113dd
Fix templating issue on compilers other than my own.

Note You need to log in before you can comment on or make changes to this bug.