Don't recompute the package graph unnecessarily in Entrypoint.

Don't recompute the package graph unnecessarily in Entrypoint.

R=rnystrom@google.com

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

[Bob Nystrom, 2014-08-20 20:22:02]

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:145: return
Future.wait(ids.map((id) {
A comment here explaining why the graph is being cached would be nice.

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:113: Package(this.pubspec, this.dir);
Doc comment.

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:148: value: (id)
=> cache.getCachedPubspec(id));
Explicitly piping these out of the solver seems a bit strange.

Maybe the pubspec cache should be more globally available? Is there any reason
not to move this up into Source?

[nweiz, 2014-08-20 20:51:14]

Code review changes

[nweiz, 2014-08-20 20:51:19]

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:145: return
Future.wait(ids.map((id) {
On 2014/08/20 20:22:02, Bob Nystrom wrote:
> A comment here explaining why the graph is being cached would be nice.

Done.

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:113: Package(this.pubspec, this.dir);
On 2014/08/20 20:22:02, Bob Nystrom wrote:
> Doc comment.

Done.

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:148: value: (id)
=> cache.getCachedPubspec(id));
On 2014/08/20 20:22:02, Bob Nystrom wrote:
> Explicitly piping these out of the solver seems a bit strange.
> 
> Maybe the pubspec cache should be more globally available? Is there any reason
> not to move this up into Source?

It seems like a big hassle to move the pubspec cache outside of the solver; we
already have a lot of mostly-global objects that are a pain to keep straight,
and I'd rather not add more. It especially doesn't seem worth it when it would
only be used in one place that's already consuming a SolveResult anyway.

Would it feel less strange to you to have the SolveResult include Package
objects instead?

[Bob Nystrom, 2014-08-20 21:30:01]

lgtm

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/489943002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:148: value: (id)
=> cache.getCachedPubspec(id));
On 2014/08/20 20:51:18, nweiz wrote:
> On 2014/08/20 20:22:02, Bob Nystrom wrote:
> > Explicitly piping these out of the solver seems a bit strange.
> > 
> > Maybe the pubspec cache should be more globally available? Is there any
reason
> > not to move this up into Source?
> 
> It seems like a big hassle to move the pubspec cache outside of the solver; we
> already have a lot of mostly-global objects that are a pain to keep straight,
> and I'd rather not add more. It especially doesn't seem worth it when it would
> only be used in one place that's already consuming a SolveResult anyway.
> 
> Would it feel less strange to you to have the SolveResult include Package
> objects instead?

Per our discussion, I think this is OK. I'd return the pubspecs and not
packages.

[nweiz, 2014-08-20 23:39:44]

Committed patchset #2 manually as 39437 (presubmit successful).