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

Increase by x4.8 performance of ToHexString #5009

Merged
merged 3 commits into from Dec 29, 2022

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Dec 14, 2022

Changes:

Is now x4.8 times faster, and Bytes.ByteArrayToHexViaLookup32Safe, Bytes.ToHexString and SpanExtensions.ToHexString all share the same implementation, rather than having 3 different ones

Method ScenarioIndex Mean Error StdDev Ratio RatioSD
Improved 0 45.281 ns 0.3482 ns 0.4993 ns 0.21 0.00
Current 0 215.867 ns 0.6967 ns 0.9991 ns 1.00 0.00
Improved 1 2.206 ns 0.2604 ns 0.3897 ns 0.91 0.17
Current 1 2.417 ns 0.0265 ns 0.0388 ns 1.00 0.00
Improved 2 49.155 ns 0.4488 ns 0.6717 ns 0.23 0.00
Current 2 217.019 ns 0.8983 ns 1.3445 ns 1.00 0.00
Improved 3 47.063 ns 0.8300 ns 1.1903 ns 0.22 0.00
Current 3 217.265 ns 1.1834 ns 1.7346 ns 1.00 0.00

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing , should you have some (optional)

Added some odd/even leading zeros tests

Further comments (optional)

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Would it make sense to optimize ByteArrayToHexViaLookup32Safe in similar way too or it is already optimal? It is used when serializing Address, Bloom, Keccak and byte[]?

At some point in the future I think we should do a little cleanup and remove obsolete implementations.

@benaadams
Copy link
Member Author

Would it make sense to optimize ByteArrayToHexViaLookup32Safe in similar way too or it is already optimal?

Can put in a quick change #5011; and do some more fancy stuff with intrinsics also

@benaadams benaadams changed the title Increase by x2.7 performance of ToHexString Increase by x4.8 performance of ToHexString Dec 15, 2022
@benaadams
Copy link
Member Author

Is now x4.8 times faster, and Bytes.ByteArrayToHexViaLookup32Safe, Bytes.ToHexString and SpanExtensions.ToHexString all share the same implementation, rather than having 3 different ones

Method ScenarioIndex Mean Error StdDev Ratio RatioSD
Improved 0 45.281 ns 0.3482 ns 0.4993 ns 0.21 0.00
Current 0 215.867 ns 0.6967 ns 0.9991 ns 1.00 0.00
Improved 1 2.206 ns 0.2604 ns 0.3897 ns 0.91 0.17
Current 1 2.417 ns 0.0265 ns 0.0388 ns 1.00 0.00
Improved 2 49.155 ns 0.4488 ns 0.6717 ns 0.23 0.00
Current 2 217.019 ns 0.8983 ns 1.3445 ns 1.00 0.00
Improved 3 47.063 ns 0.8300 ns 1.1903 ns 0.22 0.00
Current 3 217.265 ns 1.1834 ns 1.7346 ns 1.00 0.00

@benaadams
Copy link
Member Author

SafeLookup is improved x1.12 so it hasn't regressed from sharing the method

Method Mean Error StdDev Ratio RatioSD
Improved 45.74 ns 0.717 ns 1.051 ns 0.89 0.03
SafeLookup 51.40 ns 0.774 ns 1.159 ns 1.00 0.04

@LukaszRozmej
Copy link
Member

.NET 7 is now in master branch

@benaadams
Copy link
Member Author

Added the vectorized path which improves the ByteArrayToHexToHexBenchmark by x1.5

|     Method |     Mean |    Error |   StdDev | Ratio | RatioSD | Alloc Ratio |
|----------- |---------:|---------:|---------:|------:|--------:|------------:|
|   Improved | 39.96 ns | 1.264 ns | 1.892 ns |  0.89 |    0.07 |        1.00 |
| SafeLookup | 59.80 ns | 0.698 ns | 1.001 ns |  1.33 |    0.06 |        1.00 |
|   HexMateA | 45.22 ns | 1.481 ns | 2.217 ns |  1.00 |    0.00 |        1.00 |

Copy link
Contributor

@asdacap asdacap left a comment

Choose a reason for hiding this comment

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

I had to write notes to review this, LOL. This is beyond my paygrade 😄 .

@LukaszRozmej LukaszRozmej merged commit c6dce52 into NethermindEth:master Dec 29, 2022
@tkstanczak
Copy link
Member

thank you @benaadams 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants