Closed Bug 1030446 Opened 6 years ago Closed 6 years ago

OdinMonkey: simplify stack-walking


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: luke, Assigned: luke)




(4 files)

These patches simplify the existing asm.js stack-walking, preserve existing behavior/performance, and prepare for async stack-walking in bug 1027885.
Attached patch move-iteratorSplinter Review
This patch simply moves the AsmJSFrameIterator code to its own file.  This will be useful later when there is more code.
Attachment #8446229 - Flags: review?(dtc-moz)
Attached patch tweak-testsSplinter Review
This patch just changes tests to use setJitCompilerOption which allows lower iteration counts and is also useful for later testing.
Attachment #8446231 - Flags: review?(benj)
Attached patch use-exit-fpSplinter Review
This patch switches exit paths from saving the exit sp to saving the exit fp.  I really should've done it this way to begin with, it's much simpler.  It's also necessary for async stack-walking: if the profiler samples a thread while it is in a signal handler called from asm.js code (out-of-bounds, interrupt handler), we need the pointer in the stack to be coherent at every instruction.  With exitSP, there are several instructions before and after the call where no return address is pushed.  With exitFP, we can ensure that, whenever exitFP is non-null, it points to a valid frame.  This also avoids the InterruptedSP hack.
Attachment #8446233 - Flags: review?(dtc-moz)
Attached patch add-code-rangesSplinter Review
Instead of deriving the containing function from CallStack, we can instead store a separate sorted array of code ranges.  This has several advantages:
 - CallSite (which are much more numerous than CodeRange) shrinks a word
 - We can later store extra info for CodeRange that would otherwise bloat CallSite (e.g., the function start line is currently missing)
 - This will be useful with async profiling for handling entries/exites.
Attachment #8446237 - Flags: review?(dtc-moz)
Attachment #8446231 - Flags: review?(benj) → review+
Attachment #8446233 - Flags: review?(dtc-moz) → review+
Attachment #8446237 - Flags: review?(dtc-moz) → review+
Attachment #8446229 - Flags: review?(dtc-moz) → review+
Depends on: 1031877
Speculatively backed out in

This landed in and everything was fine for ten pushes.

Mats landed the should-be-unrelated and Android 2.2 Armv6 mochitest-4 started failing all the dom/asmjscache/ tests, failing "asm.js compilation is available".

I backed that out, and merged the cset before it around. The backout stopped the failure on mozilla-inbound, and the merge was fine on mozilla-central and b2g-inbound, but fx-team has the same Armv6 failure, despite not having Mats' patch.

Luke told me to just disable the tests on Armv6 and reland Mats, but I went to sleep instead, and this morning while I was discovering that we apparently don't have a condition to just disable on Armv6, and according to comments in webgl tests, we don't even have a way to detect it from runtime JS, Ms2ger landed the should-be-unrelated and restarted the failure on mozilla-inbound.

My extremely handwavy theory is that this is being misoptimized into reading uninitialized memory that random other patches change into something that causes failures.
Resolution: FIXED → ---
The dom/asmjscache tests were failing b/c isAsmJSCompilationAvailable() is returning false.  This can be false for a number of valid reasons (no VFP, signal handlers broken, page size, etc) so it really wasn't valid for the tests to be requiring this to be true.  Re-pushing these patches with an additional patch that changes the tests to succeed even if isAsmJSCompilationAvailable() = false.  Independently, we can see if this shows up (we'll see "isAsmJSCompilationAvailable is false" in the test output) and investigate.
You need to log in before you can comment on or make changes to this bug.