Cache output of dart2js compilations that went wrong on disk.

Cache output of dart2js compilations that went wrong on disk.

This change adds a out.js.cached_output (or equivalent) for compilations that fails.
There are two cases where this helps:
  Commits where the sdk does not change and but we would normally recompile since we don't have the dart2js output
  Runs on the buildbot where we do several runs on different runtimes in serial

Additionally, this introduces a check on multitest files so we don't copy the file if the content is already correct. The reason for this is that a bunch of tests depend on the copied import which will then trigger a recompilation.
This will help in the same two cases as above.

This change also changes dart2js to always output the deps file, otherwise we have no way of knowing if we can reuse the output.

R=floitsch@google.com, kustermann@google.com, whesse@google.com

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

[Bill Hesse, 2014-04-29 17:51:55]

LGTM.

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/multi...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/multi...
tools/testing/dart/multitest.dart:254: Future copy =
TestUtils.isContentEqual(srcPath, targetPath).then((equal) {
It seems to me that "copyIfDifferent" is a useful function that could be added
to TestUtils, encapsulating this.

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:216: if (_neverSkipCompilation) return new
Future.value(false);
Shouldn't these return CachedState objects?

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:253: return const CachedState(true, false);
Is CachedState(true, false) really meaning CachedState(true, ignored), since
having the right compiled output means we don't need the output?  If so, the
class should document that.  Why is true, true not a better choice here?

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:1988: }
Should we put a try-catch around this?  Or make writeCachedOutput never throw? 
Or do we want to crash in this case?

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:1883: 
In the class below this, we don't have a double line between static vars and
static functions.

[floitsch, 2014-04-29 18:07:04]

dart2js changes LGTM.

[kustermann, 2014-04-29 21:20:44]

I took a brief look, since I implemented the normal caching back in the old
days.

+1 For pushing on this. Together with merged builders, we'll be able to test
more configurations in less time :)

https://codereview.chromium.org/256743009/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/256743009/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:369:
getDepsOutput(inputProvider.sourceFiles));
This does not work in a very rare edge case:

Assuming you have a dart test X.dart importing Y.dart. But Y.dart is not present
on the filesystem. In that case [inputProvider.sourceFiles] will only contain
'X.dart' but not 'Y.dart' and the compilation will fail.

Now if someone adds 'Y.dart', we don't rerun the dart2js compilation (since
'X.dart' and the sdk did not change) -- which is broken (since adding 'Y.dart'
could make the compilation succeed).

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:96: new Future.value(const
CachedState(false, false));
It's not so nice that the Command class itself has a property of type
CacheState, which contains compilation logic (compilation logic should only live
in the CompilationCommand subclass).

The only reason why you need this CachedState class at the moment, is because
you only write the output files if the 'exitCode' was nonzero. You could write
stdout/stderr/extitCode/... always out to a file (even for successful
compilations) eliminating this problem. The cost would be one more file, with a
bit of content, but looking at what dart2js compilation currently outputs for
some tests (e.g. a polymer test):
-rw-r----- ... 1.1M ...  /tmp/foobar.js
-rw-r----- ... 23K   ...  /tmp/foobar.js.deps
-rw-r----- ... 367K ...  /tmp/foobar.js.map
-rw-r----- ... 1.6M ...  /tmp/foobar.precompiled.js

I don't think < 1k of stdout/stderr would matter much (just the deps file would
usually be 5-10x bigger than stdout/stderr).

Additional benefit: If a test fails at runtime and the compilation was skipped,
we could still show stdout/stderr from dart2js in the failure output (which we
currently don't have).

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:216: if (_neverSkipCompilation) return new
Future.value(false);
On 2014/04/29 17:51:55, Bill Hesse wrote:
> Shouldn't these return CachedState objects?

BTW: I hope we did not disable '--checked' mode when running test.dart :)

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:1660: 'timedOut': timedOut,
It may make sense to also add 'time' here, not sure though.

[ricow1, 2014-04-30 06:51:01]

Thank you for your comments.
Updated the cl to take them into account.
Especially thanks to Martin for making me think twice about the dependency logic
- removed now a bunch of code.

https://codereview.chromium.org/256743009/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/256743009/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:369:
getDepsOutput(inputProvider.sourceFiles));
On 2014/04/29 21:20:44, kustermann wrote:
> This does not work in a very rare edge case:
> 
> Assuming you have a dart test X.dart importing Y.dart. But Y.dart is not
present
> on the filesystem. In that case [inputProvider.sourceFiles] will only contain
> 'X.dart' but not 'Y.dart' and the compilation will fail.
> 
> Now if someone adds 'Y.dart', we don't rerun the dart2js compilation (since
> 'X.dart' and the sdk did not change) -- which is broken (since adding 'Y.dart'
> could make the compilation succeed).
I don't know enough about how dart2js works to decide on this, but here is a few
questions:
  By your example I assume that the files are only added after they have
actually been read in (or we validate that they are on disk? )
  If so, could we add them elsewhere so that we can combine missing input files
