v2 release with generic approach by Michal-Leszczynski · Pull Request #43 · google/btree

Michal-Leszczynski · 2022-05-06T12:56:14Z

Fixes #41

Generic approach: goos: linux goarch: amd64 pkg: github.com/google/btree cpu: Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz BenchmarkInsert-4 5942443 203.5 ns/op 19 B/op 0 allocs/op BenchmarkSeek-4 8253608 140.2 ns/op 0 B/op 0 allocs/op BenchmarkDeleteInsert-4 2772153 432.4 ns/op 0 B/op 0 allocs/op BenchmarkDeleteInsertCloneOnce-4 2724934 429.2 ns/op 0 B/op 0 allocs/op BenchmarkDeleteInsertCloneEachTime-4 881670 1337 ns/op 1886 B/op 11 allocs/op BenchmarkDelete-4 5176340 229.0 ns/op 0 B/op 0 allocs/op BenchmarkGet-4 6767662 180.4 ns/op 0 B/op 0 allocs/op BenchmarkGetCloneEachTime-4 4545211 268.4 ns/op 48 B/op 3 allocs/op BenchmarkAscend-4 22772 52821 ns/op 0 B/op 0 allocs/op BenchmarkDescend-4 23056 52434 ns/op 0 B/op 0 allocs/op BenchmarkAscendRange-4 13488 87513 ns/op 0 B/op 0 allocs/op BenchmarkDescendRange-4 10000 118079 ns/op 0 B/op 0 allocs/op BenchmarkAscendGreaterOrEqual-4 19116 62130 ns/op 0 B/op 0 allocs/op BenchmarkDescendLessOrEqual-4 12943 92503 ns/op 0 B/op 0 allocs/op BenchmarkDeleteAndRestore/CopyBigFreeList-4 205 5846645 ns/op 141780 B/op 13 allocs/op BenchmarkDeleteAndRestore/Copy-4 187 6127481 ns/op 453648 B/op 1169 allocs/op BenchmarkDeleteAndRestore/ClearBigFreelist-4 349 3452555 ns/op 783 B/op 2 allocs/op BenchmarkDeleteAndRestore/Clear-4 339 3548745 ns/op 288473 B/op 1061 allocs/op PASS ok github.com/google/btree 34.314s Interface approach: goos: linux goarch: amd64 pkg: github.com/google/btree cpu: Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz BenchmarkInsert-4 4931660 236.1 ns/op 36 B/op 0 allocs/op BenchmarkSeek-4 6406713 176.9 ns/op 7 B/op 0 allocs/op BenchmarkDeleteInsert-4 2522662 489.5 ns/op 0 B/op 0 allocs/op BenchmarkDeleteInsertCloneOnce-4 2496774 478.5 ns/op 0 B/op 0 allocs/op BenchmarkDeleteInsertCloneEachTime-4 546816 2186 ns/op 2992 B/op 11 allocs/op BenchmarkDelete-4 4529532 258.3 ns/op 0 B/op 0 allocs/op BenchmarkGet-4 5962708 204.7 ns/op 0 B/op 0 allocs/op BenchmarkGetCloneEachTime-4 4169655 292.0 ns/op 48 B/op 3 allocs/op BenchmarkAscend-4 16887 68180 ns/op 0 B/op 0 allocs/op BenchmarkDescend-4 17565 69563 ns/op 0 B/op 0 allocs/op BenchmarkAscendRange-4 10000 111384 ns/op 0 B/op 0 allocs/op BenchmarkDescendRange-4 7618 145634 ns/op 0 B/op 0 allocs/op BenchmarkAscendGreaterOrEqual-4 15567 76653 ns/op 0 B/op 0 allocs/op BenchmarkDescendLessOrEqual-4 9375 119378 ns/op 0 B/op 0 allocs/op BenchmarkDeleteAndRestore/CopyBigFreeList-4 182 6649303 ns/op 274293 B/op 14 allocs/op BenchmarkDeleteAndRestore/Copy-4 174 6796221 ns/op 860600 B/op 1152 allocs/op BenchmarkDeleteAndRestore/ClearBigFreelist-4 307 3885217 ns/op 855 B/op 2 allocs/op BenchmarkDeleteAndRestore/Clear-4 285 4168013 ns/op 538771 B/op 1042 allocs/op PASS ok github.com/google/btree 35.118s

SaveTheRbtz · 2022-05-07T00:22:05Z

+
+go 1.18
+
+require github.com/petar/GoLLRB v0.0.0-20210522233825-ae3b015fd3e9 // indirect


nit: maybe this should be removed e.g. through moving btree_mem to a separate package. Otherwise it is strange that v2 package requires gollrb

btree_mem is already in a separate main package (or am I misunderstanding your question?). Where would you want ot move it?

Sorry, the right term would be a separate module (basically a separate go.mod file). This will allow to split dependencies between benchmark and the v2 module (since v2 does not depend on gollrb).

SaveTheRbtz · 2022-05-07T00:28:21Z

+// Two Btrees using the same freelist are safe for concurrent write access.
+type FreeList[T Item[T]] struct {
+	mu       sync.Mutex
+	freelist []*node[T]


Does it make sense to make it an array instead?

Yes, but I would consider changing it in future PR, because this one is focused on replacing interfaces with generics.

SaveTheRbtz · 2022-05-07T00:30:01Z

+type Int int
+
+// Less returns true if int(a) < int(b).
+func (a Int) Less(b Int) bool {
+	return a < b
+}


should it be made private and moved to test?

I believe it's convenient for user to have the most primitive b-tree entry exported.

SaveTheRbtz · 2022-05-07T00:44:19Z

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package v2


maybe this should be a public test, i.e. use v2_test package, e.g.: https://gist.github.com/SaveTheRbtz/3fa26a476b5cacd58d5662856860f8ae

(as a minor benefit this would also make Examples copy-pastable)

I'm used to keeping tests in the same package as the code, so that I can test functions that aren't exported for better testing in general. This project tests only exported functionalities, but perhaps this will change with future PR's?
I'm just not sure about the benefits of moving tests to the separate package.

SaveTheRbtz · 2022-05-07T00:46:23Z

@@ -0,0 +1,77 @@
+// Copyright 2014 Google Inc.


should this be moved to a separate package?

SaveTheRbtz · 2022-05-07T00:50:37Z

+//
+// Write operations are not safe for concurrent mutation by multiple
+// goroutines, but Read operations are.
+type BTree[T Item[T]] struct {


should it be a non-comparable and non-copyable? e.g. through embedding:

type noCmp [0]func() type noCopy struct{} func (*noCopy) Lock() {} type BTree[T Item[T]] struct { noCopy noCmp degree int length int root *node[T] cow *copyOnWriteContext[T] }

SaveTheRbtz · 2022-05-07T00:51:14Z

+// BTree has its own FreeList, but multiple BTrees can share the same
+// FreeList.
+// Two Btrees using the same freelist are safe for concurrent write access.
+type FreeList[T Item[T]] struct {


Should it be "nocopyable" & "nocomparable", given that it is a part of a public interface?

hawkingrei · 2022-05-20T14:05:00Z

@Michal-Leszczynski We are all looking forward to merging this pr.

gconnell · 2022-05-29T21:50:54Z

Rather than create a /v2/, I propose with #45 that we simply create a generic interface as well as the non-generic, backwards-compatible Item interface, and use that for Go 1.18 and beyond to implement the backwards-compatible Item interface. Thoughts?

gconnell · 2022-06-07T05:28:04Z

Closing as #45 I believe handles this same issue. If there's any issues with that or it fails to work correctly, though, happy to reopen this.

Michal-Leszczynski added 2 commits March 31, 2022 19:29

v2 release with generic approach

93be90b

SaveTheRbtz reviewed May 7, 2022

View reviewed changes

keep94 mentioned this pull request May 24, 2022

BTree Iterators #44

Open

gconnell closed this Jun 7, 2022


		go 1.18

		require github.com/petar/GoLLRB v0.0.0-20210522233825-ae3b015fd3e9 // indirect

Conversation

Michal-Leszczynski commented May 6, 2022

Uh oh!

SaveTheRbtz May 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkingrei commented May 20, 2022

Uh oh!

gconnell commented May 29, 2022

Uh oh!

gconnell commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SaveTheRbtz May 7, 2022 •

edited

Loading