-
Notifications
You must be signed in to change notification settings - Fork 968
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
Speed up memory db by using fixed-size keys in skiplist search #387
base: master
Are you sure you want to change the base?
Conversation
After reading the commit dae8fc2, I guess the main improvement is gained from skipping most of key loading and byte comparison(by using integer comparison)? |
Primarily avoiding loading -- by minimizing the memory surface area that we have to jump around on during the search |
@@ -469,11 +478,31 @@ func (p *DB) Reset() { | |||
func New(cmp comparer.BasicComparer, capacity int) *DB { | |||
p := &DB{ | |||
cmp: cmp, | |||
quickCmp: (cmp == comparer.DefaultComparer), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's wrong here. Actually the cmp used in leveldb is iComparer
with the notion of sequence
and key type
.
It means the quickCmp
is disabled by default.
func (n node) quickCmp(key []byte) int { | ||
var other nodeInt | ||
if is64Bit { | ||
other = nodeInt(binary.BigEndian.Uint64(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not suitable to use Uint64
. Integer conversion can break the real ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can change to comparison flag to uint
. It doesn't make sense to contain negative value.
} | ||
// pad | ||
k := make([]byte, fixedKeyLength) | ||
copy(k, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind, it's right to right padding
.
This PR is a follow-up to #385, and only the last two commits are "this PR".
The first "this PR" commit contains a small command-line utility to run a set of
Put
-operations, and export the times in a json-format. The json format can be made into graphs via this little tool: https://github.com/holiman/mkchart . Note, thestats
package can be dropped, I just included it in this PR so other people can repro my results, and try out other combos.The last commit contains the actual change.
Problem
Let's take a look at the graphs for storing 4M key/value pairs. In every example, the key is
32 byte
. The graphs show value size of32
,96
and256
byte, respectively.As can be seen, when the values become larger, the
Put
operation becomes slower. This is due to the skiplist search -- for every Put, we need to find the correct skiplist position. This entails iterating thenodeData
structure, but also accessing thekvData
structure to do the key-comparisons.With larger
kvData
, we get less benefits from various caches, and the memory accesses slow the whole thing down.Solution
This PR frees up one field in the skiplist
node
, -- by packing theheight
element into thekeySize
. Theuint64
which previously heldheight
instead now holds 8 bytes of the key.During the search, instead of loading the full key, we can do a
quickCmp
using the data we've already loaded.( note: this trick can only be performed if the memdb is configured to use the
DefaultComparer
, otherwise it falls back to using the configured comparer )With this feature enabled, here are the new charts:
Impact
The total runtimes for the three examples are as follows
32:32
17.93s
10.85s
32:96
18.17s
11.83s
32:256
24.58s
13.30s
Note for 32-bit platforms
The PR as written assumes 64-bit platform, but can easily be extended to support 32-bit. In that case, we could use
4
bytes instead of8
for thequickCmp
. If we assume that1
byte of a key is application-specific prefix, that leaves3
bytes of entropy, so even that should be 'ok' for up to16M
items -- which is quite a lot for a memory db.I have experimented with a more niche variant, where the
node
is based onuint32
, and6
bytes are used. That can be achieved if the keyLength is limited to4096
, theheight
uses only one nibble, and, of course, that thekvOffset
is limited to32
bits (which is currently already the case for32-bit
platforms). However, this PR does not make any niche-specific changes to go-leveldb which would not fit the general case.Todos not fixed yet:
32-bit
platformsquickCmp
forfindLT
, and possiblySeek
andfill
.Note about benchmarks
A final note: the existing benchmarks do not quite demonstrate this issue, nor do they show any significant speed improvement in this PR (at least not the
Put
tests). The reason for that is twofold:nil
value, meaning that they do not suffer (much) from reading data from thekvData
section -- which is essentially just a tighly packed space of keys.For posterity though: