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

Removed UIntBase to avoid expensive virtual calls for base data type classes #206

Closed
wants to merge 5 commits into from
Closed

Removed UIntBase to avoid expensive virtual calls for base data type classes #206

wants to merge 5 commits into from

Conversation

osmirnov
Copy link
Contributor

Calls through virtual tables are quite expensive for such heavily used classes as base data types. Done:

  • Removed UIntBase and pushed shared methods implementation to individual class level (UInt160, UInt256)
  • Size is immutable now
  • Construction from array uses faster Array.Copy method instead LINQ method to ToArray
  • Equals methods and comparison operators were optimized by dropping duplicated if checks
  • CompareTo uses builtin comparison for arrays instead of looping now
  • ToArray returns always a new array to avoid direct access to internal one
  • Parse/TryParse methods were delegated to Helper.HexToBytes method
  • Unused code that relies on UIntBase and casts it on fly was adjusted
  • Tests for UInt160/256 were added (100% coverage)

@@ -7,13 +7,15 @@ namespace Neo.Wallets
{
public class AssetDescriptor
{
public UIntBase AssetId;
public object AssetId;
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep UIntBase but change everything to non-virtual? Or just change it to an interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can keep UIntBase or introduce IUInt interface but only shared contract between UInt160 and UInt256 is ToArray method. The rest methods are either type specific or covered by other interfaces (IEquatable, IComparable, ISerializable). I see the consumer of UIntBase/IUInt always has to cast it the underlying type to work with.

For AssetDescriptor class (and for TransferOutput class) we can come up with more descriptive design. The idea here is to move casting logic outside of the class.

Option #1:

    public class AssetDescriptor<TUInt>
    {
        public TUInt AssetId;
        public string AssetName;
        public byte Decimals;

        public AssetDescriptor(TUInt asset_id)
        {
                AssetId = asset_id
        }

Option #2:

    public class AssetDescriptor256 : IAssetDescriptor
    {
        public UInt256 AssetId;
        public string AssetName;
        public byte Decimals;

        public AssetDescriptor(UInt256 asset_id)
        {
                AssetId = asset_id
        }

        public byte[] IAssetDescriptor.GetAssestIdAsByteArray()
        {
            return AssetId.ToArray();
        }

I'd like to elaborate more on the subject but I couldn't have found any example AssetDescriptor class usage.

Copy link
Member

Choose a reason for hiding this comment

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

The usage of AssetDescriptor is in neo-gui and neo-cli.

@osmirnov
Copy link
Contributor Author

I reviewed neo-gui and neo-cli projects and reverted UIntBase back to API they use. It was discovered to be inheritance from UIntBase and UIntBase.Parse method. Let me know if I missed any.

@osmirnov osmirnov closed this May 1, 2018
@osmirnov osmirnov deleted the base-types-uint-refactoring branch May 1, 2018 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants