Skip to content

Optimize XxHash3 on ARM platform#77881

Merged
stephentoub merged 3 commits intodotnet:mainfrom
xoofx:optimize-XxHash3-ARM
Nov 5, 2022
Merged

Optimize XxHash3 on ARM platform#77881
stephentoub merged 3 commits intodotnet:mainfrom
xoofx:optimize-XxHash3-ARM

Conversation

@xoofx
Copy link
Member

@xoofx xoofx commented Nov 4, 2022

Hey there,
This PR Fixes ARM performance for XxHash3, followup of #77756

It is roughly 2.5x times faster than the previous version and 15% faster than the C++ version.

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.755)
Snapdragon Compute Platform, 1 CPU, 8 logical and 8 physical cores
.NET SDK=7.0.100-rc.2.22477.23
  [Host]     : .NET 7.0.0 (7.0.22.47203), Arm64 RyuJIT AdvSIMD
  DefaultJob : .NET 7.0.0 (7.0.22.47203), Arm64 RyuJIT AdvSIMD


|           Method |          data |       Mean |    Error |   StdDev | Ratio |
|----------------- |-------------- |-----------:|---------:|---------:|------:|
|       XXH3Native | Byte[1048576] |  70.01 us  | 0.051 us | 0.045 us |  0.45 |
|             XXH3 | Byte[1048576] |  156.05 us | 0.089 us | 0.074 us |  1.0  |
| XXH3ARMOptimized | Byte[1048576] |  61.49 us  | 0.068 us | 0.064 us |  0.39 |

cc: @stephentoub, @EgorBo

@ghost ghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Nov 4, 2022
@ghost
Copy link

ghost commented Nov 4, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Hey there,
This PR Fixes ARM performance for XxHash3, followup of #77756

It is roughly 2.5x times faster than the previous version and 15% faster than the C++ version.

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.755)
Snapdragon Compute Platform, 1 CPU, 8 logical and 8 physical cores
.NET SDK=7.0.100-rc.2.22477.23
  [Host]     : .NET 7.0.0 (7.0.22.47203), Arm64 RyuJIT AdvSIMD
  DefaultJob : .NET 7.0.0 (7.0.22.47203), Arm64 RyuJIT AdvSIMD


|           Method |          data |       Mean |    Error |   StdDev | Ratio |
|----------------- |-------------- |-----------:|---------:|---------:|------:|
|       XXH3Native | Byte[1048576] |  70.01 us  | 0.051 us | 0.045 us |  0.45 |
|             XXH3 | Byte[1048576] |  156.05 us | 0.089 us | 0.074 us |  1.0  |
| XXH3ARMOptimized | Byte[1048576] |  61.49 us  | 0.068 us | 0.064 us |  0.39 |

cc: @stephentoub, @EgorBo

Author: xoofx
Assignees: -
Labels:

area-System.IO, community-contribution
Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Nov 4, 2022 •
edited
Loading

Nice! 🙂

15% faster than the C++ version.

🚀

EgorBo
EgorBo reviewed Nov 4, 2022
View reviewed changes
EgorBo
EgorBo reviewed Nov 4, 2022
View reviewed changes
EgorBo
EgorBo reviewed Nov 4, 2022
View reviewed changes
stephentoub
stephentoub reviewed Nov 4, 2022
View reviewed changes
stephentoub
stephentoub approved these changes Nov 4, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@xoofx @stephentoub
Update src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
1a310cc 
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@xoofx xoofx mentioned this pull request Nov 5, 2022
Merged
@stephentoub stephentoub merged commit ec26662 into dotnet:main Nov 5, 2022
@stephentoub
Copy link
Member

stephentoub commented Nov 5, 2022

Thanks

xtqqczze
xtqqczze reviewed Nov 5, 2022
View reviewed changes
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
@@ -896,16 +897,31 @@ private static Vector128<ulong> Accumulate128(Vector128<ulong> accVec, byte* sou
Vector128<uint> sourceKey = sourceVec ^ secret;

// TODO: Figure out how to unwind this shuffle and just use Vector128.Multiply
Copy link
Contributor

@xtqqczze xtqqczze Nov 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still relevant, or does it now refer to code in MultiplyWideningLower?

@adamsitnik adamsitnik added this to the 8.0.0 milestone Nov 7, 2022
@adamsitnik adamsitnik added the tenet-performance Performance related issue label Nov 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBo EgorBo EgorBo left review comments

@stephentoub stephentoub stephentoub approved these changes

+1 more reviewer

@xtqqczze xtqqczze xtqqczze left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.IO community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@xoofx @EgorBo @stephentoub @xtqqczze @adamsitnik