Batch the batch compiler for tests

Batch the batch compiler for tests

This:
- Ensures that a given AST is processed once per context, avoiding AST caching issues and redundant work
- Associates file errors with their library instead of the entry point - we now only generate these once across different entry points

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/5d29e22f94243a0de73f89af84cf7671bd6cde39

[vsm, 2015-10-02 17:30:59]

vsm@google.com changed reviewers:
+ jmesserly@google.com, leafp@google.com

[Jennifer Messerly, 2015-10-02 18:22:36]

Really nice change overall. Some design thoughts, feel free to ping me.

https://codereview.chromium.org/1376123004/diff/120001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1376123004/diff/120001/lib/src/compiler.dart#...
lib/src/compiler.dart:83: final _compilationRecord = <LibraryElement, bool>{};
minor: do we care about order? if not, a HashMap is faster than a LinkedHashMap:

new HashMap<LibraryElement, bool>();

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:154: assert(_compilationRecord.containsKey(library) &&
suggestion: `_compilationRecord[library] == true` would do this in one check
(since missing entries return `null` in Dart Maps).

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:157: // Process dependences one more time to propagate
failure from cycles
interesting. kind of like a fixed point?

FWIW, I suspect this whole class would probably be simpler if we can operate on
strongly connected components. I hit similar difficulties in my incremental CL:
https://codereview.chromium.org/1322333003/ ... it's sort of similar, we need to
compute the "modified or any dependency modified bit" instead of the "failed or
any dependency failed bit" ... I wonder if similar logic would work there.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:179: if (_compilationRecord.containsKey(library)) {
this could be done in 1 lookup instead of 3:

var success = _compilationRecord[library];
if (success != null) {
  if (!success) _failure = true;
  return success;
}

Aside, I'd be tempted to rename `_failure` to `_success` so we don't have to
invert the boolean's meaning.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:197: var unit =
context.resolveCompilationUnit(element.source, library);
hmmm, this will still parse/resolve transitive libraries, is that a problem?
we were trying to get our dependencies done first, so we'd ideally get a cache
hit here. In other words, by not adding the higher level library to the cache,
we could store more of our dependencies in the AnalysisContext cache.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:198: units.add(unit);
another issue: by doing checking first, and then JS generation much later, we'd
have to keep all resolved compilation units in memory for the whole compile. Is
there a way we can structure this more incrementally?

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:219: _compilationRecord[library] = false;
if we don't need to set this right away, maybe we can avoid the hashmap set
until later?
in other words can we just set a local variable:

success = false;

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:237: if (!_compilationRecord[library]) _failure = true;
I think you can factor this so it's not doing the lookup twice here.

https://codereview.chromium.org/1376123004/diff/160001/test/codegen_test.dart
File test/codegen_test.dart (right):

https://codereview.chromium.org/1376123004/diff/160001/test/codegen_test.dart...
test/codegen_test.dart:88: compiler.compileFromUriString(filePath, (String url)
{
it might be simpler to just pass it the per file messages?

I feel like the API the test actually wants is the List<Errors> for the given
compile. Instead we set up an error stream and rely on a call back and stuff ...
it depends on some side effects/impl details. Maybe a more functional approach,
where we just return the errors, would be better.

(also AFAIK, AnalysisError has the Source which has the URI, so even given a raw
list of errors, we could figure out what file they came from)

https://codereview.chromium.org/1376123004/diff/160001/test/codegen_test.dart...
test/codegen_test.dart:93: // Messages from compiling
${path.basenameWithoutExtension(url)}.dart
another thought here... when I was looking at making compiler more incremental
(https://codereview.chromium.org/1322333003/

I actually changed it to have the compiler save messages in a text file. Not
sure if that's still worth doing. But it helps us keep things incremental even
for files that failed to compile.

https://codereview.chromium.org/1376123004/diff/160001/test/codegen_test.dart...
test/codegen_test.dart:150: // TODO(vsm): Is it okay to reuse the same context
here?  If there is
yeah, good TODO. I think this is because I didn't previously understand that SDK
had its own nested context thingy (you can tell from my "realSdkContext" name,
which is a bit misleading). We could share the context for the SDK (to avoid 4-8
second analysis or whatever it's at now), but create a new context for each of
the test configurations. That would probably be cheap and would resolve the
TODO.

[vsm, 2015-10-02 20:17:32]

Thanks - PTAL

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:154: assert(_compilationRecord.containsKey(library) &&
On 2015/10/02 18:22:36, John Messerly wrote:
> suggestion: `_compilationRecord[library] == true` would do this in one check
> (since missing entries return `null` in Dart Maps).

Done.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:157: // Process dependences one more time to propagate
failure from cycles
On 2015/10/02 18:22:36, John Messerly wrote:
> interesting. kind of like a fixed point?
> 
> FWIW, I suspect this whole class would probably be simpler if we can operate
on
> strongly connected components. I hit similar difficulties in my incremental
CL:
> https://codereview.chromium.org/1322333003/ ... it's sort of similar, we need
to
> compute the "modified or any dependency modified bit" instead of the "failed
or
> any dependency failed bit" ... I wonder if similar logic would work there.

Yes - this should compute the fixed point.  Since we have only two values
(success or failure), we only need once extra pass to get to the fixed point. 
It has to be reverse postorder to make sure we're handling back edges correctly
- target before source.

I thought about SCC, but was going for a smaller change here.  Perhaps we can
push this logic into the analyzer proper?

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:179: if (_compilationRecord.containsKey(library)) {
On 2015/10/02 18:22:36, John Messerly wrote:
> this could be done in 1 lookup instead of 3:
> 
> var success = _compilationRecord[library];
> if (success != null) {
>   if (!success) _failure = true;
>   return success;
> }
> 
> Aside, I'd be tempted to rename `_failure` to `_success` so we don't have to
> invert the boolean's meaning.

Done.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:197: var unit =
context.resolveCompilationUnit(element.source, library);
On 2015/10/02 18:22:36, John Messerly wrote:
> hmmm, this will still parse/resolve transitive libraries, is that a problem?
> we were trying to get our dependencies done first, so we'd ideally get a cache
> hit here. In other words, by not adding the higher level library to the cache,
> we could store more of our dependencies in the AnalysisContext cache.

Reordered as suggested offline.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:198: units.add(unit);
On 2015/10/02 18:22:36, John Messerly wrote:
> another issue: by doing checking first, and then JS generation much later,
we'd
> have to keep all resolved compilation units in memory for the whole compile.
Is
> there a way we can structure this more incrementally?

We need to do this.  We're not hitting this in tests as we have a lot of shallow
entry points, but we should hit this problem in big apps.  I've added a TODO
comment below when adding to pendingLibraries.  

It'd be nice to do this in a way that summaries just plug in as well.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:219: _compilationRecord[library] = false;
On 2015/10/02 18:22:36, John Messerly wrote:
> if we don't need to set this right away, maybe we can avoid the hashmap set
> until later?
> in other words can we just set a local variable:
> 
> success = false;
> 

It should not be required, but it does potentially propagate failure sooner if
we have a circular import back to library.

https://codereview.chromium.org/1376123004/diff/140001/lib/src/compiler.dart#...
lib/src/compiler.dart:237: if (!_compilationRecord[library]) _failure = true;
On 2015/10/02 18:22:36, John Messerly wrote:
> I think you can factor this so it's not doing the lookup twice here.

Done.

[Jennifer Messerly, 2015-10-02 21:02:43]

Not sure if you saw these comments:
https://codereview.chromium.org/1376123004/diff/160001/test/codegen_test.dart

LGTM though, definitely a huge improvement already :). We can keep iterating on
this as we do more incremental and more strong mode integration.

[vsm, 2015-10-02 21:11:39]

Committed patchset #10 (id:180001) manually as
5d29e22f94243a0de73f89af84cf7671bd6cde39 (presubmit successful).

[Jennifer Messerly, 2015-10-02 21:16:22]

SGTM. Thanks!