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

update:use google/btree in the genric way #14515

Merged
merged 2 commits into from
Sep 27, 2022

Conversation

wathenjiang
Copy link
Contributor

@wathenjiang wathenjiang commented Sep 24, 2022

mvcc: use google/btree in the genric way

As etcd bumped go1.19, using generic instead of cast interface{} to some type has better operation efficiency.

Signed-off-by: wathenjiang wathenjiang@tencent.com

Comment on lines 59 to 60
okeyi, _ := ti.tree.Get(keyi)
if okeyi == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
okeyi, _ := ti.tree.Get(keyi)
if okeyi == nil {
okeyi, ok := ti.tree.Get(keyi)
if !ok {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made it by rebase.

@ahrtr
Copy link
Member

ahrtr commented Sep 26, 2022

Please signoff the commit: git rebase HEAD~1 --signoff

Please also compare the performance between main and this PR.

@wathenjiang
Copy link
Contributor Author

Please signoff the commit: git rebase HEAD~1 --signoff

Please also compare the performance between main and this PR.

I would check it.

Signed-off-by: wathenjiang <wathenjiang@tencent.com>
@wathenjiang
Copy link
Contributor Author

(1) Test on put

func BenchmarkIndexPut(b *testing.B) {
	log := zap.NewNop()
	kvindex := newTreeIndex(log)

	bytesN := 64
	keys := createBytesSlice(bytesN, b.N)
	b.ResetTimer()
	for i := 1; i < b.N; i++ {
		kvindex.Put(keys[i], revision{main: int64(i), sub: int64(i)})
	}
}

Test result on the master branch:

$ go test -bench='BenchmarkIndexPut$' -count=5

goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexPut-12             1000000              2591 ns/op
BenchmarkIndexPut-12             1000000              2531 ns/op
BenchmarkIndexPut-12             1000000              2536 ns/op
BenchmarkIndexPut-12             1000000              2546 ns/op
BenchmarkIndexPut-12             1000000              2538 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  167.439s

Test result on this pr:

$ go test -bench='BenchmarkIndexPut$' -count=5

goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexPut-12             1000000              2477 ns/op
BenchmarkIndexPut-12             1000000              2380 ns/op
BenchmarkIndexPut-12             1000000              2360 ns/op
BenchmarkIndexPut-12             1000000              2396 ns/op
BenchmarkIndexPut-12             1000000              2382 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  165.841s

(2) Test on get

func BenchmarkIndexGet(b *testing.B) {
	log := zap.NewNop()
	kvindex := newTreeIndex(log)

	bytesN := 64
	keys := createBytesSlice(bytesN, b.N)
	for i := 1; i < b.N; i++ {
		kvindex.Put(keys[i], revision{main: int64(i), sub: int64(i)})
	}
	b.ResetTimer()
	for i := 1; i < b.N; i++ {
		kvindex.Get(keys[i], int64(i))
	}
}

Test result on the master branch:

$ go test -bench='BenchmarkIndexGet$' -count=5

goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexGet-12             1000000              2021 ns/op
BenchmarkIndexGet-12             1000000              2029 ns/op
BenchmarkIndexGet-12             1000000              2044 ns/op
BenchmarkIndexGet-12             1000000              1973 ns/op
BenchmarkIndexGet-12             1000000              2027 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  177.815s

Test result on this pr:

$ go test -bench='BenchmarkIndexGet$' -count=5

goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexGet-12             1000000              1985 ns/op
BenchmarkIndexGet-12             1000000              1914 ns/op
BenchmarkIndexGet-12             1000000              1900 ns/op
BenchmarkIndexGet-12             1000000              1905 ns/op
BenchmarkIndexGet-12             1000000              1894 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  177.573s

@wathenjiang
Copy link
Contributor Author

Benchmark Test shows using google/tree in generic way can bring 5%~6% performance improvement.
Of course, this is only about the Btree itself, not the overall ETCD server performance.

@wathenjiang
Copy link
Contributor Author

Related to #14463

@ahrtr
Copy link
Member

ahrtr commented Sep 27, 2022

Looks good to me, please,

  1. add the benchmark test cases into the repo;
  2. add the performance result or this issue link into the commit message.

benchmark result:
(1) master branch
$ go test -bench='BenchmarkIndexPut$' -count=5
goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexPut-12             1000000              2591 ns/op
BenchmarkIndexPut-12             1000000              2531 ns/op
BenchmarkIndexPut-12             1000000              2536 ns/op
BenchmarkIndexPut-12             1000000              2546 ns/op
BenchmarkIndexPut-12             1000000              2538 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  167.439s

$ go test -bench='BenchmarkIndexGet$' -count=5
goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexGet-12             1000000              2021 ns/op
BenchmarkIndexGet-12             1000000              2029 ns/op
BenchmarkIndexGet-12             1000000              2044 ns/op
BenchmarkIndexGet-12             1000000              1973 ns/op
BenchmarkIndexGet-12             1000000              2027 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  177.815s

(2) google/btree in the generic way
$ go test -bench='BenchmarkIndexPut$' -count=5
goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexPut-12             1000000              2477 ns/op
BenchmarkIndexPut-12             1000000              2380 ns/op
BenchmarkIndexPut-12             1000000              2360 ns/op
BenchmarkIndexPut-12             1000000              2396 ns/op
BenchmarkIndexPut-12             1000000              2382 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  165.841s

$ go test -bench='BenchmarkIndexGet$' -count=5
goos: darwin
goarch: amd64
pkg: go.etcd.io/etcd/server/v3/storage/mvcc
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIndexGet-12             1000000              1985 ns/op
BenchmarkIndexGet-12             1000000              1914 ns/op
BenchmarkIndexGet-12             1000000              1900 ns/op
BenchmarkIndexGet-12             1000000              1905 ns/op
BenchmarkIndexGet-12             1000000              1894 ns/op
PASS
ok      go.etcd.io/etcd/server/v3/storage/mvcc  177.573s

Signed-off-by: wathenjiang <wathenjiang@tencent.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @spongecaptain

@wathenjiang
Copy link
Contributor Author

And related to google/btree#45

@wathenjiang wathenjiang reopened this Sep 27, 2022
@wathenjiang
Copy link
Contributor Author

Looks good to me, please,

  1. add the benchmark test cases into the repo;
  2. add the performance result or this issue link into the commit message.

Hi, etcd is very awsome. I learn a lot from etcd.

By the way, the above close is a touch by mistake.

@ahrtr ahrtr merged commit abef537 into etcd-io:main Sep 27, 2022
@wathenjiang wathenjiang changed the title upate:use google/btree in the genric way update:use google/btree in the genric way Sep 27, 2022
ahrtr added a commit to ahrtr/etcd that referenced this pull request Oct 5, 2022
…n etcdserver must be built with go version >= 1.18

etcdserver starts to using generic in etcd-io#14515,
so any applications which depend on etcdserver must be built with go version >= 1.18.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
ahrtr added a commit to ahrtr/etcd that referenced this pull request Oct 5, 2022
…n etcdserver must be built with go version >= 1.18

etcdserver starts to using generic in etcd-io#14515,
so any applications which depend on package `go.etcd.io/etcd/server/v3' must be
built with go version >= 1.18.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
@ahrtr
Copy link
Member

ahrtr commented Oct 5, 2022

Please note that this is a breaking change. Any applications which depend on 3.6 package go.etcd.io/etcd/server/v3 must be built with go version >= 1.18.

Since golang officially supports only two latest minor releases, which means go 1.17 is out of support. So it should be fine. Anyway, I just added a breaking change item to 3.6 changelog. cc @serathius @spzala @ptabor

#14551

@ptabor
Copy link
Contributor

ptabor commented Oct 5, 2022

@serathius @ahrtr
I'm think we will soon load some 1.18 dependency in client as well. So how about simpler message:
- etcd 3.6+ release required 1.18 and it applies to any package, including client and pkg/
- etcd-server-3.6 can still be used with etcd client-v3.5 (and v3.x)... so if customer's application want's to avoid 1.18 dependency, they can still do.

@ahrtr
Copy link
Member

ahrtr commented Oct 5, 2022

Thanks @ptabor for the feedback.

In general, simpler message makes sense, I propose to change the message to something like "Applications which depend on etcd 3.6 packages must be built with go version >= 1.18". We need to clearly differentiate between etcd contributors and (etcd users or applications depending on etcd packages). The former requires go 1.19, and the latter requires go 1.18+.

With regard to the v3 API compatibility between 3.5 and 3.6, it seems be unrelated to this topic, and I don't think it should be mentioned in the breaking change section.

ahrtr added a commit to ahrtr/etcd that referenced this pull request Oct 5, 2022
…n etcd 3.6 packages must be built with go version >= 1.18

etcdserver starts to using generic in etcd-io#14515,
so any applications which depend on package `go.etcd.io/etcd/server/v3' must be
built with go version >= 1.18.

Most likely we may depends on go 1.18 new features in other packages, i.e. client, pkg,
so we just simplify the changelog message to require applications depending on any
etcd 3.6 pacakges must be built with go version >= 1.18, although only etcdserver uses
go generic for now.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
ahrtr added a commit to ahrtr/etcd that referenced this pull request Oct 5, 2022
…n etcd 3.6 packages must be built with go version >= 1.18

etcdserver starts to using generic in etcd-io#14515,
so any applications which depend on package `go.etcd.io/etcd/server/v3' must be
built with go version >= 1.18.

Most likely we may depends on go 1.18 new features in other packages, i.e. client, pkg,
so we just simplify the changelog message to require applications depending on any
etcd 3.6 pacakges must be built with go version >= 1.18, although only etcdserver uses
go generic for now.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
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.

4 participants