luci-go/common/dirwalk: Code for walking a directory tree efficiently

luci-go/common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirwalk.WalkBasic` - Uses go's builtin walk method
 * `dirwalk.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirwalk.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

After comparison on Linux the following results were found;

 * dirwalk.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirwalk.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirwalk.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

The code is all under a single directory to allow it to be moved into its own go package in the future.

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990

[mithro, 2016-06-09 16:47:01]

tansell@chromium.org changed reviewers:
+ maruel@chromium.org, tandrii@chromium.org

[mithro, 2016-06-09 16:47:02]

Hi Maruel,

This is the dirtools code split out into a separate CL with a whole bunch of
extra testing stuff added.

As they are performance based tests, they don't really fit well with the normal
go testing. Would love some advice if this is going down the right direction for
that.

I've tried to add test cases which represent a bunch of real world systems we
have.

The output of the tests is actually quite interesting. Definitely want to check
how they look on Windows+Mac.

Tim 'mithro' Ansell

[M-A Ruel, 2016-06-10 02:20:43]

On 2016/06/09 16:47:02, mithro wrote:
> Hi Maruel,
> 
> This is the dirtools code split out into a separate CL with a whole bunch of
> extra testing stuff added.
> 
> As they are performance based tests, they don't really fit well with the
normal
> go testing. Would love some advice if this is going down the right direction
for
> that.
> 
> I've tried to add test cases which represent a bunch of real world systems we
> have.
> 
> The output of the tests is actually quite interesting. Definitely want to
check
> how they look on Windows+Mac.
> 
> Tim 'mithro' Ansell

You can't "read" a directory on Windows so you'll have to adapt your code.

I agree using the Go native benchmark is not really appropriate but all the perf
stuff could be merged into a single file, both generation and the different kind
of walking.

Thanks

[mithro, 2016-06-23 07:18:49]

Description was changed from

==========
common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirtools.WalkBasic` - Uses go's builtin walk method
 * `dirtools.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirtools.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

The `walk.sh` creates a bunch of typical directory structures using `gendir` and
then runs `walkdir` using the different walking methods. The extra parameters
are passed to the script are passed to `walkdir`.

Examples;
```
./scripts/perf/walk.sh --repeat 10
./scripts/perf/walk.sh --repeat 10 --do hash
./scripts/perf/walk.sh --repeat 10 --do phash
```

After comparison on Linux the following results where found;

 * dirtools.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirtools.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirtools.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

This code was split out of https://codereview.chromium.org/2014243002

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

to

==========
luci-go/common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirtools.WalkBasic` - Uses go's builtin walk method
 * `dirtools.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirtools.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

The `walk.sh` creates a bunch of typical directory structures using `gendir` and
then runs `walkdir` using the different walking methods. The extra parameters
are passed to the script are passed to `walkdir`.

Examples;
```
./scripts/perf/walk.sh --repeat 10
./scripts/perf/walk.sh --repeat 10 --do hash
./scripts/perf/walk.sh --repeat 10 --do phash
```

After comparison on Linux the following results where found;

 * dirtools.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirtools.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirtools.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

This code was split out of https://codereview.chromium.org/2014243002

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

[mithro, 2016-09-07 06:09:40]

Description was changed from

==========
luci-go/common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirtools.WalkBasic` - Uses go's builtin walk method
 * `dirtools.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirtools.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

The `walk.sh` creates a bunch of typical directory structures using `gendir` and
then runs `walkdir` using the different walking methods. The extra parameters
are passed to the script are passed to `walkdir`.

Examples;
```
./scripts/perf/walk.sh --repeat 10
./scripts/perf/walk.sh --repeat 10 --do hash
./scripts/perf/walk.sh --repeat 10 --do phash
```

After comparison on Linux the following results where found;

 * dirtools.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirtools.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirtools.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

This code was split out of https://codereview.chromium.org/2014243002

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

to

==========
luci-go/common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirwalk.WalkBasic` - Uses go's builtin walk method
 * `dirwalk.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirwalk.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

The `test.sh` creates a bunch of typical directory structures using `gendir` and
then runs `walkdir` using the different walking methods. The extra parameters
are passed to the script are passed to `walkdir`.

Examples;
```
./common/dirwalk/test.sh --repeat 10
./common/dirwalk/test.sh --repeat 10 --do hash
./common/dirwalk/test.sh --repeat 10 --do phash
```

After comparison on Linux the following results where found;

 * dirwalk.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirwalk.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirwalk.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

The code is all under a single directory to allow it to be moved into its own go
package in the future.

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

[M-A Ruel, 2016-09-15 14:31:03]

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer.go
File common/dirwalk/observer.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:7: /**
Always use // comments for consistency. I do it the ol' ye cranky style: Ctrl-V,
3LI, "// ", Esc.


(I lied, I do use Arrow keys but shuuuut, don't tell anymore, in particular
stip!)

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:16: LargeFile(filename string)
why not just have OnFile(path string, content []byte) and pass nil on content
when the file is deemed "large enough".

In fact, I'd rather have Error() be merged in too.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:23: Finished()
It's finished when the function returns.


Overall, this whole interface can be replaced by a single callback.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/test.sh
File common/dirwalk/test.sh (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/test.sh#...
common/dirwalk/test.sh:1: #!/bin/bash
I'd like to have the test to be split up in a separate CL, that could be
committed first. It needs to be made to work on Windows, so I recommend writing
this in Go too. It's not super efficient LoC-wise but it'll make the whole thing
more maintainable.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/te...
File common/dirwalk/tests/test1.json (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/te...
common/dirwalk/tests/test1.json:1: {
I think these json could be integrated in the executable as premade
configurations.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/generate.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:22: } else {
}
no need for else

same below

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:38: func randStr(r *rand.Rand,
length uint64, runes []rune) string {
I'd prefer length to be int.

It's a bit confusing that length is in runes, not in bytes.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:64: type FileType int
I'd prefer to call it Compressibility and have a N levels. The fact it contains
ascii or full 256 range is irrelevant.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:88: // FIXME: Maybe some UTF-8
characters?
It depends; for file paths, you want UTF-8. For content, you want bytes.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:102: log.Fatal(err)
return an error, no need to crash.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:112: case FILETYPE_BIN_RAND:
You should create a generator so basically:


// Get returns a generator of data that is deterministic.
func (f FileType) Get(r *rand.Rand) io.Reader {
  // Returns a struct that implements io.Reader and potentially io.WriterTo for
performance.
}

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:160: func GenerateFiles(r
*rand.Rand, dir string, num uint64, filesize_min uint64, filesize_max uint64) {
One thing I care a lot about is to ensure that the generated tree is always
exactly the same on all platforms. The current code design makes it tricky to
guarantee this property.

I feel like a more structured approach could be useful;
type TreeGenerator struct {
  // Include all properties, including the structs below.
}

then it would have only one public method:
func (t*TreeGenerator) Generator(dest string) error {
  ...
}

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/main.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:7: // Quick tool for generating
directories to walk.
Move this to a real package docstring.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:26: var config = flag.String("config",
"", "JSON config file for generating test file.")
I'm a big fan of putting these inside the main function. I'm also a big fan of
the 
func mainImpl() error { ... }
pattern so you can return an error and it is nicely printed out, instead of
log.Fatal(), which is only useful for toys IMHO.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:37: configdata, err :=
ioutil.ReadFile(*config)
I don't think we need that much configurability. A few named configurations is
likely enough IMHO.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:58: if err := os.RemoveAll(*outdir);
err != nil {
Remove the remove support. Caller can handle that.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:64: if *seed != 4 && settings.Seed !=
*seed {
I don't see why we put settings.Seed. There's no need for this knob.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:71: if err := os.MkdirAll(*outdir,
0755); err != nil {
0700

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:75: GenerateTree(r, *outdir,
&settings.Tree)
something like:
return settings.GenerateTree(*outdir, *seed)

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/walkdir/main.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:7: // Quick tool for generating
directories to walk.
same

I know it's quick, and it's a tool (it's an executable), just say what it does.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:24: var method =
flag.String("method", "simple", "Method used to walk the tree")
Having these inside main(), as I noted in the other CL, would make this code
more readable.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:34: // Walker which does nothing but
count the files of each type
// NullWalker implements Walker. It only count the number of files of each type.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:36: smallfiles uint64
does this need to be uint64 or int would do? I think yes

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:52: // Walker which just prints the
filenames of everything
Same for the rest

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
File common/dirwalk/walkbasic.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
common/dirwalk/walkbasic.go:13: /**
same

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
common/dirwalk/walkbasic.go:43: obs.Finished()
I don't see the point, the caller knows when it's finished; the function
returns.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walknost...
File common/dirwalk/walknostat.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walknost...
common/dirwalk/walknostat.go:5: /**
same

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkpara...
File common/dirwalk/walkparallel.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkpara...
common/dirwalk/walkparallel.go:7: WARNING: THIS FUNCTION IS SLOWER THAN THE
NON-PARALLEL VERSION!
// Testing shows that this function is slower than the non-parallel version on
all desktop class OSes when using local disk.

This may not be true on shared drive, non desktop OS, etc.

[mithro, 2016-09-20 12:41:45]

Hi Marc,

I had a bunch of questions regarding your comments. 

I will upload a version which uses generators for random data production but
wanted to get the other changes and comments out first.

The primary is I don't quite understand how to change the command line tools.

It sounds like you might want to use these tests as more general performance
tests?

Tim 'mithro' Ansell

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer.go
File common/dirwalk/observer.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:7: /**
On 2016/09/15 14:31:02, M-A Ruel wrote:
> Always use // comments for consistency. I do it the ol' ye cranky style:
Ctrl-V,
> 3LI, "// ", Esc.
> 
> 
> (I lied, I do use Arrow keys but shuuuut, don't tell anymore, in particular
> stip!)

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:16: LargeFile(filename string)
On 2016/09/15 14:31:02, M-A Ruel wrote:
> why not just have OnFile(path string, content []byte) and pass nil on content
> when the file is deemed "large enough".

I originally had a single call back which provided a bunch more information (the
file size and such) but ended up culling back to providing the bare minimum for
both types.

I think having separate callbacks makes it clearer what the expected information
for each type of file is.

> In fact, I'd rather have Error() be merged in too.

I don't quite understand how the Error() would be merged? Just as another
argument? You then end up with a bunch of optional arguments? With that you end
up with;
 File(filename string, alldata []byte (or nil), err error (or nil))

The Start/Finish directory functions are likely to be needed in the future (as
they are useful indicators for better sharding then just every N files) which
means this interface is going to expand then too.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:23: Finished()
On 2016/09/15 14:31:02, M-A Ruel wrote:
> It's finished when the function returns.
> 
> 
> Overall, this whole interface can be replaced by a single callback.

See my comments below.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/test.sh
File common/dirwalk/test.sh (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/test.sh#...
common/dirwalk/test.sh:1: #!/bin/bash
On 2016/09/15 14:31:02, M-A Ruel wrote:
> I'd like to have the test to be split up in a separate CL, that could be
> committed first. It needs to be made to work on Windows, so I recommend
writing
> this in Go too. It's not super efficient LoC-wise but it'll make the whole
thing
> more maintainable.

This file is just to make it easy to run all the performance tests at once (so I
can step away for a coffee) -- I wouldn't expect to use if you wanted to run
these tests continuously.

Should I just remove the file?

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/generate.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:22: } else {
On 2016/09/15 14:31:02, M-A Ruel wrote:
> }
> no need for else
> 
> same below

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:38: func randStr(r *rand.Rand,
length uint64, runes []rune) string {
On 2016/09/15 14:31:02, M-A Ruel wrote:
> I'd prefer length to be int.

Negative length isn't valid though?

> It's a bit confusing that length is in runes, not in bytes.

Strings are in runes?

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:64: type FileType int
On 2016/09/15 14:31:02, M-A Ruel wrote:
> I'd prefer to call it Compressibility and have a N levels. The fact it
contains
> ascii or full 256 range is irrelevant.

I think it is the *type* which is the important factor here. There can be a
pretty big difference between repeated random text and Lorem Ipsum repeated
which is closer to human language. Some other interesting types of data to be
added in the future might be "empty file", "file filled with a single type of
byte" (IE all zeros) or "jpeg image data", etc.

The compressibility is just a side effect of the *type* of data I have selected
so far.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:102: log.Fatal(err)
On 2016/09/15 14:31:02, M-A Ruel wrote:
> return an error, no need to crash.

We can't really recover in this case?

We depend on the random being called in exactly the same order for the output to
be identical.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:112: case FILETYPE_BIN_RAND:
On 2016/09/15 14:31:02, M-A Ruel wrote:
> You should create a generator so basically:
> 
> 
> // Get returns a generator of data that is deterministic.
> func (f FileType) Get(r *rand.Rand) io.Reader {
>   // Returns a struct that implements io.Reader and potentially io.WriterTo
for
> performance.
> }

There was a specific reason I didn't do a generator like this but I can't seem
to figure it out now.

I'll upload a version which changes to that format shortly.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:160: func GenerateFiles(r
*rand.Rand, dir string, num uint64, filesize_min uint64, filesize_max uint64) {
On 2016/09/15 14:31:02, M-A Ruel wrote:
> One thing I care a lot about is to ensure that the generated tree is always
> exactly the same on all platforms. The current code design makes it tricky to
> guarantee this property.
> 
> I feel like a more structured approach could be useful;
> type TreeGenerator struct {
>   // Include all properties, including the structs below.
> }
> 
> then it would have only one public method:
> func (t*TreeGenerator) Generator(dest string) error {
>   ...
> }

This kind of function kind of exists at the bottom of this file (see lines
243-246). 

Would it be enough to just make the other GenerateDirs/GenerateFiles functions
private?

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/main.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:7: // Quick tool for generating
directories to walk.
On 2016/09/15 14:31:02, M-A Ruel wrote:
> Move this to a real package docstring.

This docstring is only for the tool though? (The whole package is "dirwalk"
right?)

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:26: var config = flag.String("config",
"", "JSON config file for generating test file.")
On 2016/09/15 14:31:03, M-A Ruel wrote:
> I'm a big fan of putting these inside the main function. I'm also a big fan of
> the 
> func mainImpl() error { ... }
> pattern so you can return an error and it is nicely printed out, instead of
> log.Fatal(), which is only useful for toys IMHO.

Do you have an example? I'm not sure I understand...

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:37: configdata, err :=
ioutil.ReadFile(*config)
On 2016/09/15 14:31:02, M-A Ruel wrote:
> I don't think we need that much configurability. A few named configurations is
> likely enough IMHO.

We have 6 configs so far, it is likely we'll end up getting quite a few more.

If the configs are all inside this tool, then we need support for things like
"listing the configs" (where as we can just list a directory).

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:58: if err := os.RemoveAll(*outdir);
err != nil {
On 2016/09/15 14:31:02, M-A Ruel wrote:
> Remove the remove support. Caller can handle that.

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:64: if *seed != 4 && settings.Seed !=
*seed {
On 2016/09/15 14:31:02, M-A Ruel wrote:
> I don't see why we put settings.Seed. There's no need for this knob.

A config which wants to produce identical output needs to set the seed.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:71: if err := os.MkdirAll(*outdir,
0755); err != nil {
On 2016/09/15 14:31:02, M-A Ruel wrote:
> 0700

Why 0700 rather than 0755?

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/walkdir/main.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:7: // Quick tool for generating
directories to walk.
On 2016/09/15 14:31:03, M-A Ruel wrote:
> same
> 
> I know it's quick, and it's a tool (it's an executable), just say what it
does.

Done.

This comment was totally wrong anyway.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:24: var method =
flag.String("method", "simple", "Method used to walk the tree")
On 2016/09/15 14:31:03, M-A Ruel wrote:
> Having these inside main(), as I noted in the other CL, would make this code
> more readable.

See above.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:34: // Walker which does nothing but
count the files of each type
On 2016/09/15 14:31:03, M-A Ruel wrote:
> // NullWalker implements Walker. It only count the number of files of each
type.

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:36: smallfiles uint64
On 2016/09/15 14:31:03, M-A Ruel wrote:
> does this need to be uint64 or int would do? I think yes

Well, it can't be negative?

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/walkdir/main.go:52: // Walker which just prints the
filenames of everything
On 2016/09/15 14:31:03, M-A Ruel wrote:
> Same for the rest

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
File common/dirwalk/walkbasic.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
common/dirwalk/walkbasic.go:13: /**
On 2016/09/15 14:31:03, M-A Ruel wrote:
> same

I assume you mean regarding the comment style?

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
common/dirwalk/walkbasic.go:43: obs.Finished()
On 2016/09/15 14:31:03, M-A Ruel wrote:
> I don't see the point, the caller knows when it's finished; the function
> returns.

It is normal that the observer needs to be notified that it will get no more
input (so it can do things like finalize files and such). 

There are two options here;
 * This interface does the notification.
 * The caller does the notification.

If the caller does the notification then there is always the chance that they
forget to actually do the call.

As well, if you have an observer which is doing something with the callbacks in
parallel, the finish call is the only way to guarantee that the observer has
finished by the time this call ends. You can see this used in the
ParallelHashWalker observer.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walknost...
File common/dirwalk/walknostat.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walknost...
common/dirwalk/walknostat.go:5: /**
On 2016/09/15 14:31:03, M-A Ruel wrote:
> same

I assume you mean regarding the comment style?

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkpara...
File common/dirwalk/walkparallel.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkpara...
common/dirwalk/walkparallel.go:7: WARNING: THIS FUNCTION IS SLOWER THAN THE
NON-PARALLEL VERSION!
On 2016/09/15 14:31:03, M-A Ruel wrote:
> // Testing shows that this function is slower than the non-parallel version on
> all desktop class OSes when using local disk.
> 
> This may not be true on shared drive, non desktop OS, etc.

Done.

The problem is frequently around locking of the directory nodes.

[M-A Ruel, 2016-09-20 16:37:27]

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer.go
File common/dirwalk/observer.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:16: LargeFile(filename string)
On 2016/09/20 12:41:43, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > why not just have OnFile(path string, content []byte) and pass nil on
content
> > when the file is deemed "large enough".
> 
> I originally had a single call back which provided a bunch more information
(the
> file size and such) but ended up culling back to providing the bare minimum
for
> both types.
> 
> I think having separate callbacks makes it clearer what the expected
information
> for each type of file is.
> 
> > In fact, I'd rather have Error() be merged in too.
> 
> I don't quite understand how the Error() would be merged? Just as another
> argument? You then end up with a bunch of optional arguments? With that you
end
> up with;
>  File(filename string, alldata []byte (or nil), err error (or nil))
> 
> The Start/Finish directory functions are likely to be needed in the future (as
> they are useful indicators for better sharding then just every N files) which
> means this interface is going to expand then too.

I disagree. The walk is finished when the function accepting this interface
returns. There's still no need for Finished().

I prefer File(filename string, alldata []byte, err error)
Then as a matter of fact, you don't even need an interface anymore, just a
function call back. It's much simpler.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/test.sh
File common/dirwalk/test.sh (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/test.sh#...
common/dirwalk/test.sh:1: #!/bin/bash
On 2016/09/20 12:41:44, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > I'd like to have the test to be split up in a separate CL, that could be
> > committed first. It needs to be made to work on Windows, so I recommend
> writing
> > this in Go too. It's not super efficient LoC-wise but it'll make the whole
> thing
> > more maintainable.
> 
> This file is just to make it easy to run all the performance tests at once (so
I
> can step away for a coffee) -- I wouldn't expect to use if you wanted to run
> these tests continuously.
> 
> Should I just remove the file?

Yes, we do not use bash unless it is expressively posix specific. As I said,
this has to be reimplemented in Go. I'm fine with the idea, just not the
language.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/generate.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:38: func randStr(r *rand.Rand,
length uint64, runes []rune) string {
On 2016/09/20 12:41:44, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > I'd prefer length to be int.
> 
> Negative length isn't valid though?

uint is sparingly used in Go, it's really only used when there's bit
manipulation.

> > It's a bit confusing that length is in runes, not in bytes.
> 
> Strings are in runes?

len(string) is in bytes.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:102: log.Fatal(err)
On 2016/09/20 12:41:44, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > return an error, no need to crash.
> 
> We can't really recover in this case?
> 
> We depend on the random being called in exactly the same order for the output
to
> be identical.

You return the error, the error is surfaced to the program, the program returns
with exit code 1. I prefer this file to not import log at all.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:197: log.Fatal(err)
same here.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/main.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:7: // Quick tool for generating
directories to walk.
On 2016/09/20 12:41:45, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > Move this to a real package docstring.
> 
> This docstring is only for the tool though? (The whole package is "dirwalk"
> right?)

Yes, but tools still show up in godoc.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:26: var config = flag.String("config",
"", "JSON config file for generating test file.")
On 2016/09/20 12:41:44, mithro wrote:
> On 2016/09/15 14:31:03, M-A Ruel wrote:
> > I'm a big fan of putting these inside the main function. I'm also a big fan
of
> > the 
> > func mainImpl() error { ... }
> > pattern so you can return an error and it is nicely printed out, instead of
> > log.Fatal(), which is only useful for toys IMHO.
> 
> Do you have an example? I'm not sure I understand...

Here's the simplest example I could find:
https://github.com/maruel/dlibox/blob/master/go/pio/cmd/ir/main.go

the other tools in luci-go generally have subcommands so their design is
different.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:37: configdata, err :=
ioutil.ReadFile(*config)
On 2016/09/20 12:41:44, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > I don't think we need that much configurability. A few named configurations
is
> > likely enough IMHO.
> 
> We have 6 configs so far, it is likely we'll end up getting quite a few more.

Why?
- layout tests
- compile step
- small test

I don't see how this could scale more than 10 or so.

> If the configs are all inside this tool, then we need support for things like
> "listing the configs" (where as we can just list a directory).

Fine for now.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:64: if *seed != 4 && settings.Seed !=
*seed {
On 2016/09/20 12:41:45, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > I don't see why we put settings.Seed. There's no need for this knob.
> 
> A config which wants to produce identical output needs to set the seed.

Yes but why make it configurable and why not make it default to 0?

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:71: if err := os.MkdirAll(*outdir,
0755); err != nil {
On 2016/09/20 12:41:44, mithro wrote:
> On 2016/09/15 14:31:02, M-A Ruel wrote:
> > 0700
> 
> Why 0700 rather than 0755?

Because there's no point in other users to read this.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
File common/dirwalk/walkbasic.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkbasi...
common/dirwalk/walkbasic.go:43: obs.Finished()
On 2016/09/20 12:41:45, mithro wrote:
> On 2016/09/15 14:31:03, M-A Ruel wrote:
> > I don't see the point, the caller knows when it's finished; the function
> > returns.
> 
> It is normal that the observer needs to be notified that it will get no more
> input (so it can do things like finalize files and such). 
> 
> There are two options here;
>  * This interface does the notification.
>  * The caller does the notification.
> 
> If the caller does the notification then there is always the chance that they
> forget to actually do the call.
> 
> As well, if you have an observer which is doing something with the callbacks
in
> parallel, the finish call is the only way to guarantee that the observer has
> finished by the time this call ends. You can see this used in the
> ParallelHashWalker observer.

The caller knows the walk terminated because WalkBasic() returns. You are
over-designing here.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkpara...
File common/dirwalk/walkparallel.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/walkpara...
common/dirwalk/walkparallel.go:7: WARNING: THIS FUNCTION IS SLOWER THAN THE
NON-PARALLEL VERSION!
On 2016/09/20 12:41:45, mithro wrote:
> On 2016/09/15 14:31:03, M-A Ruel wrote:
> > // Testing shows that this function is slower than the non-parallel version
on
> > all desktop class OSes when using local disk.
> > 
> > This may not be true on shared drive, non desktop OS, etc.
> 
> Done.
> 
> The problem is frequently around locking of the directory nodes.

That is actually a more helpful comment that should be put on the WalkParallel
docstring.

https://codereview.chromium.org/2054763004/diff/120001/common/dirwalk/observe...
File common/dirwalk/observer.go (right):

https://codereview.chromium.org/2054763004/diff/120001/common/dirwalk/observe...
common/dirwalk/observer.go:13: 
No empty line, otherwise the comment is not associated with the symbol.

Comment must start with the symbol name.
No need to state it is an interface.

So

// WalkObserver receives the results ...

[mithro, 2016-09-22 09:46:01]

Description was changed from

==========
luci-go/common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirwalk.WalkBasic` - Uses go's builtin walk method
 * `dirwalk.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirwalk.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

The `test.sh` creates a bunch of typical directory structures using `gendir` and
then runs `walkdir` using the different walking methods. The extra parameters
are passed to the script are passed to `walkdir`.

Examples;
```
./common/dirwalk/test.sh --repeat 10
./common/dirwalk/test.sh --repeat 10 --do hash
./common/dirwalk/test.sh --repeat 10 --do phash
```

After comparison on Linux the following results where found;

 * dirwalk.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirwalk.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirwalk.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

The code is all under a single directory to allow it to be moved into its own go
package in the future.

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

to

==========
luci-go/common/dirtools: Tools for walking a directory tree efficiently

This CL adds code to walk directory trees efficiently. It adds three methods of
walking which have the same interface;

 * `dirwalk.WalkBasic` - Uses go's builtin walk method
 * `dirwalk.WalkParallel` - Uses a worker pool to do the walk on many threads.
 * `dirwalk.WalkNoStat` - Uses a custom method which doesn't call stat.

It also adds a bunch of tools for examining the performance of these methods;

 * `gendir` - Takes a json description and generates a directory.
 * `walkdir` - Walks a directory doing tests.

After comparison on Linux the following results were found;

 * dirwalk.WalkNoStat is generally fastest for large number of small files
   (such as test1, test4, test5 & test6) and not measurable slower in other
   cases.

 * dirwalk.WalkParallel is *much* slower in almost all cases (even than the
   built in walk)
   - In some cases it can deliver the same wall clock time for large increase
     in CPU seconds used.

 * dirwalk.WalkBasic has very strong performance for the lines of code needed.

 * Doing a parallel hash increases performance sometimes and decreases it in
   others.

The code is all under a single directory to allow it to be moved into its own go
package in the future.

Bugs:
 * https://github.com/luci/luci-go/issues/9
 * https://crbug.com/598990

BUG=598990
==========

[mithro, 2016-09-22 11:19:59]

Hi Maruel,

I've refactored this CL a bunch to match the io.Reader style for the file
content generator. I also split the walker implementations out into their own
files to make them clearer. I still think the observer interface needs to be
more than a single callback.

Tim 'mithro' Ansell

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer.go
File common/dirwalk/observer.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/observer...
common/dirwalk/observer.go:16: LargeFile(filename string)
On 2016/09/20 at 16:37:26, M-A Ruel wrote:
> On 2016/09/20 12:41:43, mithro wrote:
> > On 2016/09/15 14:31:02, M-A Ruel wrote:
> > > why not just have OnFile(path string, content []byte) and pass nil on
content
> > > when the file is deemed "large enough".
> > 
> > I originally had a single call back which provided a bunch more information
(the
> > file size and such) but ended up culling back to providing the bare minimum
for
> > both types.
> > 
> > I think having separate callbacks makes it clearer what the expected
information
> > for each type of file is.
> > 
> > > In fact, I'd rather have Error() be merged in too.
> > 
> > I don't quite understand how the Error() would be merged? Just as another
> > argument? You then end up with a bunch of optional arguments? With that you
end
> > up with;
> >  File(filename string, alldata []byte (or nil), err error (or nil))
> > 
> > The Start/Finish directory functions are likely to be needed in the future
(as
> > they are useful indicators for better sharding then just every N files)
which
> > means this interface is going to expand then too.

You seemed to miss the part where I mentioned the StartDir/FinishDir functions
will probably be need

> I disagree. The walk is finished when the function accepting this interface
returns. There's still no need for Finished().

Without the Finished callback I can't figure out how to make the
ParallelHashWalker work in the walkdir tool without needing special casing for
that walker.

The ParallelHashWalker doesn't finish until a hash has been done on all paths,
that occur well after the last path has been found. Without the Finished call
the program exits when the last path has been found and thus doesn't processing
the remaining queued files.

> I prefer File(filename string, alldata []byte, err error)
> Then as a matter of fact, you don't even need an interface anymore, just a
function call back. It's much simpler.

Something else I forgot to mention is that the error relates to a given path -
you don't know if that path is a file, directory or something else. Calling
"File(<path>, nil, <error>)" when the error is for a directory is actually going
to cause confusion.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/generate.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:102: log.Fatal(err)
On 2016/09/20 at 16:37:27, M-A Ruel wrote:
> On 2016/09/20 12:41:44, mithro wrote:
> > On 2016/09/15 14:31:02, M-A Ruel wrote:
> > > return an error, no need to crash.
> > 
> > We can't really recover in this case?
> > 
> > We depend on the random being called in exactly the same order for the
output to
> > be identical.
> 
> You return the error, the error is surfaced to the program, the program
returns with exit code 1. I prefer this file to not import log at all.

Bubbling that error all the way up to the top seems like a lot of effort for
very little extra value? 

Hitting this is a fatal error that you can't recover from....

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/generate.go:112: case FILETYPE_BIN_RAND:
On 2016/09/15 at 14:31:02, M-A Ruel wrote:
> You should create a generator so basically:
> 
> 
> // Get returns a generator of data that is deterministic.
> func (f FileType) Get(r *rand.Rand) io.Reader {
>   // Returns a struct that implements io.Reader and potentially io.WriterTo
for performance.
> }

Done.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
File common/dirwalk/tests/tools/gendir/main.go (right):

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:7: // Quick tool for generating
directories to walk.
On 2016/09/20 at 16:37:27, M-A Ruel wrote:
> On 2016/09/20 12:41:45, mithro wrote:
> > On 2016/09/15 14:31:02, M-A Ruel wrote:
> > > Move this to a real package docstring.
> > 
> > This docstring is only for the tool though? (The whole package is "dirwalk"
> > right?)
> 
> Yes, but tools still show up in godoc.

The part I didn't understand was that you wanted this as a string for the
"package main" in this directory. (The idea that there are multiple "package
main" things I still find very confusing to me.)

I have done that.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:26: var config = flag.String("config",
"", "JSON config file for generating test file.")
On 2016/09/20 at 16:37:27, M-A Ruel wrote:
> On 2016/09/20 12:41:44, mithro wrote:
> > On 2016/09/15 14:31:03, M-A Ruel wrote:
> > > I'm a big fan of putting these inside the main function. I'm also a big
fan of
> > > the 
> > > func mainImpl() error { ... }
> > > pattern so you can return an error and it is nicely printed out, instead
of
> > > log.Fatal(), which is only useful for toys IMHO.
> > 
> > Do you have an example? I'm not sure I understand...
> 
> Here's the simplest example I could find:
> https://github.com/maruel/dlibox/blob/master/go/pio/cmd/ir/main.go
> 
> the other tools in luci-go generally have subcommands so their design is
different.

Done for both tools.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:37: configdata, err :=
ioutil.ReadFile(*config)
On 2016/09/20 at 16:37:27, M-A Ruel wrote:
> On 2016/09/20 12:41:44, mithro wrote:
> > On 2016/09/15 14:31:02, M-A Ruel wrote:
> > > I don't think we need that much configurability. A few named
configurations is
> > > likely enough IMHO.
> > 
> > We have 6 configs so far, it is likely we'll end up getting quite a few
more.
> 
> Why?
> - layout tests
> - compile step
> - small test
> 
> I don't see how this could scale more than 10 or so.
> 
> > If the configs are all inside this tool, then we need support for things
like
> > "listing the configs" (where as we can just list a directory).
> 
> Fine for now.

These tests are designed to provide a set of different configurations which can
uncover different performance problems. They aren't designed to exactly emulate
any of the real world structures.

They should cover;

 * Depth of the directory structure (shallow or deep with different branching
factors)
 * File distribution (large number in one directory, large number over many
directories)
 * The mixture of files and directories (just directories, just files, mixture)
 * Different file content types (small files, large files, mixture) etc.

These tests helped me understand how the walking was performing and where the
bottlenecks were coming from.

We probably *also* want to have tests which emulate the real world structures
you pointed out.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:64: if *seed != 4 && settings.Seed !=
*seed {
On 2016/09/20 at 16:37:27, M-A Ruel wrote:
> On 2016/09/20 12:41:45, mithro wrote:
> > On 2016/09/15 14:31:02, M-A Ruel wrote:
> > > I don't see why we put settings.Seed. There's no need for this knob.
> > 
> > A config which wants to produce identical output needs to set the seed.
> 
> Yes but why make it configurable and why not make it default to 0?

Seed of 0 is special cased as "no-seed" in most systems, thus a default of zero
isn't a great idea.

https://codereview.chromium.org/2054763004/diff/80001/common/dirwalk/tests/to...
common/dirwalk/tests/tools/gendir/main.go:71: if err := os.MkdirAll(*outdir,
0755); err != nil {
On 2016/09/20 at 16:37:27, M-A Ruel wrote:
> On 2016/09/20 12:41:44, mithro wrote:
> > On 2016/09/15 14:31:02, M-A Ruel wrote:
> > > 0700
> > 
> > Why 0700 rather than 0755?
> 
> Because there's no point in other users to read this.

Done.

[M-A Ruel, 2016-09-23 01:48:17]

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/tests/t...
File common/dirwalk/tests/tools/walkdir/walkers_phash.go (right):

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/tests/t...
common/dirwalk/tests/tools/walkdir/walkers_phash.go:38: var filecount uint64 = 0
=0 not needed

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/tests/t...
common/dirwalk/tests/tools/walkdir/walkers_phash.go:41: filecount += 1
++

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/walk_no...
File common/dirwalk/walk_nostat.go (right):

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/walk_no...
common/dirwalk/walk_nostat.go:101: sort.Sort(SmallFileByName(smallfiles))
Why pass the file names in order?

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/walk_pa...
File common/dirwalk/walk_parallel.go (right):

https://codereview.chromium.org/2054763004/diff/220001/common/dirwalk/walk_pa...
common/dirwalk/walk_parallel.go:72: sort.Strings(dircontents)
same

[mithro, 2016-11-08 03:48:38]

tansell@chromium.org changed reviewers:
+ djd@chromium.org, mcgreevy@golang.org

[mithro, 2016-11-08 03:48:39]

Hi everyone,

Based on the feedback from Dave Day (djd@) and Marc, I have done a major rewrite
of this CL. Moving forward, Dave and Michael as Go experts will be the primary
reviewers for this code.

The major changes are;
 * The walkers now have a single callback function.
 * The random file generators are now io.Reader objects.
 * Detangled the idea for walking from processing the files. This cleans up the
"Finished" problem.
 * Added callback for when all the contents of a directory are processed.
 * Added a verifying file processor which checks walkers are doing things
correctly.
 * Improved GoLint compliance.

There are a bunch of things which still need tweaking, but wanted to get this in
front of Dave and Michael to start looking at / thinking about.

Doing this refactor seems to have reduced the performance of the WalkNoStat, so
I still need to figure out what is going on there and get it back to the
original performance. 

The performance of the random generators has also decreased, but as that tool is
not performance critical I'm not going to bother with it.

As well, it seems WalkBasic has gotten faster in Go 1.7 compared to Go 1.5 / Go
1.6

Thanks!

Tim 'mithro' Ansell

[mcgreevy_g, 2017-06-27 03:29:16]

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

[mcgreevy_g, 2017-06-27 03:29:17]

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/tests/t...
File common/dirwalk/tests/tools/gendir/content.go (right):

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/tests/t...
common/dirwalk/tests/tools/gendir/content.go:43: reader :=
textRandomGenerator{r: r}
return &textRandomGenerator{r: r}

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/tests/t...
File common/dirwalk/tests/tools/gendir/generate.go (right):

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/tests/t...
common/dirwalk/tests/tools/gendir/generate.go:30: for written < fileSize {
I'm pretty sure that you can achieve this by just wrapping fileContent in an
io.LimitedReader, and then using io.Copy to f from the wrapped reader.

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/tests/t...
common/dirwalk/tests/tools/gendir/generate.go:83: for true {
This can be expressed as:

for {

Note: this pattern also occurs elsewhere in this file.

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/walk_no...
File common/dirwalk/walk_nostat.go (right):

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/walk_no...
common/dirwalk/walk_nostat.go:27: if err != io.EOF && err != nil {
Note: it's valid for a Reader to return a nil error and then return io.EOF with
0 bytes read on the next call to Read.

https://codereview.chromium.org/2054763004/diff/260001/common/dirwalk/walk_no...
common/dirwalk/walk_nostat.go:41: // This file was bigger than the block size
Or, perhaps it was equal to the block size and io.EOF will be returned on the
next call to Read.