Closed Bug 784568 Opened 12 years ago Closed 12 years ago

IonMonkey: Simplify call paths

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

Details

Attachments

(1 file)

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.
Attached patch Sanitize calls.Splinter Review
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.
Attachment #654396 - Flags: review?(dvander)
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/
Attachment #654396 - Flags: review?(dvander) → review+
(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.
http://hg.mozilla.org/projects/ionmonkey/rev/df9b62214347
I'll open a separate bug for part 2 in deference to policy.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/projects/ionmonkey/rev/ecf3af6113dd
Fix templating issue on compilers other than my own.
You need to log in before you can comment on or make changes to this bug.