[WIP] VerifyDepTree and DigestFromDirectory use godirwalk #1084

karrick · 2017-08-29T14:57:36Z

What does this do / why do we need it?

Alters digest code to use godirwalk project. godirwalk handles symlinked directories on Windows correctly, and is faster than filepath.Walk on both unix and Windows.

What should your reviewer look out for in this PR?

This adds godirwalk as a project dependency for dep.
godirwalk has a very similar, but not identical, API to filepath.Walk.

Do you need help or clarification on anything?

I recall @sdboyer mentioning that there were some tests in the other portions of dep that were not working on Windows. I wonder whether those are related to the errant handling of symlinks to directories that happens with filepath on Windows, and whether godirwalk could address that.

Which issue(s) does this PR fix?

Related to #121.

jmank88 · 2017-08-29T18:06:14Z

internal/gps/pkgtree/digest.go

+			case modeType&os.ModeNamedPipe != 0:
+				return nil // nothing more to do for this type
+			case modeType&os.ModeSocket != 0:
+				return nil // nothing more to do for this type


Could this block of cases be simplified a bit to something like this:

case modeType&(os.ModeDir|os.ModeDevice|os.ModeCharDevice|os.ModeNamedPipe|os.ModeSocket) != 0: return nil // nothing more to do for these types

Close:

case modeType&(os.ModeDir|os.ModeDevice|os.ModeCharDevice|os.ModeNamedPipe|os.ModeSocket) != 0:

Pushing update, and thanks!

karrick · 2017-08-30T19:33:00Z

I'd like to hold off on merging this just yet.

sdboyer · 2017-08-30T22:25:54Z

ready for review now, though?

…

On 08/30 19:33, Karrick McDermott wrote: I'd like to hold off on merging this just yet. -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #1084 (comment)

karrick · 2017-08-30T22:42:28Z

It looks like the speedup on Mac is significantly more than on linux. Here, walking a large directory tree is as fast as doing so with find utility:

hostname:walk-fast karrick$ du -sh ~/src
 26G    /Users/karrick/src
hostname:walk-fast karrick$ for i in 1 2 3 ; do time walk-fast ~/src | wc -l ; done                                                                                             
  542946                                     

real    0m6.048s                             
user    0m1.279s                             
sys     0m2.667s                             
  542946                                     

real    0m6.022s                             
user    0m1.267s                             
sys     0m2.633s                             
  542946                                     

real    0m6.079s                             
user    0m1.278s                             
sys     0m2.682s                             
hostname:walk-fast karrick$ for i in 1 2 3 ; do time walk-stdlib ~/src | wc -l ; done                                                                                           
  542946                                     

real    0m29.369s                            
user    0m1.994s                             
sys     0m24.758s                            
  542946                                     

real    0m29.139s                            
user    0m1.987s                             
sys     0m25.127s                            
  542946                                     

real    0m29.092s                            
user    0m2.010s                             
sys     0m24.917s                            
hostname:walk-fast karrick$ for i in 1 2 3 ; do time find ~/src | wc -l ; done
  542946

real    0m5.979s
user    0m0.310s
sys     0m2.724s
  542946

real    0m5.823s
user    0m0.313s
sys     0m2.578s
  542946

real    0m5.723s
user    0m0.307s
sys     0m2.564s

karrick · 2017-08-30T22:43:26Z

When compared on Linux:

[karrick@linuxBox walk-fast]$ du -sh ~/src
36G     /home/karrick/src
[karrick@linuxBox walk-fast]$ for i in 1 2 3 ; do time walk-fast ~/src | wc -l ; done
829481

real    0m5.443s
user    0m4.246s
sys     0m3.698s
829481

real    0m5.415s
user    0m4.006s
sys     0m3.932s
829481

real    0m5.433s
user    0m4.418s
sys     0m3.529s
[karrick@linuxBox walk-fast]$ for i in 1 2 3 ; do time walk-stdlib ~/src | wc -l ; done
829481

real    0m8.298s
user    0m4.789s
sys     0m6.034s
829481

real    0m8.442s
user    0m4.835s
sys     0m6.144s
829481

real    0m8.352s
user    0m4.759s
sys     0m6.084s
[karrick@linuxBox walk-fast]$ for i in 1 2 3 ; do time find ~/src | wc -l ; done
829481

real    0m1.492s
user    0m0.669s
sys     0m0.921s
829481

real    0m1.474s
user    0m0.698s
sys     0m0.873s
829481

real    0m1.495s
user    0m0.636s
sys     0m0.949s

