Simplify Command classes.

Simplify Command classes.

- Get rid of separate CommandBuilder class and singleton pattern. It
  was being passed around explicitly even though half of the places
  that received a CommandBuilder as a parameter still directly called
  CommandBuilder.instance instead of using it.
- Get rid of Command caching. As far as I can tell, it makes no
  measurable difference in runtime performance or memory usage. Even
  with a large invocation of a lot of configurations and tests, the
  Command classes don't seem to be a significant use of memory.
- Shorten the factory names. "get" adds no value, and we know it
  returns a "Command" since it's on Command.

R=whesse@google.com

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

[Bob Nystrom, 2017-06-12 22:41:28]

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

[Bob Nystrom, 2017-06-12 22:41:29]

I split this into two patch sets for ease of review.

The first contains the actual refactoring. The second just moves big chunks of
code into separate files, but the code itself is unchanged (aside from imports).

[Bill Hesse, 2017-06-13 18:15:53]

On 2017/06/12 22:41:29, Bob Nystrom wrote:
> I split this into two patch sets for ease of review.
> 
> The first contains the actual refactoring. The second just moves big chunks of
> code into separate files, but the code itself is unchanged (aside from
imports).

I'd like to get Martin and Rico to also comment, since the implementation
of the dependency graph removed a lot of duplicate runs of dart2js and they
were the bottleneck.

But if we have something in place to check that cycle times don't regress with
this
change, then it is fine - we can always add it back.

Is the dependency graph also being removed with this change?  It was only
introduced
to merge commands, using the singletons.

In the long run, I am more in favor of what some have suggested, of doing the
dart2js
compilation for tests once, and uploading them as a downloadable tarball, to be
run
on individual browsers and shells.  If this can be made as efficient as doing
the compilation
and run on the same machine, it may be easier to debug and run.

[Bill Hesse, 2017-06-13 18:16:18]

Description was changed from

==========
Simplify Command classes.

- Get rid of separate CommandBuilder class and singleton pattern. It
  was being passed around explicitly even though half of the places
  that received a CommandBuilder as a parameter still directly called
  CommandBuilder.instance instead of using it.
- Get rid of Command caching. As far as I can tell, it makes no
  measurable difference in runtime performance or memory usage. Even
  with a large invocation of a lot of configurations and tests, the
  Command classes don't seem to be a significant use of memory.
- Shorten the factory names. "get" adds no value, and we know it
  returns a "Command" since it's on Command.
==========

to

==========
Simplify Command classes.

- Get rid of separate CommandBuilder class and singleton pattern. It
  was being passed around explicitly even though half of the places
  that received a CommandBuilder as a parameter still directly called
  CommandBuilder.instance instead of using it.
- Get rid of Command caching. As far as I can tell, it makes no
  measurable difference in runtime performance or memory usage. Even
  with a large invocation of a lot of configurations and tests, the
  Command classes don't seem to be a significant use of memory.
- Shorten the factory names. "get" adds no value, and we know it
  returns a "Command" since it's on Command.
==========

[Bill Hesse, 2017-06-13 18:16:19]

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

[Bob Nystrom, 2017-06-14 00:03:54]

On 2017/06/13 18:15:53, Bill Hesse wrote:
> On 2017/06/12 22:41:29, Bob Nystrom wrote:
> > I split this into two patch sets for ease of review.
> > 
> > The first contains the actual refactoring. The second just moves big chunks
of
> > code into separate files, but the code itself is unchanged (aside from
> imports).
> 
> I'd like to get Martin and Rico to also comment, since the implementation
> of the dependency graph removed a lot of duplicate runs of dart2js and they
> were the bottleneck.

Assuming I did this correctly, there should be no change in how the dependency
graph works.

> But if we have something in place to check that cycle times don't regress with
> this
> change, then it is fine - we can always add it back.

In my local testing, I didn't notice any timing changes, but I may not have
poked the right configurations to demonstrate the problem. If you have any you
want me to try, I'd be happy to locally.

> Is the dependency graph also being removed with this change?  It was only
> introduced
> to merge commands, using the singletons.

No, the dependency graph is still there and works the same as it did before.

