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

[fixes] UInt160 Class #3422

Merged
merged 24 commits into from
Aug 29, 2024
Merged

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Jul 18, 2024

Change Log

  • Fixed GetHashCode method
  • Added implicit string to UInt160
  • Added implicit byte[] to UInt160
  • Fixed accuracy for TryParse methods test cases
  • Added more test cases for new features

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (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)
  • This change requires a documentation update

How Has This Been Tested?

Unit Tests Locally

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
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Jim8y
Jim8y previously approved these changes Jul 19, 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.

In general I think that this PR is slower than the previous version, and it doesn't fix nothing.

src/Neo/UInt160.cs Outdated Show resolved Hide resolved
@cschuchardt88
Copy link
Member Author

In general I think that this PR is slower than the previous version, and it doesn't fix nothing.

What makes you think this?

Using Unsafe.As is same in method BinaryPrimitives.ReadInt32LittleEndian as well, the different is now it can run on BigEndian machines.

Plus I

  • Added implicit string to UInt160
  • Added implicit byte[] to UInt160

So you can do UInt160.Zero == "0x0000000000000000000000000000000000000000" or UInt160 hash = "0xff00000000000000000000000000000000000001" or UInt160 hash = new byte[20];

Fixed collision with GetHashCode which is a bug.

Just read 1st post

@Jim8y
Copy link
Contributor

Jim8y commented Jul 23, 2024

I will do a benchmark for you chris, wait me for one day.

@Jim8y
Copy link
Contributor

Jim8y commented Aug 1, 2024

  1. Generator1:

    Method Mean Error StdDev
    TestOldUInt160Gernerator1 0.0020 ns 0.0006 ns 0.0005 ns
    TestGernerator1 0.0000 ns 0.0000 ns 0.0000 ns
  2. Generator2:

    Method Mean Error StdDev
    TestOldUInt160Gernerator2 6.9133 ns 0.0236 ns 0.0185 ns
    TestGernerator2 12.2943 ns 0.0358 ns 0.0318 ns
  3. CompareTo:

    Method Mean Error StdDev
    TestOldUInt160CompareTo 13.1190 ns 0.0148 ns 0.0124 ns
    TestCompareTo 16.0586 ns 0.0244 ns 0.0204 ns
  4. Equals:

    Method Mean Error StdDev
    TestOldUInt160Equals 6.9993 ns 0.0133 ns 0.0125 ns
    TestEquals 14.4972 ns 0.0230 ns 0.0192 ns
  5. Parse:

    Method Mean Error StdDev
    TestOldUInt160Parse 379.4385 ns 0.5059 ns 0.4225 ns
    TestParse 627.1156 ns 9.2775 ns 8.2242 ns
  6. TryParse:

    Method Mean Error StdDev
    TestOldUInt160TryParse 421.7860 ns 2.1910 ns 1.9423 ns
    TestTryParse 17062.1403 ns 59.4341 ns 49.6302 ns
  7. OperatorLarger:

    Method Mean Error StdDev
    TestOldUInt160OperatorLarger 1.4663 ns 0.0034 ns 0.0028 ns
    TestOperatorLarger 1.4726 ns 0.0046 ns 0.0041 ns
  8. OperatorLargerAndEqual:

    Method Mean Error StdDev
    TestOldUInt160OperatorLargerAndEqual 1.4642 ns 0.0009 ns 0.0008 ns
    TestOperatorLargerAndEqual 1.4887 ns 0.0199 ns 0.0176 ns
  9. OperatorSmaller:

    Method Mean Error StdDev
    TestOldUInt160OperatorSmaller 1.4639 ns 0.0017 ns 0.0014 ns
    TestOperatorSmaller 1.4830 ns 0.0185 ns 0.0164 ns
  10. OperatorSmallerAndEqual:

    Method Mean Error StdDev
    TestOldUInt160OperatorSmallerAndEqual 1.4653 ns 0.0032 ns 0.0025 ns
    TestOperatorSmallerAndEqual 1.4748 ns 0.0160 ns 0.0150 ns

@Jim8y
Copy link
Contributor

Jim8y commented Aug 1, 2024

@cschuchardt88 @shargon please check the benchmark, some methods become way slower than before.

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.

I think that this pr should not be merged because it was not proved that is faster

@cschuchardt88
Copy link
Member Author

GetHashCode still needs to be fixed. Its so small the difference whos going to see the difference. Plus we don't know what he testing here. CPU, Memory or etc.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Aug 1, 2024

