Closed Bug 1031261 Opened 7 years ago Closed 7 years ago

Remove fixed timeout in test_profiler_actor.js

Categories

(DevTools :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: jseward, Assigned: jseward)

References

Details

Attachments

(1 file, 1 obsolete file)

toolkit/devtools/server/tests/unit/test_profiler_actor.js starts
the SPS profiler and waits a fixed 200ms before asking for a sample.
This fails miserably in a situation where native unwinding is 
required, since it can take SPS somewhere between 2 and 30 seconds
to load the Dwarf CFI for libxul and other libraries and start
producing samples.
Attached patch bug1031261-1.cset (obsolete) — Splinter Review
Use binary exponential backoff, minimum delay 100ms, max
delay 25.6 seconds, so as to tolerate slow configurations
without wasting lots of time in fast configurations.
Attachment #8447121 - Flags: review?(jimb)
Possibly related to bug 873855.
Blocks: 1022583
Comment on attachment 8447121 [details] [diff] [review]
bug1031261-1.cset

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

The duplication here is too much. Let's go around once more on this.

::: toolkit/devtools/server/tests/unit/test_profiler_actor.js
@@ +136,5 @@
> +      do_check_eq(typeof aResponse.profile.meta.platform, "string");
> +      do_check_eq(typeof aResponse.profile.threads, "object");
> +      do_check_eq(typeof aResponse.profile.threads[0], "object");
> +      do_check_eq(typeof aResponse.profile.threads[0].samples, "object");
> +      do_check_neq(aResponse.profile.threads[0].samples.length, 0);

This could be made clearer, with much less duplication of code.

First, every response to a 'getProfile' request should include a profile
property and have the right general shape. The only thing that we want to
wait for is whether we've gotten any samples yet.

Second, we can more clearly distinguish the case where we've given up from
the case where we get a bad response.

So this could be done as follows:


function attempt(aDelayMS)
{
  ... /* spin for the designated time */ ...
  aClient.request({ to: aProfiler, type: "getProfile" }, function (aResponse) {
    // These should be present in any valid getProfile response.
    do_check_eq(typeof aResponse.profile, "object");
    do_check_eq(typeof aResponse.profile.meta, "object");
    do_check_eq(typeof aResponse.profile.meta.platform, "string");
    do_check_eq(typeof aResponse.profile.threads, "object");
    do_check_eq(typeof aResponse.profile.threads[0], "object");
    do_check_eq(typeof aResponse.profile.threads[0].samples, "object");

    // If there are no samples, wait longer.
    if (aResponse.profile.threads[0].samples.length == 0) {
      if (aDelayMS < 20000) {
	do_print("attempt: no samples, going around again");
	return attempt(aDelayMS * 2);
      } else {
        do_check_true(false, "Waited 20000ms but no samples were collected");
        return;
      }
    }

    ... /* check only the aspects not checked above */ ...
}
Attachment #8447121 - Flags: review?(jimb)
Restructured as suggested -- duplication removed, and marginally
improved comments.
Attachment #8447121 - Attachment is obsolete: true
Attachment #8454545 - Flags: review?(jimb)
Attachment #8454545 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/b29081ee4dcb
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.