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

Parallelize submission #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JensRantil
Copy link
Contributor

Make the flush operation execute in parallel.

These optimizations have been mentioned in #23.

Additional comments:

  • I'm not sure they are worth incorporating as they might do more harm than good. That said, maybe they can foster a good discussion about possible speed optimizations. I don't know.
  • I know good Go practices are to avoid the sync package and only use channels. In this case, I thought sync.WaitGroup felt like they were making the code more understandable. I might be wrong.

If you have multiple different timers then this will speed up the
processing time.
@JensRantil
Copy link
Contributor Author

Fixed merge conflicts. Feel free to revisit.

@jehiah
Copy link
Member

jehiah commented May 27, 2014

Thanks @JensRantil for this contributions. Can you share some benchmark deltas for this changeset?

@JensRantil
Copy link
Contributor Author

Can you share some benchmark deltas for this changeset?

Feels like I made this pull request an eternity ago. Anyway, here are some benchmarks:


➜  statsdaemon git:(master) go test -bench=.
PASS
BenchmarkManyDifferentSensors          1    6731116058 ns/op
BenchmarkOneBigTimer           1    6180231467 ns/op
BenchmarkLotsOfTimers          1    6790958617 ns/op
ok      github.com/bitly/statsdaemon    35.471s
➜  statsdaemon git:(master) git checkout parallelize-submission
➜  statsdaemon git:(parallelize-submission) go test -bench=.
PASS
BenchmarkManyDifferentSensors          1    3241844699 ns/op
BenchmarkOneBigTimer           1    3378769659 ns/op
BenchmarkLotsOfTimers          1    6503543688 ns/op
ok      github.com/bitly/statsdaemon    30.082s

So, master is faster or equal in speed.

Oh, also, the tests seem to pass despite Travis failing this:

➜  statsdaemon git:(parallelize-submission) go test
PASS
ok      github.com/bitly/statsdaemon    0.018s

@ploxiln
Copy link
Contributor

ploxiln commented Jun 6, 2014

travis test failure was "index out of range" when indexing into position 0 of the slice t:

func processTimer(buffer *bytes.Buffer, now int64, pctls Percentiles, u string, t Uint64Slice) {
    sort.Sort(t)
    min := t[0]

Also, it failed while testing with go1.0.3 - is it possible that some difference between the version of go you use and go1.0.3 means t could be an empty slice here? Or maybe a race condition when you enable multiple GOMAXPROCS?

@JensRantil
Copy link
Contributor Author

I tried to recreate this. So far, nothing:

➜  statsdaemon git:(parallelize-submission) ✗ go version
go version go1.2.2 darwin/amd64
➜  statsdaemon git:(parallelize-submission) ✗ go test
PASS
ok      github.com/bitly/statsdaemon    0.009s
➜  statsdaemon git:(parallelize-submission) ✗ GOMAXPROCS=8 go test
PASS
ok      github.com/bitly/statsdaemon    0.010s

I am running Go 1.1 branch. Do you have a Go 1.0 installation at hand perhaps? Can you recreate this?

defer wg.Done()
numchan <- processTimers(&buffer, now, percentThreshold)
}()
wg.Done()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wg.Done()
wg.Wait()

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

Successfully merging this pull request may close these issues.

3 participants