Delay Firefox browser startup in test scripts, to flatten load.

Delay Firefox browser startup in test scripts, to flatten load.

BUG=https://github.com/dart-lang/sdk/issues/26060
R=johnniwinther@google.com

Committed: https://github.com/dart-lang/sdk/commit/40b69fd615afad6beb7e625757bc176964325f90

[Bill Hesse, 2016-04-13 13:36:41]

whesse@google.com changed reviewers:
+ kustermann@google.com

[Bill Hesse, 2016-04-13 13:36:41]

See issue 26060 for information on tests of this fix.

[Bill Hesse, 2016-04-25 08:49:10]

whesse@google.com changed reviewers:
+ ahe@google.com
- kustermann@google.com

[ahe, 2016-04-25 11:17:21]

As far as I understand this change, the idea is that starting Firefox starves
the CPU cycles of the dart2js process.

I've heard about some process scheduling where one favors interactive processes,
I don't know if this is what Windows does. If so, that's most likely the root
cause and what should be configured differently.

I'm not really sure how adding a 3 second delay to firefox is going to solve the
underlying problem: starting firefox starves dart2js. My guess is that you're
just lucky that the 3 second delay prevents any compilation task from timing
out, but the underlying problem still remain lurking.

I think the real issue here is that test.dart ignores the -jN option. It should
only start N processes in parallel, but starts 2N processes (N compilations and
N browser processes).

If N was the true limit on the number of concurrent tasks started by test.dart,
I think this problem would go away as each task always have a dedicated core.

I recognize the need for a short-term solution, but I'm not convinced that
there's a plan to fix this problem: the todo talks about "when dart2js stops
hanging under load," but I don't think the problem is with dart2js.

