717698 - Add support infrastructure for about:jank

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Adds a profiling mode that only records samples when we haven't spun the event loop

Comment 1

[(no longer active)]

It would be a good idea to actually ask somebody for review!  ;-)

Comment 2

[(no longer active)]

Comment on attachment 588122 [details] [diff] [review]
Adds a profiling mode that only records samples when we haven't spun the event loop

>+  if (mJankOnly) {
>+    // only record the events when we have a we haven't seen a tracer event for 100ms
>+    if (!sLastTracerEvent.IsNull()) {
>+      TimeDuration delta = sample->timestamp - sLastTracerEvent;
>+      if (delta.ToMilliseconds() > 100.0) {
> #ifdef USE_BACKTRACE
>-  doBacktrace(mProfile);
>+        doBacktrace(mProfile);
> #else
>-  doSampleStackTrace(mStack, mProfile, sample);
>+        doSampleStackTrace(mStack, mProfile, sample);
>+#endif
>+      }
>+    }
>+  } else {
>+#ifdef USE_BACKTRACE
>+    doBacktrace(mProfile);
>+#else
>+    doSampleStackTrace(mStack, mProfile, sample);
> #endif

Please implement this like this:

bool skipBacktrace = false;
if (mJankOnly) {
  if (hittingTheEventLoopRegularly) {
    skipBacktrace = true;
  }
}

if (!skipBacktrace) {
 // #ifdefed cruft
}

The this would be:

if (!mJankOnly) {
>-  if (!sLastTracerEvent.IsNull()) {
>-    TimeDuration delta = sample->timestamp - sLastTracerEvent;
>-    mProfile.addTag(ProfileEntry('r', delta.ToMilliseconds()));
>+    if (!sLastTracerEvent.IsNull()) {
>+      TimeDuration delta = sample->timestamp - sLastTracerEvent;
>+      mProfile.addTag(ProfileEntry('r', delta.ToMilliseconds()));
>+    }
>   }
> }
>

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: v2

Comment 4

[(no longer active)]

Comment on attachment 588171 [details] [diff] [review]
v2

r- because the code will not compile!

r+ with the following:

* Please rename the local doBacktrace variable to not hide the function with the same name.
* Please verify that the Time stuff you're using do not lock on Windows.

Comment 5

[Benoit Girard (:BenWa)]

I don't really like patch. We can just parse out the profile in java and filter any sample with a 'r-X' where X > 100ms. This approach is more flexible.

Comment 6

[(no longer active)]

But that would require about:jank to constantly dump the profile data, right?  That way it could itself become a source of jank...

Comment 7

[Benoit Girard (:BenWa)]

You could do it after, but then you would have to buffer a lot of samples you would throw away. That's a good point.

Comment 8

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/9dea63ddf4e2 (inbound merge)

Comment 9

[Jeff Muizelaar [:jrmuizel]]

what is about:responsiveness?

Comment 10

[Robert Claypool]

from #developers
[2012-01-17 15:38:01] <edmorley> I thought people weren't going to use the term jank any more? but seems like we're going to have about:jank now :-( (about:responsiveness might have sounded a bit better)