1040390 - use JS::ProfilingFrameIterator in Gecko profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Luke Wagner [:luke]]

Bug 1027885 adds a JS::ProfilingFrameIterator that can be used to unwind the stack at an arbitrary PC (when profiling mode is enabled).  Currently it only walks asm.js frames.  Later, we'll move Ion/Baseline/Interp from using the psuedo stack to JS::ProfilingFrameIterator which should reduce profiling overhead and simplify the code.

Comment 1

[Luke Wagner [:luke]]

Attachment: combined.diff (applies to d83c2125ae04)

Woohoo!  I have it working for JS-only profiling.  JS+C++ is next.  I played around with it on some small testcases as well as Unity/Epic and it seems to give reasonable results.  I'd be curious if you could try this out Jukka and see if it works.

Comment 2

[Luke Wagner [:luke]]

Attachment: change-run-script

From IRC, this seems to be a bug to all of us that RunScript doesn't push an isCpp frame so that the subsequent isJs frames can use its stackAddress.  This fixes some stack anomalies I was seeing.

Comment 3

[Luke Wagner [:luke]]

Attachment: stack-address

This patch adds a stackAddress to JS::ProfilingFrameIterator, necessary for ordering JS frames wrt pseudo stack frames.

Comment 4

[Luke Wagner [:luke]]

Attachment: gen-label

JS::ProfilingFrameIterator's previous interface was pretty awkward and somewhat inefficient for use in TableTicker.cpp.  What we really need is for it to just return a simple const char* label, so that's what this patch changes it to do.

Comment 5

[Luke Wagner [:luke]]

Attachment: sps-integration

This patch does the SPS integration.  Pretty simple, were it not for the terrible USE_NEXT_CPP_STACK_ADDRESS hack explained in the comment in this patch.  Fortunately, we can remove that hack when we switch all JS frames to JS::ProfilingFrameIterator and kill isJs().

Comment 6

[Douglas Crosher [:dougc]]

Comment on attachment 8461232 [details] [diff] [review]
gen-label

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

Looks good.

::: js/src/jit/AsmJSModule.cpp
@@ +1540,5 @@
> +    // the name of some Function CodeRange. This involves malloc() so do it now
> +    // since, once we start sampling, we'll be in a signal-handing context where
> +    // we cannot malloc.
> +    if (enabled) {
> +        profilingLabels_.resize(names_.length());

Good.

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 8461230 [details] [diff] [review]
change-run-script

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

I'm assuming you checked and ensured that nothing is depending on RunScript pushing a non-JS frame.  That said, this makes sense.

Comment 8

[Luke Wagner [:luke]]

(In reply to Kannan Vijayan [:djvj] from comment #7)
Yeah, I did some grepping and both Benoit and I did some simple testing.

Comment 9

[Luke Wagner [:luke]]

Attachment: combined.diff (applies to d83c2125ae04)

This version fixes a silly bug where JS_CODEGEN_ARM not being defined outside js/src caused the 'lr' register of RegisterState to be uninitialized.  With this, I can profile a build of WMW I had lying around on Fennec.

Still need to support merging with native stacks.

Comment 10

[Douglas Crosher [:dougc]]

(In reply to Luke Wagner [:luke] from comment #9)
> Created attachment 8461726 [details] [diff] [review]
> combined.diff (applies to d83c2125ae04)
> 
> This version fixes a silly bug where JS_CODEGEN_ARM not being defined
> outside js/src caused the 'lr' register of RegisterState to be
> uninitialized.  With this, I can profile a build of WMW I had lying around
> on Fennec.

Looks like the wrong patch was attached and the wrong patch marked as obsolete?

Comment 11

[Benoit Girard (:BenWa)]

Ahh, I was looking for this review in my queue earlier today but couldn't find it.

Comment 12

[Luke Wagner [:luke]]

Attachment: combined.diff (applies to d83c2125ae04)

Oops, you're right, wrong patch.  I wrote the code for combined native+JS stack walking... I just haven't rebooted into Windows to test it yet.  What's nice is this patch uses a single algorithm to merge JS+pseudo and JS+pseudo+native.

Comment 13

[Luke Wagner [:luke]]

Attachment: sps-integration

This patch does the SPS integration, including native stack integration.  I tested on OSX and Windows with the Gecko Profiler addon and it seemed to work.  So I think this is about done (for a first landing).

Comment 14

[Luke Wagner [:luke]]

Attachment: combined.diff (applied to be9512657201)

Comment 15

[Luke Wagner [:luke]]

Attachment: xpcshell-test

Comment 16

[Jukka Jylänki]

Hey, this is great, I'll definitely have to give this a go asap!

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 8462361 [details] [diff] [review]
sps-integration

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

::: js/public/ProfilingStack.h
@@ +69,4 @@
>      };
>  
>      MOZ_BEGIN_NESTED_ENUM_CLASS(Category, uint32_t)
>          OTHER    = 0x04,

You need to bump these.

::: tools/profiler/TableTicker.cpp
@@ +345,2 @@
>  {
> +  // Pseudo-frames with the ASMJS flag are just annotations should not be

and should not be*

@@ +472,5 @@
> +      // Currently, only asm.js frames use the JS stack and Ion/Baseline/Interp
> +      // frames use the pseudo stack. In the optimized asm.js->Ion call path, no
> +      // isCpp frame is pushed, leading to the callstack:
> +      //   old | pseudo isCpp | asm.js | pseudo isJs | new
> +      // Since there is no intervening isCpp pseudo frame between asm.js and

interleaving (or interweaving)*

@@ +480,5 @@
> +      // before entering asm.js is flagged with StackEntry::ASMJS. When we see
> +      // this flag, we first push all the asm.js frames (up to the next frame
> +      // with a stackAddress) before pushing the isJs frames. (This and the
> +      // above isJs special cases can be removed once all JS execution modes
> +      // switch from the pseudo stack to the JS stack.)

Probably should mention that there's no Ion->ASM fast path.

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8462856 [details] [diff] [review]
xpcshell-test

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

::: tools/profiler/tests/test_asm.js
@@ +28,5 @@
> +        // A frame for |asmjs_function| and |ffi_function| have been pushed.  Do
> +        // a sequence of increasingly long spins until we get a sample.
> +        var delayMS = 5;
> +        while (1) {
> +            do_print("loop: ms = " + delayMS);

This looks like it more something for debugging then something that should stick.

@@ +38,5 @@
> +            } while (Date.now() - then < delayMS);
> +            profile = p.getProfileData().threads[0].samples;
> +            if (profile.length > 0 || delayMS > 30000)
> +                return;
> +            delayMS *= 2;

Nice, a lot of the the other test give a lot of random orange because they are racy but this will properly sleep until a sample is collected.

Except that you start the profiler on line 24, so if you get a sample before you enter the asmjs code then you might return from this early before you have collected the sample that you want. You probably want to check that you have the sample that you want rather than any sample.

Comment 19

[Luke Wagner [:luke]]

(In reply to Benoit Girard (:BenWa) from comment #17)
> >      MOZ_BEGIN_NESTED_ENUM_CLASS(Category, uint32_t)
> >          OTHER    = 0x04,
> 
> You need to bump these.

D'oh!  Good catch.

Comment 20

[Luke Wagner [:luke]]

Attachment: xpcshell-test

After posting this test, I spent a few hours debugging 32-bit only failures.  It turns out that the test this was copy-pasted from (test_enterjit_osr.js) set mEntrySize to 100 which means that, if a single stack-trace is takes >100 ProfileEntries, you never get a valid trace.  While debugging this I also discovered the race you mentioned and so that should be fixed also.

Comment 21

[Luke Wagner [:luke]]

And with that, looking green: https://tbpl.mozilla.org/?tree=Try&rev=e8734e11de81

Comment 22

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ba4cd9b513
https://hg.mozilla.org/integration/mozilla-inbound/rev/274c7f646f58
https://hg.mozilla.org/integration/mozilla-inbound/rev/7797ecb20e4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9352885113
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ee804e9629

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Luke Wagner [:luke] from comment #2)
> Created attachment 8461230 [details] [diff] [review]
> change-run-script
> 
> From IRC, this seems to be a bug to all of us that RunScript doesn't push an
> isCpp frame so that the subsequent isJs frames can use its stackAddress. 
> This fixes some stack anomalies I was seeing.

So this will break hang reporting, which relies on having a JS frame to detect JS script hangs. (Most times there will not be additional JS frames, because the profiler is not running.) Maybe we should push both a JS and a CPP frame.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42ee804e9629
https://hg.mozilla.org/mozilla-central/rev/dc9352885113
https://hg.mozilla.org/mozilla-central/rev/7797ecb20e4b
https://hg.mozilla.org/mozilla-central/rev/274c7f646f58
https://hg.mozilla.org/mozilla-central/rev/b6ba4cd9b513