https://codereview.chromium.org/1882813002/diff/1/tools/testing/dart/browser_...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1882813002/diff/1/tools/testing/dart/browser_...
tools/testing/dart/browser_controller.dart:771: return new Future.delayed(new
Duration(seconds: 3)).then((_) {
return new Future.delayed(new Duration(seconds: 3), () {
  return startBrowserProcess(_binary, args, environment: environment);
});

[Bill Hesse, 2016-04-25 12:09:03]

Description was changed from

==========
Delay Firefox browser startup in test scripts, to flatten load.

BUG=https://github.com/dart-lang/sdk/issues/26060
==========

to

==========
Delay Firefox browser startup in test scripts, to flatten load.

BUG=https://github.com/dart-lang/sdk/issues/26060
==========

[Bill Hesse, 2016-04-25 12:09:03]

whesse@google.com changed reviewers:
+ sgjesse@google.com, sigmund@google.com

[Bill Hesse, 2016-04-25 12:10:25]

On 2016/04/25 11:17:21, ahe wrote:
> As far as I understand this change, the idea is that starting Firefox starves
> the CPU cycles of the dart2js process.
> 
> I've heard about some process scheduling where one favors interactive
processes,
> I don't know if this is what Windows does. If so, that's most likely the root
> cause and what should be configured differently.
> 
> I'm not really sure how adding a 3 second delay to firefox is going to solve
the
> underlying problem: starting firefox starves dart2js. My guess is that you're
> just lucky that the 3 second delay prevents any compilation task from timing
> out, but the underlying problem still remain lurking.
> 
> I think the real issue here is that test.dart ignores the -jN option. It
should
> only start N processes in parallel, but starts 2N processes (N compilations
and
> N browser processes).
> 
> If N was the true limit on the number of concurrent tasks started by
test.dart,
> I think this problem would go away as each task always have a dedicated core.
> 
> I recognize the need for a short-term solution, but I'm not convinced that
> there's a plan to fix this problem: the todo talks about "when dart2js stops
> hanging under load," but I don't think the problem is with dart2js.
> 
>
https://codereview.chromium.org/1882813002/diff/1/tools/testing/dart/browser_...
> File tools/testing/dart/browser_controller.dart (right):
> 
>
https://codereview.chromium.org/1882813002/diff/1/tools/testing/dart/browser_...
> tools/testing/dart/browser_controller.dart:771: return new Future.delayed(new
> Duration(seconds: 3)).then((_) {
> return new Future.delayed(new Duration(seconds: 3), () {
>   return startBrowserProcess(_binary, args, environment: environment);
> });


As discussed offline, I feel the evidence indicates that the problem is not the
total number
of processes being greater than the number of CPUs, and that the problem really
is in
dart2js batch mode, that it can enter a persistent unresponsive state.  This
should not
happen, even when a process is starved for seconds at a time.

Since this can be reproduced locally on a Windows machine, I strongly feel that
the issue
filed against dart2js is sufficient to fix the underlying issue, and the only
question
is whether a workaround should be applied in the meantime.  If this workaround
isn't
definite enough, and the buildbots are still flaky, then I am fine applying a
radical change, like
reducing the number of processes to N/2 or even 1.

I don't think that fixing the symptom is bad because it reduces the pressure on
the dart2js team
to find and fix the underlying issue.  We have processes where bug priorities
can be judged
by the teams, and they decide where to allocate resources to fix things. 

I'll pass this review up to the leads for dart2js and infra, since the question
of applying
temporary workarounds in our testing, and whether this is causing long-term
instability and
unmaintainability, is important.

Increasing the delay to 10 seconds, to be very conservative.

[Siggi Cherem (dart-lang), 2016-04-25 18:21:41]

+Johnni

Johnni might be able to give a better estimate of how much work it could take to
properly fix the issue in the dart2js's batch mode.

If it can be done in a day or so, then we can avoid putting this in, but
otherwise, my preference is to add the workaround so we can continue being fully
focused on the shared frontend effort.

Some ideas we might want to consider long term:
- would it make sense to do all batch compiling first, before we launch any
browser?
- would it make sense to make batch compiling on-demand, when the browser
requests for a specific .js file?

https://codereview.chromium.org/1882813002/diff/20001/tools/testing/dart/brow...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1882813002/diff/20001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:771: return new Future.delayed(new
Duration(seconds: 10)).then((_) {
since this is only causing issues in windows, can we conditionally perform the
delay only there?

[Johnni Winther, 2016-04-26 08:47:49]

On 2016/04/25 18:21:41, Siggi Cherem (dart-lang) wrote:
> +Johnni
> 
> Johnni might be able to give a better estimate of how much work it could take
to
> properly fix the issue in the dart2js's batch mode.
> 
> If it can be done in a day or so, then we can avoid putting this in, but
> otherwise, my preference is to add the workaround so we can continue being
fully
> focused on the shared frontend effort.
> 
> Some ideas we might want to consider long term:
> - would it make sense to do all batch compiling first, before we launch any
> browser?
> - would it make sense to make batch compiling on-demand, when the browser
> requests for a specific .js file?
> 
>
https://codereview.chromium.org/1882813002/diff/20001/tools/testing/dart/brow...
> File tools/testing/dart/browser_controller.dart (right):
> 
>
https://codereview.chromium.org/1882813002/diff/20001/tools/testing/dart/brow...
> tools/testing/dart/browser_controller.dart:771: return new Future.delayed(new
> Duration(seconds: 10)).then((_) {
> since this is only causing issues in windows, can we conditionally perform the
> delay only there?

I recommend committing this workaround as I have no easy-fix in sight.

[Johnni Winther, 2016-04-26 13:31:09]

johnniwinther@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2016-04-26 13:31:09]

lgtm

[Bill Hesse, 2016-04-26 13:33:56]

Description was changed from

==========
Delay Firefox browser startup in test scripts, to flatten load.

BUG=https://github.com/dart-lang/sdk/issues/26060
==========

to

==========
Delay Firefox browser startup in test scripts, to flatten load.

BUG=https://github.com/dart-lang/sdk/issues/26060
R=johnniwinther@google.com

Committed:
https://github.com/dart-lang/sdk/commit/40b69fd615afad6beb7e625757bc176964325f90
==========

[Bill Hesse, 2016-04-26 13:33:58]

Committed patchset #2 (id:20001) manually as
40b69fd615afad6beb7e625757bc176964325f90 (presubmit successful).

[Bill Hesse, 2016-04-26 19:25:08]

This caused the test step to fail on the buildbots, probably because a browser
was starting after everything shut down.  Reverting in CL
https://codereview.chromium.org/1921553005/

https://codereview.chromium.org/1882813002/diff/20001/tools/testing/dart/brow...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1882813002/diff/20001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:771: return new Future.delayed(new
Duration(seconds: 10)).then((_) {
On 2016/04/25 18:21:41, Siggi Cherem (dart-lang) wrote:
> since this is only causing issues in windows, can we conditionally perform the
> delay only there?

We only test on linux and windows, and this delay is not very significant at
all, and this object does not have information about which platform it is
running on.  So I prefer not to do this.  If we need to, we can query the
Platform.isWindows value from dart:io Platform, so it is doable.