Last Comment Bug 719176 - Add a temporary buffer of samples so that we can make the decision about whether to include them later
: Add a temporary buffer of samples so that we can make the decision about whet...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-18 12:01 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-01-21 05:55 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a temporary buffer of samples so that we can make the decision about whether to include them later (4.32 KB, patch)
2012-01-18 12:01 PST, Jeff Muizelaar [:jrmuizel]
ehsan: feedback-
Details | Diff | Splinter Review
v2 (4.93 KB, patch)
2012-01-18 16:10 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-18 12:01:21 PST
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.
Comment 1 :Ehsan Akhgari 2012-01-18 12:57:19 PST
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.  :-)
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-01-18 16:10:54 PST
Created attachment 589691 [details] [diff] [review]
v2

This should be much simpler and to your taste.
Comment 3 :Ehsan Akhgari 2012-01-18 17:10:07 PST
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.
Comment 4 :Ehsan Akhgari 2012-01-19 14:14:24 PST
Comment on attachment 589691 [details] [diff] [review]
v2

When I said I wanna look at it again, I was probably thinking of another patch!
Comment 5 Ed Morley [:emorley] 2012-01-21 05:55:32 PST
https://hg.mozilla.org/mozilla-central/rev/40130b602934

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