Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

another example that uses KEY uint64, VALUE string #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yozgatsi
Copy link

@yozgatsi yozgatsi commented Oct 3, 2018

Hi folks,

In case there is interest, wanted to share an example I created following your instructions and code.
Hope this is in line with your contribution process, let me know otherwise.

Best,
Tavit

@cznic cznic self-requested a review October 3, 2018 19:29
@navytux
Copy link
Collaborator

navytux commented Oct 4, 2018

For the reference: another example that generates b-like package with static types, but also with staticly defined cmp function and adjusted kd: https://lab.nexedi.com/kirr/neo/blob/fcab58ca/go/zodb/storage/fs1/fsb/gen-fsbtree

// $ go test -bench 1e3 example/all_kuint64vstring_test.go example/kuint64vstring.go
// goos: linux
// goarch: amd64
// BenchmarkSetSeq1e3-4 10000 131588 ns/op
Copy link
Owner

Choose a reason for hiding this comment

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

I notice this benchmark is noticeable slower wrt the old ones on Go 1.7rc1 above. I wonder why. Do you know?

I also need to rerun the benchmarks again on the same machine (i5-4670 CPU @ 3.4GHz) with Go1.11.1. That will be possible only later this day. Of course, that does not block this PR.

Copy link
Owner

@cznic cznic left a comment

Choose a reason for hiding this comment

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

I've done

==== jnml@r550:~/src/github.com/cznic/b> git checkout -b yozgatsi-master master
Switched to a new branch 'yozgatsi-master'
==== jnml@r550:~/src/github.com/cznic/b> git pull https://github.com/yozgatsi/b.git master
From https://github.com/yozgatsi/b
 * branch            master     -> FETCH_HEAD
Updating 35e9bbe..4b08323
Fast-forward
 CONTRIBUTORS                       |    1 +
 doc.go                             |   28 +++-
 example/all_kuint64vstring_test.go | 1227 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 example/kuint64vstring.go          |  908 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 2162 insertions(+), 2 deletions(-)
 create mode 100644 example/all_kuint64vstring_test.go
 create mode 100644 example/kuint64vstring.go
==== jnml@r550:~/src/github.com/cznic/b> cd example/
/home/jnml/src/github.com/cznic/b/example
==== jnml@r550:~/src/github.com/cznic/b/example> make
go fmt
all_kuint64vstring_test.go
go test -i
go test
# github.com/cznic/b/example [github.com/cznic/b/example.test]
./kuint64vstring.go:14:2: kx redeclared in this block
	previous declaration at ./int.go:14:7
./kuint64vstring.go:15:2: kd redeclared in this block
	previous declaration at ./int.go:15:7
./kuint64vstring.go:29:2: btDPool redeclared in this block
	previous declaration at ./int.go:29:2
./kuint64vstring.go:30:2: btEPool redeclared in this block
	previous declaration at ./int.go:30:2
./kuint64vstring.go:31:2: btTPool redeclared in this block
	previous declaration at ./int.go:31:2
./kuint64vstring.go:32:2: btXPool redeclared in this block
	previous declaration at ./int.go:32:2
./kuint64vstring.go:35:6: btTpool redeclared in this block
	previous declaration at ./int.go:35:6
./kuint64vstring.go:43:6: btEpool redeclared in this block
	previous declaration at ./int.go:43:6
./kuint64vstring.go:58:2: Cmp redeclared in this block
	previous declaration at ./int.go:58:2
./kuint64vstring.go:60:2: d redeclared in this block
	previous declaration at ./int.go:60:2
./kuint64vstring.go:60:2: too many errors
FAIL	github.com/cznic/b/example [build failed]
Makefile:15: recipe for target 'editor' failed
make: *** [editor] Error 2
==== jnml@r550:~/src/github.com/cznic/b/example>

I think the solution is to put your example in its own directory.

@cznic
Copy link
Owner

cznic commented Oct 4, 2018

@navytux I noticed the 126 value for kd in fsbtree.go. I think I know why it's not a power of 2, but I wonder how much of an improvement you have measured memory and speed wise - provided you have made such measurements.

If the improvements are above noise level, I would like a PR from you for this change. Thanks.

@navytux
Copy link
Collaborator

navytux commented Oct 4, 2018

@cznic, I do not recall all the details now, but kd was tuned so that sizeof(d) is ~ 1 page. Here is what I could find about benchmarks:

https://lab.nexedi.com/kirr/neo/commit/6793678b

IndexLoad loads db index from a file, which inside essentially performs many b.Tree.Set: https://lab.nexedi.com/kirr/neo/blob/fcab58ca/go/zodb/storage/fs1/index.go#L223.

It was definitely a measurable improvement, though a dedicated bulk-loading API would be much better.

If it makes sense to you I could try to work on related PR, but it won't be fast.

@cznic
Copy link
Owner

cznic commented Oct 4, 2018

It was definitely a measurable improvement, though a dedicated bulk-loading API would be much better.

Please suggest the API. No promises above I'll consider it. Thanks.

If it makes sense to you I could try to work on related PR, but it won't be fast.

Go forward if you like to at any pace. Just please tune kd for the default type which is interface{}. Your 126 kd is specific to your key/value usage case.

I'm curious if setting kd to say 31 instead of 32 would make some nice difference. I could try it, but you've found the opimization opportunity so I would prefer if you keep the credit ;-)

@navytux
Copy link
Collaborator

navytux commented Oct 4, 2018

Please suggest the API. No promises above I'll consider it. Thanks.

Some time ago I was experimenting with trying to make Get/Set auto detect bulk retrieve/loading cases and optimize appripriately:

https://lab.nexedi.com/kirr/b/compare/master...x/bulkload?expand_all_diffs=1 (scroll down to diffs) +
https://lab.nexedi.com/kirr/b/commit/93348d0f

If I recall correctly it was indeed ~ 8x faster, but regular random access got a bit (~5% IIRC) slower. That's why I did not posted my changes.

I do not have a particular idea for dedicated bulk loading API for now as I'm not currently working on BTrees. Whenever/if I have a chance to come back to this topic I will try to speak up with the API proposal.


About kd tuning - please feel free to do it, as I'm not currently working on BTrees and it is hard to tell when I could find time to work on it next time. There is no need to keep me in the hall of fame, but if you really want to preserve the credits you could mention me and reference our herby conversation in the commit.

@cznic
Copy link
Owner

cznic commented Oct 4, 2018

Okay, thanks for the input.

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

Successfully merging this pull request may close these issues.

3 participants