Make FNV hash compatible across endianness #9489

JanKrivanek · 2023-12-04T11:46:04Z

Context

#9387 (comment)
Current FNV hashing (introduced in #9387) is dependent on endian-ness.

Changes Made

Removed the span version of hashing - as it doesn't bring any benefits and is endian-ness dependent

Perf testing

Method	Runtime	StrLength	Mean	Error	StdDev
ComputeHash64_Span	.NET 8.0	10	9.876 ns	0.0483 ns	0.0403 ns
ComputeHash64_Compatible	.NET 8.0	10	8.067 ns	0.0588 ns	0.0550 ns
ComputeHash64_Span	.NET 4.7.2	10	26.883 ns	0.1432 ns	0.1269 ns
ComputeHash64_Compatible	.NET 4.7.2	10	10.654 ns	0.0881 ns	0.0824 ns
ComputeHash64_Span	.NET 8.0	100	155.327 ns	1.1980 ns	1.0620 ns
ComputeHash64_Compatible	.NET 8.0	100	149.432 ns	0.1521 ns	0.1348 ns
ComputeHash64_Span	.NET 4.7.2	100	181.392 ns	2.6582 ns	2.3564 ns
ComputeHash64_Compatible	.NET 4.7.2	100	155.239 ns	0.7283 ns	0.6456 ns
ComputeHash64_Span	.NET 8.0	1000	1,633.550 ns	2.5869 ns	2.1602 ns
ComputeHash64_Compatible	.NET 8.0	1000	1,625.889 ns	1.1307 ns	1.0023 ns
ComputeHash64_Span	.NET 4.7.2	1000	1,683.154 ns	10.6791 ns	9.9893 ns
ComputeHash64_Compatible	.NET 4.7.2	1000	1,637.241 ns	3.0075 ns	2.8132 ns
ComputeHash64_Span	.NET 8.0	10000	16,693.668 ns	90.8291 ns	80.5176 ns
ComputeHash64_Compatible	.NET 8.0	10000	16,301.102 ns	66.2687 ns	61.9877 ns
ComputeHash64_Span	.NET 4.7.2	10000	16,596.230 ns	68.7995 ns	64.3551 ns
ComputeHash64_Compatible	.NET 4.7.2	10000	16,413.434 ns	63.6246 ns	59.5145 ns
ComputeHash64_Span	.NET 8.0	100000	164,662.715 ns	588.7268 ns	550.6954 ns
ComputeHash64_Compatible	.NET 8.0	100000	174,713.916 ns	1,744.7746 ns	2,502.3036 ns
ComputeHash64_Span	.NET 4.7.2	100000	172,695.281 ns	3,451.7767 ns	4,239.0918 ns
ComputeHash64_Compatible	.NET 4.7.2	100000	165,095.489 ns	520.5961 ns	461.4949 ns
ComputeHash64_Span	.NET 8.0	1000000	1,644,626.828 ns	8,012.8957 ns	7,103.2233 ns
ComputeHash64_Compatible	.NET 8.0	1000000	1,632,366.042 ns	5,566.6398 ns	5,207.0384 ns
ComputeHash64_Span	.NET 4.7.2	1000000	1,660,009.049 ns	5,599.0299 ns	5,237.3361 ns
ComputeHash64_Compatible	.NET 4.7.2	1000000	1,642,601.532 ns	7,044.1614 ns	5,882.1938 ns

(".NET Framework 4.7.2" shortened to ".NET 4.7.2" for the content brevity)

The test harness:

public class Benchmarks
{
    [Params(10, 100, 1000, 10000, 100000, 1000000)]
    public int StrLength;
    private string _str;

    [GlobalSetup]
    public void Setup()
    {
        _str = CreateRandomBase64String(StrLength);
    }

    // 64 bit FNV prime and offset basis for FNV-1a.
    private const long fnvPrimeA64Bit = 1099511628211;
    private const long fnvOffsetBasisA64Bit = unchecked((long)14695981039346656037);

    [Benchmark]
    public long ComputeHash64_Span()
    {
        string text = _str;

        long hash = fnvOffsetBasisA64Bit;

        ReadOnlySpan<byte> span = MemoryMarshal.Cast<char, byte>(text.AsSpan());
        foreach (byte b in span)
        {
            hash = unchecked((hash ^ b) * fnvPrimeA64Bit);
        }

        return hash;
    }

    [Benchmark]
    public long ComputeHash64_Compatible()
    {
        string text = _str;

        long hash = fnvOffsetBasisA64Bit;
        unchecked
        {
            for (int i = 0; i < text.Length; i++)
            {
                char ch = text[i];
                byte b = (byte)ch;
                hash ^= b;
                hash *= fnvPrimeA64Bit;

                b = (byte)(ch >> 8);
                hash ^= b;
                hash *= fnvPrimeA64Bit;
            }
        }

        return hash;
    }

    public static string CreateRandomBase64String(int length)
    {
        const int eachStringCharEncodesBites = 6; // 2^6 = 64
        const int eachByteHasBits = 8;
        const double bytesNumNeededForSingleStringChar = eachStringCharEncodesBites / (double)eachByteHasBits;

        int randomBytesNeeded = (int)Math.Ceiling(length * bytesNumNeededForSingleStringChar);

        byte[] randomBytes = new byte[randomBytesNeeded];
        new Random().NextBytes(randomBytes);
        //Base64: A-Z a-z 0-9 +, /, =
        var randomBase64String = Convert.ToBase64String(randomBytes);
        return randomBase64String.Substring(0, length);
    }
}

JanKrivanek · 2023-12-04T11:47:35Z

FYI: @KalleOlaviNiemitalo, @uweigand - thank you for all the input!

rainersigwald · 2023-12-04T17:20:31Z

What runtime were those results from? I'd expect Span to be beneficial only on netcore.

JanKrivanek · 2023-12-04T18:01:17Z

Ah - I forgot to mention that - good point!

Yes - those were for NET 8.0 only. 3.5 need to use the 'compatible' version anyways. The 4.7.2 can use the Span version, but it as well perform slightly worse (overall the numbers were quite similar - ~~but let me run both scenarios and ammend the numbers for documentation purposes~~ amended)

ladipro

Thank you for following through!

Forgind · 2023-12-22T20:35:53Z

Since the compatible version seems to be faster anyway, this change seems reasonable, but what does that have to do with endianness? It seems like the span version and the not-span version do the same thing...

JanKrivanek · 2023-12-22T20:44:36Z

Since the compatible version seems to be faster anyway, this change seems reasonable, but what does that have to do with endianness? It seems like the span version and the not-span version do the same thing...

Casting byte ptr to char ptr leads to proper reordering of bytes (if needed) in memory by runtime (so that they are Little Endian, regardless of the actual storage arch)
When using the span version - we are basically handling the bytes on our own - so getting them in order as they are actually stored.

#9387 (comment) has more details

Forgind

Looks good!

Forgind · 2023-12-23T00:36:59Z

Since the compatible version seems to be faster anyway, this change seems reasonable, but what does that have to do with endianness? It seems like the span version and the not-span version do the same thing...

Casting byte ptr to char ptr leads to proper reordering of bytes (if needed) in memory by runtime (so that they are Little Endian, regardless of the actual storage arch) When using the span version - we are basically handling the bytes on our own - so getting them in order as they are actually stored.

#9387 (comment) has more details

Thanks for the explanation! Sorry I didn't follow that link before 🙂

Looks like there's a conflict.

KirillOsenkov · 2024-01-09T03:05:13Z

We also have an Fnv1a implementation in the binlog writer:

msbuild/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs

Lines 1314 to 1342 in 10f2f32

    
           internal static class FnvHash64 
        
           { 
        
               public const ulong Offset = 14695981039346656037; 
        
               public const ulong Prime = 1099511628211; 
        
               public static ulong GetHashCode(string text) 
        
               { 
        
                   ulong hash = Offset; 
        
                   unchecked 
        
                   { 
        
                       for (int i = 0; i < text.Length; i++) 
        
                       { 
        
                           char ch = text[i]; 
        
                           hash = (hash ^ ch) * Prime; 
        
                       } 
        
                   } 
        
                   return hash; 
        
               } 
        
               public static ulong Combine(ulong left, ulong right) 
        
               { 
        
                   unchecked 
        
                   { 
        
                       return (left ^ right) * Prime; 
        
                   } 
        
               } 
        
           }

See my analysis here:
https://github.com/KirillOsenkov/MSBuildStructuredLog/wiki/String-Hashing

Would be interesting to add this hash implementation to my benchmarks and compare:
https://github.com/KirillOsenkov/Benchmarks/blob/main/src/Tests/StringHash.Fnv.cs
https://github.com/KirillOsenkov/Benchmarks/blob/f2c45821c2cf7243b040d2c1db5904bab8134cf8/src/Tests/StringHash.cs#L73

JanKrivanek · 2024-01-09T11:46:08Z

We also have an Fnv1a implementation in the binlog writer:

msbuild/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs

Lines 1314 to 1342 in 10f2f32

internal static class FnvHash64

{

public const ulong Offset = 14695981039346656037;

public const ulong Prime = 1099511628211;

public static ulong GetHashCode(string text)

{

ulong hash = Offset;

unchecked

{

for (int i = 0; i < text.Length; i++)

{

char ch = text[i];

hash = (hash ^ ch) * Prime;

}

}

return hash;

}

public static ulong Combine(ulong left, ulong right)

{

unchecked

{

return (left ^ right) * Prime;

}

}

}

See my analysis here: https://github.com/KirillOsenkov/MSBuildStructuredLog/wiki/String-Hashing

Would be interesting to add this hash implementation to my benchmarks and compare: https://github.com/KirillOsenkov/Benchmarks/blob/main/src/Tests/StringHash.Fnv.cs https://github.com/KirillOsenkov/Benchmarks/blob/f2c45821c2cf7243b040d2c1db5904bab8134cf8/src/Tests/StringHash.cs#L73

Yeah - I 'borrowed' that one (ComputeHash64Fast) :-) and quoted the source (your analysis):

https://github.com/dotnet/msbuild/pull/9489/files#diff-ed2e36ca70a3a73e4379cf4470aa8f4f492b961eabfaa15127b21efcff6706f1R58

Other than the Span usage the implementations are identical. However the Span usage didn't provide any significant benefit and required coditional compilation for .NET 3.5 - hence was pulled out.

Note though that currently this work is on waiting banch till the #9572 is fixed.

rokonec · 2024-01-12T08:16:19Z

Blocking issue in CPS.

JanKrivanek · 2024-02-08T13:50:54Z

Superseded by #9721

Make FNV hash compatible across endianness

89e6be8

JanKrivanek requested a review from ladipro December 4, 2023 11:46

JanKrivanek changed the base branch from main to vs17.9 December 4, 2023 13:13

ladipro approved these changes Dec 5, 2023

View reviewed changes

rokonec assigned JanKrivanek Dec 6, 2023

rokonec self-requested a review December 6, 2023 13:43

JanKrivanek added the do-not-merge label Dec 11, 2023

Forgind approved these changes Dec 23, 2023

View reviewed changes

JanKrivanek changed the base branch from vs17.9 to main January 4, 2024 17:15

JanKrivanek mentioned this pull request Feb 8, 2024

Reintroduce FNV hashing #9721

Merged

JanKrivanek closed this Feb 8, 2024

Make FNV hash compatible across endianness #9489

Make FNV hash compatible across endianness #9489

Uh oh!

Conversation

JanKrivanek commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Perf testing

Uh oh!

JanKrivanek commented Dec 4, 2023

Uh oh!

rainersigwald commented Dec 4, 2023

Uh oh!

JanKrivanek commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Forgind commented Dec 22, 2023

Uh oh!

JanKrivanek commented Dec 22, 2023

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Forgind commented Dec 23, 2023

Uh oh!

KirillOsenkov commented Jan 9, 2024

Uh oh!

JanKrivanek commented Jan 9, 2024

Uh oh!

rokonec commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JanKrivanek commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JanKrivanek commented Dec 4, 2023 •

edited

Loading

JanKrivanek commented Dec 4, 2023 •

edited

Loading

rokonec commented Jan 12, 2024 •

edited

Loading