Closed Bug 1497407 Opened Last year Closed Last year

Tests for profiler counters

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(1 file)

Additional gtests and xpcshell tests for profiler counters (bug 1464509)
Comment on attachment 9015425 [details] [diff] [review]
gtests and xpcshell test update for profiler counters

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

I'm worried that this test may have a too high rate of intermittent failures. Do you have a try push with a bunch of retriggers for it?

::: tools/profiler/tests/gtest/GeckoProfiler.cpp
@@ +522,5 @@
>    for (int i = 0; i < 10; i++) {
>      profiler_add_marker("M5", MakeUnique<GTestMarkerPayload>(i));
>    }
>  
> +  // Warning: this could be racy

How so?

@@ +561,5 @@
> +  PR_Sleep(PR_MillisecondsToInterval(10));
> +  AUTO_PROFILER_COUNT_TOTAL(TestCounter, 7);
> +  PR_Sleep(PR_MillisecondsToInterval(10));
> +  AUTO_PROFILER_COUNT_TOTAL(TestCounter, -17);
> +  PR_Sleep(PR_MillisecondsToInterval(10));

This looks rather brittle. I think we need a profiler_block_until_sample_collected function.
> I'm worried that this test may have a too high rate of intermittent
> failures. Do you have a try push with a bunch of retriggers for it?

I'll push a bunch of retriggers.  I've also run it many times locally
 
> ::: tools/profiler/tests/gtest/GeckoProfiler.cpp
> @@ +522,5 @@
> >    for (int i = 0; i < 10; i++) {
> >      profiler_add_marker("M5", MakeUnique<GTestMarkerPayload>(i));
> >    }
> >  
> > +  // Warning: this could be racy
> 
> How so?

right below what's shown it has:
  // The second set of GTestMarkerPayloads should not have been streamed.
  ASSERT_TRUE(GTestMarkerPayload::sNumCreated == 20);
(etc)
This assumes that between profiler_start() and the immediately-following stream_json call that no profiler sampling tick has fired.  The first test has this:
  // Sleep briefly to ensure a sample is taken and the pending markers are
  // processed.
It's very unlikely that they'll get streamed, and I presume we haven't seen it fail in automation.  Perhaps it's impossible, but glancing at this without deep investigation made me wary of it, since it seems to be relying on no sampling tick happening.

I left my initial comment there more as a warning; I could reword or remove it.

> 
> @@ +561,5 @@
> > +  PR_Sleep(PR_MillisecondsToInterval(10));
> > +  AUTO_PROFILER_COUNT_TOTAL(TestCounter, 7);
> > +  PR_Sleep(PR_MillisecondsToInterval(10));
> > +  AUTO_PROFILER_COUNT_TOTAL(TestCounter, -17);
> > +  PR_Sleep(PR_MillisecondsToInterval(10));
> 
> This looks rather brittle. I think we need a
> profiler_block_until_sample_collected function.

The earlier code uses 500ms, which seemed overkill to me, given a default capture rate of 1ms.

I can easily increase it.  We can also file a bug on switching all of these to use such a function.
(In reply to Randell Jesup [:jesup] from comment #3)
> > I'm worried that this test may have a too high rate of intermittent
> > failures. Do you have a try push with a bunch of retriggers for it?
> 
> I'll push a bunch of retriggers.  I've also run it many times locally
>  
> > ::: tools/profiler/tests/gtest/GeckoProfiler.cpp
> > @@ +522,5 @@
> > >    for (int i = 0; i < 10; i++) {
> > >      profiler_add_marker("M5", MakeUnique<GTestMarkerPayload>(i));
> > >    }
> > >  
> > > +  // Warning: this could be racy
> > 
> > How so?
> 
> right below what's shown it has:
>   // The second set of GTestMarkerPayloads should not have been streamed.
>   ASSERT_TRUE(GTestMarkerPayload::sNumCreated == 20);
> (etc)
> This assumes that between profiler_start() and the immediately-following
> stream_json call that no profiler sampling tick has fired.  The first test
> has this:
>   // Sleep briefly to ensure a sample is taken and the pending markers are
>   // processed.

The difference between the two is that the first set of markers is added while the profiler is running. In the second case is added while the profiler is stopped; those get destroyed immediately, they're not put into any queue. Even if the sampler ticks, we won't stream these markers because they no longer exist.

> I left my initial comment there more as a warning; I could reword or remove
> it.

Please remove it, so that nobody spends time investigating it. (If it was actually valid, I'd instead ask you to change it to give a more precise hint, e.g. something like "this is racy because it assumes that no sampling tick happens between the profiler being started and the profile being streamed".)
Comment on attachment 9015425 [details] [diff] [review]
gtests and xpcshell test update for profiler counters

Let's bump it to 200 and land it.
Attachment #9015425 - Flags: review?(mstange) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57d01ae278bc
gtests and xpcshell test update for profiler counters r=mstange
> I'll push a bunch of retriggers.  I've also run it many times locally

Note you can also use the verify, that runs tests several times, but also enables chaos mode.
Locally, you just add `--verify` to your test command.

See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Test_Verification for more information about that.
https://hg.mozilla.org/mozilla-central/rev/57d01ae278bc
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.