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(rollups): rollup a batch if more than 2 seconds elapsed since last batch #6118

Merged
merged 5 commits into from
Aug 4, 2020

Conversation

parasssh
Copy link
Contributor

@parasssh parasssh commented Jul 31, 2020

fix(rollup): rollup a batch every 2 seconds or 64 keys


This change is Reviewable

…and atomics. Benchmark shows no degradation.
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain and @martinmr)

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

What's the benchmark result? Can you paste the results here?

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain)

@parasssh
Copy link
Contributor Author

parasssh commented Jul 31, 2020

What's the benchmark result? Can you paste the results here?

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain)

  1. Benchmark with using atomic operations on the lastSentTs
Running tool: /usr/local/go/bin/go test -benchmem -run=^$ github.com/dgraph-io/dgraph/posting -bench ^(BenchmarkIncrRollupAtomic)$

[Decoder]: Using assembly version of decoder
badger 2020/07/30 19:27:26 INFO: All 0 tables opened in 0s
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/dgraph/posting
BenchmarkIncrRollupAtomic-8   	18172532	        65.5 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/dgraph-io/dgraph/posting	1.277s
  1. Benchmark without using atomic on lastSentTs.
Running tool: /usr/local/go/bin/go test -benchmem -run=^$ github.com/dgraph-io/dgraph/posting -bench ^(BenchmarkIncrRollupAtomic)$

[Decoder]: Using assembly version of decoder
badger 2020/07/30 19:31:23 INFO: All 0 tables opened in 0s
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/dgraph/posting
BenchmarkIncrRollupAtomic-8   	17659142	        65.2 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/dgraph-io/dgraph/posting	1.241s

So, (1) and (2) are comparable at ~65 ns/op

However, I have not done the benchmark directly on master as it is today. Let me do that next.

@parasssh
Copy link
Contributor Author

parasssh commented Jul 31, 2020

  1. Benchmark on master without any changes.
Running tool: /usr/local/go/bin/go test -benchmem -run=^$ github.com/dgraph-io/dgraph/posting -bench ^(BenchmarkIncrRollupAtomic)$

[Decoder]: Using assembly version of decoder
badger 2020/07/30 19:35:36 INFO: All 0 tables opened in 0s
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/dgraph/posting
BenchmarkIncrRollupAtomic-8   	60290652	        18.8 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/dgraph-io/dgraph/posting	1.177s

So, on master, it is ~19 ns/op but it suffers from the issue that rollups dont happen as often.
With my branch it is ~65 ns/op as seen in the previous comment but it fixes above issue by forcing rollup if >2 sec has elapsed since the last time.

FWIW, I live-loaded 21 million data set with master and my branch. It took 13m30sec and 13m49sec respectively. So, not much change there.

cc @manishrjain

@parasssh
Copy link
Contributor Author

parasssh commented Jul 31, 2020

The time.Now().Unix() is the culprit. It is expensive in itself. It adds ~40ns/op.

@parasssh
Copy link
Contributor Author

Based on the above comments, I think we can still use this fix.
The Benchmark is a tight loop that does millions of operations in 1-2 sec. This may not be the real world scenario.

parasssh added 2 commits August 4, 2020 09:40
…vel var and atomics. Benchmark shows no degradation."

This reverts commit 905f703.
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @parasssh)


posting/mvcc.go, line 120 at r2 (raw file):

dorollup

nit: doRollup

@parasssh parasssh merged commit aca0921 into master Aug 4, 2020
@parasssh parasssh deleted the paras/incr_rollup_atomic branch August 4, 2020 22:28
parasssh pushed a commit that referenced this pull request Aug 4, 2020
parasssh pushed a commit that referenced this pull request Aug 4, 2020
parasssh pushed a commit that referenced this pull request Aug 4, 2020
parasssh pushed a commit that referenced this pull request Aug 4, 2020
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