The Command classes still implement hashCode and ==(), so two commands with the
same data are still the "same" as far as the dependency graph's maps are
concerned. Now we just don't actually canonicalize the instances so that they
are *identical* too. As far as I can tell, there's little value in doing so.

Cheers!

– bob

[Bill Hesse, 2017-06-14 15:44:44]

On 2017/06/14 00:03:54, Bob Nystrom wrote:
> On 2017/06/13 18:15:53, Bill Hesse wrote:
> > On 2017/06/12 22:41:29, Bob Nystrom wrote:
> > > I split this into two patch sets for ease of review.
> > > 
> > > The first contains the actual refactoring. The second just moves big
chunks
> of
> > > code into separate files, but the code itself is unchanged (aside from
> > imports).
> > 
> > I'd like to get Martin and Rico to also comment, since the implementation
> > of the dependency graph removed a lot of duplicate runs of dart2js and they
> > were the bottleneck.
> 
> Assuming I did this correctly, there should be no change in how the dependency
> graph works.
> 
> > But if we have something in place to check that cycle times don't regress
with
> > this
> > change, then it is fine - we can always add it back.
> 
> In my local testing, I didn't notice any timing changes, but I may not have
> poked the right configurations to demonstrate the problem. If you have any you
> want me to try, I'd be happy to locally.
> 
> > Is the dependency graph also being removed with this change?  It was only
> > introduced
> > to merge commands, using the singletons.
> 
> No, the dependency graph is still there and works the same as it did before.
> 
> The Command classes still implement hashCode and ==(), so two commands with
the
> same data are still the "same" as far as the dependency graph's maps are
> concerned. Now we just don't actually canonicalize the instances so that they
> are *identical* too. As far as I can tell, there's little value in doing so.
> 
> Cheers!
> 
> – bob

I also think the command singletons, and also some other canonical classes, were
added to reduce the memory usage
of test.dart.  If all tests were run in all vm configurations, we were running
out of heap (we had 1 gig or more of
memory used).  Most of this was actual strings in the commands, so making sure
strings weren't copied any more than
necessary was key to reducing memory use.  But I don't think we run such big
combined configurations anymore.

[Bob Nystrom, 2017-06-14 18:03:51]

On 2017/06/14 15:44:44, Bill Hesse wrote:
> On 2017/06/14 00:03:54, Bob Nystrom wrote:
> > On 2017/06/13 18:15:53, Bill Hesse wrote:
> > > On 2017/06/12 22:41:29, Bob Nystrom wrote:
> > > > I split this into two patch sets for ease of review.
> > > > 
> > > > The first contains the actual refactoring. The second just moves big
> chunks
> > of
> > > > code into separate files, but the code itself is unchanged (aside from
> > > imports).
> > > 
> > > I'd like to get Martin and Rico to also comment, since the implementation
> > > of the dependency graph removed a lot of duplicate runs of dart2js and
they
> > > were the bottleneck.
> > 
> > Assuming I did this correctly, there should be no change in how the
dependency
> > graph works.
> > 
> > > But if we have something in place to check that cycle times don't regress
> with
> > > this
> > > change, then it is fine - we can always add it back.
> > 
> > In my local testing, I didn't notice any timing changes, but I may not have
> > poked the right configurations to demonstrate the problem. If you have any
you
> > want me to try, I'd be happy to locally.
> > 
> > > Is the dependency graph also being removed with this change?  It was only
> > > introduced
> > > to merge commands, using the singletons.
> > 
> > No, the dependency graph is still there and works the same as it did before.
> > 
> > The Command classes still implement hashCode and ==(), so two commands with
> the
> > same data are still the "same" as far as the dependency graph's maps are
> > concerned. Now we just don't actually canonicalize the instances so that
they
> > are *identical* too. As far as I can tell, there's little value in doing so.
> > 
> > Cheers!
> > 
> > – bob
> 
> I also think the command singletons, and also some other canonical classes,
were
> added to reduce the memory usage
> of test.dart.  If all tests were run in all vm configurations, we were running
> out of heap (we had 1 gig or more of
> memory used).  Most of this was actual strings in the commands, so making sure
> strings weren't copied any more than
> necessary was key to reducing memory use.  But I don't think we run such big
> combined configurations anymore.

Yeah, my assumption was that this was about memory usage.

