Last Comment Bug 772078 - Stopping the profiler doesn't remove JS entries
: Stopping the profiler doesn't remove JS entries
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Alex Crichton [:acrichto]
:
Mentors:
Depends on: 774330
Blocks: 761261
  Show dependency treegraph
 
Reported: 2012-07-09 08:16 PDT by Benoit Girard (:BenWa)
Modified: 2012-07-16 17:20 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.06 KB, patch)
2012-07-09 14:46 PDT, Alex Crichton [:acrichto]
bgirard: feedback+
Details | Diff | Review
patch (10.59 KB, patch)
2012-07-10 10:34 PDT, Alex Crichton [:acrichto]
no flags Details | Diff | Review
patch (10.64 KB, patch)
2012-07-10 10:40 PDT, Alex Crichton [:acrichto]
no flags Details | Diff | Review
patch2 (22.78 KB, patch)
2012-07-10 15:24 PDT, Alex Crichton [:acrichto]
luke: review+
Details | Diff | Review

Description Benoit Girard (:BenWa) 2012-07-09 08:16:48 PDT
I think the only solution here is to label which entries belong to JS and remove them after stopping because they could be any where within the stack.

The problem gets worse when you start/stop the profiler several times. The root becomes a long list of non sense JS frames.
Comment 1 Alex Crichton [:acrichto] 2012-07-09 10:25:41 PDT
This could get interesting in a couple of ways. It should be fairly easy to remove js profiling entries because the listed stack pointer for all of them is NULL. The one marker placed by C++ could easily be removed if profiling was turned off. Would this mean that the stack would then be compressed? If all js profiling entries were removed, would all entries above them be moved down? It makes sense to me, but it would currently trip an assertion in the one C++ entry. This could be removed, however.

Another problem would be where you turn profiling on, JS pushes a few labels, you then turn profiling off, all the labels are removed, and then profiling is turned back on. Upon leaving the JS functions they would know that they've "pushed" and profiling is turned on, so they'd try to pop. The stack wouldn't have the JS labels, however, and this would end badly.

One possible solution is to only ever turn JS profiling off if there's no JS frames on the stack to begin with. That would resolve both the problems and probably just be easier all around. It might be hard for SPS to know when there are no JS entries on the stack (without a full scan), so perhaps this is a job for the JS engine whenever you turn of profiling with a NULL stack? The same logic could even be extended to if you turn it on in the middle of some JS, the actual "turn on" is deferred until there's no JS running.
Comment 2 Alex Crichton [:acrichto] 2012-07-09 14:46:51 PDT
Created attachment 640375 [details] [diff] [review]
patch

Benoit, I ran the profiler and it looked like it didn't have any lingering JS frames once JS was turned off (after collecting data with JS turned on). Can you confirm that this is taking care of JS frames?
Comment 3 Benoit Girard (:BenWa) 2012-07-10 07:48:54 PDT
Comment on attachment 640375 [details] [diff] [review]
patch

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