karrick · 2017-08-30T22:56:17Z

The changes to dep to use godirwalk are quite trivial.

Earlier today I was running some benchmarks, and I had found a bug in godirwalk code.

I would appreciate any thoughts on godirwalk code, however.

karrick · 2017-08-30T22:57:40Z

On Windows, walk-fast runs in half the time that walk-stdlib runs, by the way.

karrick · 2017-08-30T23:05:37Z

I have fixed the bug, but would appreciate any extra eyeballs on the code, and thoughts folks might have.

karrick · 2017-08-31T15:58:22Z

It's interesting to note that while godirwalk is definitely faster that filepath.Walk, in situations where the main bottleneck is doing some I/O on the discovered nodes, the performance gain is often marginalized.

However, using godirwalk in DigestFromDirectory and VerifyDepTree I was still able to get a performance improvement, although because file I/O now dominates the algorithm's time, the performance improvement is maybe 5--10% on darwin, and nearly 30% on my linux box.

[karrick@macHost]$ go test -bench Flame
goos: darwin
goarch: amd64
pkg: github.com/golang/dep/internal/gps/pkgtree
BenchmarkFlameDigestFromDirectoryFilepath-8            1	26226528585 ns/op
BenchmarkFlameDigestFromDirectory-8                    1	24313511131 ns/op
PASS
ok      github.com/golang/dep/internal/gps/pkgtree	50.561s

[karrick@linuxHost pkgtree]$ go test -bench Flame
goos: linux
goarch: amd64
pkg: github.com/golang/dep/internal/gps/pkgtree
BenchmarkFlameDigestFromDirectoryFilepath-12                   1        31579248880 ns/op
BenchmarkFlameDigestFromDirectory-12                           1        20610669195 ns/op
PASS
ok      github.com/golang/dep/internal/gps/pkgtree      52.233s

karrick · 2017-08-31T16:27:23Z

Okay, the previous benchmark was created by traversing the testdata_digest directory.

As expected, when I benchmark using $GOPATH/src, the process is much more I/O focussed, and the performance gains from traversing the tree are greatly mitigated by the additional I/O load of reading the files.

On RHEL 6.x

[karrick@linuxHost pkgtree]$ go test -bench FlameDigest
goos: linux
goarch: amd64
pkg: github.com/golang/dep/internal/gps/pkgtree
BenchmarkFlameDigestFromDirectoryFilepath-12                   1        5931829679 ns/op
BenchmarkFlameDigestFromDirectory-12                           1        5864872923 ns/op
PASS
ok      github.com/golang/dep/internal/gps/pkgtree      11.837s
[karrick@linuxHost pkgtree]$ du -sh ~/go/src
1.4G    /home/karrick/go/src

On macOS Sierra

[karrick@darwinHost pkgtree]$ go test -bench FlameDigest
goos: darwin
goarch: amd64
pkg: github.com/golang/dep/internal/gps/pkgtree
BenchmarkFlameDigestFromDirectoryFilepath-8   	       1	3563277755 ns/op
BenchmarkFlameDigestFromDirectory-8           	       1	3365962160 ns/op
PASS
ok  	github.com/golang/dep/internal/gps/pkgtree	6.949s
[karrick@darwinHost pkgtree]$ du -sh ~/go/src
1.6G	/Users/karrick/go/src

* found by megacheck :)

karrick · 2017-09-05T16:24:28Z

Rebased my fork due to upstream changes...

sdboyer · 2017-09-05T17:38:07Z

gonna try to get on reviewing this a bit later this week - sorry, taken up with case sensitivity stuff rn.

karrick · 2017-09-05T17:47:48Z

Case insensitivity... Not fun. I'm happy to help out on this project still. And looking forward to getting the digest sum added to the Gopkg.lock file.

karrick · 2017-09-06T14:41:07Z

It looks like the BoltDB tests are failing, causing the builds to not pass.

https://travis-ci.org/golang/dep/jobs/272128978

karrick · 2017-09-06T14:46:19Z

The tests that use the bolt db cache are passing on localhost, so it might be a race condition in source_cache_bolt_test.go...

karrick · 2017-09-06T17:55:23Z

@jmank88,

I have performed a fresh checkout on upstream on my Windows box, and running the bolt db tests, and sometimes they pass, and sometimes they fail.

jmank88 · 2017-09-06T19:06:00Z

Restarted jobs

* The setup and validation for all the importer testcases are the same, they just need to handle calling the importer properly. * Flag importer tests to run in parallel now that they've been cleaned up.

