Performance hit by commit 'Use UnitConverter to do conversion functions (e5aba5b)'

[giantmustache]

Describe the bug
Since release 4.117 we are experiencing some serious performance issues. I have been able to trace them back in the build logs of UnitsNet. This commit logs a warning about performance with a ratio of 40x: e5aba5b

I have also noticed that the build of 4.121 still has this issue: https://github.com/angularsen/UnitsNet/runs/5207495227?check_suite_focus=true#step:7:2963

I am really noticing this slowdown in my project and I was wondering what the cause could be and how we could resolve it.

[tmilnthorp]

See #1049

I have not upgraded in my projects yet, but I think this will bite me too. I use UnitsNet in code that is run on the UI thread to do real time conversions.

[mmano]

I experiment v5-alpha5 but I've also noticed some unexpected performances issues for the purpose. As detailed in #1049, the conversion functors make use of interface (IQuantity), leading to a few dictionary lookup, and boxing issues. In my point of view, if one wants to statically use the units, one should not unexpectedly inherits from the dynamic implementation ... because that ruins all the benefits of a struct-based design.

[angularsen]

There is a conflict between performance and extensibility. I'm not sure I see a way to support extensibility without sacrificing some performance at least, but am open to ideas.

Some options:

• Don't let extensibility override built-in functions (try built-in first for performance), as proposed by ⚡ Prefer built-in conversions over extensibility for perf #1049. I can roll with that, since I'm not sure I see a good usecase to override built-in functions except to duct-tape fix bugs, but I would rather merge those fixes into the library.
• Use generics instead of boxing with IQuantity (not sure how much effect this has on performance). Some details here ⚡ Prefer built-in conversions over extensibility for perf #1049 (comment).

[oysteinkrog]

I am also seeing a lot of boxing allocations due to this our large application (Swing/Motion Catalyst) I am in favor of merging #1049.

[angularsen]

I merged #1049 . It should fix the performance regression, but I can't immediately recognize that it helps all that much with allocations @oysteinkrog .

Are there any particular hotspots for allocations?

[angularsen]

Release UnitsNet/4.142.0 · angularsen/UnitsNet

[oysteinkrog]

I merged #1049 . It should fix the performance regression, but I can't immediately recognize that it helps all that much with allocations @oysteinkrog .

Are there any particular hotspots for allocations?

Yes.
The boxing allocations I saw during memory profiling (with dotMemory) was from UnitConverter.SetConversionFunction().TypelessConversionFunction():

It is the casting of the struct unit types to the IQuantity interface that was causing the boxing allocations in my profiling.

Unfortunately I see now that the changes just merged still use ToUnit(), which in turn use TryGetConversionFunction() so I think it will still hit the same problem.
I started working on some fixes for this, but it gets hairy pretty quickly if you want to completely avoid boxing.
I ended up having to use multiple ConversionFunction instances inside UnitConverter, one for the default typeless/boxing conversion, and then two for double/decimal-based unit types.
This is not an elegant solution at all, I was prototyping to see if it would be possible ;)

I'm going on vacation for 4-5 weeks now so I can't work on this any more right now, but my broken branch is here:
https://github.com/oysteinkrog/UnitsNet/commits/wip