Last Comment Bug 717698 - Add support infrastructure for about:jank
: Add support infrastructure for about:jank
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 11:27 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-03-11 14:17 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds a profiling mode that only records samples when we haven't spun the event loop (1.79 KB, patch)
2012-01-12 11:27 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review-
Details | Diff | Splinter Review
v2 (1.65 KB, patch)
2012-01-12 13:25 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-12 11:27:21 PST
Created attachment 588122 [details] [diff] [review]
Adds a profiling mode that only records samples when we haven't spun the event loop
Comment 1 :Ehsan Akhgari 2012-01-12 11:50:37 PST
It would be a good idea to actually ask somebody for review!  ;-)
Comment 2 :Ehsan Akhgari 2012-01-12 11:54:50 PST
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] 2012-01-12 13:25:25 PST
Created attachment 588171 [details] [diff] [review]
v2
Comment 4 :Ehsan Akhgari 2012-01-12 13:33:28 PST
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) 2012-01-12 13:40:24 PST
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 :Ehsan Akhgari 2012-01-12 13:57:02 PST
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) 2012-01-12 13:59:01 PST
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) 2012-01-16 19:44:57 PST
https://hg.mozilla.org/mozilla-central/rev/9dea63ddf4e2 (inbound merge)
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-01-17 12:48:01 PST
what is about:responsiveness?
Comment 10 Robert Claypool 2012-01-17 12:58:06 PST
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)

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