The closure created by t.Run wasn't copying the testcase value, so the test was being run on the wrong values.

When a lock hint is empty, nothing should be added to the lock

Fixes an error when the git user is not set.

Use the test helper so that it can check for any problems up-front.

Disable until we can figure out why this is causing the following error on the AppVeyor(Windows) builds: "remote repository at https://github.com/carolynvs/deptest-importers does not exist, or is inaccessible"

googlebot · 2017-09-10T14:58:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

sdboyer

just some small things.

are there any notable differences between the previous internal implementation and what's in godirwalk? i don't think there are, and i'd assume you'd have mentioned some, but making sure 😄

sdboyer · 2017-09-10T14:50:38Z

internal/gps/pkgtree/digest_test.go

+	}
+}
+
+const flameIterations = 100000


could you explain the benefit of having a fixed number of iterations, rather than relying (as BenchmarkDigestFromDirectory() does) on the testing system to either a) empirically determine an appropriate threshold and/or b) allow the user to control it via CLI args?

When comparing flame graphs, I want to compare how many cycles were spent in the corresponding functions for the same input. Those numbers are skewed if we run version A 500 times, and version B only 100 times. Therefore, forcing the iteration count to a particular number ensures an apples to apples comparison.

gotcha, wfm 👍

sdboyer · 2017-09-10T14:51:47Z

internal/gps/pkgtree/newLine.go

@@ -0,0 +1,103 @@
+package pkgtree


need a copyright header (ugh, this should've caused our travis tests to fail...).

also, for consistency, filename should be snaky, not camel.

I actually prefer snaky filenames, or even better with hyphens, but most other Go projects use camel case for file names. Is there something I don't know about, or just a convention localized to dep?

it's the uniform approach in both stdlib and the toolchain, afaik. that's the convention we're aiming to follow.

karrick · 2017-09-10T14:59:52Z

Somehow rebasing from upstream/master and pushing to my fork causes some problems. I will take a look at this later.

sdboyer · 2017-09-15T23:51:49Z

do they really? huh. that's not been my subjective experience. only standard I'm really following here is stdlib/the toolchain.

…

On September 15, 2017 6:53:34 PM EDT, Karrick McDermott ***@***.***> wrote: karrick commented on this pull request. > @@ -0,0 +1,103 @@ +package pkgtree I actually prefer snaky filenames, or even better with hyphens, but most other Go projects use camel case for file names. Is there something I don't know about, or just a convention localized to `dep`? -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #1084 (comment)

karrick · 2017-09-16T07:10:29Z

I don't mind renaming the files, however I've been on business trip this past week and procrastinating the experience of understanding what I did wrong when I attempted my last git rebase. Thanks for your patience.

sdboyer · 2017-09-20T01:19:43Z

yeah, seems like we've got some rebase weirdness. i'm happy to help you sort out a workable process - here, or pinging me on slack works, as well 😄

sdboyer · 2017-10-02T00:25:02Z

would it be easier to close this out and open a fresh PR?

sdboyer · 2017-11-15T06:21:25Z

i'm going to close this out and open a fresh PR once i have a chance to get back to this.

karrick requested a review from sdboyer as a code owner August 29, 2017 14:57

googlebot added the cla: yes label Aug 29, 2017

karrick mentioned this pull request Aug 29, 2017

Adapt goimports' fast filepath.Walk impl for ListPackages #419

Closed

jmank88 reviewed Aug 29, 2017

View reviewed changes

karrick added 8 commits September 5, 2017 12:09

digest uses godirwalk

502d835

more simplified node type detection

f149f35

digestNoAction constant name describes purpose of values

6dd14f9

updated godirwalk

157c30f

updated godirwalk

ab1cacf

update godirwalk to cleaner version v1.2.4

4109fb9

eliminated unused variables

be2c05c

* found by megacheck :)

vendor'd package in accordance with guidance from travis ci

ac6f562

karrick force-pushed the master branch from cc9e04a to ac6f562 Compare September 5, 2017 16:23

ibrasho added the area: gps label Sep 9, 2017

carolynvs and others added 17 commits September 10, 2017 10:57

cmd/dep: Move import test execution into testcase

fa8e165

* The setup and validation for all the importer testcases are the same, they just need to handle calling the importer properly. * Flag importer tests to run in parallel now that they've been cleaned up.

cmd/dep: Validate warnings in import testcases

acd473f

cmd/dep: Document every baseImporter function

f0e0e3a

