fe: minor change to benchmarks to match what the runners expect

fe: minor change to benchmarks to match what the runners expect and some minor refactoring.

I'm happy to split the style/refactorings as separate CLs if you prefer. Here is a summary of the changes:
  - update the reported strings to match the names expected by our benchmark runners ("unlinked summaize + parse" => "unlinked_summarize")
  - invert the benchmark results (like Paul did in 2551533003)
  - stop counting input-size while scanning, count that separately once
  - combine the multiple summarization steps into a single function
  - clean up the logic that dispatches into each benchmark

R=paulberry@google.com

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

[Siggi Cherem (dart-lang), 2016-12-29 16:37:49]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-12-29 17:26:43]

Description was changed from

==========
fe benchmarks: make a couple simplifications, invert how output is reported too

I'm making a bundle of changes together. I copied here also the change Paul did
in issue 2551533003 to invert how the data is reported.
==========

to

==========
fe: minor change to benchmarks to match what the runners expect and some minor
refactoring.

I'm happy to split the style/refactorings as separate CLs if you prefer. Here is
a summary of the changes:
  - update the reported strings to match the names expected by our benchmark
runners ("unlinked summaize => "unlinked_summarize")
  - invert the benchmark results (like Paul did in 2551533003)
  - stop counting input-size while scanning, count that separately once
  - combine the multiple summarization steps into a single function
  - clean up the logic that dispatches into each benchmark
==========

[Siggi Cherem (dart-lang), 2016-12-29 17:27:24]

Description was changed from

==========
fe: minor change to benchmarks to match what the runners expect and some minor
refactoring.

I'm happy to split the style/refactorings as separate CLs if you prefer. Here is
a summary of the changes:
  - update the reported strings to match the names expected by our benchmark
runners ("unlinked summaize => "unlinked_summarize")
  - invert the benchmark results (like Paul did in 2551533003)
  - stop counting input-size while scanning, count that separately once
  - combine the multiple summarization steps into a single function
  - clean up the logic that dispatches into each benchmark
==========

to

==========
fe: minor change to benchmarks to match what the runners expect and some minor
refactoring.

I'm happy to split the style/refactorings as separate CLs if you prefer. Here is
a summary of the changes:
  - update the reported strings to match the names expected by our benchmark
runners ("unlinked summaize + parse" => "unlinked_summarize")
  - invert the benchmark results (like Paul did in 2551533003)
  - stop counting input-size while scanning, count that separately once
  - combine the multiple summarization steps into a single function
  - clean up the logic that dispatches into each benchmark
==========

[Siggi Cherem (dart-lang), 2016-12-29 17:27:43]

sigmund@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2017-01-03 17:58:34]

lgtm assuming doc comment is fixed.

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
File pkg/front_end/tool/perf.dart (right):

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:146: /// Produces linked summaries for every file
in [files] and reports the time
Doc comment is no longer accurate.

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:155: report('unlink extract',
unlinkedSummarizeTimer.elapsedMicroseconds);
I don't really have strong feelings about nomenclature here, but can you help me
understand why "extract" is a meaningful name for this benchmark?

[Siggi Cherem (dart-lang), 2017-01-04 00:01:04]

thanks Paul!

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
File pkg/front_end/tool/perf.dart (right):

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:146: /// Produces linked summaries for every file
in [files] and reports the time
On 2017/01/03 17:58:34, Paul Berry wrote:
> Doc comment is no longer accurate.

Done.

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:155: report('unlink extract',
unlinkedSummarizeTimer.elapsedMicroseconds);
On 2017/01/03 17:58:34, Paul Berry wrote:
> I don't really have strong feelings about nomenclature here, but can you help
me
> understand why "extract" is a meaningful name for this benchmark?

In all honestly, I'm not convinced about "extract" either. The main requirement
I have is that the runner relies on matching the benchmark name (which is an
argument to this script) and the name given to `report`. It's ok to report extra
data, and the runner will ignore it. I believe we will mostly use the data of
"unlinked summarize + parse" as a benchmark, which is why I made that be the one
with the benchmark name "unlinked_summarize" and gave this a different name.

Some ideas:
 (a) do not report the "extract" result at all
 (b) create 2 benchmarks:
    - unlinked_summarize: summarize alone (like you had before my change, aka
"extract")
    - unlinked_summarize_from_source: includes parse time (and maybe scan time
too?)
 (c) change the runner script to aggregate data itself. 

It is much easier to maintain things if we don't do (c). I'm totally fine with
either (a) or (b).

[Paul Berry, 2017-01-04 13:55:44]

lgtm

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
File pkg/front_end/tool/perf.dart (right):

https://codereview.chromium.org/2602003002/diff/40001/pkg/front_end/tool/perf...
pkg/front_end/tool/perf.dart:155: report('unlink extract',
unlinkedSummarizeTimer.elapsedMicroseconds);
On 2017/01/04 00:01:04, Siggi Cherem (dart-lang) wrote:
> On 2017/01/03 17:58:34, Paul Berry wrote:
> > I don't really have strong feelings about nomenclature here, but can you
help
> me
> > understand why "extract" is a meaningful name for this benchmark?
> 
> In all honestly, I'm not convinced about "extract" either. The main
requirement
> I have is that the runner relies on matching the benchmark name (which is an
> argument to this script) and the name given to `report`. It's ok to report
extra
> data, and the runner will ignore it. I believe we will mostly use the data of
> "unlinked summarize + parse" as a benchmark, which is why I made that be the
one
> with the benchmark name "unlinked_summarize" and gave this a different name.
> 
> Some ideas:
>  (a) do not report the "extract" result at all
>  (b) create 2 benchmarks:
>     - unlinked_summarize: summarize alone (like you had before my change, aka
> "extract")
>     - unlinked_summarize_from_source: includes parse time (and maybe scan time
> too?)
>  (c) change the runner script to aggregate data itself. 
> 
> It is much easier to maintain things if we don't do (c). I'm totally fine with
> either (a) or (b).

In that case I'd prefer (b), on the theory that tracking more information is
better than less :)

[Siggi Cherem (dart-lang), 2017-01-05 19:31:43]

Description was changed from

==========
fe: minor change to benchmarks to match what the runners expect and some minor
refactoring.

I'm happy to split the style/refactorings as separate CLs if you prefer. Here is
a summary of the changes:
  - update the reported strings to match the names expected by our benchmark
runners ("unlinked summaize + parse" => "unlinked_summarize")
  - invert the benchmark results (like Paul did in 2551533003)
  - stop counting input-size while scanning, count that separately once
  - combine the multiple summarization steps into a single function
  - clean up the logic that dispatches into each benchmark
==========

to

==========
fe: minor change to benchmarks to match what the runners expect and some minor
refactoring.

I'm happy to split the style/refactorings as separate CLs if you prefer. Here is
a summary of the changes:
  - update the reported strings to match the names expected by our benchmark
runners ("unlinked summaize + parse" => "unlinked_summarize")
  - invert the benchmark results (like Paul did in 2551533003)
  - stop counting input-size while scanning, count that separately once
  - combine the multiple summarization steps into a single function
  - clean up the logic that dispatches into each benchmark

R=paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-01-05 19:31:44]

Committed patchset #3 (id:60001) manually as
cc3682f134ba5dd2f121bcb1864876efa3eb257c (presubmit successful).

[Siggi Cherem (dart-lang), 2017-01-05 19:32:27]

Thanks Paul! I went ahead and added both, submitting...