Jeff Muizelaar noticed a severe preformance degradation on our Octane scores when profiling is enabled (~9000 with profiling, ~18000 without). Talked to Benoit and he feels this is not an acceptable level of perf degradation for profiling. He strongly recommends disabling of profiling of inline frames. For my part, I feel that being able to profile inlined frames is useful. Benoit suggested a compromise of having a pref (off by default) that enabled profiling of inlined ion frames. This sounds reasonable to me.

A related idea: in bug 947996 I'm planning to add a new type of SPS entry that describes a contiguous set of asm.js function calls such that the profiler can walk this region of the stack using a [return-pc -> stack-frame-size] map. I was thinking that Ion could do the same type of thing and avoid all this runtime shadow-stack pushing/popping from jit code. I think we already keep much of this information for ScriptFrameIter anyhow.

Is bug 841646 relevant here? The workaround to disable parallel compilation when the SPS profiler is enabled still seems to be in place: https://hg.mozilla.org/mozilla-central/annotate/383ccc9eb488/js/src/ion/Ion.cpp#l1372

(In reply to Luke Wagner [:luke] from comment #1) > A related idea: in bug 947996 I'm planning to add a new type of SPS entry > that describes a contiguous set of asm.js function calls such that the > profiler can walk this region of the stack using a [return-pc -> > stack-frame-size] map. I was thinking that Ion could do the same type of > thing and avoid all this runtime shadow-stack pushing/popping from jit code. > I think we already keep much of this information for ScriptFrameIter anyhow. So I suppose we'd be pushing this new entry type only on full function activations (ignoring inline activations), and the profiler would gather the inline frame info from the returnaddr map? And presumably this new SPS entry would be pushed/popped in sync with the jitcode activation? This should be easier for Ion to implement than the "ideal" solution (maintain frame pointers + static table of jitcode regions to logical call stack). Odin has an easier time implementing the ideal solution as it doesn't invalidate jitcode. Ion would have to maintain the static table in coordination with the profiler so that table entries for jitcode that has been discarded are kept around until we know that the profiler won't need them anymore.

(In reply to Kannan Vijayan [:djvj] from comment #3) The approach I was going to take was to push a single SPS frame per asm.js *module* activation (that is, one per contiguous set of intra-module asm.js->asm.js function calls), storing the immutable [return-pc -> stack-frame-size] mapping in the AsmJSModule. You're right, that Ion would have a harder time since the mapping would be runtime- or compartment-wide and mutable which means you'd have to deal with race conditions with the profiler. One idea is that we could have a disable/enable interrupts hook that the we'd call to guard the mutation of the [return-pc -> stack-frame-size] map. On Unices, we'd just pthread_sigmask-disable SIGPROF; on OSX/Windows it looks like we have a sampler thread that manually pauses/get-contexts the other threads so we should be able to set some global atomic variable that says "hold off". Another question is whether we'd need to discard all jit code when profiling is activated like we do now. Certainly it'd be nice to avoid this (both from a complexity and usability pov), but I wonder how expensive this table would be to maintain all the time. One idea is that we could probably rebuild this table when profiling is enabled by iterating over all IonScripts.

Initial patch to stop profiling inline frames. Turns it into an option on js_JitOptions. Option is always false for now. Checking against try: https://tbpl.mozilla.org/?tree=Try&rev=8f2d8b7e5fa5

Rebased to tip and re-tried, looking pretty good: https://tbpl.mozilla.org/?tree=Try&rev=f18305d1721a The oranges from the previous run seem to be intermittents.

Tested in browser with a simple testcase. The inlined function which should be running ~60% of the time shows up in profiles now as running for 1% of the time. These remaining hits are most likely due to time in execution spent in the interpreter and in baseline.

Comment on attachment 8361267 [details] [diff] [review] stop-profiling-inline-frames.patch Review of attachment 8361267 [details] [diff] [review]: ----------------------------------------------------------------- 1) It seems that atm it can only get flipped by recompiling? Wouldn't the SPS profiler want to dynamically change this option? Shouldn't this get exposed in about:config? 1) Did you gave it some thought on what happens when 'js_JitOptions.profileInlineFrames' gets toggled? Is it possible we get in a inconsistent state? I.e. codegen runs with "profileInlineFrames" off in a function and it gets toggled on and we bail afterwards. It seems like InitFromBailout would mark the inline frame as "BaselineFrame::HAS_PUSHED_SPS_FRAME" 2) I would prefer the bool "inlinedFunction" instead of "forInline" and "forInlineEnter". ::: js/src/jit/IonInstrumentation.h @@ +28,5 @@ > { > JS_ASSERT(pc != nullptr); > } > > + void leave(MacroAssembler &masm, Register reg, bool forInlineEnter=false) { Let the '=' sign breath. (add spaces ;)) ::: js/src/jit/IonMacroAssembler.cpp @@ +1262,5 @@ > > setupUnalignedABICall(1, temp); > movePtr(ImmPtr(output), temp); > passABIArg(temp); > + callWithABINoProfiling(JS_FUNC_TO_DATA_PTR(void *, Printf0_)); What is the consensus here? When should we use "callWithABINoProfiling". Doesn't that mean it won't show up as VM call in SPS traces? What about? - MacroAssembler::tracelogStart - MacroAssembler::tracelogStop - MacroAssembler::tracelogLog Should they also use "NoProfiling"?

Found problems in the patch with manual testing. Been tracking them down today. Updated patch fixes some issues, but I'm still seeing a crash on octane. Posting second WIP patch.

Fixes additional bug where during bailout of Ion code with multiple inlined frames, the SPS entry for the outermost caller in the sequence is left with an uninitialized PC.

Hey decoder and/or gkw: There still seems to be a crash in this bug that's happening quite late when running a DEBUG build with profiling enabled on Octane v2. I can replicate that issue, but debugging it is hard as it takes a long time to replicate the bug with each diagnostic change. I was hoping I could get you to fuzz this patch and smoke out any remaining issues that way.

The following test fails on m-c rev bfe4ed6d47ce (non-threadsafe debug build) with the patch (options --ion-eager): enableSPSProfilingAssertions(false); test(""); test("function g(v, value)\n"); function test(c) { function y(x) { new Function(x)(); }; y(c); } Assertion failure: stack_[*size_].script() == script, at vm/SPSProfiler.cpp:152 A similarly looking test is also crashing in an opt-build on the heap.

Awesome! Found and fixed. Will put up updated patch after lunch.

Updated patch with fix for test-case above. Still crashing with profiling turned on when running Octane box2d.

Previous patch didn't build properly. This one does.

(In reply to Kannan Vijayan [:djvj] from comment #16) > Created attachment 8366906 [details] [diff] [review] > stop-profiling-inline-frames.patch > > Previous patch didn't build properly. This one does. I think I'm also hitting build problems: In file included from /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/dbg-objdir/js/src/Unified_cpp_js_src6.cpp:56:0: /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../js/src/jsapi.cpp: In function ‘void JS_ShutDown()’: /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../js/src/jsapi.cpp:618:23: error: ‘WorkerThreadState’ was not declared in this scope In file included from /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/dbg-objdir/js/src/Unified_cpp_js_src6.cpp:56:0: /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../js/src/jsapi.cpp: In function ‘bool JS::CanCompileOffThread(JSContext*, const JS::ReadOnlyCompileOptions&, size_t)’: /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../js/src/jsapi.cpp:4518:25: warning: unused variable ‘HUGE_LENGTH’ [-Wunused-variable] In the directory /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/dbg-objdir/js/src The following command failed to execute properly: /usr/bin/ccache c++ -o Unified_cpp_js_src6.o -c -I../../dist/system_wrappers -include /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../config/gcc_hidden.h -DENABLE_PARALLEL_JS -DENABLE_BINARYDATA -DEXPORT_JS_API -DNO_NSPR_10_SUPPORT -DUSE_ZLIB -I/home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../js/src -I. -I/home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../js/src/../../mfbt/double-conversion -I/home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../intl/icu/source/common -I/home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/../../intl/icu/source/i18n -I../../dist/include -fPIC -DMOZILLA_CLIENT -include ../../js/src/js-confdefs.h -MD -MP -MF .deps/Unified_cpp_js_src6.o.pp -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -O3 -freorder-blocks -fno-omit-frame-pointer -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 /home/gkwong/Desktop/js-dbg-64-dm-er-linux-mozilla-central-167616-cafe909f7e07-7N_Soj-patched/compilePath/js/src/dbg-objdir/js/src/Unified_cpp_js_src6.cpp make[3]: *** [Unified_cpp_js_src6.o] Error 1 make[3]: *** Waiting for unfinished jobs.... Tested on Linux 64-bit debug non-threadsafe deterministic build.

> I think I'm also hitting build problems: Never mind this, I've filed bug 969769 and seems to only involve non-threadsafe builds, and is unrelated to the patch here.

Comment on attachment 8366906 [details] [diff] [review] stop-profiling-inline-frames.patch Nothing found after an overnight run.

Comment on attachment 8366906 [details] [diff] [review] stop-profiling-inline-frames.patch The following test crashes (m-c f6ab28f98ee5, 64 bit debug+opt build): enableSPSProfilingAssertions((false)); eval('\ (function() {\ "use asm";\ function f(z) {\ z = z|0;\ return (((0xc0000000 >>> z) >> 0) % ~ 1)|0;\ }\ '); Backtrace: Program received signal SIGSEGV, Segmentation fault. prepareForOOL (this=0x18ba450) at js/src/vm/SPSProfiler.h:341 341 frames[0].pc = frames[0].script->code(); (gdb) bt #0 prepareForOOL (this=0x18ba450) at js/src/vm/SPSProfiler.h:341 #1 js::jit::CodeGeneratorShared::generateOutOfLineCode (this=0x18b9950) at js/src/jit/shared/CodeGenerator-shared.cpp:94 #2 0x0000000000747f70 in js::jit::CodeGeneratorX86Shared::generateOutOfLineCode (this=0x18b9950) at js/src/jit/shared/CodeGenerator-x86-shared.cpp:319 #3 0x00000000005812f8 in js::jit::CodeGenerator::generateAsmJS (this=0x18b9950) at js/src/jit/CodeGenerator.cpp:6072 #4 0x000000000060f4e5 in js::jit::GenerateCode (mir=0x18bda60, lir=0x18bee60, maybeMasm=0x7fffffff8730) at js/src/jit/Ion.cpp:1485 #5 0x000000000052c661 in GenerateCode (lir=..., mir=..., func=..., m=...) at js/src/jit/AsmJS.cpp:5375 #6 CheckFunctionsSequential (m=...) at js/src/jit/AsmJS.cpp:5459 #7 0x000000000051e691 in CheckModule (cx=0x17feee0, parser=..., stmtList=0x1809460, moduleOut=<optimized out>, compilationTimeReport=0x7fffffff9510) at js/src/jit/AsmJS.cpp:6844 #8 0x000000000051fe46 in js::CompileAsmJS (cx=0x17feee0, parser=..., stmtList=<optimized out>, validated=0x7fffffff957f) at js/src/jit/AsmJS.cpp:6930 There might be more patch-specific failures, I'm still triaging :)

Thanks for that! Finally addressed this bug and it fixes my octane crash. One last try run and this should be ready for review.

Finally ready for review. Still green on try: https://tbpl.mozilla.org/?tree=Try&rev=40f7f7de1a4c (the orange bc are from the parent rev). This patch disables profiling of inline frames by default (although adds a js_JitOptions flag for enabling it). Most of it is just finnicky pseudostack fiddling at various corner cases (bailouts, invalidations, exceptions, etc.)

I don't think my questions from the first review were answered? (In reply to Hannes Verschore [:h4writer] (PTO March 3-7) from comment #8) > 1) Did you gave it some thought on what happens when > 'js_JitOptions.profileInlineFrames' gets toggled? Is it possible we get in a > inconsistent state? I.e. codegen runs with "profileInlineFrames" off in a > function and it gets toggled on and we bail afterwards. It seems like > InitFromBailout would mark the inline frame as > "BaselineFrame::HAS_PUSHED_SPS_FRAME" This still bothers me a lot. Having it in js_JitOptions.profileInlineFrames is a step closer to toggle during runtime. But currently this will fail horribly, I think ?! I think we will need to keep this information on the IonScript to make sure we always push the correct number of frames during bailout. (In reply to Hannes Verschore [:h4writer] (PTO March 3-7) from comment #8) > What is the consensus here? When should we use "callWithABINoProfiling". > Doesn't that mean it won't show up as VM call in SPS traces? What about? > - MacroAssembler::tracelogStart > - MacroAssembler::tracelogStop > - MacroAssembler::tracelogLog > Should they also use "NoProfiling"? I'm on PTO next week. So I will only be able to look closely at it afterwards.

(In reply to Hannes Verschore [:h4writer] (PTO March 3-7) from comment #8) > 1) It seems that atm it can only get flipped by recompiling? Wouldn't the > SPS profiler want to dynamically change this option? Shouldn't this get > exposed in about:config? > > 1) Did you gave it some thought on what happens when > 'js_JitOptions.profileInlineFrames' gets toggled? Is it possible we get in a > inconsistent state? I.e. codegen runs with "profileInlineFrames" off in a > function and it gets toggled on and we bail afterwards. It seems like > InitFromBailout would mark the inline frame as > "BaselineFrame::HAS_PUSHED_SPS_FRAME" It should never be toggled. I'm just leaving it there so that if JS engine devs need it for some reason. I try to avoid DEFINES. There area lot of JIT options we can't change at runtime. I don't see it being that problematic. I could document this in a comment.. or remove the field entirely and just have a method that returns a constant bool and document this in the method comment.... > What is the consensus here? When should we use "callWithABINoProfiling". > Doesn't that mean it won't show up as VM call in SPS traces? What about? > - MacroAssembler::tracelogStart > - MacroAssembler::tracelogStop > - MacroAssembler::tracelogLog > Should they also use "NoProfiling"? Probably, yeah. Having these enabled during profiling is going to screw with traces anyway. None of these should be doing profiling. I've modified them to be NoProfiling.

Addressed review comments.

Comment on attachment 8384668 [details] [diff] [review] stop-profiling-inline-frames-rev3.patch Review of attachment 8384668 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay (due to PTO). Looks good to me. I have some questions for clarifications. And from what I see this contains the code for disabling entries inlined frames and also some bugfixes, not related to that? I'm thinking about the changes in CodeGenerator-shared.cpp and SPSProfiler.h. Would it be possible to split these? So a patch that fixes SPS bugs and one that disables the sps for inlined frames? Can you also give some small explanation for the changes in CodeGenerator-shared.cpp and SPSProfiler.h? ::: js/src/jit/BaselineBailouts.cpp @@ +959,5 @@ > // argument check, then: > // Top SPS profiler entry would be for callee frame. > // Ion would already have pushed an SPS entry for this frame. > // The pc for this entry would be set to nullptr. > // Make sure it's set to script->pc. This comment should be under if (js_JitOptions.profileInlineFrames) {. Since it applies to that logic. ::: js/src/jit/CodeGenerator.cpp @@ +7850,5 @@ > + if (!callVM(SPSEnterInfo, lir)) > + return false; > + restoreLive(lir); > + } > + sps_.pushManual(lir->script(), masm, temp, /* inlinedFunction = */ inlinedFunction); I don't see the need for this adjustment. MFunctionBoundary::Enter is only called from IonBuilder::build, which is always not-inlined. So inlinedFunction should be always false, right? @@ +7855,4 @@ > return true; > } > > + return sps_.push(lir->script(), masm, temp, /* inlinedFunction = */ inlinedFunction); same here. @@ +7876,5 @@ > + sps_.skipNextReenter(); > + if (!callVM(SPSExitInfo, lir)) > + return false; > + restoreLive(lir); > + } Ok, MFunctionBoundary::Exit got me confused. I would think it means the opposite of Enter. So only called when leaving MReturn on the topmost (not inlined script). Now it seems Exit is also called on returns in inlined frames? This feels wrong to me? Could you enlighten me? ::: js/src/vm/SPSProfiler.h @@ +276,5 @@ > { > /* Because of inline frames, this is a nested structure in a vector */ > struct FrameState { > JSScript *script; // script for this frame, nullptr if not pushed yet > + jsbytecode *pc; add comment, like others

(In reply to Hannes Verschore [:h4writer] (PTO March 3-7) from comment #26) > And from what I see this contains the code for disabling entries inlined > frames and also some bugfixes, not related to that? > I'm thinking about the changes in CodeGenerator-shared.cpp and > SPSProfiler.h. Would it be possible to split these? > So a patch that fixes SPS bugs and one that disables the sps for inlined > frames? The "bug" being fixed CodeGenerator-shared.cpp does not manifest except with this patch. Adding pc tracking to the ion frame instrumnetation when compiling causes the bug that needs to be fixed, so this is legitimately part of this patch. I'd prefer to keep it as one patch. > Can you also give some small explanation for the changes in > CodeGenerator-shared.cpp and SPSProfiler.h? I commented above prepareForOOL and finishOOL in SPSProfiler.h. The CodeGenerator-shared.cpp commentary would be redundant. I think the reader can just as well go to the |prepareForOOL| and |finishOOL| methods and see what they do and read their comments if they're curious. > This comment should be under if (js_JitOptions.profileInlineFrames) {. > Since it applies to that logic. Done. > I don't see the need for this adjustment. > > MFunctionBoundary::Enter is only called from IonBuilder::build, which is > always not-inlined. So inlinedFunction should be always false, right? Inline_Enter falls-through to the Enter case. > Ok, MFunctionBoundary::Exit got me confused. I would think it means the > opposite of Enter. So only called when leaving MReturn on the topmost (not > inlined script). Now it seems Exit is also called on returns in inlined > frames? This feels wrong to me? Could you enlighten me? Inlined code subgraphs in the MIRGraph will emit a MFunctionBoundary::Exit. The Inline_Exit is added by the caller builder (from inlineScriptedCall), to mark the join-point of all inlined functions that return to the same point in the caller. > add comment, like others Done.

Could it be you forgot to upload the new patch?

Indeed I did. New patch. The new changes are basically just comment-related, not code-related.

Comment on attachment 8391226 [details] [diff] [review] stop-profiling-inline-frames-rev4.patch Review of attachment 8391226 [details] [diff] [review]: ----------------------------------------------------------------- lgtm