Add support infrastructure for about:jank

RESOLVED FIXED in mozilla12

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 588122 [details] [diff] [review]
Adds a profiling mode that only records samples when we haven't spun the event loop
Attachment #588122 - Flags: review?
It would be a good idea to actually ask somebody for review!  ;-)
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()));
>+    }
>   }
> }
>
Attachment #588122 - Flags: review? → review-
(Assignee)

Comment 3

5 years ago
Created attachment 588171 [details] [diff] [review]
v2
Attachment #588122 - Attachment is obsolete: true
Attachment #588171 - Flags: review?(ehsan)
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.
Attachment #588171 - Flags: review?(ehsan) → review+
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.
But that would require about:jank to constantly dump the profile data, right?  That way it could itself become a source of jank...
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.
https://hg.mozilla.org/mozilla-central/rev/9dea63ddf4e2 (inbound merge)
Assignee: nobody → jmuizelaar
Target Milestone: --- → mozilla12
Version: unspecified → Trunk
Summary: Add support infrastructure for about:jank → Add support infrastructure for about:responsiveness
(Assignee)

Comment 9

5 years ago
what is about:responsiveness?

Comment 10

5 years ago
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)
Summary: Add support infrastructure for about:responsiveness → Add support infrastructure for about:jank

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Component: General → Gecko Profiler
QA Contact: general → gecko-profiler
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.