I did some before/after profiling and as far as I can tell, instances of Command
and its subclasses are a significant user of memory. I didn't think about the
strings that they reference.

I just did a little local testing and if I run:

./tools/test.py -m release language corelib co19 lib standalone --list

Then allocation profile doesn't look very scary to me (though it's entirely
possible I don't know how to read the results well):

http://imgur.com/a/wrtMc

Strings are the biggest user of memory, but it's still not a huge amount.

[Bill Hesse, 2017-06-15 12:08:40]

lgtm

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
File tools/testing/dart/command_output.dart (right):

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
tools/testing/dart/command_output.dart:151: class BrowserCommandOutputImpl
extends CommandOutputImpl {
I just renamed BrowserCommandOutputImpl to ContentShellCommandOutputImpl in a
different CL that landed.

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
tools/testing/dart/command_output.dart:307: class HTMLBrowserCommandOutputImpl
extends BrowserCommandOutputImpl {
I think this class may not be used any more.  Can you find a configuration that
will reach this?  Issue filed https://github.com/dart-lang/sdk/issues/29869

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
tools/testing/dart/command_output.dart:453: class BrowserControllerTestOutcome
extends CommandOutputImpl
This really should be named like the others, as
BrowserControllerCommandOutputImpl (or even BrowserCommandOutputImpl, since that
name is freed by my rename of the existing class).

[Bob Nystrom, 2017-06-15 21:19:07]

Description was changed from

==========
Simplify Command classes.

- Get rid of separate CommandBuilder class and singleton pattern. It
  was being passed around explicitly even though half of the places
  that received a CommandBuilder as a parameter still directly called
  CommandBuilder.instance instead of using it.
- Get rid of Command caching. As far as I can tell, it makes no
  measurable difference in runtime performance or memory usage. Even
  with a large invocation of a lot of configurations and tests, the
  Command classes don't seem to be a significant use of memory.
- Shorten the factory names. "get" adds no value, and we know it
  returns a "Command" since it's on Command.
==========

to

==========
Simplify Command classes.

- Get rid of separate CommandBuilder class and singleton pattern. It
  was being passed around explicitly even though half of the places
  that received a CommandBuilder as a parameter still directly called
  CommandBuilder.instance instead of using it.
- Get rid of Command caching. As far as I can tell, it makes no
  measurable difference in runtime performance or memory usage. Even
  with a large invocation of a lot of configurations and tests, the
  Command classes don't seem to be a significant use of memory.
- Shorten the factory names. "get" adds no value, and we know it
  returns a "Command" since it's on Command.

R=whesse@google.com

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

[Bob Nystrom, 2017-06-15 21:19:08]

Committed patchset #4 (id:60001) manually as
a6ca718e987608a2e3dde5c3540f6a97aba2cc48 (presubmit successful).

[Bob Nystrom, 2017-06-15 21:19:29]

Thanks!

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
File tools/testing/dart/command_output.dart (right):

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
tools/testing/dart/command_output.dart:151: class BrowserCommandOutputImpl
extends CommandOutputImpl {
On 2017/06/15 12:08:40, Bill Hesse wrote:
> I just renamed BrowserCommandOutputImpl to ContentShellCommandOutputImpl in a
> different CL that landed.

Yup, I saw that when I merged. Renamed it here too.

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
tools/testing/dart/command_output.dart:307: class HTMLBrowserCommandOutputImpl
extends BrowserCommandOutputImpl {
On 2017/06/15 12:08:40, Bill Hesse wrote:
> I think this class may not be used any more.  Can you find a configuration
that
> will reach this?  Issue filed https://github.com/dart-lang/sdk/issues/29869

It doesn't seem to be used, but the corresponding BrowserTestCommand does get
constructed. It just doesn't seem to make it's way to createCommandOutput() as
far as I can tell. But I'm not super confident about that, so I'll leave it
alone for now.

https://codereview.chromium.org/2933973002/diff/20001/tools/testing/dart/comm...
tools/testing/dart/command_output.dart:453: class BrowserControllerTestOutcome
extends CommandOutputImpl
On 2017/06/15 12:08:40, Bill Hesse wrote:
> This really should be named like the others, as
> BrowserControllerCommandOutputImpl (or even BrowserCommandOutputImpl, since
that
> name is freed by my rename of the existing class).

Done.