-
Notifications
You must be signed in to change notification settings - Fork 60
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
Support for unsigned integer values #99
Open
bmatsuo
wants to merge
58
commits into
master
Choose a base branch
from
bmatsuo/value-interface
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The intial benchmark results are not impressive. Writing a bunch of size_t values doesn't appear to be significantly faster than encoding uint64 values in big-endian. And, when optmized to eliminate allocations big-endian byte slices have consistently better performance characteristics.
It doesn't seem like a good idea to expose the String function because there may be a StringValue implementation of Value in the future. So this type either needs to be implemented premptively as a stub or omitted for the time being.
This will provide better grouping of topics in godoc. Uint stuff will all be together and Uintptr stuff will all be together. The byte-related Value stuff won't really be together, but that is a tragedy of evolution. Maybe for lmdb-go 2.0 this part of the Value and FixedPage interfaces can be made consistent for all data types.
Instead using a method named Put, the name Append is used to more closely match what it is doing. This also more transparently exposes slice appending semantics and its quirks (which were already there).
There is some additional overhead processing integer Multi variants but I couldn't trust users if I were to make the types raw byte slices.
Using the word Value so much was really confusing because LMDB is a key-value store and the word "value" already has a reserved meaning.
This is where the Data interface is defined as well as the generic BytesData type. This is also where the FixedPage interface is defined because it seems like the most appropriate place, though the generic Multi type has been left if val.go, mostly for historical reasons. It may be moved later.
It's not really clear how those functions should behave and the data conversion functions provided should provide more help overall.
The are faster.
The convention in go is to differentiate functions based on their type using a name suffix. The previous names did not meet this convention and it inconsistent where other functions in the package would follow convention.
It turns out I have been using the official terminology backwards. LMDB associates keys with data. Keys and data are both values (this was a bit of a facepalm given the type names).
Fixes #103 This will effectively deprecate Cursor.PutMulti once merged. It can survive for a while longer. But PutMultiple is a bit simpler if somewhat slower. The overhead of interfaces should be dominated by the actual work of inserting items into the database in practice.
bmatsuo
changed the title
Support for unsigned integer data
Support for unsigned integer values
Feb 12, 2017
bmatsuo
force-pushed
the
bmatsuo/value-interface
branch
from
February 12, 2017 17:04
6eeba8b
to
9f15f38
Compare
I am still working back from a spot where there were way too many panics. A new type, Stride, has been introduced with a method Stride.Multiple which has the same signature as MultipleCUint and MultipleCSizet.
These methods can be useful occasionally. Examples were added to demonstrate when slice syntax can be used instead. Also, this change renames the receiver in CUintValue and CSizetValue methods to be u and z resectively. This will help drive the naming convention that the Value* convertion functions adopted.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #11
I am opening this now because it is close to done. But it is probably not done right now. It is a rather large change though. So I am letting it bake a here for a little bit.
The one thing that differs from the discussion in #11 is that the interface here is called Data, not Value. The word "value" was too overloaded already. Data is pretty precise, and shorter by one character. So I have opted to use it.edit: The only unfortunate thing about the use of the word Data is its appearance near the beginning of the function index generated bygo doc
. This is fairly minor, but it does bug me with the number of conversion functions I had to define for working with the output of Txn.GetData and Cursor.GetData.edit: I have actually stopped using an interface. The base code is the same and I can reintroduce an interface if needed in the future. But the API felt unwieldy and even without the new interfaces the API change is not negligible. But I think the result is quite clean now, and it's a few ns faster than the version that used interfaces.