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

Rewrite delta to use only a single goroutine #373

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

julianbrost
Copy link
Contributor

@julianbrost julianbrost commented Sep 23, 2021

The current delta implementation uses two goroutines that read from their own channels but update shared maps all the time. Rewriting it to just use a single goroutine make almost no difference in wall clock time (seems to even be a little faster) but removes quite a lot of lock contention so that the total CPU time needed decreases quite a bit.

For the following test, I increased the limit in the benchmark (didn't want to commit this as that test needs about 12GB RAM).

Before

$ go test -o test -c github.com/icinga/icingadb/pkg/icingadb
$ ./test -test.bench=.
goos: linux
goarch: amd64
pkg: github.com/icinga/icingadb/pkg/icingadb
cpu: Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
BenchmarkDelta/1024-8         	    1310	    793142 ns/op
BenchmarkDelta/2048-8         	     800	   1518578 ns/op
BenchmarkDelta/4096-8         	     375	   3093924 ns/op
BenchmarkDelta/8192-8         	     177	   6498656 ns/op
BenchmarkDelta/16384-8        	      81	  13897188 ns/op
BenchmarkDelta/32768-8        	      38	  29977836 ns/op
BenchmarkDelta/65536-8        	      18	  65276838 ns/op
BenchmarkDelta/131072-8       	       8	 143929741 ns/op
BenchmarkDelta/262144-8       	       4	 311318287 ns/op
BenchmarkDelta/524288-8       	       2	 677328462 ns/op
BenchmarkDelta/1048576-8      	       1	1581513429 ns/op
BenchmarkDelta/2097152-8      	       1	3370276475 ns/op
BenchmarkDelta/4194304-8      	       1	7291033169 ns/op
BenchmarkDelta/8388608-8      	       1	15628877888 ns/op
BenchmarkDelta/16777216-8     	       1	33995732865 ns/op
PASS
$ \time -v ./test -test.bench=BenchmarkDelta/16777216
goos: linux
goarch: amd64
pkg: github.com/icinga/icingadb/pkg/icingadb
cpu: Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
BenchmarkDelta/16777216-8         	       1	32999606020 ns/op
PASS
	Command being timed: "./test -test.bench=BenchmarkDelta/16777216"
	User time (seconds): 81.54
	System time (seconds): 5.71
	Percent of CPU this job got: 222%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:39.26
[...]

After

$ go test -o test -c github.com/icinga/icingadb/pkg/icingadb
$ ./test -test.bench=.
goos: linux
goarch: amd64
pkg: github.com/icinga/icingadb/pkg/icingadb
cpu: Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
BenchmarkDelta/1024-8         	    1412	    746115 ns/op
BenchmarkDelta/2048-8         	     753	   1449967 ns/op
BenchmarkDelta/4096-8         	     379	   2929251 ns/op
BenchmarkDelta/8192-8         	     178	   6508426 ns/op
BenchmarkDelta/16384-8        	      92	  14127214 ns/op
BenchmarkDelta/32768-8        	      37	  30665427 ns/op
BenchmarkDelta/65536-8        	      18	  64991758 ns/op
BenchmarkDelta/131072-8       	       8	 137032624 ns/op
BenchmarkDelta/262144-8       	       4	 291233974 ns/op
BenchmarkDelta/524288-8       	       2	 642120690 ns/op
BenchmarkDelta/1048576-8      	       1	1545381880 ns/op
BenchmarkDelta/2097152-8      	       1	3273877521 ns/op
BenchmarkDelta/4194304-8      	       1	6717818583 ns/op
BenchmarkDelta/8388608-8      	       1	14430346934 ns/op
BenchmarkDelta/16777216-8     	       1	31069273615 ns/op
PASS
$ \time -v ./test -test.bench=BenchmarkDelta/16777216
goos: linux
goarch: amd64
pkg: github.com/icinga/icingadb/pkg/icingadb
cpu: Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
BenchmarkDelta/16777216-8         	       1	30448680513 ns/op
PASS
	Command being timed: "./test -test.bench=BenchmarkDelta/16777216"
	User time (seconds): 65.70
	System time (seconds): 4.16
	Percent of CPU this job got: 193%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:36.20
[...]

Comparison

Test Before After Change
n = 1024 793 us 746 us -5.9 %
n = 2048 1518 us 1449 us -4.5 %
n = 4096 3093 us 2929 us -5.3 %
n = 8192 6498 us 6508 us +0.1 %
n = 16384 13897 us 14127 us +1.6 %
n = 32768 29977 us 30665 us +2.2 %
n = 65536 65276 us 64991 us -0.4 %
n = 131072 143 ms 137 ms -4.7 %
n = 262144 311 ms 291 ms -6.4 %
n = 524288 677 ms 642 ms -5.1 %
n = 1048576 1581 ms 1545 ms -2.2 %
n = 2097152 3370 ms 3273 ms -2.8 %
n = 4194304 7291 ms 6717 ms -7.8 %
n = 8388608 15628 ms 14430 ms -7.6 %
n = 16777216 33995 ms 31069 ms -8.6 %

(Don't give too much on the exact numbers, that was just a single run but it matches my general impression from multiple runs: there's no huge change but it tends to even be slightly faster.)

Also the CPU time (user+sys) went from 87.25s to 69.86s (-20 %), that's the main benefit of this PR.

refs #361 (Making delta single-threaded was originally part of that PR but now got moved here. There's a bit of history/context over there.)

@cla-bot cla-bot bot added the cla/signed label Sep 23, 2021
@julianbrost julianbrost added the enhancement New feature or request label Sep 23, 2021
@julianbrost julianbrost force-pushed the feature/single-threaded-delta branch from 71fca86 to 6556197 Compare September 23, 2021 13:28
@@ -40,101 +38,83 @@ func (delta *Delta) Wait() error {
return <-delta.done
}

func checksumsMatch(a, b contracts.Entity) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I would move this function after Delta.run() so that private functions come after public ones and Delta methods stay together.

@julianbrost julianbrost force-pushed the feature/single-threaded-delta branch from 6556197 to 2198cdb Compare September 24, 2021 09:35
@julianbrost julianbrost force-pushed the feature/single-threaded-delta branch from 2198cdb to 66d9b0e Compare September 24, 2021 09:52
@julianbrost
Copy link
Contributor Author

Rebased due to merge conflict in go.mod.

@julianbrost julianbrost merged commit 6e3df7d into master Sep 24, 2021
@julianbrost julianbrost deleted the feature/single-threaded-delta branch September 24, 2021 14:53
@lippserd lippserd added this to the v1.0.0-rc2 milestone Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants