Skip to content

Changing extension methods to use generics. Allows for broader use wh… #485

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

Merged

Conversation

tmilnthorp
Copy link
Collaborator

Changing extension methods to use generics. Allows for broader use wherever Convert.ToDouble can be used, and also reduces library size significantly.

Work towards reducing library size #372.

This has decreased the size from 1451 KB to 994 KB.

…erever Convert.ToDouble can be used, and also reduces library size significantly.
@angularsen
Copy link
Owner

angularsen commented Sep 26, 2018

Very nice! Can double that by removing nullables in a breaking change, and even more by moving all of it out to separate nuget.

@jnyrup
Copy link
Contributor

jnyrup commented Nov 1, 2018

Are you aware that this change introduces boxing?
Convert.ToDouble now resolves to Convert.ToDouble(object) which boxes T value into an object.

Here's a very raw benchmark which shows that:

  • verifies that Convert.ToDouble(object) boxes/unboxes T value and hence the 24 byte allocation on a x64 architecture
  • by using T : IConvertible this can be made 10 times faster and be allocation free again.
[MemoryDiagnoser]
public class ConverterBenchmarks
{
    [Params(1)]
    public int Value { get; set; }

    [Benchmark(Baseline = true)]
    public double ConvertToDouble() => Value;

    [Benchmark]
    public double ConvertToDoubleOfT() => ConvertToDoubleOfT(Value);

    private static double ConvertToDoubleOfT<T>(T value) => Convert.ToDouble(value);

    [Benchmark]
    public double GenericIConvertibleToDouble() => GenericIConvertibleToDouble(Value);

    private static double GenericIConvertibleToDouble<T>(T value)
        where T : IConvertible
    {
        return value.ToDouble(CultureInfo.InvariantCulture);
    }

    [Benchmark]
    public double IConvertibleToDouble() => IConvertibleToDouble(Value);

    private static double IConvertibleToDouble(IConvertible value) => value.ToDouble(CultureInfo.InvariantCulture);
}
BenchmarkDotNet=v0.11.2, OS=Windows 10.0.17134.345 (1803/April2018Update/Redstone4)
Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
Frequency=2531250 Hz, Resolution=395.0617 ns, Timer=TSC
  [Host]     : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3190.0
  DefaultJob : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3190.0


                      Method | Value |      Mean |     Error |    StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
---------------------------- |------ |----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
          ConvertToDoubleOfT |     1 | 9.2419 ns | 0.2244 ns | 0.3811 ns |  1.00 |    0.00 |      0.0076 |           - |           - |                24 B |
 GenericIConvertibleToDouble |     1 | 0.8586 ns | 0.0550 ns | 0.0514 ns |  0.09 |    0.01 |           - |           - |           - |                   - |
        IConvertibleToDouble |     1 | 5.1089 ns | 0.1788 ns | 0.1672 ns |  0.55 |    0.03 |      0.0076 |           - |           - |                24 B |

@tmilnthorp
Copy link
Collaborator Author

Yes, but we target .NET Standard 1.0 for v3.x, which does not support IConvertible.

These methods are all deprecated and removed in v4.

@angularsen worth reverting for v3.x? Size vs performance.

@angularsen
Copy link
Owner

angularsen commented Nov 3, 2018

@jnyrup Awesome benchmark and thanks for pointing this out! I learned something today.

As @tmilnthorp very well points out, we can't easily fix this in v3 due to netstandard1.0 support and I think I would prefer smaller binary size over unnecessary boxing allocations for number extension methods. Those methods are only syntactic sugar really, and not something you would do in an intensive for-loop if performance matters to you. I personally wouldn't use UnitsNet at all if performance was the focus in a piece of code. We even removed those extension methods entirely in v4 because they only add syntactic sugar value and they greatly increased the binary size of the library (we have 800+ units).

TL;DR I prefer to keep the lower binary size over boxing overhead for number extension methods in v3. For v4, this discussion is moot. If you can see a way to easily fix/improve this for v3, though, I'm more than happy to receive a PR on it!

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.

3 participants