Remove only local variables/functions from the cache.

Remove only local variables/functions from the cache.

This is a temporary measure to restore incremental resolution in most
cases. In the future we should avoid putting constant variables into
cache at all and enforce this on the cache.put() level.

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2016-02-10 20:56:34]

LGTM

[scheglov, 2016-02-10 20:58:34]

Description was changed from

==========
Remove only local variables/functions from the cache.

This is a temporary measure to restore incremental resolution in most
cases. In the future we should avoid putting constant variables into
cache at all and enforce this on the cache.put() level.

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

to

==========
Remove only local variables/functions from the cache.

This is a temporary measure to restore incremental resolution in most
cases. In the future we should avoid putting constant variables into
cache at all and enforce this on the cache.put() level.

R=brianwilkerson@google.com
BUG=

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

[scheglov, 2016-02-10 20:58:35]

Committed patchset #1 (id:1) manually as
e71080901fa5bd7a43c47a705c22b592a3312f8c (presubmit successful).

[skybrian, 2016-02-13 07:30:28]

On 2016/02/10 20:58:35, scheglov wrote:
> Committed patchset #1 (id:1) manually as
> e71080901fa5bd7a43c47a705c22b592a3312f8c (presubmit successful).

This is a regression. My stress test started failing again, so I spent the
evening debugging it.

Please tell me about this sort of thing in the future.

[Brian Wilkerson, 2016-02-13 15:58:45]

Brian,

> This is a regression. My stress test started failing again ...

Obviously we need a unit test that will fail when there is such a regression.
Please add one.

> Please tell me about this sort of thing in the future.

Tell you about what sort of thing exactly?

[skybrian, 2016-02-13 18:39:21]

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

[skybrian, 2016-02-13 18:39:22]

https://codereview.chromium.org/1682333003/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right):

https://codereview.chromium.org/1682333003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:2070: if (element is
LocalVariableElement ||
This check is missing DefaultParameterElementImpl and ConstFieldElementImpl (at
least).

It looks like changing LocalVariableElement to LocalElement might cover it.

But it seems like this check should be reversed. If there are Elements that we
know are safe to move while still in the cache, we can check for those cases and
skip cache.remove(). If we haven't thought about a particular case then we
should default to removing them from the cache, which is always safe but might
be unnecessary.

[skybrian, 2016-02-13 18:41:03]

On 2016/02/13 15:58:45, Brian Wilkerson wrote:
> Brian,
> 
> > This is a regression. My stress test started failing again ...
> 
> Obviously we need a unit test that will fail when there is such a regression.
> Please add one.
> 
> > Please tell me about this sort of thing in the future.
> 
> Tell you about what sort of thing exactly?

If I submitted a change to fix a bug, it seems like I should be a reviewer if
that code is changed again or partially reversed.

[Brian Wilkerson, 2016-02-13 19:32:28]

> If I submitted a change to fix a bug, it seems like I should be a reviewer if
> that code is changed again or partially reversed.

Perhaps. (Though I'm honestly not sure how that would work from a purely
logistic perspective.)

But it seems to me that the role of unit tests is to ensure that if the code is
changed in a way that breaks a contract the problem will be discovered early. If
there had been a unit test to ensure that the contract your changes fixed wasn't
re-broken then the test would have failed. In that case you wouldn't have needed
to be a reviewer because the test would have filled the same role.

At this point, we need to (a) fix the bug that was re-introduced and (b) add a
unit test to ensure that it isn't inadvertently broken again. I'm guessing that
you understand better than anyone what contract was broken, so you're probably
the best person to write the test. You and Konstantin can figure out who's going
to fix the bug :-)

[skybrian, 2016-02-13 20:48:17]

On 2016/02/13 19:32:28, Brian Wilkerson wrote:
> Perhaps. (Though I'm honestly not sure how that would work from a purely
> logistic perspective.)

I agree that it's not practical in general, but we just fixed this bug two weeks
ago, so I still think I should have been cc-ed in this case.

Longer term, I think I need to make the stress test a continuous build. For now,
perhaps I can write up directions on how to run it, so we can do that manually
when changing incremental resolver code.

> 
> But it seems to me that the role of unit tests is to ensure that if the code
is
> changed in a way that breaks a contract the problem will be discovered early.
If
> there had been a unit test to ensure that the contract your changes fixed
wasn't
> re-broken then the test would have failed. In that case you wouldn't have
needed
> to be a reviewer because the test would have filled the same role.

Yes, more tests are better and I'll write them for these two new cases. But I
didn't write them before because I didn't know they existed. From a code
coverage point of view, there was no if statement before so this line was
covered. (In theory, there should now be a test giving an example of when we
shouldn't clear the cache entry for performance reasons, justifying the if
statement. It wouldn't have prevented the bug, though.)

Tests can't enforce this contract. I will revive my patch to do a runtime check
to make sure Elements used as cache keys aren't mutated (which is how I tracked
this down).

I think the idea that unit tests can substitute for code review is a
misunderstanding of what tests are capable of. Tests should be a backup for code
review, where we (informally) prove the code safe by thinking through all
possible cases. When that's not practical (and the analyzer code base and the
complexity of the Dart language make it difficult), we should make sure we
choose safe defaults for tricky cases we didn't think of.