-
Notifications
You must be signed in to change notification settings - Fork 25
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
add the ability to leverage zero-copy on blockstores. #75
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
store.go
Outdated
if err != nil { | ||
return err | ||
} | ||
f := func(b []byte) error { |
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.
can we avoid the extra function call on each get?
We can do the direct atlas decode on the get path, and declare the same thing as a private method rather than an anonymous function that has to be re-allocated each Get
that we use in the View case.
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.
Pretty sure this would get inlined, but let me check.
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.
No inlining happens, and Go benchmark shows that there's an extra alloc. I pushed 4c9b009 to incur in the (inevitable) callback alloc only in the zero-copy path.
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.
so good
Some datastores like badger offer a zero-copy query that loads an mmapped byte slice. For those, we can avoid the slice alloc + copy of the standard blockstore store get, but it's important that the byte slice does not escape, as it would be unsafe.
This patch introduces support for blockstores that support deserializing/processing the value in-place.