cache pub list results

cache pub list results

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

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

[Paul Berry, 2015-02-20 22:00:25]

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

[Paul Berry, 2015-02-20 22:00:26]

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
(right):

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:64: *
The modification time of the cache file when it was last read
s/read/accessed/g

We update the modification time when writing the cache too.

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:106:
modificationStamps.forEach((String path, int modificationStamp) {
I'd recommend changing this to an ordinary "for" loop so that you can break out
of it early if a dependency is found that has changed.

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:122: //
Create a new entry if one does not already exist
It's risky to do this here, since it leaves _cache[folder.path] in an invalid
state which will persist if an exception gets thrown.  I'd suggest deleting this
if-block and doing the update to the cache in a single atomic operation (see
suggestion below on line 144).

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:136:
Map<String, int> modificationStamps = new Map<String, int>();
Nit:  I believe the standard dart idiom for creating an empty map is

<String, int>{}

instead of 

new Map<String, int>()

(this is a really minor complaint though--they should compile down to the exact
same CPU instructions)

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:144:
Map entry = _cache[folder.path];
These three lines can be replaced with a single atomic update:

_cache[folder.path] = <String, Map>{ pubListResultKey: obj,
modificationStampsKey: modificationStamps };

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/memory_file_system.dart:179: class
_MemoryRestorePoint {
I'm not really comfortable adding this level of transactional support to the
memory file system, especially given that (a) I don't see it being used very
often, and (b) it doesn't notify watchers properly.

How about if instead we just add optional "timestamp" arguments to the newFile
and newFolder functions above, which allow specifying the timestamp of the
files/folders to be created.  That would be sufficient for the testing you're
trying to do in this CL, and it would preserve correct file watching semantics.

[Brian Wilkerson, 2015-02-20 22:05:36]

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

[Brian Wilkerson, 2015-02-20 22:05:36]

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
(right):

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:136:
Map<String, int> modificationStamps = new Map<String, int>();
True, but what we really want, for efficiency's sake, is

  new HashMap<String, int>()

[Paul Berry, 2015-02-20 22:28:05]

On 2015/02/20 22:05:36, Brian Wilkerson wrote:
>
https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
> File pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
> (right):
> 
>
https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
> pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:136:
> Map<String, int> modificationStamps = new Map<String, int>();
> True, but what we really want, for efficiency's sake, is
> 
>   new HashMap<String, int>()

Fair enough, although I'd be very surprised if this particular case ever became
a hot enough code path for it to matter :)

[Paul Berry, 2015-02-20 22:56:09]

Unrelated to previous discussion: I just noticed that the two files that were
added are located in new subdirectories called "source":

pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
pkg/analysis_server/test/source/caching_put_package_map_provider_test.dart

Can you help me understand the convention you're trying to introduce here?  Is
there something different about these new files that make them more "sourcey"
than our previous source files?

[danrubel, 2015-02-21 03:09:30]

PTAL
- addressed comments
- rerun pub if dependency changed while running
- gracefully degrade during file I/O exceptions

On 2015/02/20 22:56:09, Paul Berry wrote:
> Unrelated to previous discussion: I just noticed that the two files that were
> added are located in new subdirectories called "source":
> 
> pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
> pkg/analysis_server/test/source/caching_put_package_map_provider_test.dart
> 
> Can you help me understand the convention you're trying to introduce here?  Is
> there something different about these new files that make them more "sourcey"
> than our previous source files?

PubPackageMapProvider, the class I'm extending, is in pkg/analyzer/lib/source.
These new classes are on the analysis_server side, so I added new 'source'
directories there. That said, I'm happy to move them somewhere else if that is
preferred.

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
(right):

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:64: *
The modification time of the cache file when it was last read
On 2015/02/20 22:00:25, Paul Berry wrote:
> s/read/accessed/g
> 
> We update the modification time when writing the cache too.

Done.

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:106:
modificationStamps.forEach((String path, int modificationStamp) {
On 2015/02/20 22:00:26, Paul Berry wrote:
> I'd recommend changing this to an ordinary "for" loop so that you can break
out
> of it early if a dependency is found that has changed.

That would require a hash lookup on each iteration.
Which is more efficient?

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:122: //
Create a new entry if one does not already exist
On 2015/02/20 22:00:25, Paul Berry wrote:
> It's risky to do this here, since it leaves _cache[folder.path] in an invalid
> state which will persist if an exception gets thrown.  I'd suggest deleting
this
> if-block and doing the update to the cache in a single atomic operation (see
> suggestion below on line 144).

Done.

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:144:
Map entry = _cache[folder.path];
On 2015/02/20 22:00:25, Paul Berry wrote:
> These three lines can be replaced with a single atomic update:
> 
> _cache[folder.path] = <String, Map>{ pubListResultKey: obj,
> modificationStampsKey: modificationStamps };

Great suggestion. Done.

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/memory_file_system.dart:179: class
_MemoryRestorePoint {
On 2015/02/20 22:00:26, Paul Berry wrote:
> I'm not really comfortable adding this level of transactional support to the
> memory file system, especially given that (a) I don't see it being used very
> often, and (b) it doesn't notify watchers properly.
> 
> How about if instead we just add optional "timestamp" arguments to the newFile
> and newFolder functions above, which allow specifying the timestamp of the
> files/folders to be created.  That would be sufficient for the testing you're
> trying to do in this CL, and it would preserve correct file watching
semantics.

How do we simulate revoked and restored access? That's what I'm trying to test
with this code.

[scheglov, 2015-02-22 19:31:30]

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

[scheglov, 2015-02-22 19:31:31]

Just small comments for memory fs.

https://codereview.chromium.org/941883002/diff/20001/pkg/analyzer/lib/file_sy...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/20001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:51: * Simulate revoked file
access
This comment describes the method from the perspective of one its applications.
It would be easier to understand if we explicitly stated that it will return a
snapshot and clear everything.

https://codereview.chromium.org/941883002/diff/20001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:54: clear() {
Could we have some type here and in "restore"?
Even if we don't have any methods in the interface, just an empty marker
interface would be nicer than "dynamic" IMHO.

[Paul Berry, 2015-02-23 18:54:58]

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
(right):

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:106:
modificationStamps.forEach((String path, int modificationStamp) {
On 2015/02/21 03:09:30, danrubel wrote:
> On 2015/02/20 22:00:26, Paul Berry wrote:
> > I'd recommend changing this to an ordinary "for" loop so that you can break
> out
> > of it early if a dependency is found that has changed.
> 
> That would require a hash lookup on each iteration.
> Which is more efficient?

Since calling res.exists and res.createSource().modificationStamp both require
hitting the filesystem, I believe that the extra hash lookup will be tiny by
comparison.

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/memory_file_system.dart:179: class
_MemoryRestorePoint {
On 2015/02/21 03:09:30, danrubel wrote:
> On 2015/02/20 22:00:26, Paul Berry wrote:
> > I'm not really comfortable adding this level of transactional support to the
> > memory file system, especially given that (a) I don't see it being used very
> > often, and (b) it doesn't notify watchers properly.
> > 
> > How about if instead we just add optional "timestamp" arguments to the
newFile
> > and newFolder functions above, which allow specifying the timestamp of the
> > files/folders to be created.  That would be sufficient for the testing
you're
> > trying to do in this CL, and it would preserve correct file watching
> semantics.
> 
> How do we simulate revoked and restored access? That's what I'm trying to test
> with this code.

I was imagining that in the 'project removed then restored' test, before the
call to resProvider.clear(), we would make a note of the timestamps of
/tmp/proj1/packages/foo, /tmp/proj1/pubspec.yaml, and the parent directories
/tmp/proj1/packages and /tmp/proj1.  Then, instead of calling
resProvider.restore(restorePoint), we would simply call resProvider.newFile and
resProvider.newFolder to recreate the files and folders, supplying the old
timestamps so that they files and folders don't look new.

It should behave precisely the same as what you've written, with the
disadvantage that it's a little less general (since you have to hardcode the
specific files and folders to be created/destroyed in the test) and the
advantage that the watchers will be notified properly.

[danrubel, 2015-02-24 02:29:45]

Comments addressed. PTAL

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart
(right):

https://codereview.chromium.org/941883002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/source/caching_pub_package_map_provider.dart:106:
modificationStamps.forEach((String path, int modificationStamp) {
On 2015/02/23 18:54:58, Paul Berry wrote:
> On 2015/02/21 03:09:30, danrubel wrote:
> > On 2015/02/20 22:00:26, Paul Berry wrote:
> > > I'd recommend changing this to an ordinary "for" loop so that you can
break
> > out
> > > of it early if a dependency is found that has changed.
> > 
> > That would require a hash lookup on each iteration.
> > Which is more efficient?
> 
> Since calling res.exists and res.createSource().modificationStamp both require
> hitting the filesystem, I believe that the extra hash lookup will be tiny by
> comparison.

Good point. Done.

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/1/pkg/analyzer/lib/file_system...
pkg/analyzer/lib/file_system/memory_file_system.dart:179: class
_MemoryRestorePoint {
On 2015/02/23 18:54:58, Paul Berry wrote:
> On 2015/02/21 03:09:30, danrubel wrote:
> > On 2015/02/20 22:00:26, Paul Berry wrote:
> > > I'm not really comfortable adding this level of transactional support to
the
> > > memory file system, especially given that (a) I don't see it being used
very
> > > often, and (b) it doesn't notify watchers properly.
> > > 
> > > How about if instead we just add optional "timestamp" arguments to the
> newFile
> > > and newFolder functions above, which allow specifying the timestamp of the
> > > files/folders to be created.  That would be sufficient for the testing
> you're
> > > trying to do in this CL, and it would preserve correct file watching
> > semantics.
> > 
> > How do we simulate revoked and restored access? That's what I'm trying to
test
> > with this code.
> 
> I was imagining that in the 'project removed then restored' test, before the
> call to resProvider.clear(), we would make a note of the timestamps of
> /tmp/proj1/packages/foo, /tmp/proj1/pubspec.yaml, and the parent directories
> /tmp/proj1/packages and /tmp/proj1.  Then, instead of calling
> resProvider.restore(restorePoint), we would simply call resProvider.newFile
and
> resProvider.newFolder to recreate the files and folders, supplying the old
> timestamps so that they files and folders don't look new.
> 
> It should behave precisely the same as what you've written, with the
> disadvantage that it's a little less general (since you have to hardcode the
> specific files and folders to be created/destroyed in the test) and the
> advantage that the watchers will be notified properly.

I have moved this functionality into the test itself.

https://codereview.chromium.org/941883002/diff/20001/pkg/analyzer/lib/file_sy...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/20001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:51: * Simulate revoked file
access
On 2015/02/22 19:31:31, scheglov wrote:
> This comment describes the method from the perspective of one its
applications.
> It would be easier to understand if we explicitly stated that it will return a
> snapshot and clear everything.

Good point. I reworked this into a deleteFolder method with an appropriate
description.

https://codereview.chromium.org/941883002/diff/20001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:54: clear() {
On 2015/02/22 19:31:31, scheglov wrote:
> Could we have some type here and in "restore"?
> Even if we don't have any methods in the interface, just an empty marker
> interface would be nicer than "dynamic" IMHO.

That makes sense. Instead it was decided that a restore point did not make
sense, so I moved the restore point functionality out of this class and into the
test itself.

[scheglov, 2015-02-24 17:44:54]

Just comments.

https://codereview.chromium.org/941883002/diff/40001/pkg/analyzer/lib/file_sy...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/40001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:49: void
deleteFolder(String path) {
Should we add new tests for this method?

https://codereview.chromium.org/941883002/diff/40001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:58: throw 'failed to delete
resouce: $child';
resource

[danrubel, 2015-02-25 19:43:05]

Comments addressed. PTAL.

https://codereview.chromium.org/941883002/diff/40001/pkg/analyzer/lib/file_sy...
File pkg/analyzer/lib/file_system/memory_file_system.dart (right):

https://codereview.chromium.org/941883002/diff/40001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:49: void
deleteFolder(String path) {
On 2015/02/24 17:44:54, scheglov wrote:
> Should we add new tests for this method?

Good point. I thought this class was test code, but it's not. Test added.

https://codereview.chromium.org/941883002/diff/40001/pkg/analyzer/lib/file_sy...
pkg/analyzer/lib/file_system/memory_file_system.dart:58: throw 'failed to delete
resouce: $child';
On 2015/02/24 17:44:54, scheglov wrote:
> resource

Done.

[Paul Berry, 2015-02-25 21:02:21]

lgtm

[scheglov, 2015-02-25 21:04:44]

lgtm

[danrubel, 2015-02-26 17:24:26]

Committed patchset #6 (id:100001) manually as 44062 (presubmit successful).