Base58 perfomance patch#3186
Conversation
|
@kzorin52 thank you very much for contributing to neo. This pr looks good to me. |
src/Neo/Cryptography/Base58.cs
Outdated
| { | ||
| int digit = Alphabet.IndexOf(input[i]); | ||
| if (digit < 0) | ||
| var digit = s_mapBase58[(byte)input[i]]; |
There was a problem hiding this comment.
Change to char array and avoid cast?
There was a problem hiding this comment.
Char wouldn't work I don't think. Char uses encoding.
src/Neo/Cryptography/Base58.cs
Outdated
| private static int LeadingBase58Zeros(string collection) | ||
| { | ||
| int i = 0; | ||
| for (; i < collection.Length && collection[i] == s_zeroChar; i++) { } |
|
@shargon all fixed 🥳 |
vncoelho
left a comment
There was a problem hiding this comment.
I tested here and it is working basically.
However, no UTs nor benchmark.
src/Neo/Cryptography/Base58.cs
Outdated
| public const string Alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; | ||
|
|
||
| private static readonly char s_zeroChar = Alphabet[0]; | ||
| private static readonly Lazy<IReadOnlyDictionary<char, int>> s_alphabetDic = new(() => Enumerable.Range(0, Alphabet.Length).ToDictionary(t => Alphabet[t], t => t)); |
There was a problem hiding this comment.
This cost more in the current code in resources of memory management for the GC in dotnet.
|
@cschuchardt88 Okay, I'll do benchmarks and profiling of the code |
AnnaShaleva
left a comment
There was a problem hiding this comment.
The optimisation itself is legit, the compatibility is preserved, but you're right in that we need some benchmarks. If possible, then for multiple architectures.
|
I'm sorry, I've been really busy lately. As soon as I have free time, I will be able to make the planned benchmarks. |
|
@cschuchardt88 hi! like this? |
shargon
left a comment
There was a problem hiding this comment.
It should be good if we hit in tests all the array values
Description
Various Base58 performance improvements:
Decode():IndexOf()to Base58 character map.TakeWhile(...).Count()to efficient variant, without memory and cpu-time lossbyte[] Concat()to singleSpan<byte>Type of change
How Has This Been Tested?
Checklist:
P.S. It is my first time opening a pull request, please correct me if I've done something wrong.