Skip to content

Conversation

ajwerner
Copy link

@ajwerner ajwerner commented May 18, 2025

Fixes #95.

Supersedes #98

@marvin-j97
Copy link
Contributor

marvin-j97 commented May 18, 2025

With the Borrow<Q> trait bound it's not possible to use InternalKeyRef that would allow to not build an InternalKey (which involves a heap allocation).
Need to use equivalent::Comparable instead.

See: #98

// for the crate to work correctly. Anything larger than that will work.
//
// TODO: Justify this size.
const DEFAULT_BUFFER_SIZE: usize = (32 << 10) - size_of::<AtomicUsize>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to play with this a bit - but should probably be much higher by default: 1 MB or so?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it should be bigger than 32k, but 1MiB might be too big. The keys and values are not inline, it’s just the metadata. The questions I’d have are how expensive is allocating a new block, and how expensive is inserting into the skip map. My guess is that the alloc is not likely worse than 10us (it’s probably way less) and the inserts are ~100ns. If you can fit 1000 in here (if we say the average links is 32 and the key and value are each 32 bytes), then you’ll have spent at least 10x as long doing the inserting. In practice I think the mallocs even with zeroing is a lot cheaper. The benchmarks I was playing with don’t show much win above 256KiB.

unsafe impl<const N: usize> Send for Arenas<N> {}
unsafe impl<const N: usize> Sync for Arenas<N> {}

pub(crate) struct Arenas<const BUFFER_SIZE: usize = DEFAULT_BUFFER_SIZE> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually, for write transactions, the size should be much smaller (so that small transactions don't overallocate too much) - so this needs to be a non-generic parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I can do that.

Copy link
Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

I’ll play with updating. It’s pretty hard to not leak the node from the previous update without hooking up a free list but it’s also not so hard to add one.

unsafe impl<const N: usize> Send for Arenas<N> {}
unsafe impl<const N: usize> Sync for Arenas<N> {}

pub(crate) struct Arenas<const BUFFER_SIZE: usize = DEFAULT_BUFFER_SIZE> {
Copy link
Author

Choose a reason for hiding this comment

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

Okay, I can do that.

// for the crate to work correctly. Anything larger than that will work.
//
// TODO: Justify this size.
const DEFAULT_BUFFER_SIZE: usize = (32 << 10) - size_of::<AtomicUsize>();
Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it should be bigger than 32k, but 1MiB might be too big. The keys and values are not inline, it’s just the metadata. The questions I’d have are how expensive is allocating a new block, and how expensive is inserting into the skip map. My guess is that the alloc is not likely worse than 10us (it’s probably way less) and the inserts are ~100ns. If you can fit 1000 in here (if we say the average links is 32 and the key and value are each 32 bytes), then you’ll have spent at least 10x as long doing the inserting. In practice I think the mallocs even with zeroing is a lot cheaper. The benchmarks I was playing with don’t show much win above 256KiB.

@marvin-j97
Copy link
Contributor

Todo:

@marvin-j97 marvin-j97 added good first issue Good for newcomers help wanted Extra attention is needed labels Jul 12, 2025
@marvin-j97 marvin-j97 changed the base branch from main to 3.0.0 September 6, 2025 20:29
@marvin-j97
Copy link
Contributor

I've tried rebasing this onto the v3 branch.

Outlook now: we don't need a free list because all insertions will be unique (because of unique sequence number).

@marvin-j97
Copy link
Contributor

Removing the const generic N parameter is actually a bit harder than expected because it defines the layout of Buffer; changing it to static makes pointer derefs fail with misaligned pointer dereference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed performance test type:memtable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use skiplist tailored for LSM-tree

2 participants