The default bug view has changed. See this FAQ.

Add a temporary buffer of samples so that we can make the decision about whether to include them later

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
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 589599 [details] [diff] [review]
Add a temporary buffer of samples so that we can make the decision about whether to include them later

This lets about:jank keep all of the samples that occurred during a point when we > 100 ms event latency.
Attachment #589599 - Flags: feedback?(ehsan)
Attachment #589599 - Flags: feedback?(bgirard)
Comment on attachment 589599 [details] [diff] [review]
Add a temporary buffer of samples so that we can make the decision about whether to include them later

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

Here's what I would like to see in the final patch:

1. Please make it so that this infrastructure has as little impact as possible on the performance of the built-in profiler.  For example, see if you can avoid the second buffer?
2. Please make the code simpler to comprehend.  This patch makes understanding and modifying the code harder than it needs to be.  I would like to see the final code be easier to understand than the existing code.  :-)
Attachment #589599 - Flags: feedback?(ehsan) → feedback-
(Assignee)

Comment 2

5 years ago
Created attachment 589691 [details] [diff] [review]
v2

This should be much simpler and to your taste.
Attachment #589599 - Attachment is obsolete: true
Attachment #589599 - Flags: feedback?(bgirard)
Attachment #589691 - Flags: review?(ehsan)
Comment on attachment 589691 [details] [diff] [review]
v2

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

This looks good overall, but I would like to look at another version of this patch which addresses the comments below.

::: tools/profiler/sps/TableTicker.cpp
@@ +184,5 @@
> +
> +  // discards all of the entries since the last flush()
> +  // NOTE: that if mWritePos happens to wrap around past
> +  // mLastFlushPos we actually only discard mWritePos - mLastFlushPos entries
> +  //     r          f    w

Please add an agenda explaining what r, f and w are.

@@ +223,5 @@
> +  // |-----------------------------|
> +  //        r       w
> +  // |-----------------------------|
> +  // |ABCDEFdefghijklmnopqrstuvwxyz|
> +  // |-----------------------------|

Honestly I didn't review the comments that closely, but I trust you on them.  :-)

@@ +307,4 @@
>    {
>      mUseStackWalk = hasFeature(aFeatures, aFeatureCount, "stackwalk");
>      mProfile.addTag(ProfileEntry('m', "Start"));
> +    // It's probably worth splitting the jank profiler out from the regular profiler at some point

Nit: mark this as TODO please, so that vim would do its magic tricks when you open this file.

@@ +434,5 @@
>    }
>  }
>  
> +/* used to keep track of the last event that we sampled during */
> +unsigned int sLastSampledEventGeneration;

Initialize to 0 please, and make it static.

@@ +438,5 @@
> +unsigned int sLastSampledEventGeneration;
> +
> +/* a counter that's incremented everytime we get responsiveness event
> + * note: it might also be worth tracking everytime we go around
> + * the event loop */

Nit: English!

@@ +439,5 @@
> +
> +/* a counter that's incremented everytime we get responsiveness event
> + * note: it might also be worth tracking everytime we go around
> + * the event loop */
> +unsigned int sCurrentEventGeneration;

Ditto.

@@ -394,4 @@
>      }
>    }
>  
> -  if (recordSample) {

|recordSample| will be unused at this point, right?  Should you remove it?

@@ +691,4 @@
>      TimeDuration delta = aTime - sLastTracerEvent;
>      sResponsivenessTimes[sResponsivenessLoc++] = delta.ToMilliseconds();
>    }
> +  sCurrentEventGeneration++;

Hmm, if my calculations are correct, this will overflow in about 500 days.  So this will not be a problem.  But I would add a comment explaining why you're not handling the overflow here.
Attachment #589691 - Flags: review?(ehsan)
Comment on attachment 589691 [details] [diff] [review]
v2

When I said I wanna look at it again, I was probably thinking of another patch!
Attachment #589691 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/40130b602934
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.