Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix data race #63

Merged
merged 1 commit into from
Mar 25, 2019
Merged

fix data race #63

merged 1 commit into from
Mar 25, 2019

Conversation

apocelipes
Copy link
Contributor

No description provided.

@boyter
Copy link
Owner

boyter commented Mar 24, 2019

I’m currently away so I cannot review this too well. Looks like a good thing to merge though. Thanks for the PR. I will either merge it in sometime next week or perhaps @dbaggerman will have a look for you.

@dbaggerman
Copy link
Collaborator

I'm not entirely clear on what's being fixed, as most of these are already protected by a mutex. Setting startTime in the workers might be racy though (I'm guessing that's set in the inner loop so it's timing from the first item received?).
Overall, atomic vs sync.Mutex is a wash performance wise:

% hyperfine -w 2 'scc' 'pr-scc'
Benchmark #1: scc
  Time (mean ± σ):      14.2 ms ±   1.7 ms    [User: 18.8 ms, System: 14.1 ms]
  Range (min … max):    11.6 ms …  22.8 ms    191 runs
 
Benchmark #2: pr-scc
  Time (mean ± σ):      14.3 ms ±   1.9 ms    [User: 18.7 ms, System: 14.9 ms]
  Range (min … max):    12.1 ms …  22.5 ms    186 runs
 
Summary
  'scc' ran
    1.01 ± 0.18 times faster than 'pr-scc'

So there's no particular reason to switch, but I'm not really attached to using mutexes.

Overall, looks like it contains some changes that aren't strictly necessary but otherwise does clean up a couple of possibly racy bits.

I see no harm in merging it.

@dbaggerman dbaggerman merged commit b92cfa5 into boyter:master Mar 25, 2019
@apocelipes
Copy link
Contributor Author

@dbaggerman
Thank you for your code review.
If you compile and run scc with go build -race, you will see several warnings about "data race", some of which are in the code set by the works setup startTime. The runtime of multiple goroutines is undefined, so unprotected reading and setting of the value of startTime is likely to result in data race. And after the comparison, the assignment is non-atomic, so it may be interrupted, and atomic provides the primitive of the relevant function, so I fixed it with atomic.
There is also a data race when reading totalCount, because another goroutine is writing data while reading the value of the variable (the read is protected without a mutex, so data competition will occur). In addition, if the critical section only contains the addition, subtraction and reading of values, the mutex is too cumbersome, so I also chose the atomic operation.
Compared to the original code, the use of atomic operations will definitely lose a bit of performance, but it can make the program more robust, and at present these performance losses are not very serious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants