-
Notifications
You must be signed in to change notification settings - Fork 82
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
Reworked api to be more in line with native lib #125
Conversation
These changes look pretty great. Great work on the tests too! I'll take a closer look today/tomorrow |
Added Span<byte> overloads for cursor methods Added more test coverage
4f3d0dd
to
70c53ec
Compare
Read Before:BenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.6 (18G5033) [Darwin 18.7.0]
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.102
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
DefaultJob : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
Write Before:BenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.6 (18G5033) [Darwin 18.7.0]
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.102
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
DefaultJob : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
Read After:BenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.6 (18G5033) [Darwin 18.7.0]
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.102
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
DefaultJob : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
Read After w/CopyToNewArray:BenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.6 (18G5033) [Darwin 18.7.0]
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.102
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
DefaultJob : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
Write After:BenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.6 (18G5033) [Darwin 18.7.0]
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.102
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
DefaultJob : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
|
Nice buff to read perf! |
@AlgorithmsAreCool I moved some of your methods to extension methods instead. Also after reviewing the GetResult a bit more, I think having incorrectly sized buffer is likely something you would only hit when getting started with the lib and so I favored an Exception here instead. I also reworded one of your tests once I gathered what you were trying to do, let me know if I misunderstood your intentions. I think the benchmarks show one interesting trait by changing the functions to return the result code and the MDBValue, for instances the array isn't copied or used, there's no added memory overhead. I think the results between the two approaches might be comparable perf-wise, if maybe slightly faster with the new changes but probably negligibly so. |
You also fixed alot of my bad spelling 😁.
I am inclined to agree. Besides this cursor API is zero copy and more flexible. Good Call |
Read w/value.AsSpan().CopyTo(ValueBuffer):We have a winnerBenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.6 (18G5033) [Darwin 18.7.0]
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.102
[Host] : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
DefaultJob : .NET Core 3.1.2 (CoreCLR 4.700.20.6602, CoreFX 4.700.20.6702), X64 RyuJIT
|
} | ||
|
||
/// <summary> | ||
/// Return key/data at current cursor position | ||
/// </summary> | ||
/// <returns>Key/data at current cursor position</returns> | ||
public KeyValuePair<MDBValue, MDBValue> GetCurrent() | ||
public (MDBResultCode resultCode, MDBValue key, MDBValue value) GetCurrent() |
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.
I am a bit concerned by this (even though it was basically the same before).
Per LMDB docs for mdb_cursor_get and then the referenced mdb_get docs :
Note
The memory pointed to by the returned values is owned by the database. The caller need not dispose of the memory, and may not modify it in any way. ...
So if the pattern is to use MDBValue.AsSpan(), the library user might be tempted to update the values. Perhaps since the library errs towards safety we might consider returning these as ReadOnlySpan instead of MDBValue.
The downside is that Span can't be used as a generic type argument and therefore can't be used in a tuple/valuetuple. The workaround would be to upgrade the tuple to a readonly ref struct instead. All of the extension methods would still work however.
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.
I think if folks are going to work that hard to shoot themselves in the foot... Let's let the dust settle and see if there's any feedback. I think I'm okay with this direction for the time being.
@CoreyKaylor So apparently, if i don't click "Submit Review" my comments don't appear. I'm sorry i thought i left that comment last night. But the changes look good i don't see any correctness issues. |
This is the first pass at the proposed cursor api changes. It changes the underlying check / throwing behavior and favors the result code. I wanted to get something up to share and get some feedback on the changes. I'm also finding that it makes the API generally much easier to work with to return the result codes IMO.
Fixes #124
Fixes #123