Cache flushing implementation for the task model.

Cache flushing implementation for the task model.

R=brianwilkerson@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=45597

[Brian Wilkerson, 2015-05-07 14:19:19]

LGTM

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:519: * [currentSize] lass then
[maxSize].
"lass then" --> "less than"

If it's intentional that this method might leave currentSize >= maxSize (because
we won't flush priority files), then we should document that fact.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:545: * Notifies this manager that the
corresponding analysis context is acive.
"acive" --> "active"

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:565: recentlyUsed.add(result);
Given that an access is a use, shouldn't the result be unconditionally added?

In fact, in the old code only an access was considered a use, storing a value
wasn't.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:609: * ones.
I'm guessing that this is effectively the same as "context._cache". Perhaps we
should just make the context's cache visible through InternalAnalysisContext.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:670: entry._partition = this;
It would be good to ensure that "entry._partition" is null on entry as a way of
catching bugs.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:671: entry._target = target;
It seems strange to assign the target here. It would seem more natural to assign
it outside this method, but I don't know what the implications of that choice
would be, so perhaps this is actually better. In any case, it's something we can
revisit later.

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

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:108: const SimpleResultCachingPolicy(1 <<
20, 1 << 20);
Why not just "-1" (until we have time to figure out what the right value really
is)?

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:318: cachingPolicy: AST_CACHING_POLICY);
"UsedImportedElements" is not an AstNode, so this looks wrong.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:325: cachingPolicy: AST_CACHING_POLICY);
This looks wrong too.

[scheglov, 2015-05-07 15:49:27]

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:519: * [currentSize] lass then
[maxSize].
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> "lass then" --> "less than"
> 
> If it's intentional that this method might leave currentSize >= maxSize
(because
> we won't flush priority files), then we should document that fact.

Done.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:545: * Notifies this manager that the
corresponding analysis context is acive.
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> "acive" --> "active"

Done.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:565: recentlyUsed.add(result);
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> Given that an access is a use, shouldn't the result be unconditionally added?
> 
> In fact, in the old code only an access was considered a use, storing a value
> wasn't.

Yes, access is a use, but only if the result was already stored.
We need values so that we could compute their sizes and manage the list.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:609: * ones.
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> I'm guessing that this is effectively the same as "context._cache". Perhaps we
> should just make the context's cache visible through InternalAnalysisContext.

There are some problems with it in tests.
I will do this in the next CL.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:670: entry._partition = this;
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> It would be good to ensure that "entry._partition" is null on entry as a way
of
> catching bugs.

Done.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:671: entry._target = target;
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> It seems strange to assign the target here. It would seem more natural to
assign
> it outside this method, but I don't know what the implications of that choice
> would be, so perhaps this is actually better. In any case, it's something we
can
> revisit later.

Yes.
Probably more natural would be to make target a final field (we don't repurpose
entries for different targets) and use it during "put".

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

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:108: const SimpleResultCachingPolicy(1 <<
20, 1 << 20);
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> Why not just "-1" (until we have time to figure out what the right value
really
> is)?

Done.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:318: cachingPolicy: AST_CACHING_POLICY);
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> "UsedImportedElements" is not an AstNode, so this looks wrong.

Done.

https://codereview.chromium.org/1133513003/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:325: cachingPolicy: AST_CACHING_POLICY);
On 2015/05/07 14:19:19, Brian Wilkerson wrote:
> This looks wrong too.

Done.

[scheglov, 2015-05-07 15:50:30]

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