Prototype for package-specific cache partitions

Prototype for package-specific cache partitions

R=scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/110c0f7e0ea61c73a96e765305a20aa09256e6c8

[scheglov, 2016-08-11 18:48:34]

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

[scheglov, 2016-08-11 18:48:35]

lgtm

Maybe we could add a dependency cycle test to make sure that it is handled
correctly.

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/package.dart (right):

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:50: * which the package at the given
[packageaPath] depends. The [packageMap]
[packagePath]

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:67: processedPaths.add(packagePath);
Does it handle correctly circular dependencies?

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:75: for (String dependency in
dependencies) {
dependencies.forEach(process);

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:78: List<String>
transitiveDependencies = new List<String>.from(processedPaths);
processedPaths.toList() would be shorted IMHO.

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:94: return new
List<String>.from(dependencies);
Same here.
dependencies.toList() lets us to avoid repeating type arguments.

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:102: void
_gatherDependencies(HashSet<String> dependencies, YamlMap yamlMap) {
This function is used only once, probably could be inlined.
Well, unless you want to support also other kinds of dependencies, like
dev_dependencies.

The name looks like it would make more than it does - I expected it to make some
recursive gathering.
Instead it just appends dependencies from a map.

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:139: final String packageId;
Why not just "id"?
We have already told that it's a package :-)

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:208:
.convertPackagesToMap(packages);
It seems that this method should be a function or static.

[Brian Wilkerson, 2016-08-11 20:58:02]

ptal

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/package.dart (right):

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:50: * which the package at the given
[packageaPath] depends. The [packageMap]
done

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:67: processedPaths.add(packagePath);
> Does it handle correctly circular dependencies?

Yes. It adds each packageName to processedPackages (line 61) and won't recurse
if the name was already added. But when I added a test I discovered that it
could add the package for which dependencies were being computed, and we don't
want that, so I fixed that bug.

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:75: for (String dependency in
dependencies) {
done

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:78: List<String>
transitiveDependencies = new List<String>.from(processedPaths);
done

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:94: return new
List<String>.from(dependencies);
done

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:102: void
_gatherDependencies(HashSet<String> dependencies, YamlMap yamlMap) {
It's messy to inline this because of limitations on type promotion and needing
extra local variables. I renames it instead.

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:139: final String packageId;
Because I like to type :-)

done

https://codereview.chromium.org/2237893002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/package.dart:208:
.convertPackagesToMap(packages);
It makes use of both instance fields; how could it be static?

[scheglov, 2016-08-11 21:03:09]

LGTM

[Brian Wilkerson, 2016-08-11 21:04:21]

Description was changed from

==========
Prototype for package-specific cache partitions
==========

to

==========
Prototype for package-specific cache partitions

R=scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/110c0f7e0ea61c73a96e765305a20aa09256e6c8
==========

[Brian Wilkerson, 2016-08-11 21:04:23]

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