cmd/dep: Simplify pinned constraint check

7d5628c

cmd/dep: fix parallel tests

7e85ef6

The closure created by t.Run wasn't copying the testcase value, so the test was being run on the wrong values.

cmd/dep: verify empty locks are skipped

49a2eb4

When a lock hint is empty, nothing should be added to the lock

gps: source cache: fix flaky bolt test clock by forcing future timestamp

0321346

Fix naming convention for test

94dec0f

Nicer error reporting in test

59d1d75

Don't rely on local git config in test

5332f3a

Fixes an error when the git user is not set.

Fix test when git is not set up

f380ff7

Fix file:// URI on Windows

5c9e128

gps: fix unwrapVcsErr to handle nil causes

3d84357

gps: add unwrapVcsErr nil cause test

4626497

cmd/dep: fix setting importer tests as parallel

5e17c34

Use the test helper so that it can check for any problems up-front.

cmd/dep: disable running importer tests in parallel

a1f23c2

Disable until we can figure out why this is causing the following error on the AppVeyor(Windows) builds: "remote repository at https://github.com/carolynvs/deptest-importers does not exist, or is inaccessible"

Clarify when to run ensure if vendor is not committed

b40a93c

karrick requested a review from carolynvs as a code owner September 10, 2017 14:58

googlebot added cla: no and removed cla: yes labels Sep 10, 2017

sdboyer suggested changes Sep 10, 2017

View reviewed changes

carolynvs removed their request for review September 19, 2017 01:28

sdboyer closed this Nov 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] VerifyDepTree and DigestFromDirectory use godirwalk #1084

[WIP] VerifyDepTree and DigestFromDirectory use godirwalk #1084

karrick commented Aug 29, 2017

jmank88 Aug 29, 2017 •

edited

Loading

karrick Aug 29, 2017

karrick commented Aug 30, 2017

sdboyer commented Aug 30, 2017 via email

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 31, 2017 •

edited

Loading

karrick commented Aug 31, 2017

karrick commented Sep 5, 2017

sdboyer commented Sep 5, 2017

karrick commented Sep 5, 2017

karrick commented Sep 6, 2017

karrick commented Sep 6, 2017

karrick commented Sep 6, 2017

jmank88 commented Sep 6, 2017

googlebot commented Sep 10, 2017

sdboyer left a comment

sdboyer Sep 10, 2017

karrick Sep 15, 2017

sdboyer Oct 2, 2017

sdboyer Sep 10, 2017

karrick Sep 15, 2017

sdboyer Oct 2, 2017

karrick commented Sep 10, 2017

sdboyer commented Sep 15, 2017 via email

karrick commented Sep 16, 2017

sdboyer commented Sep 20, 2017

sdboyer commented Oct 2, 2017

sdboyer commented Nov 15, 2017

[WIP] VerifyDepTree and DigestFromDirectory use godirwalk #1084

[WIP] VerifyDepTree and DigestFromDirectory use godirwalk #1084

Conversation

karrick commented Aug 29, 2017

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

jmank88 Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

karrick Aug 29, 2017

Choose a reason for hiding this comment

karrick commented Aug 30, 2017

sdboyer commented Aug 30, 2017 via email

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 30, 2017

karrick commented Aug 31, 2017 • edited Loading

karrick commented Aug 31, 2017

On RHEL 6.x

On macOS Sierra

karrick commented Sep 5, 2017

sdboyer commented Sep 5, 2017

karrick commented Sep 5, 2017

karrick commented Sep 6, 2017

karrick commented Sep 6, 2017

karrick commented Sep 6, 2017

jmank88 commented Sep 6, 2017

googlebot commented Sep 10, 2017

sdboyer left a comment

Choose a reason for hiding this comment

sdboyer Sep 10, 2017

Choose a reason for hiding this comment

karrick Sep 15, 2017

Choose a reason for hiding this comment

sdboyer Oct 2, 2017

Choose a reason for hiding this comment

sdboyer Sep 10, 2017

Choose a reason for hiding this comment

karrick Sep 15, 2017

Choose a reason for hiding this comment

sdboyer Oct 2, 2017

Choose a reason for hiding this comment

karrick commented Sep 10, 2017

sdboyer commented Sep 15, 2017 via email

karrick commented Sep 16, 2017

sdboyer commented Sep 20, 2017

sdboyer commented Oct 2, 2017

sdboyer commented Nov 15, 2017

jmank88 Aug 29, 2017 •

edited

Loading

karrick commented Aug 31, 2017 •

edited

Loading