Speed up directory listing in pub.

Speed up directory listing in pub.

Now that the Dart core sources are provided to barback, we walk the sdk's lib directory. In the built SDK, that isn't too bad, but in the source repo, that includes all of the sources for dart2js and pub, including pub's tests.

This means running pub's tests is noticeably slower. Even outside of the repo, the source map change bumped up startup time by about half a second.

This helps that, along with any other place that lists a directory.

R=nweiz@google.com

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

[nweiz, 2014-03-25 18:55:07]

Can we update the explicit Directory.list behavior in BuildEnvironment to use
[listDir] now?

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

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/io.dart:248: var entities = new
Directory(dir).listSync(recursive: recursive);
Add a comment that there is a known bug with this, and reference issue 4794.
Explain why we're using it despite the bug.

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
File sdk/lib/_internal/pub/test/io_test.dart (right):

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/io_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
Since we don't have a custom implementation of directory listing anymore, we
should probably get rid of all of these tests that don't explicitly test the
hidden-file behavior we add.

[Bob Nystrom, 2014-03-25 20:04:19]

> Can we update the explicit Directory.list behavior in BuildEnvironment to use
[listDir] now?

I think the stuff in BuildEnvironment is still better for its use case.
listDir() will recurse into "packages/" directories and build up a large
intermediate list which then has to get filtered back down. BuildEnvironment's
specialized code avoids that.

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

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/io.dart:248: var entities = new
Directory(dir).listSync(recursive: recursive);
On 2014/03/25 18:55:08, nweiz wrote:
> Add a comment that there is a known bug with this, and reference issue 4794.
> Explain why we're using it despite the bug.

This is specified behavior now. The docs say: "If a symbolic link makes a loop
in the file system, then a recursive listing will not follow a link twice in the
same recursive descent, but will report it as a Link the second time it is
seen."

It's not the behavior I would want, but it is well-defined.

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
File sdk/lib/_internal/pub/test/io_test.dart (right):

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/io_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2014/03/25 18:55:08, nweiz wrote:
> Since we don't have a custom implementation of directory listing anymore, we
> should probably get rid of all of these tests that don't explicitly test the
> hidden-file behavior we add.

I figured it doesn't hurt to have them as regression tests. But I can take them
out if you like.

[nweiz, 2014-03-25 20:14:11]

lgtm

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

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/io.dart:248: var entities = new
Directory(dir).listSync(recursive: recursive);
On 2014/03/25 20:04:20, Bob Nystrom wrote:
> On 2014/03/25 18:55:08, nweiz wrote:
> > Add a comment that there is a known bug with this, and reference issue 4794.
> > Explain why we're using it despite the bug.
> 
> This is specified behavior now. The docs say: "If a symbolic link makes a loop
> in the file system, then a recursive listing will not follow a link twice in
the
> same recursive descent, but will report it as a Link the second time it is
> seen."
> 
> It's not the behavior I would want, but it is well-defined.

That doesn't make it not a bug; if you want, call it a bug in the function
specification. From the perspective of a user of pub, recursive symlinks produce
unexpected output; they don't care what the documentation of some function their
tool uses says.

I'm not saying trading correctness for speed isn't worth it, I'm just saying we
should document the tradeoff we've made and why we made it.

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
File sdk/lib/_internal/pub/test/io_test.dart (right):

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/io_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2014/03/25 20:04:20, Bob Nystrom wrote:
> On 2014/03/25 18:55:08, nweiz wrote:
> > Since we don't have a custom implementation of directory listing anymore, we
> > should probably get rid of all of these tests that don't explicitly test the
> > hidden-file behavior we add.
> 
> I figured it doesn't hurt to have them as regression tests. But I can take
them
> out if you like.

I'd rather avoid pub being in the position of being the regression test suite
for dart:io as much as possible.

[Bob Nystrom, 2014-03-26 00:51:37]

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

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/io.dart:248: var entities = new
Directory(dir).listSync(recursive: recursive);
On 2014/03/25 20:14:12, nweiz wrote:
> On 2014/03/25 20:04:20, Bob Nystrom wrote:
> > On 2014/03/25 18:55:08, nweiz wrote:
> > > Add a comment that there is a known bug with this, and reference issue
4794.
> > > Explain why we're using it despite the bug.
> > 
> > This is specified behavior now. The docs say: "If a symbolic link makes a
loop
> > in the file system, then a recursive listing will not follow a link twice in
> the
> > same recursive descent, but will report it as a Link the second time it is
> > seen."
> > 
> > It's not the behavior I would want, but it is well-defined.
> 
> That doesn't make it not a bug; if you want, call it a bug in the function
> specification.

Absent some platonic ideal of the correct behavior, I think "does exactly what
it's documented to do" can't be fairly described as a bug. I wouldn't argue that
the behavior is *good*, but it's certainly *intentional*.

> From the perspective of a user of pub, recursive symlinks produce
> unexpected output; they don't care what the documentation of some function
their
> tool uses says.
> 
> I'm not saying trading correctness for speed isn't worth it, I'm just saying
we
> should document the tradeoff we've made and why we made it.

Done.

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
File sdk/lib/_internal/pub/test/io_test.dart (right):

https://codereview.chromium.org/211373003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/io_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2014/03/25 20:14:12, nweiz wrote:
> On 2014/03/25 20:04:20, Bob Nystrom wrote:
> > On 2014/03/25 18:55:08, nweiz wrote:
> > > Since we don't have a custom implementation of directory listing anymore,
we
> > > should probably get rid of all of these tests that don't explicitly test
the
> > > hidden-file behavior we add.
> > 
> > I figured it doesn't hurt to have them as regression tests. But I can take
> them
> > out if you like.
> 
> I'd rather avoid pub being in the position of being the regression test suite
> for dart:io as much as possible.

OK, done.

[Bob Nystrom, 2014-03-26 00:51:53]

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