-
-
Notifications
You must be signed in to change notification settings - Fork 86
Allocation free GuidConverter for NET #266
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
Conversation
Pull Request Test Coverage Report for Build 15107804566Details
💛 - Coveralls |
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.
Pull Request Overview
This PR updates the Guid conversion routines to reduce allocations by using direct byte manipulation for converting between DuckDBHugeInt and Guid types under .NET 6.0 or greater. Key changes include:
- Using ArrayPool with direct byte conversion in ConvertToGuid.
- Implementing a new ToHugeInt method that reconstructs a Guid's byte order via direct memory manipulation.
- Maintaining backward compatibility for earlier .NET versions with the original string-based conversion.
Comments suppressed due to low confidence (1)
DuckDB.NET.Data/Extensions/GuidConverter.cs:59
- [nitpick] Consider renaming 'ConvertToGuid' to 'ToGuid' to align with the naming convention used in 'ToHugeInt', enhancing consistency.
public static Guid ConvertToGuid(this DuckDBHugeInt input)
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.
Pull Request Overview
The PR replaces string-based GUID conversion with direct byte manipulation and ArrayPool usage to reduce allocations in GuidConverter.
- Introduces an allocation-free
ConvertToGuidimplementation for .NET 6+ usingArrayPool<byte>and bit reordering. - Adds a new
ToHugeIntmethod that writes GUID bytes into a 32-byte buffer and reorders them for DuckDB. - Retains the original fallback logic for earlier frameworks under
!NET6_0_OR_GREATER.
|
@alfeg What do you think about this implementation? https://github.com/Giorgi/DuckDB.NET/blob/Guid-Converter/DuckDB.NET.Data/Extensions/GuidConverter.cs Or even this: https://github.com/Giorgi/DuckDB.NET/blob/Guid-Converter/DuckDB.NET.Data/Extensions/GuidConverter.cs |
|
@Giorgi links looks to be same, but I do like this implementation. Seems to work nice and fast
I'm very surprised how much a difference .Net 9 provide against .Net 8 |
|
What we could also do is allocate only 16 bytes and do in-place swapping instead of copying 16 bytes to another 16 bytes. Not sure if that will have any impact though. |
|
@Giorgi for .Net 8/9 they are allocated in stack. I don't think we will be able to find any measurable difference there. I guess currently compiler is able to unwind loop and produce highly optimized code. Not sure if we will be able to do better. |
|
That's right but I wonder why on .Net 8 there is more memory allocation with the new implementation considering that the string is no longer allocated. |
|
@Giorgi seems to be some issue in On the bright side - it's now 3x times faster |
|
@alfeg That's a great improvement but it's kind of funny that we started with memory allocation improvement and ended with a bit of regression 😀 |
|
By the way, did you benchmark the other method as well? |
|
@Giorgi no I didn't. By the way, thanks for this project! |
Hi,
Thanks for great lib.
We use DuckDB for in-process analyze of large amount of data from MSSQL. This involves movement of billion rows into DuckDB using Appender.
Rider noticed large amount of allocations from AppendValue()
I've rewrite GuidConverter to use direct bytes manipulation instead of strings for
DuckDBHugeIntconversion from/to Guid. Under .Net 9.0 there are no allocations, under .Net 8.0 - a bit reduced. Perf gains not that visibleSource code of benchmark: DuckDbNet.GuidConverterBench.zip