update the analyzer and analysis server perf tags

update the analyzer and analysis server perf tags

BUG=
R=brianwilkerson@google.com, scheglov@google.com

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

[devoncarew, 2017-06-03 18:55:12]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, danrubel@google.com, scheglov@google.com

[devoncarew, 2017-06-03 18:55:13]

- add some notion of a hierarchy to PerformanceTags (via dot separated names)
- remove some unused PerformanceTags and hook existing ones up to the driver
(for analysis, hints, lints, parsing, scanning, and reading from the driver
cache)

We're capturing a lot more of the actual places we're spending time in the
diagnostics /profile page (with more work left to do).

[devoncarew, 2017-06-03 18:55:45]

Description was changed from

==========
dartfmt


update tag names in analyzer_cli


update the analyzer and analysis server perf tags

BUG=
==========

to

==========
update the analyzer and analysis server perf tags

BUG=
==========

[Brian Wilkerson, 2017-06-03 23:16:37]

lgtm

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/file_state.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:415: // TODO(devoncarew): We
likely shouldn't be doing raw prints here.
You're right. We should minimally use the instrumentation log to capture the
exception.

[scheglov, 2017-06-04 00:30:45]

I'm looking forward to seeing this statistics :-)

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/driver.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/driver.dart:983: List<int> bytes =
_byteStore.get(key);
Maybe wrap with measuring only this statement.
Also, this function is missing "return null;" at the end.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/file_state.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:415: // TODO(devoncarew): We
likely shouldn't be doing raw prints here.
On 2017/06/03 23:16:37, Brian Wilkerson wrote:
> You're right. We should minimally use the instrumentation log to capture the
> exception.

This branch does not work yet for anyone but us, analyzer developers.
So, it does not matter what we're doing here :-)

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:432: CompilationUnit unit =
parser.parseCompilationUnit(token);
Instrument parsing here.
This will cover parsing for unlinked summaries and for analysis (see my comment
in LibraryAnalyzer).

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart:73: units[_library] =
_parse(_library);
Most of the work of _parse() is in FileState.parse(), which should be
instrumented.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/driver.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:20:
PerformanceStatistics.analyzer.createChild('driver');
Hm...
We use this tag for different "drivers".

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:22: final PerformanceTag driverCacheTag =
driverTag.createChild('cache');
Unless I miss something, we use this for the different driver.
So, maybe we should move it there.

[devoncarew, 2017-06-04 01:45:52]

Thanks - updated. I suspect there's some more low-hanging fruit here in terms of
capturing other areas where we're spending time.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/file_state.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:415: // TODO(devoncarew): We
likely shouldn't be doing raw prints here.
On 2017/06/04 00:30:44, scheglov wrote:
> On 2017/06/03 23:16:37, Brian Wilkerson wrote:
> > You're right. We should minimally use the instrumentation log to capture the
> > exception.
> 
> This branch does not work yet for anyone but us, analyzer developers.
> So, it does not matter what we're doing here :-)

Removed the TODO: - I expect this will get cleaned up as the code develops.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:432: CompilationUnit unit =
parser.parseCompilationUnit(token);
On 2017/06/04 00:30:44, scheglov wrote:
> Instrument parsing here.
> This will cover parsing for unlinked summaries and for analysis (see my
comment
> in LibraryAnalyzer).

I instrumented the whole (enclosing) method with the parsing tag. The parts of
this method that are tokenizing related will not count for the parsing tag -
only one is active at a time.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart:73: units[_library] =
_parse(_library);
On 2017/06/04 00:30:44, scheglov wrote:
> Most of the work of _parse() is in FileState.parse(), which should be
> instrumented.

I moved the instrumentation there.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/driver.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:20:
PerformanceStatistics.analyzer.createChild('driver');
On 2017/06/04 00:30:44, scheglov wrote:
> Hm...
> We use this tag for different "drivers".

Updated this one to be 'taskDriver'.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:22: final PerformanceTag driverCacheTag =
driverTag.createChild('cache');
On 2017/06/04 00:30:44, scheglov wrote:
> Unless I miss something, we use this for the different driver.
> So, maybe we should move it there.

Moved these to 'analysis/driver.dart'.

There are a lot of drivers :)

[devoncarew, 2017-06-04 01:45:53]

Thanks - updated. I suspect there's some more low-hanging fruit here in terms of
capturing other areas where we're spending time.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/file_state.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:415: // TODO(devoncarew): We
likely shouldn't be doing raw prints here.
On 2017/06/04 00:30:44, scheglov wrote:
> On 2017/06/03 23:16:37, Brian Wilkerson wrote:
> > You're right. We should minimally use the instrumentation log to capture the
> > exception.
> 
> This branch does not work yet for anyone but us, analyzer developers.
> So, it does not matter what we're doing here :-)

Removed the TODO: - I expect this will get cleaned up as the code develops.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/file_state.dart:432: CompilationUnit unit =
parser.parseCompilationUnit(token);
On 2017/06/04 00:30:44, scheglov wrote:
> Instrument parsing here.
> This will cover parsing for unlinked summaries and for analysis (see my
comment
> in LibraryAnalyzer).

I instrumented the whole (enclosing) method with the parsing tag. The parts of
this method that are tokenizing related will not count for the parsing tag -
only one is active at a time.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart:73: units[_library] =
_parse(_library);
On 2017/06/04 00:30:44, scheglov wrote:
> Most of the work of _parse() is in FileState.parse(), which should be
> instrumented.

I moved the instrumentation there.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/driver.dart (right):

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:20:
PerformanceStatistics.analyzer.createChild('driver');
On 2017/06/04 00:30:44, scheglov wrote:
> Hm...
> We use this tag for different "drivers".

Updated this one to be 'taskDriver'.

https://codereview.chromium.org/2917183003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/driver.dart:22: final PerformanceTag driverCacheTag =
driverTag.createChild('cache');
On 2017/06/04 00:30:44, scheglov wrote:
> Unless I miss something, we use this for the different driver.
> So, maybe we should move it there.

Moved these to 'analysis/driver.dart'.

There are a lot of drivers :)

[scheglov, 2017-06-04 01:51:29]

LGTM

[devoncarew, 2017-06-04 02:51:29]

Description was changed from

==========
update the analyzer and analysis server perf tags

BUG=
==========

to

==========
update the analyzer and analysis server perf tags

BUG=
R=brianwilkerson@google.com, scheglov@google.com

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

[devoncarew, 2017-06-04 02:51:30]

Committed patchset #2 (id:20001) manually as
0b79e4d20de07bd1e8c1d153d5c766c3d983c82a (presubmit successful).

[Paul Berry, 2017-06-05 12:42:33]

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

[Paul Berry, 2017-06-05 12:42:34]

https://codereview.chromium.org/2917183003/diff/20001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/pub_package_map_provider.dart (right):

https://codereview.chromium.org/2917183003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/pub_package_map_provider.dart:12: import
'package:analysis_server/src/analysis_server.dart';
A file in package:analyzer can't import a file from package:analysis_server,
because analysis_server isn't published on pub (it only exists in the SDK).  Is
there some other way we can accomplish this?

[devoncarew, 2017-06-05 13:35:45]

https://codereview.chromium.org/2917183003/diff/20001/pkg/analyzer/lib/source...
File pkg/analyzer/lib/source/pub_package_map_provider.dart (right):

https://codereview.chromium.org/2917183003/diff/20001/pkg/analyzer/lib/source...
pkg/analyzer/lib/source/pub_package_map_provider.dart:12: import
'package:analysis_server/src/analysis_server.dart';
On 2017/06/05 12:42:33, Paul Berry wrote:
> A file in package:analyzer can't import a file from package:analysis_server,
> because analysis_server isn't published on pub (it only exists in the SDK). 
Is
> there some other way we can accomplish this?

Thanks for the heads up! Yes, it would be possible to move the tag call back up
to the caller of computePackageMap(). This was creating some misleading time in
perf profile, but would be one way of solving the import issue.

[Paul Berry, 2017-06-05 15:41:57]

On 2017/06/05 13:35:45, devoncarew wrote:
>
https://codereview.chromium.org/2917183003/diff/20001/pkg/analyzer/lib/source...
> File pkg/analyzer/lib/source/pub_package_map_provider.dart (right):
> 
>
https://codereview.chromium.org/2917183003/diff/20001/pkg/analyzer/lib/source...
> pkg/analyzer/lib/source/pub_package_map_provider.dart:12: import
> 'package:analysis_server/src/analysis_server.dart';
> On 2017/06/05 12:42:33, Paul Berry wrote:
> > A file in package:analyzer can't import a file from package:analysis_server,
> > because analysis_server isn't published on pub (it only exists in the SDK). 
> Is
> > there some other way we can accomplish this?
> 
> Thanks for the heads up! Yes, it would be possible to move the tag call back
up
> to the caller of computePackageMap(). This was creating some misleading time
in
> perf profile, but would be one way of solving the import issue.

Sounds good to me!  Would you mind putting together a CL?