Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

status: make collectConstraints() concurrent #1413

Merged
merged 3 commits into from
Dec 18, 2017

Conversation

darkowlzz
Copy link
Collaborator

What does this do / why do we need it?

It makes collectConstraints() concurrent. We need this to reduce the running time of status, which increased recently to ~30s due to introduction of collectConstraints(). This change brings the time down to ~5s.

Verbose output shows constraints collection progress:

$ dep status -v
Collecting project constraints:
(1/15) github.com/Masterminds/semver
(2/15) github.com/Masterminds/vcs
(3/15) github.com/armon/go-radix
(4/15) github.com/boltdb/bolt
....
(12/15) github.com/sdboyer/constext
(13/15) golang.org/x/net
(14/15) golang.org/x/sync
(15/15) golang.org/x/sys
Checking upstream projects:
(1/15) github.com/Masterminds/semver
(2/15) github.com/Masterminds/vcs
(3/15) github.com/armon/go-radix
(4/15) github.com/boltdb/bolt
....

What should your reviewer look out for in this PR?

Concurrency implementation.

Do you need help or clarification on anything?

Should we return errors in collectConstraints()? I didn't change the return signature initially while implementing but now I think maybe we should. With this change, any error in collectConstraints() won't affect status but would print the error only when in verbose mode.

@darkowlzz darkowlzz added this to the v0.4.0 milestone Nov 29, 2017
@darkowlzz
Copy link
Collaborator Author

darkowlzz commented Nov 29, 2017

I think there was no returned error in the initial signature of collectConstraints() because it wasn't implemented (empty function). I'll create another PR addressing error. I don't want this performance issue to be affected by that. It gets hard to work when status takes half a minute to show results 😄

@sdboyer sdboyer removed this from the v0.4.0 milestone Nov 29, 2017
@darkowlzz darkowlzz force-pushed the concurrent-collectconstraints branch 5 times, most recently from 29be995 to 88ac250 Compare December 6, 2017 12:09
@darkowlzz
Copy link
Collaborator Author

So the reason for the test failures is getDirectDependencies(). It always returned error on Windows, "analysis of current project's packages failed: The filename, directory name, or volume label syntax is incorrect.", which is coming from failure in ListPackages(). But since the error wasn't being returned, it didn't affect the tests.
Now, since the error is being handled, it's resulting in test failure.

I think the error is related to the test setup. Something to do with the created temporary directory structure that's causing ListPackages() to fail.

@darkowlzz darkowlzz force-pushed the concurrent-collectconstraints branch 4 times, most recently from 17797a9 to 8a978e5 Compare December 6, 2017 13:35
@darkowlzz
Copy link
Collaborator Author

Alright, got it! I wasn't setting root of the project.

p.SetRoot(filepath.Join(pwd, "src"))

fixed it. No errors.

So, collectConstraints() returns constraintsCollection (never nil, empty if there's nothing) and a slice of errors. These errors can then be used by the caller. In case of runStatusAll(), we don't want to fail the whole command on any error while collecting constraints, so, all the errors are collected and counted. This count is added to the errCount of runStatusAll(), which tells the user if anything fails, without failing completing. When status -v is run, those errors are printed on the screen.

Also, since even partial results are not bad in this case, I think we need not use golang.org/x/sync/errgroup, but sync.WaitGroup is good for this purpose.

Sorting of projectConstraints is required to have a consistent returned
value from collectConstraints().
Errors from collectConstraints() should not result in failure of status.
These errors are logged to stderr and the error count is added to
`errCount` of `runStatusAll()`. This results in letting the user know
about incomplete status result, keeping the status output clean. Running
status in verbose mode would show all those errors.
@sdboyer sdboyer merged commit 67c45f4 into golang:master Dec 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants