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

Base58 perfomance patch #3186

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

kzorin52
Copy link

Description

Various Base58 performance improvements:

Decode():

  1. Replaced slow IndexOf() to Base58 character map
  2. Replaced .TakeWhile(...).Count() to efficient variant, without memory and cpu-time loss
  3. Replaced byte[] Concat() to single Span<byte>
  4. Fixed: zeros byte array creation only if decoded is zero

Type of change

  • Bug fix (non-breaking change)

How Has This Been Tested?

  • UT_Base58.TestEncodeDecode
  • UT_StdLib.TestBinary

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas - no hard-to-understand areas
  • I have made corresponding changes to the documentation - public API do not changed
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works - benchmarks only
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules - no dependent changes

P.S. It is my first time opening a pull request, please correct me if I've done something wrong.

@Jim8y
Copy link
Contributor

Jim8y commented Mar 28, 2024

@kzorin52 thank you very much for contributing to neo. This pr looks good to me.

@cschuchardt88 @shargon @AnnaShaleva @vncoelho

@@ -74,19 +84,28 @@ public static byte[] Decode(string input)
var bi = BigInteger.Zero;
for (int i = 0; i < input.Length; i++)
{
int digit = Alphabet.IndexOf(input[i]);
if (digit < 0)
var digit = s_mapBase58[(byte)input[i]];
Copy link
Member

Choose a reason for hiding this comment

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

Change to char array and avoid cast?

Copy link
Member

Choose a reason for hiding this comment

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

Char wouldn't work I don't think. Char uses encoding.

private static int LeadingBase58Zeros(string collection)
{
int i = 0;
for (; i < collection.Length && collection[i] == s_zeroChar; i++) { }
Copy link
Member

Choose a reason for hiding this comment

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

Cache length?

src/Neo/Cryptography/Base58.cs Show resolved Hide resolved
@kzorin52
Copy link
Author

@shargon all fixed 🥳

shargon
shargon previously approved these changes Mar 29, 2024
Jim8y
Jim8y previously approved these changes Mar 29, 2024
vncoelho
vncoelho previously approved these changes Mar 29, 2024
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I tested here and it is working basically.
However, no UTs nor benchmark.

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

This needs to optimized with benchmarks data to show that it indeed faster or we can't merge to repo.

This change is not important.

@@ -27,6 +28,9 @@ public static class Base58
/// </summary>
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));
Copy link
Member

Choose a reason for hiding this comment

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

This cost more in the current code in resources of memory management for the GC in dotnet.

src/Neo/Cryptography/Base58.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Base58.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Base58.cs Outdated Show resolved Hide resolved
@kzorin52
Copy link
Author

@cschuchardt88 Okay, I'll do benchmarks and profiling of the code

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

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.

@kzorin52
Copy link
Author

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.

@shargon
Copy link
Member

shargon commented Oct 17, 2024

@cschuchardt88

/* Unmerged change from project 'Neo(net8.0)'
Before:
            -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
            -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
            -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 2, 3, 4, 5, 6,
            7, 8, -1, -1, -1, -1, -1, -1, -1, 9, 10, 11, 12, 13, 14, 15, 16, -1, 17,
            18, 19, 20, 21, -1, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, -1, -1,
            -1, -1, -1, -1, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, -1, 44, 45,
            46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, -1, -1, -1, -1, -1,
After:
            -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
            -1,
            -1,
            -1,
            -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
            -1,
            -1,
            -1,
            -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
            0,
            1,
            2,
            3,
            4,
            5,
            6,
            7,
            8,
            -1, -1, -1, -1,
            -1,
            -1,
            -1,
            9,
            10,
            11,
            12,
            13,
            14,
            15,
            16,
            -1,
            17,
            18,
            19,
            20,
            21,
            -1,
            22,
            23,
            24,
            25,
            26,
            27,
            28,
            29,
            30,
            31,
            32,
            -1,
            -1,
            -1,
            -1,
            -1,
            -1,
            33,
            34,
            35,
            36,
            37,
            38,
            39,
            40,
            41,
            42,
            43,
            -1,
            44,
            45,
            46,
            47,
            48,
            49,
            50,
            51,
            52,
            53,
            54,
            55,
            56,
            57,
            -1, -1,
            -1,
            -1,
            -1,
*/

We have something wrong in the format

@cschuchardt88
Copy link
Member

@shargon there is no fix. Its a bug in C# 12.0. If it isnt on a single line, it will format on newline for each element in the array.

Workaround is to wrap

#pragma warning disable format // add this line

        private static readonly int[] s_ints =
        [
            0, 1, 2, 3, 4
        ];

#pragma warning restore format // add this line

@kzorin52
Copy link
Author

@cschuchardt88 hi! like this?

cschuchardt88
cschuchardt88 previously approved these changes Oct 18, 2024
Jim8y
Jim8y previously approved these changes Nov 6, 2024
@shargon shargon dismissed stale reviews from cschuchardt88 and Jim8y via 8c2c577 November 6, 2024 12:39
shargon
shargon previously approved these changes Nov 6, 2024
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

It should be good if we hit in tests all the array values

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.

7 participants