Closed Bug 1287161 Opened 4 years ago Closed 4 years ago

Add TraceLogger support in Baseline and ICs; expand coverage of VM calls

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mismith, Assigned: mismith)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

71.81 KB, patch
Details | Diff | Splinter Review
12.88 KB, patch
Details | Diff | Splinter Review
I needed to collect performance data for VM calls within Baseline and the ICs, so I implemented emitTraceLoggerEvent{Start/Stop} for BaselineCompiler and SharedIC. I also went through and added names to all FunctionInfo instances.

All VM calls should be tracked now, except for two places:

- I wasn't sure how to properly set this up around tailCallVM. Advice welcome!
- The Assembler files for different platforms have a VM call for "HandleDebugTrap". I just haven't got around to implementing TraceLogging for these call sites yet.

Incidentally, this fixes an issue where the argument order for BaselineScript::New was out of sync between the .h and .cpp files.
Assignee: nobody → mismith
Status: NEW → ASSIGNED
Sorry for how long this patch is! A lot of it is just adding names to the FunctionInfo.
Comment on attachment 8771484 [details] [diff] [review]
Add TraceLogging to Baseline and ICs; record more VM calls

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

Thanks for doing this! That will be a major addition.

I didn't do a full review, since I'm gonna give some feedback that might change the approach.
Therefore only giving feedback.

A VM call does:
(1) ion/baseline/... -> (2) VM Wrapper code -> (3) C++ function

For Ion it made sense to add the probing at (1). Because this makes it (eventually) possible to dynamical enable/disable the logging for VM calls.
Ion just discards the IonScript when we change what we want to log at runtime. Little caveat: it is not possible currently. You cannot switch VMSpecific at runtime currently.

Going to baseline we also don't have that benefit. And tail calls really makes it even harder.
Given this outcome I think we can accept that it won't be possible to enable/disable VMSpecific/VM at runtime (for now).

As a result this simplifies everything a bit. We can just add the logging in (2) and we will automatically have ion vm calls / baseline vm calls and also baseline tail calls.
The wrapper code is generated in the function "generateVMWrapper", specific for every platform.
- Can you move the code there? 
- And remove the tracelogger code for VM and VMSpecific in the ion callVM?
- And open a follow-up bug to make it toggle-able at runtime and cc me? (I can explain how to do that or you can focus on other things and I'll do it one day ;))
Attachment #8771484 - Flags: review?(hv1989) → feedback+
I'm working on this again right now. To start with, I've split out adding names to all VM FunctionInfo (and removing the unknown name constructors from FunctionInfo) as a separate patch.
Attachment #8771484 - Attachment is obsolete: true
Attachment #8778496 - Flags: review?(hv1989)
I've created bug 1292710 as the follow-up.
No longer blocks: 1292710
Comment on attachment 8778496 [details] [diff] [review]
Part 1: Add names to all VM FunctionInfo; remove unknown name constructor

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

::: js/src/jit/VMFunctions.h
@@ -48,5 @@
>      TailCall,
>      NonTailCall
>  };
>  
> -static const char UnknownVMFunction[]        = "Unknown VM call";

awesome we cannot have not-annotated vm calls anymore!
Attachment #8778496 - Flags: review?(hv1989) → review+
Comment on attachment 8778506 [details] [diff] [review]
Part 2: Move TraceLogger VM events into generateVMWrapper

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

Since this is the same code on all platforms, can we share it?

Create private "JitRuntime::generateTLEnterVM" and "JitRuntime::generateTLEnterVM" and put the code in jit/CodeGenerator.cpp.
And call these functions in generateVMCall on the appropriate places.

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +839,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStartId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

Put this in JitRuntime::generateTLEnterVM
and call generateTLEnterVM here.

@@ +889,5 @@
> +            masm.tracelogStopId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif
> +

Put this in JitRuntime::generateTLExitVM
and call generateTLExitVM here.

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +658,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStartId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

@@ +706,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStopId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

::: js/src/jit/mips32/Trampoline-mips32.cpp
@@ +818,5 @@
> +            masm.tracelogStartId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif
> +

ditto

@@ +878,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStopId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +778,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStartId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

@@ +825,5 @@
> +            masm.tracelogStopId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif
> +

ditto

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +735,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStartId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

@@ +780,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStopId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +756,5 @@
> +        if (vmSpecificEventEnabled)
> +            masm.tracelogStartId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif

ditto

@@ +809,5 @@
> +            masm.tracelogStopId(loggerReg, event.payload()->textId(), /* force = */ true);
> +        masm.Pop(loggerReg);
> +    }
> +#endif
> +

ditto
Attachment #8778506 - Flags: review?(hv1989) → review+
Okay, I've made the TL code shared between platforms.
Attachment #8778506 - Attachment is obsolete: true
Attachment #8779049 - Flags: review?(hv1989)
Comment on attachment 8779049 [details] [diff] [review]
Part 2: Move TraceLogger VM events into generateVMWrapper

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

Thanks! Good work.
Attachment #8779049 - Flags: review?(hv1989) → review+
has problems to apply:

Hunk #6 FAILED at 1298
1 out of 45 hunks FAILED -- saving rejects to file js/src/jit/BaselineCompiler.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh file_1287161.txt
Flags: needinfo?(mismith)
Keywords: checkin-needed
Attachment #8779049 - Attachment is obsolete: true
Flags: needinfo?(mismith)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df4b1bf3a37
Add names to all VM FunctionInfo; remove unknown name constructor. r=hv1989
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e56bed23225
Move TraceLogger VM events into generateVMWrapper. r=hv1989
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7df4b1bf3a37
https://hg.mozilla.org/mozilla-central/rev/9e56bed23225
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.