with the actual files and thereby eliminate this problem?

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/multi...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/multi...
tools/testing/dart/multitest.dart:254: Future copy =
TestUtils.isContentEqual(srcPath, targetPath).then((equal) {
On 2014/04/29 17:51:55, Bill Hesse wrote:
> It seems to me that "copyIfDifferent" is a useful function that could be added
> to TestUtils, encapsulating this.

Done.

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:96: new Future.value(const
CachedState(false, false));
On 2014/04/29 21:20:44, kustermann wrote:
> It's not so nice that the Command class itself has a property of type
> CacheState, which contains compilation logic (compilation logic should only
live
> in the CompilationCommand subclass).
This is actually no different then the fact that it already had a
outputIsUpToDate function which is only relevant for compilation commands!
There is a tradeoff here, do you want to to split using is XCommand in specific
code or do you want additional stuff in your main interface. Look below why this
is no longer a problem.
> 
> The only reason why you need this CachedState class at the moment, is because
> you only write the output files if the 'exitCode' was nonzero. You could write
> stdout/stderr/extitCode/... always out to a file (even for successful
> compilations) eliminating this problem. The cost would be one more file, with
a
> bit of content, but looking at what dart2js compilation currently outputs for
> some tests (e.g. a polymer test):
> -rw-r----- ... 1.1M ...  /tmp/foobar.js
> -rw-r----- ... 23K   ...  /tmp/foobar.js.deps
> -rw-r----- ... 367K ...  /tmp/foobar.js.map
> -rw-r----- ... 1.6M ...  /tmp/foobar.precompiled.js
> 
> I don't think < 1k of stdout/stderr would matter much (just the deps file
would
> usually be 5-10x bigger than stdout/stderr).
This is probably not the general case, I would expect simple tests to have a
much lower difference. That said, I am not concerned about disk space here.
I actually started out by doing this, but decided against it  since it
complicated the general logic in the RunningProcess class. It just occurred to
me now that there is no reason whatsoever that I should still be looking at the
timestamp of the dart2js generated file - I should just be looking at the cached
CommandOutput file - I don't know why I had in my mind that I needed to look at
both.

I changed now the logic to only look at the new file, much better and smaller
code - removed CachedState
> 
> Additional benefit: If a test fails at runtime and the compilation was
skipped,
> we could still show stdout/stderr from dart2js in the failure output (which we
> currently don't have).

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:216: if (_neverSkipCompilation) return new
Future.value(false);
On 2014/04/29 21:20:44, kustermann wrote:
> On 2014/04/29 17:51:55, Bill Hesse wrote:
> > Shouldn't these return CachedState objects?
> 
> BTW: I hope we did not disable '--checked' mode when running test.dart :)
We did not

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:216: if (_neverSkipCompilation) return new
Future.value(false);
On 2014/04/29 17:51:55, Bill Hesse wrote:
> Shouldn't these return CachedState objects?

Done.

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:253: return const CachedState(true, false);
On 2014/04/29 17:51:55, Bill Hesse wrote:
> Is CachedState(true, false) really meaning CachedState(true, ignored), since
> having the right compiled output means we don't need the output?  If so, the
> class should document that.  Why is true, true not a better choice here?
Because it would not be true - if we have up to date compilation output it means
that compilation succeeded, i.e., we would not have saved the command output -
it is not up to date.
That said, I removed CachedState class again

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:1660: 'timedOut': timedOut,
On 2014/04/29 21:20:44, kustermann wrote:
> It may make sense to also add 'time' here, not sure though.

I did so at first, but removed it, since it would give counter intuitive numbers
in the reporting (with the --time flag). I would be surprised to see compilation
taking the top 20 spots when I did not do any compilations :-)

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_runner.dart:1988: }
On 2014/04/29 17:51:55, Bill Hesse wrote:
> Should we put a try-catch around this?  Or make writeCachedOutput never throw?

> Or do we want to crash in this case?

Changed writeCachedOutput to not ever throw

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/256743009/diff/20001/tools/testing/dart/test_...
tools/testing/dart/test_suite.dart:1883: 
On 2014/04/29 17:51:55, Bill Hesse wrote:
> In the class below this, we don't have a double line between static vars and
> static functions.

Done.

[ricow1, 2014-04-30 06:51:19]

This is still pending info about dart2js on missing files

[ricow1, 2014-05-01 16:40:38]

I added now a "fix" for the missing imports case - please let me know if that is
sufficient.

[kustermann, 2014-05-01 17:02:43]

lgtm

But you should incorporate this also into the test (see
tests/standalone/io/skipping_dart2js_compilations_test.dart)

https://codereview.chromium.org/256743009/diff/100001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/256743009/diff/100001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:1646: 'timedOut': timedOut,
Indentation.

[kustermann, 2014-05-01 22:50:04]

Another comment (see also previous one).

https://codereview.chromium.org/256743009/diff/100001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/256743009/diff/100001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:1644: 'stderr': stderr,
Are these stdout/stderr List<int>?

It might not be the best encoding, in particular since JSON.decode() might
actually allocate List<int> with 4 bytes per entry!

[Changing stdout/stderr to String would be an option]

[ricow1, 2014-05-02 07:20:44]

On 2014/05/01 22:50:04, kustermann wrote:
> Another comment (see also previous one).
> 
>
https://codereview.chromium.org/256743009/diff/100001/tools/testing/dart/test...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://codereview.chromium.org/256743009/diff/100001/tools/testing/dart/test...
> tools/testing/dart/test_runner.dart:1644: 'stderr': stderr,
> Are these stdout/stderr List<int>?
> 
> It might not be the best encoding, in particular since JSON.decode() might
> actually allocate List<int> with 4 bytes per entry!
> 
> [Changing stdout/stderr to String would be an option]
That will not happen in this cl

[ricow1, 2014-05-14 12:54:14]

Committed patchset #7 manually as r36162 (presubmit successful).