You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Provide [ReadOnly]Span<char> overloads for the methods #211
The overloads currently exposed in the public API are all using string parameters. Since .NET Core, spans have been widely preferred for a variety of reasons involving performance, and are adopted in many cases.
Proposed Solution
Either change all string parameters to ReadOnlySpan<char> parameters, or provide overloads taking ROS<char> parameters, optionally using the OverloadResolutionPriority attribute to enhance their visibility.
For HashPassword, we can also provide overloads accepting the input as ROS<char>, and outputting the value into a user-provided Span<char> buffer:
The current workaround is to manually convert the ROS<char> into a newly allocated string in order to pass it into this library's functions. This is suboptimal for critical performance scenaria.
Additional context
With the 5.0.0 release nearing, this would also be a good breaking change to include. Another release with even more breaking changes would probably not be ideal.
@Rekkonnect its a work in progress as it kicked off splitting the framework from the standard a tad more brutally than the inline changes made the previous time in order to chase the chain through the various methods and to rebuild various benchmarks around the changes.
That's some very good improvement. I'm only not sure about returning a new byte[], we could have a parameter accepting a writable Span to contain the output bytes in. This would be passed by the user and should always accommodate the expected output byte count. That way we avoid one more allocation and it should also be faster.
The purpose of this is to allow passing a reusable output buffer across multiple calls of the function. An overload that allocates the returned byte array is also good to have because users may not want to reuse the result buffer, avoiding the necessary step of allocating the output buffer in calling the API.
public static string HashPassword(ReadOnlySpan<char> inputKey, ReadOnlySpan<char> salt)
{
Span<char> outputBuffer = stackalloc char[60];
HashPassword(inputKey, salt, outputBuffer, out var outputBufferWritten);
return new string(outputBuffer[..outputBufferWritten]);
}
There'll always be an allocation at some point with a string as the benchmarks are testing the same default method signature end to end so they're technically going to be worst case.
Activity
ChrisMcKee commentedon May 7, 2025
@Rekkonnect its a work in progress as it kicked off splitting the framework from the standard a tad more brutally than the inline changes made the previous time in order to chase the chain through the various methods and to rebuild various benchmarks around the changes.
https://github.com/BcryptNet/bcrypt.net/tree/ReadonlySpanSupport
Rekkonnect commentedon May 7, 2025
That's some very good improvement. I'm only not sure about returning a new byte[], we could have a parameter accepting a writable Span to contain the output bytes in. This would be passed by the user and should always accommodate the expected output byte count. That way we avoid one more allocation and it should also be faster.
The purpose of this is to allow passing a reusable output buffer across multiple calls of the function. An overload that allocates the returned byte array is also good to have because users may not want to reuse the result buffer, avoiding the necessary step of allocating the output buffer in calling the API.
ChrisMcKee commentedon May 7, 2025
@Rekkonnect The base https://github.com/BcryptNet/bcrypt.net/blob/ReadonlySpanSupport/src/BCrypt.Net/BCryptBase.cs#L297 takes an ouput buffer. I've tried to keep the api relatively low effort, hence the overload can just return string as normal.
and
There'll always be an allocation at some point with a string as the benchmarks are testing the same default method signature end to end so they're technically going to be worst case.
Rekkonnect commentedon May 7, 2025
Right I missed that. It's all good then
danielcrenna commentedon Sep 6, 2025
There are other reasons to use
Span<T>, such as compatibility withCryptographicOperations.ZeroMemoryandCryptographicOperations.FixedTimeEquals.Is there a timeline for this?
ChrisMcKee commentedon Sep 7, 2025
Soon. I got slightly distracted with benchmark.net and loading different package versions in.