Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace the internal sha512 implementation with .Net built in sha512 #390

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Replace the internal sha512 implementation with .Net built in sha512 #390

merged 3 commits into from
Feb 20, 2024

Conversation

niklasfp
Copy link
Contributor

This pr replaces the internal implementation of Sha512 to use the Sha512 in the .Net framwork.

It also removes the files that were there only to support the internal Sha512 implementation.

The benefits are:

  • Simplification by removing code not needed to be maintained
  • Easier path to e.g. remove allocations in Nkey because of the methods that supports spans etc.
  • Less allocations
  • Speed

Removed methods

This pr also removes some methods from the Sha512 internal wrapper class, they were all unused internal methods.

public void Update(ArraySegment<byte> data)
public void Finalize(ArraySegment<byte> output)
public static byte[] Hash(byte[] data)

Micro benchmarks

BenchmarkDotNet v0.13.12, macOS Sonoma 14.3.1 (23D60) [Darwin 23.3.0]
Apple M2 Pro, 1 CPU, 12 logical and 12 physical cores
.NET SDK 8.0.101
  [Host]   : .NET 8.0.1 (8.0.123.58001), Arm64 RyuJIT AdvSIMD
  ShortRun : .NET 8.0.1 (8.0.123.58001), Arm64 RyuJIT AdvSIMD

Job=ShortRun  IterationCount=3  LaunchCount=1  
WarmupCount=3  

Main
| Method     | Iter | Mean     | Error   | StdDev  | Gen0     | Allocated |
|----------- |----- |---------:|--------:|--------:|---------:|----------:|
| NKeyCreate | 5000 | 142.9 ms | 5.23 ms | 0.29 ms | 500.0000 |   4.08 MB |

This pr
| Method     | Iter | Mean     | Error   | StdDev  | Gen0     | Allocated |
|----------- |----- |---------:|--------:|--------:|---------:|----------:|
| NKeyCreate | 5000 | 142.8 ms | 2.23 ms | 0.12 ms | 250.0000 |    2.9 MB |
 
Main 
| Method     | Iter | DataSize | Mean      | Error    | StdDev   | Gen0     | Allocated |
|----------- |----- |--------- |----------:|---------:|---------:|---------:|----------:|
| Sha512Hash | 5000 | 1024     |  17.95 ms | 0.346 ms | 0.019 ms | 187.5000 |    1.6 MB |
| Sha512Hash | 5000 | 25600    | 396.04 ms | 4.489 ms | 0.246 ms |        - |    1.6 MB |

This pr
| Method     | Iter | DataSize | Mean      | Error     | StdDev    | Gen0    | Allocated |
|----------- |----- |--------- |----------:|----------:|----------:|--------:|----------:|
| Sha512Hash | 5000 | 1024     |  4.991 ms | 0.3914 ms | 0.0215 ms | 46.8750 | 429.69 KB |
| Sha512Hash | 5000 | 25600    | 84.237 ms | 1.3626 ms | 0.0747 ms |       - | 429.81 KB |

@niklasfp
Copy link
Contributor Author

@mtmk is this a flapper: https://github.com/nats-io/nats.net.v2/actions/runs/7855013069/job/21436316896?pr=390#step:7:302

It's the Slow_consumer test, and seems to work fine locally.

@mtmk
Copy link
Collaborator

mtmk commented Feb 10, 2024

@mtmk is this a flapper: https://github.com/nats-io/nats.net.v2/actions/runs/7855013069/job/21436316896?pr=390#step:7:302

It's the Slow_consumer test, and seems to work fine locally.

yes. we have #344 to take care of them soon.

/// <summary>
/// Allocation and initialization of the new SHA-512 object.
/// </summary>
public Sha512()
{
_buffer = new byte[BlockSize];//todo: remove allocation
Init();
_sha512Inner = SHA512.Create();
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this disposable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this disposable?

Yes it is, doh!! I'll add disposable on the wrapper and make sure we dispose it where appropriate
Sorry about that, should have caught this myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtmk Added IDisposable and usings where needed.

Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM thanks @niklasfp 💯

@mtmk mtmk added this to the 2.1.1 milestone Feb 15, 2024
Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit b68542e into nats-io:main Feb 20, 2024
10 checks passed
@niklasfp niklasfp deleted the replace-internal-sha512-implementation branch February 20, 2024 14:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants