Add contextual information about identifiers to the parser listener API.

Add contextual information about identifiers to the parser listener API.

R=ahe@google.com, scheglov@google.com

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

[Paul Berry, 2017-02-22 00:47:27]

paulberry@google.com changed reviewers:
+ ahe@google.com, scheglov@google.com

[Paul Berry, 2017-02-22 00:47:27]

This is a bit of a new idea so bear with me :)

Previously, AstBuilder used begin...() events to track information about the
state of the parser so that it could take the right action when an identifier
was encountered.  I've been finding it difficult to reassure myself that 

I've been finding it difficult to reassure myself that the AstBuilder tracks the
parser state correctly in all circumstances, especially when error recovery is
involved (for example, when a begin...() method sets isFirstIdentifier to
`true`, is there ever a circumstance where the corresponding end...() method
needs to restore the old state?)  And I'm worried that things are going to get
even more complicated as we add more resolution logic to AstBuilder.

This CL eliminates the guesswork by letting the parser tell the listener exactly
what the context is at the time of the call to handleIdentifier.
WDYT?

I wasn't able to measure a significant performance difference with
fasta_perf.dart.

[scheglov, 2017-02-22 01:01:50]

LGTM!

[ahe, 2017-02-22 09:46:20]

LGTM!

isFirstIdentifier is number one on my list of hacks that I'd like to get rid of
from Fasta. So this is awesome!

I would like to remove isFirstIdentifier from body_builder, but you could also
take a stab at that if you like.

Could you run the parser test suite and check that it doesn't regress in
performance as well:

  ./xcodebuild/ReleaseX64/dart pkg/testing/bin/testing.dart
--config=pkg/front_end/test/fasta/testing.json parser

Expect to see a few errors as it basically tries to parse everything in your SDK
checkout.

I know, this should also be set up as an automated test, and it should also be
tracked on golem. I will make sure that happens, but for now, I'd really
appreciate if you could check it locally.

The comment below isn't an action item for you.

https://codereview.chromium.org/2711463005/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2711463005/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/parser.dart:1671: parseIdentifier(token,
IdentifierContext.localAccessorDeclaration);
Not 100% sure, but I think dart2js sometimes uses this for parsing top-level
functions and methods. However, as long as it doesn't use this value it
shouldn't matter.

Also, it's something that should be fixed in dart2js. It interferes with error
recovery.

[Paul Berry, 2017-02-22 17:15:23]

On 2017/02/22 09:46:20, ahe wrote:
> LGTM!
> 
> isFirstIdentifier is number one on my list of hacks that I'd like to get rid
of
> from Fasta. So this is awesome!
> 
> I would like to remove isFirstIdentifier from body_builder, but you could also
> take a stab at that if you like.
> 
> Could you run the parser test suite and check that it doesn't regress in
> performance as well:
> 
>   ./xcodebuild/ReleaseX64/dart pkg/testing/bin/testing.dart
> --config=pkg/front_end/test/fasta/testing.json parser
> 
> Expect to see a few errors as it basically tries to parse everything in your
SDK
> checkout.
> 
> I know, this should also be set up as an automated test, and it should also be
> tracked on golem. I will make sure that happens, but for now, I'd really
> appreciate if you could check it locally.

Test execution time before the change: 48.3s +/- 0.3s
Test execution time after the change: 50.0s +/- 0.5s

(uncertainty computed by standard deviation of the mean)

Propagating uncertainties, it's slower by 1.7s +/- 0.6s.  That's nearly 3 sigma,
so I believe that's statistically significant.

Was this just a test of parsing or a test of the full dart2js compilation
pipeline?  If it's just parsing, I would be inclined to go ahead with the
change, on the grounds that (a) we all seem to like it, and (b) I believe
parsing is a fairly small proportion of total compilation time, so the overall
performance penalty should be pretty tiny.

If it's a full run of the dart2js compilation pipeline, then maybe we shouldn't
go through with the change.

I'll wait until I hear back from you before committing (we can talk at our 9:30
meeting if you like).

[ahe, 2017-02-22 17:23:47]

On 2017/02/22 17:15:23, Paul Berry wrote:
> On 2017/02/22 09:46:20, ahe wrote:
> > LGTM!
> > 
> > isFirstIdentifier is number one on my list of hacks that I'd like to get rid
> of
> > from Fasta. So this is awesome!
> > 
> > I would like to remove isFirstIdentifier from body_builder, but you could
also
> > take a stab at that if you like.
> > 
> > Could you run the parser test suite and check that it doesn't regress in
> > performance as well:
> > 
> >   ./xcodebuild/ReleaseX64/dart pkg/testing/bin/testing.dart
> > --config=pkg/front_end/test/fasta/testing.json parser
> > 
> > Expect to see a few errors as it basically tries to parse everything in your
> SDK
> > checkout.
> > 
> > I know, this should also be set up as an automated test, and it should also
be
> > tracked on golem. I will make sure that happens, but for now, I'd really
> > appreciate if you could check it locally.
> 
> Test execution time before the change: 48.3s +/- 0.3s
> Test execution time after the change: 50.0s +/- 0.5s
> 
> (uncertainty computed by standard deviation of the mean)
> 
> Propagating uncertainties, it's slower by 1.7s +/- 0.6s.  That's nearly 3
sigma,
> so I believe that's statistically significant.

I'm going to ask you for help with statistics in the future.

> Was this just a test of parsing or a test of the full dart2js compilation
> pipeline?  If it's just parsing, I would be inclined to go ahead with the
> change, on the grounds that (a) we all seem to like it, and (b) I believe
> parsing is a fairly small proportion of total compilation time, so the overall
> performance penalty should be pretty tiny.
> 
> If it's a full run of the dart2js compilation pipeline, then maybe we
shouldn't
> go through with the change.
> 
> I'll wait until I hear back from you before committing (we can talk at our
9:30
> meeting if you like).

I'm inclined to agree with you, but let's discuss this at our meeting.

[Paul Berry, 2017-02-22 19:58:40]

On 2017/02/22 17:23:47, ahe wrote:
> On 2017/02/22 17:15:23, Paul Berry wrote:
> > On 2017/02/22 09:46:20, ahe wrote:
> > > LGTM!
> > > 
> > > isFirstIdentifier is number one on my list of hacks that I'd like to get
rid
> > of
> > > from Fasta. So this is awesome!
> > > 
> > > I would like to remove isFirstIdentifier from body_builder, but you could
> also
> > > take a stab at that if you like.
> > > 
> > > Could you run the parser test suite and check that it doesn't regress in
> > > performance as well:
> > > 
> > >   ./xcodebuild/ReleaseX64/dart pkg/testing/bin/testing.dart
> > > --config=pkg/front_end/test/fasta/testing.json parser
> > > 
> > > Expect to see a few errors as it basically tries to parse everything in
your
> > SDK
> > > checkout.
> > > 
> > > I know, this should also be set up as an automated test, and it should
also
> be
> > > tracked on golem. I will make sure that happens, but for now, I'd really
> > > appreciate if you could check it locally.
> > 
> > Test execution time before the change: 48.3s +/- 0.3s
> > Test execution time after the change: 50.0s +/- 0.5s
> > 
> > (uncertainty computed by standard deviation of the mean)
> > 
> > Propagating uncertainties, it's slower by 1.7s +/- 0.6s.  That's nearly 3
> sigma,
> > so I believe that's statistically significant.
> 
> I'm going to ask you for help with statistics in the future.
> 
> > Was this just a test of parsing or a test of the full dart2js compilation
> > pipeline?  If it's just parsing, I would be inclined to go ahead with the
> > change, on the grounds that (a) we all seem to like it, and (b) I believe
> > parsing is a fairly small proportion of total compilation time, so the
overall
> > performance penalty should be pretty tiny.
> > 
> > If it's a full run of the dart2js compilation pipeline, then maybe we
> shouldn't
> > go through with the change.
> > 
> > I'll wait until I hear back from you before committing (we can talk at our
> 9:30
> > meeting if you like).
> 
> I'm inclined to agree with you, but let's discuss this at our meeting.

New timing data from running the full Fasta pipeline:

Before: 54.40 +/- 0.10 s
After: 54.29 +/- 0.17 s

Difference: 0.11 +/- 0.2 s speedup (0.2 +/- 0.36 %)

The difference was smaller than sigma, so it was very likely due to random
variation.  (Also the new version was faster, which is even more reason to
believe it was random variation).  And even if it were real, it's a small enough
effect that I think we are safe to commit this.

[Paul Berry, 2017-02-22 20:42:40]

Description was changed from

==========
Add contextual information about identifiers to the parser listener API.
==========

to

==========
Add contextual information about identifiers to the parser listener API.

R=ahe@google.com, scheglov@google.com

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

[Paul Berry, 2017-02-22 20:42:41]

Committed patchset #2 (id:20001) manually as
3fbeb5d06428270e87bc82b8faacb6ab73a0b0e5 (presubmit successful).