@shargon @Jim8y
I changed anything back but see below.

As you can see everything is the same but TryParse Parse and GetHashCode.

Tests are not accurate

BenchmarkDotNet v0.13.12

  • Windows 11 (10.0.22631.3958/23H2/2023Update/SunValley3)
  • Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores

.NET SDK 8.0.303

  • [Host] : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2
  • DefaultJob : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2
Method Mean Error StdDev Median
TestOldUInt160Gernerator1 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns
TestGernerator1 0.0008 ns 0.0019 ns 0.0017 ns 0.0000 ns
TestOldUInt160Gernerator2 8.3340 ns 0.0470 ns 0.0439 ns 8.3340 ns
TestGernerator2 8.4243 ns 0.0423 ns 0.0396 ns 8.4151 ns
TestOldUInt160CompareTo 5.2123 ns 0.0784 ns 0.0733 ns 5.1946 ns
TestCompareTo 5.1717 ns 0.0329 ns 0.0308 ns 5.1673 ns
TestOldUInt160Equals 0.0002 ns 0.0011 ns 0.0009 ns 0.0000 ns
TestEquals 2.0883 ns 0.0101 ns 0.0094 ns 2.0849 ns
TestOldUInt160Parse 473.5925 ns 2.1056 ns 1.9696 ns 473.6506 ns
TestParse 307.7447 ns 1.0599 ns 0.8850 ns 307.9052 ns
TestOldUInt160TryParse 497.2763 ns 3.7228 ns 3.3002 ns 496.1108 ns
TestTryParse 348.7883 ns 1.8946 ns 1.7723 ns 348.6179 ns
TestOldUInt160OperatorLarger 1.1933 ns 0.0152 ns 0.0127 ns 1.1879 ns
TestOperatorLarger 1.1194 ns 0.0081 ns 0.0068 ns 1.1185 ns
TestOldUInt160OperatorLargerAndEqual 1.1216 ns 0.0113 ns 0.0100 ns 1.1191 ns
TestOperatorLargerAndEqual 1.1198 ns 0.0111 ns 0.0098 ns 1.1172 ns
TestOldUInt160OperatorSmaller 1.1200 ns 0.0094 ns 0.0083 ns 1.1220 ns
TestOperatorSmaller 1.1204 ns 0.0108 ns 0.0101 ns 1.1175 ns
TestOldUInt160OperatorSmallerAndEqual 1.1245 ns 0.0065 ns 0.0061 ns 1.1249 ns
TestOperatorSmallerAndEqual 1.1204 ns 0.0080 ns 0.0071 ns 1.1209 ns

@cschuchardt88 cschuchardt88 changed the title [fix/expand] UInt160 Class [fixes] UInt160 Class Aug 1, 2024
@cschuchardt88 cschuchardt88 added feature Type: Large changes or new features and removed optimization Optmization issues labels Aug 1, 2024
src/Neo/UInt160.cs Show resolved Hide resolved
src/Neo/UInt160.cs Show resolved Hide resolved
src/Neo/UInt160.cs Outdated Show resolved Hide resolved
@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Aug 2, 2024

@Jim8y @shargon @vncoelho @roman-khimov
For hex is should start with 0x not 0X. Should we continue to allow 0X prefix for Parse and TryParse methods? Or will this mess up neogo and contracts along with dev-pack?

@roman-khimov
Copy link
Contributor

I've never seen 0X being used and some of NeoGo code expects only 0x, but at the same time if it was allowed previously then we better just make it compatible and forget about it.

@cschuchardt88
Copy link
Member Author

@shargon check to see if all your changes have been made.

@Jim8y
Copy link
Contributor

Jim8y commented Aug 12, 2024

What we say about this pr, are we going to merge this one? i run the bechmark again, the performance is still slower for some methods.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Aug 23, 2024

What we say about this pr, are we going to merge this one? i run the bechmark again, the performance is still slower for some methods.

  1. A bug fix with GetHashCode.
  2. Makes TryParse faster.
  3. Adds two features.
UInt160 bytes = new byte[20];
UInt160 strs = "0x1230000000000000000000000000000000000000";

@shargon
Copy link
Member

shargon commented Aug 23, 2024

If the performance is not lower, I'm good with the code

@cschuchardt88
Copy link
Member Author

@Jim8y

@shargon shargon merged commit 5fe5462 into neo-project:master Aug 29, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Type: Large changes or new features waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants