Initial LibraryBundleCache implementation.

Initial LibraryBundleCache implementation.

For now the cache is very conservative and considers and change of any
source in the closure as a reason to invalidate the bundle. I don't
know whether caching Pub packages information would require less
conservative implementation or not.

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

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

[Brian Wilkerson, 2016-03-16 22:36:34]

LGTM, but I suspect there are lots of errors I wouldn't recognize as problems.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:805: * Each item of this list is the
`true` if the corresponding name usage
nit: "is the" --> "is"

[Paul Berry, 2016-03-17 18:11:32]

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/incremental_cache.dart (right):

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:45: List<int> hash =
_getLibraryClosureHash(library.source);
This seems wasteful.  _getLibraryClosureHash() is going to immediately read all
the files that _writeUris() just wrote into the cache.  Could we avoid the extra
disk I/O by having _writeUris return a map containing the data it just wrote?

Also, I assume that in the long term you are planning to add some logic which
removes files from the cache if it gets too big.  Once that logic is added,
there will be a race condition: if another process remove a file from the cache
after it is written by _writeUris() and before it is read by
_getLibraryClosureHash(), _getLibraryClosureHash will fail.  This race condition
would also be solved by having _writeUris return a map, since
_getLibraryClosureHash would just use the data in the map rather than reading it
from disk.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:51:
(cacheFolder.getChild('$hashStr.sum') as File).writeAsBytesSync(bytes);
There is a small but nonzero danger that this could lead to corrupted data, if
(a) the analyzer is killed while the file operation is in progress, or (b) there
are two analyzer processes running, and one of them tries to read a file that is
still in the process of being written by the other.

The usual way of dealing with this is to write the file to a temporary filename
that is unique to this process (e.g. "$pid.tmp") and then once it has been fully
written, rename it to the final filename.  That way readers only ever see a
complete file.

(Note that if the analyzer is killed while writing to "$pid.tmp", that fill will
remain in the filesystem, so before writing "$pid.tmp", you should check whether
such a file already exists, and remove it if it does).

Also make sure to handle the case where two processes simultaneously try to
generate the same output file.  They will write their two files to separate
"$pid.tmp" files, but then they will both try to rename them to the same final
filename.  On some OSes (Windows I think), the second rename will fail.  That's
ok, though, because as long as one of the renames succeeded, the correct file
has been written to its final location.  So you should be able to just ignore
the error if the rename fails.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:91: throw new
StateError('Unable to resolve $partUri in $librarySource');
As a general rule the analyzer should be able to do all of its tasks without
throwing exceptions even when presented with illegal code from the user.  I
believe this breaks that, because illegal code from the user could cause an
unresolvable URI.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:100: throw new
StateError('Unable to resolve $refUri in $librarySource');
Similar issue here.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:143: String sourceText =
source.contents.data;
What happens if source refers to a non-existent file?  (I believe this could
happen if the code being analyzed contains an import of a non-existent file)

[scheglov, 2016-03-17 21:08:14]

PTAL

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/incremental_cache.dart (right):

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:45: List<int> hash =
_getLibraryClosureHash(library.source);
On 2016/03/17 18:11:32, Paul Berry wrote:
> This seems wasteful.  _getLibraryClosureHash() is going to immediately read
all
> the files that _writeUris() just wrote into the cache.  Could we avoid the
extra
> disk I/O by having _writeUris return a map containing the data it just wrote?
> 
> Also, I assume that in the long term you are planning to add some logic which
> removes files from the cache if it gets too big.  Once that logic is added,
> there will be a race condition: if another process remove a file from the
cache
> after it is written by _writeUris() and before it is read by
> _getLibraryClosureHash(), _getLibraryClosureHash will fail.  This race
condition
> would also be solved by having _writeUris return a map, since
> _getLibraryClosureHash would just use the data in the map rather than reading
it
> from disk. 

Done.
We now cache CacheLibraryUris objects.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:51:
(cacheFolder.getChild('$hashStr.sum') as File).writeAsBytesSync(bytes);
On 2016/03/17 18:11:32, Paul Berry wrote:
> There is a small but nonzero danger that this could lead to corrupted data, if
> (a) the analyzer is killed while the file operation is in progress, or (b)
there
> are two analyzer processes running, and one of them tries to read a file that
is
> still in the process of being written by the other.
> 
> The usual way of dealing with this is to write the file to a temporary
filename
> that is unique to this process (e.g. "$pid.tmp") and then once it has been
fully
> written, rename it to the final filename.  That way readers only ever see a
> complete file.
> 
> (Note that if the analyzer is killed while writing to "$pid.tmp", that fill
will
> remain in the filesystem, so before writing "$pid.tmp", you should check
whether
> such a file already exists, and remove it if it does).
> 
> Also make sure to handle the case where two processes simultaneously try to
> generate the same output file.  They will write their two files to separate
> "$pid.tmp" files, but then they will both try to rename them to the same final
> filename.  On some OSes (Windows I think), the second rename will fail. 
That's
> ok, though, because as long as one of the renames succeeded, the correct file
> has been written to its final location.  So you should be able to just ignore
> the error if the rename fails.

Done.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:91: throw new
StateError('Unable to resolve $partUri in $librarySource');
On 2016/03/17 18:11:32, Paul Berry wrote:
> As a general rule the analyzer should be able to do all of its tasks without
> throwing exceptions even when presented with illegal code from the user.  I
> believe this breaks that, because illegal code from the user could cause an
> unresolvable URI.

readBundle() already ignores exception, as if there were no corresponding
bundle. I've added the corresponding try/catch to putLibrary() so that we don't
fail on write. I doubt that many packages have illegal code, so we can check and
decide how to handle it better later.

https://codereview.chromium.org/1807673006/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/incremental_cache.dart:143: String sourceText =
source.contents.data;
On 2016/03/17 18:11:32, Paul Berry wrote:
> What happens if source refers to a non-existent file?  (I believe this could
> happen if the code being analyzed contains an import of a non-existent file)

It will throw an exception, which we will catch and treat is a not available
summary.

[scheglov, 2016-03-18 03:10:56]

Description was changed from

==========
Initial LibraryBundleCache implementation.

For now the cache is very conservative and considers and change of any
source in the closure as a reason to invalidate the bundle. I don't
know whether caching Pub packages information would require less
conservative implementation or not.

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

to

==========
Initial LibraryBundleCache implementation.

For now the cache is very conservative and considers and change of any
source in the closure as a reason to invalidate the bundle. I don't
know whether caching Pub packages information would require less
conservative implementation or not.

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

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

[scheglov, 2016-03-18 03:10:58]

Committed patchset #3 (id:40001) manually as
132a13977bc17039cd3b207842aa9cd92b54bc04 (presubmit successful).

[scheglov, 2016-03-18 03:15:01]

Paul, it seems that the problems in the CL were mostly about concurrency, and
are solved in the latest patch or can be solved later.
So, given that the code is not in production, I'd like to commit it to be able
to work from it forward.

Of course any new problems that you will notice in the CL will be fixed in
following CLs.