Skip to content
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

Make SequentialGuidValueGenerator non-allocating #30610

Closed
NKnusperer opened this issue Apr 3, 2023 · 3 comments · Fixed by #30649
Closed

Make SequentialGuidValueGenerator non-allocating #30610

NKnusperer opened this issue Apr 3, 2023 · 3 comments · Fixed by #30649
Labels
area-perf area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution customer-reported type-enhancement
Milestone

Comments

@NKnusperer
Copy link
Contributor

The current implementation of SequentialGuidValueGenerator.Next() allocates some heap memory due to byte[] usages:

public override Guid Next(EntityEntry entry)
{
var guidBytes = Guid.NewGuid().ToByteArray();
var counterBytes = BitConverter.GetBytes(Interlocked.Increment(ref _counter));
if (!BitConverter.IsLittleEndian)
{
Array.Reverse(counterBytes);
}
guidBytes[08] = counterBytes[1];
guidBytes[09] = counterBytes[0];
guidBytes[10] = counterBytes[7];
guidBytes[11] = counterBytes[6];
guidBytes[12] = counterBytes[5];
guidBytes[13] = counterBytes[4];
guidBytes[14] = counterBytes[3];
guidBytes[15] = counterBytes[2];
return new Guid(guidBytes);
}

We can rewrite this to a Span based version to avoid any allocations like this:

public override Guid Next(EntityEntry entry)
{
    Span<byte> guidBytes = stackalloc byte[16];
    Guid.NewGuid().TryWriteBytes(guidBytes);
    long incrementedCounter = Interlocked.Increment(ref _counter);
    Span<byte> counterBytes = MemoryMarshal.AsBytes(MemoryMarshal.CreateSpan(ref incrementedCounter, 1));
    
    if (!BitConverter.IsLittleEndian)
    {
        counterBytes.Reverse();
    }
    
    guidBytes[08] = counterBytes[1];
    guidBytes[09] = counterBytes[0];
    guidBytes[10] = counterBytes[7];
    guidBytes[11] = counterBytes[6];
    guidBytes[12] = counterBytes[5];
    guidBytes[13] = counterBytes[4];
    guidBytes[14] = counterBytes[3];
    guidBytes[15] = counterBytes[2];
    
    return new Guid(guidBytes);
}

Comparison:

BenchmarkDotNet=v0.13.5, OS=Windows 10 (10.0.19045.2728/22H2/2022Update)
11th Gen Intel Core i7-11850H 2.50GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=7.0.202
  [Host]     : .NET 6.0.15 (6.0.1523.11507), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.15 (6.0.1523.11507), X64 RyuJIT AVX2


| Method |     Mean |    Error |   StdDev | Ratio |   Gen0 | Allocated | Alloc Ratio |
|------- |---------:|---------:|---------:|------:|-------:|----------:|------------:|
|    Old | 65.44 ns | 0.611 ns | 0.572 ns |  1.00 | 0.0057 |      72 B |        1.00 |
|    New | 58.45 ns | 0.639 ns | 0.598 ns |  0.89 |      - |         - |        0.00 |

I can open a PR once this has been reviewed and approved.

@roji
Copy link
Member

roji commented Apr 5, 2023

@NKnusperer LGTM - go ahead and submit a PR.

@NKnusperer
Copy link
Contributor Author

@roji I just noticed that

Span<byte> counterBytes = MemoryMarshal.AsBytes(MemoryMarshal.CreateSpan(ref incrementedCounter, 1));

can also be expressed as

Span<byte> counterBytes = stackalloc byte[sizeof(long)];
BitConverter.TryWriteBytes(counterBytes, incrementedCounter);

which is probably the preferred version right?

@roji
Copy link
Member

roji commented Apr 5, 2023

Sure, that's slightly better. You should also be able to just MemoryMarshal.Write, which is very similar but slightly terser (no real need for Try here).

@ajcvickers ajcvickers added this to the Backlog milestone Apr 5, 2023
@ajcvickers ajcvickers modified the milestones: Backlog, 8.0.0-preview4 Apr 20, 2023
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Apr 20, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0-preview4, 8.0.0 Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants