Concurrent ensure add #1218
Concurrent ensure add #1218
Conversation
This change makes `ensure -add` concurrently add the dependencies and uses golang.org/x/sync/syncmap for a concurrent map to replace addInstructions map. Also, logs a message when the sources are being fetched.
darkowlzz
force-pushed
the
concurrent-ensure-add
branch
from
85cf348 to
9b70fc8
Compare
cmd/dep/ensure.go
Outdated
| return errors.Errorf("can only specify rules once per project being added; rules were given at least twice for %s", pc.Ident.ProjectRoot) | ||
| someConstraint := !gps.IsAny(pc.Constraint) || pc.Ident.Source != "" | ||
|
|
||
| instrInterface, has := addInstructions.LoadOrStore(pc.Ident.ProjectRoot, addInstruction{}) |
There was a problem hiding this comment.
I think this is racy. In between storing the zero value and executing the else block to set the fields, another goroutine could load and read the (zero) fields in the has block.
I also wonder the same thing about two goroutines loading, and seeing no constraint set (so no error detected), then racing to set the constraint field.
There was a problem hiding this comment.
ah! right. So creating a new struct with sync.Mutex and maintaining the lock through out this would be better right?
Also, I see the same syncmap is available in sync/map. Same function signatures. Is it like the golang.org packages are eventually moved into the the stdlib?
EDIT: ah ! I see syncmap was a prototype. It's in stdlib officially in go1.9. Will remove it.
Package syncmap provides a concurrent map implementation. It is a prototype for a proposed addition to the sync package in the standard library. (https://golang.org/issue/18177)
There was a problem hiding this comment.
And if we use mutex, there's no need of syncmap.
cmd/dep/ensure.go
Outdated
| if has { | ||
| // Multiple packages from the same project were specified as | ||
| // arguments; make sure they agree on declared constraints. | ||
| // TODO(sdboyer) until we have a general method for checking constraint equality, only allow one to declare |
There was a problem hiding this comment.
We have Constraint.identical now.
There was a problem hiding this comment.
Let's fix this in another PR.
cmd/dep/ensure.go
Outdated
|
|
||
| addInstructions[pc.Ident.ProjectRoot] = instr | ||
| addInstructions.Store(pc.Ident.ProjectRoot, instr) |
There was a problem hiding this comment.
Isn't instr still in the syncmap the whole time?
There was a problem hiding this comment.
If not, then my race comment is probably off too (or half wrong at least).
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| IMPROVEMENTS: | |||
|
|
|||
| * `dep ensure -add` now concurrently fetches the source and adds the projects. | |||
| (#1218) | |||
darkowlzz
force-pushed
the
concurrent-ensure-add
branch
from
9b70fc8 to
f8e46b4
Compare
darkowlzz
force-pushed
the
concurrent-ensure-add
branch
from
f8e46b4 to
e7899a3
Compare
What does this do / why do we need it?
This change makes
ensure -addconcurrently add the dependenciesand.uses golang.org/x/sync/syncmap for a concurrent map to replace
addInstructions map
Also, logs a message when the sources are being fetched.
Sources are enumerated only in verbose mode.
Related to TODOs:
TODO(sdboyer) return all errors, not just the first one we encounter
TODO(sdboyer) do these concurrently?
What should your reviewer look out for in this PR?
Concurrency code
and the usage of syncmap.Do you need help or clarification on anything?
No.
Which issue(s) does this PR fix?
fixes #1202