::: js/src/vm/SPSProfiler.cpp
@@ +110,5 @@
> +    JS_ASSERT(enabled());
> +    for (int i = 0; i < max_ && i < *size_; i++) {
> +        if (stack_[i].sp != NULL)
> +            continue;
> +        memmove(&stack_[i], &stack_[i + 1], (*size_ - i) * sizeof(stack_[0]));

It would be easier and simpler to keep a write/read pointer.
Comment 4 Benoit Girard (:BenWa) 2012-07-10 09:46:40 PDT
Comment on attachment 640375 [details] [diff] [review]
patch

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

f+ Works well.
Comment 5 Alex Crichton [:acrichto] 2012-07-10 10:34:58 PDT
Created attachment 640673 [details] [diff] [review]
patch

Same goal as the previous patch, but with a few extra minor cleanups. Also took Benoit's advice because memcpy just makes things harder...
Comment 6 Alex Crichton [:acrichto] 2012-07-10 10:40:36 PDT
Created attachment 640674 [details] [diff] [review]
patch

Was compiling in release, not debug mode, so forgot the guard parameter.
Comment 7 Luke Wagner [:luke] 2012-07-10 14:06:01 PDT
IIUC, the fact that the patch clears out JS frames when profiling is turned off isn't an external requirement (in fact, it seems undesirable since it breaks stacky-ness), but rather an internal solution to the problem that, when we turn off profiling, we stop popping the SPS stack.

Could another solution be to change Probes::exitScript to take out the "rt->spsProfiler.enabled()" conjunct (so that we would still pop any left-over frames)?  There is also the corner case where, if we enter newly-compiled jit code for an already-SPS-pushed frame, we won't pop the SPS stack when we pop the frame.  However, to fix this we could simply add:

  if (fp->hasSPSFrame())
      return Compile_Skipped;

to mjit::CanMethodJIT which guards all entrance into jit code.  (This requires adding a StackFrame *fp argument, but it is trivially available at all callsites.)  (I'd also add an assert in EnterMethodJIT that !fp->hasSPSFrame.)

Are there any more edge cases we need to worry about?
Comment 8 Alex Crichton [:acrichto] 2012-07-10 14:16:35 PDT
I feel like this would require a shift in the API to make sense. Otherwise if you uninstall profiling by setting the stack to NULL, all the future pops don't know what to pop from unless we keep that around.

Currently this isn't a problem, but then the API would be something like: If you uninstall, make sure the stack stays around for "awhile" because we're going to keep using it.

I do prefer your idea of having everything naturally pop its own frame, even if profiling was turned off. I think that we'd need a separation of installing a profiling stack and then enabling/disabling profiling. Something like:

  js::SetRuntimeProfilingStack(ProfileEntry *stack, ...)

If stack is non-null, assert the previous stack is NULL. If the stack is NULL, assert that the current stack is non-null and that there are no JS entries remaining on the stack? In reality I don't think SetRuntimeProfilingStack would ever be invoked with NULL (at least not currently), but the assertions would be a sanity check.

  js::EnableRuntimeProfilingStack(bool enabled)

If enabling, assert that we're not already enabled and that the stack is non NULL. If we're disabling, assert that the stack is non NULL, we're enabled, and then do what luke suggested. I can't think of other edge cases, but I'd run a few tests to make sure.

Does that sound reasonable?
Comment 9 Luke Wagner [:luke] 2012-07-10 14:21:45 PDT
(In reply to Alex Crichton [:acrichto] from comment #8)
> I feel like this would require a shift in the API to make sense. Otherwise
> if you uninstall profiling by setting the stack to NULL, all the future pops
> don't know what to pop from unless we keep that around.

Good point!

> Currently this isn't a problem, but then the API would be something like: If
> you uninstall, make sure the stack stays around for "awhile" because we're
> going to keep using it.

Since the rest of the C++-y RAII guards are going to pop themselves from the stack (e.g., SPSEntryMarker) regardless of whether profiling is enabled, I think we can make the same assumption, but I like your API idea below.

> If the stack is NULL,

Let's assert non-null, if we can.

> Does that sound reasonable?

Sounds great to me!
Comment 10 Alex Crichton [:acrichto] 2012-07-10 15:24:08 PDT
Created attachment 640820 [details] [diff] [review]
patch2
Comment 11 Luke Wagner [:luke] 2012-07-11 14:43:23 PDT
Comment on attachment 640820 [details] [diff] [review]
patch2

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

Looks good, on request:

::: js/src/jsfriendapi.cpp
@@ +882,1 @@
>      ReleaseAllJITCode(rt->defaultFreeOp());

/*
 * multi-line comments need leading/trailing * line
 */

Could we also put ReleaseAllJITCode in SPSProfiler::enable() ?
Comment 12 Luke Wagner [:luke] 2012-07-11 14:43:40 PDT
s/on request/one request/
Comment 13 Alex Crichton [:acrichto] 2012-07-11 16:26:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/17bc02a42a1a
Comment 14 Ed Morley [:emorley] 2012-07-12 09:36:54 PDT
https://hg.mozilla.org/mozilla-central/rev/17bc02a42a1a

Note You need to log in before you can comment on or make changes to this bug.