Enum boxing optimizations (UnitKey) #1507
Conversation
- introduce the UnitKey struct as a replacement for the conversions from TUnit to Enum - re-map the UnitAbbreviationCache using the UnitKey for the concurrent dictionary - remove all Convert.ToInt32(..) calls - optimize the QuantityInfoLookup collections - refactored some of the Quantity/UnitConverter code (using the UnitParser/UnitAbbreviationsCache) - added some tests and benchmarks covering the new modifications
lipchev
changed the title
UnitsNet.Benchmark/Conversions/FromString/QuantityFromStringBenchmarks.cs
Outdated
Show resolved
Hide resolved
UnitsNet.Benchmark/Conversions/FromString/QuantityFromUnitAbbreviationBenchmarks.cs
Outdated
Show resolved
Hide resolved
UnitsNet.Benchmark/Conversions/FromString/QuantityFromUnitNameBenchmarks.cs
Outdated
Show resolved
Hide resolved
UnitsNet.Benchmark/Conversions/ToString/ToStringWithDefaultPrecisionBenchmarks.cs
Outdated
Show resolved
Hide resolved
UnitsNet/CustomCode/Quantity.cs
Outdated
| public static QuantityInfo[] Infos => Default.Infos; | ||
| public static IReadOnlyCollection<QuantityInfo> Infos => Quantities.Infos; |
There was a problem hiding this comment.
This is the second breaking change: same deal- can't have users setting the values of the array.
I would also like to make Quantity.ByName dictionary read-only:
/// <summary>
/// All QuantityInfo instances mapped by quantity name that are present in the <see cref="UnitsNetSetup.Default"/> configuration.
/// </summary>
public static IReadOnlyDictionary<string, QuantityInfo> ByName => Quantities.ByName;The generated part of the Quantity class should only contain a single collection of QuantityInfo that we should pass to the QuantityInfoLookup. Here's what I have in the other project:
public partial class Quantity
{
/// <summary>
/// Serves as a repository for predefined quantity conversion mappings, facilitating the automatic generation and retrieval of unit conversions in the UnitsNet library.
/// </summary>
internal static class Provider
{
/// <summary>
/// All QuantityInfo instances that are present in UnitsNet by default.
/// </summary>
internal static IReadOnlyCollection<QuantityInfo> DefaultQuantities => new QuantityInfo[]
{
AbsorbedDoseOfIonizingRadiation.Info,
| /// <summary> | ||
| /// Get a list of quantities having the given base dimensions. | ||
| /// </summary> | ||
| /// <param name="quantityInfos">The type of quantity mapping information.</param> | ||
| /// <param name="baseDimensions">The base dimensions to match.</param> | ||
| public static IEnumerable<QuantityInfo> GetQuantitiesWithBaseDimensions(this IEnumerable<QuantityInfo> quantityInfos, | ||
| BaseDimensions baseDimensions) | ||
| { | ||
| if (baseDimensions is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(baseDimensions)); | ||
| } | ||
|
|
||
| return quantityInfos.Where(info => info.BaseDimensions.Equals(baseDimensions)); | ||
| } |
There was a problem hiding this comment.
This extensions doesn't have to be public: it's currently only used from the Quantity class.
| /// Quantity value type, such as <see cref="Length"/> or <see cref="Mass"/>. | ||
| /// Quantity value type, such as <see cref="Length" /> or <see cref="Mass" />. | ||
| /// </summary> | ||
| public Type ValueType { get; } | ||
| public Type QuantityType { get; } | ||
|
|
||
| /// <inheritdoc cref="QuantityType" /> | ||
| [Obsolete("Replaced by the QuantityType property.")] | ||
| [DebuggerBrowsable(DebuggerBrowsableState.Never)] | ||
| public Type ValueType | ||
| { | ||
| get => QuantityType; | ||
| } |
There was a problem hiding this comment.
I hope you don't mind- this has been bugging me for ages...
There was a problem hiding this comment.
Agreed, but why not just remove it since this is targeting v6?
We could deprecate in v5 instead and offer a similar change there.
| [Obsolete("Methods accepting a culture name are deprecated in favor of using an instance of the IFormatProvider.")] | ||
| public static double ConvertByAbbreviation(double fromValue, string quantityName, string fromUnitAbbrev, string toUnitAbbrev, string? culture) | ||
| { | ||
| if (!TryGetUnitType(quantityName, out Type? unitType)) | ||
| throw new UnitNotFoundException($"The unit type for the given quantity was not found: {quantityName}"); | ||
|
|
||
| var cultureInfo = CultureHelper.GetCultureOrInvariant(culture); | ||
|
|
||
| var fromUnit = UnitsNetSetup.Default.UnitParser.Parse(fromUnitAbbrev, unitType, cultureInfo); // ex: ("m", LengthUnit) => LengthUnit.Meter | ||
| var fromQuantity = Quantity.From(fromValue, fromUnit); | ||
| return ConvertByAbbreviation(fromValue, quantityName, fromUnitAbbrev, toUnitAbbrev, CultureHelper.GetCultureOrInvariant(culture)); | ||
| } | ||
|
|
||
| var toUnit = UnitsNetSetup.Default.UnitParser.Parse(toUnitAbbrev, unitType, cultureInfo); // ex:("cm", LengthUnit) => LengthUnit.Centimeter | ||
| return fromQuantity.As(toUnit); | ||
| /// <summary> | ||
| /// Convert between any two quantity units by their abbreviations, such as converting a "Length" of N "m" to "cm". | ||
| /// This is particularly useful for creating things like a generated unit conversion UI, | ||
| /// where you list some selectors: | ||
| /// a) Quantity: Length, Mass, Force etc. | ||
| /// b) From unit: Meter, Centimeter etc if Length is selected | ||
| /// c) To unit: Meter, Centimeter etc if Length is selected | ||
| /// </summary> | ||
| /// <param name="fromValue"> | ||
| /// Input value, which together with <paramref name="fromUnitAbbrev" /> represents the quantity to | ||
| /// convert from. | ||
| /// </param> | ||
| /// <param name="quantityName">The invariant quantity name, such as "Length". Does not support localization.</param> | ||
| /// <param name="fromUnitAbbrev">The abbreviation of the unit in the given culture, such as "m".</param> | ||
| /// <param name="toUnitAbbrev">The abbreviation of the unit in the given culture, such as "m".</param> | ||
| /// <param name="formatProvider"> | ||
| /// The format provider to use for lookup. Defaults to <see cref="System.Globalization.CultureInfo.CurrentCulture" /> | ||
| /// if null. | ||
| /// </param> | ||
| /// <example>double centimeters = ConvertByName(5, "Length", "m", "cm"); // 500</example> | ||
| /// <returns>Output value as the result of converting to <paramref name="toUnitAbbrev" />.</returns> | ||
| /// <exception cref="QuantityNotFoundException"> | ||
| /// Thrown when no quantity information is found for the specified quantity name. | ||
| /// </exception> | ||
| /// <exception cref="UnitNotFoundException">No units match the abbreviation.</exception> | ||
| /// <exception cref="AmbiguousUnitParseException">More than one unit matches the abbreviation.</exception> | ||
| public static double ConvertByAbbreviation(double fromValue, string quantityName, string fromUnitAbbrev, string toUnitAbbrev, IFormatProvider? formatProvider) |
There was a problem hiding this comment.
I've added an IFormatProvider? overload and deprecated the whole CultureHelper class:
string -> CultureInfo conversions are not in the scope of UnitsNet
If you want to- we could also make this for v5 and have the string? overload directly removed in v6.
There was a problem hiding this comment.
Oh I think I see what the deal is- the UnitParser doesn't really care about the IFormat part of the IFormatProvider, just the CultureName.. I wonder if there is a more standard way of dealing with things that depend solely on the language / region (and not that number-formatting stuff).
There was a problem hiding this comment.
Given that we ultimately depend on the ResourceManager, and it takes a CultureInfo? as it's second parameter, I think the right way (another breaking change) would be to make everything that doesn't involve formatting the number (i.e. everything in the UnitParser, UnitAbbreviationsCache, as well as this new overload that I just added) use a CultureInfo? (with the rest of the code safe-casting the IFormatProvider?)..
There was a problem hiding this comment.
I was about to comment on the same thing reading the above obsoletion of string-based culture names.
I'm not sure how I feel about string vs IFormatProvider, since we don't really use the culture formatting for anything, we just rely on the culture name.
If we end up just getting cached CultureInfo instances via CultureInfo.GetCultureInfo(name) from these strings anyway, I guess we could argue to pass in those instance to begin with.
And yes, it makes more sense to pass in CultureInfo here rather than IFormatProvider + safe-casting.
Just to mention, ResourceManager has some built-in logic for locale fallback: https://learn.microsoft.com/en-us/globalization/locale/fallback
|
Oh, I just remembered that there are two more benchmarks that I forgot to post: Before:
After:
|
|
TryParseInvalidUnitBenchmarks.cs Before:
After:
Eh, yeah - I've made the number of iterations a constant 1000 but didn't bother to update the result (I used to think it would make sense to provide the number of iterations in the report). |
…r with IReadOnlyCollection<QuantityInfo> - UnitsNetSetup: replaced the ICollection<QuantityInfo> parameter with IReadOnlyCollection<QuantityInfo> - UnitAbbreviationsCache: introduced a constructor with a list of quantities, and improve the comments of the other constructors - added tests and benchmarks covering the UnitAbbreviations initializations
UnitsNet.Benchmark/Initializations/UnitAbbreviationsCacheInitializationBenchmarks.cs
Outdated
Show resolved
Hide resolved
…sing backing fields) - added some benchmarks for the Enum/UnitKey - updated some typos for the TryParseUnitBenchmarks and introduced an explict Culture for the Parse/TryParse unit benchmarks
…ionary and optimized the lazy initializer
…d the _quantitiesByName into a regular (lazy-loaded) dictionary - UnitsNetSetup and UnitAbbreviationsCache constructors changed from IReadOnlyCollection<QuantityInfo> into IEnumerable<QuantityInfo> - changed the Quantity.Infos from IReadOnlyCollection to IReadOnlyList
|
@angularsen I think this is ready, I don't have anything more to add for the time being. There is of course a |
|
Oh boy, I'll need some time to just read through this 😄 |
Don't be deterred, it's mostly just benchmark results (and the TLDR is that the numbers are better). By the way, I don't know if you're also having this issue with Rider: I've opened the ticket 2 months ago, and it doesn't seem to have gotten much traction - if you're seeing a similar issue please up-vote it (with this PR you may be seeing something like 'No unit information found for key..`). |
| } | ||
| // TODO this is certain to have terrible performance (especially on the first run) | ||
| // TODO we should consider adding a (lazy) dictionary for these | ||
| // Use case-sensitive match to reduce ambiguity. |
There was a problem hiding this comment.
I don't have a good context here, but why can we skip case-insensitive matching here?
At least for parsing, I find it convenient that the library is case-insensitive as long as it is not ambiguous.
There was a problem hiding this comment.
Ok I think I follow, we want UnitParser to contain the logic for case-insensitive-if-unique, and let UnitAbbreviationsCache be a more straight forward lookup with less logic?
I guess UnitParser could pass a case-sensitivity option to the lookup in UnitAbbreviationsCache, maybe.
There was a problem hiding this comment.
I don't have a good context here, but why can we skip case-insensitive matching here?
At least for parsing, I find it convenient that the library is case-insensitive as long as it is not ambiguous.
There are some units (between different quantities) which aren't ambiguous when compared in a case-sensitive manner (I did a test - and from memory there were around 20 units in total, for which this was helpful).
There was a problem hiding this comment.
Ok I think I follow, we want
UnitParserto contain the logic for case-insensitive-if-unique, and letUnitAbbreviationsCachebe a more straight forward lookup with less logic?I guess
UnitParsercould pass a case-sensitivity option to the lookup inUnitAbbreviationsCache, maybe.
First part of the plan was to move everything to their domain of responsibility. This shouldn't be part of the UnitAbbreviationsCach, in my other project this is part of the UnitParser:
private List<(UnitInfo UnitInfo, string Abbreviation)> FindAllMatchingUnitsForCulture(string unitAbbreviation, CultureInfo culture,
StringComparison comparison)My long-term plan (haven't done it yet) was to create a Caching version of the UnitParser that is able to satisfy the previous method signature (likely this would be backed by a case-insensitive dictionary).
Currently every call to this function, is causing the UnitAbbreviations to be get fully-cached, so we might just as well save the mapped abbreviations.
| /// Quantity value type, such as <see cref="Length"/> or <see cref="Mass"/>. | ||
| /// Quantity value type, such as <see cref="Length" /> or <see cref="Mass" />. | ||
| /// </summary> | ||
| public Type ValueType { get; } | ||
| public Type QuantityType { get; } | ||
|
|
||
| /// <inheritdoc cref="QuantityType" /> | ||
| [Obsolete("Replaced by the QuantityType property.")] | ||
| [DebuggerBrowsable(DebuggerBrowsableState.Never)] | ||
| public Type ValueType | ||
| { | ||
| get => QuantityType; | ||
| } |
There was a problem hiding this comment.
Agreed, but why not just remove it since this is targeting v6?
We could deprecate in v5 instead and offer a similar change there.
| [Obsolete("Methods accepting a culture name are deprecated in favor of using an instance of the IFormatProvider.")] | ||
| public static double ConvertByAbbreviation(double fromValue, string quantityName, string fromUnitAbbrev, string toUnitAbbrev, string? culture) | ||
| { | ||
| if (!TryGetUnitType(quantityName, out Type? unitType)) | ||
| throw new UnitNotFoundException($"The unit type for the given quantity was not found: {quantityName}"); | ||
|
|
||
| var cultureInfo = CultureHelper.GetCultureOrInvariant(culture); | ||
|
|
||
| var fromUnit = UnitsNetSetup.Default.UnitParser.Parse(fromUnitAbbrev, unitType, cultureInfo); // ex: ("m", LengthUnit) => LengthUnit.Meter | ||
| var fromQuantity = Quantity.From(fromValue, fromUnit); | ||
| return ConvertByAbbreviation(fromValue, quantityName, fromUnitAbbrev, toUnitAbbrev, CultureHelper.GetCultureOrInvariant(culture)); | ||
| } | ||
|
|
||
| var toUnit = UnitsNetSetup.Default.UnitParser.Parse(toUnitAbbrev, unitType, cultureInfo); // ex:("cm", LengthUnit) => LengthUnit.Centimeter | ||
| return fromQuantity.As(toUnit); | ||
| /// <summary> | ||
| /// Convert between any two quantity units by their abbreviations, such as converting a "Length" of N "m" to "cm". | ||
| /// This is particularly useful for creating things like a generated unit conversion UI, | ||
| /// where you list some selectors: | ||
| /// a) Quantity: Length, Mass, Force etc. | ||
| /// b) From unit: Meter, Centimeter etc if Length is selected | ||
| /// c) To unit: Meter, Centimeter etc if Length is selected | ||
| /// </summary> | ||
| /// <param name="fromValue"> | ||
| /// Input value, which together with <paramref name="fromUnitAbbrev" /> represents the quantity to | ||
| /// convert from. | ||
| /// </param> | ||
| /// <param name="quantityName">The invariant quantity name, such as "Length". Does not support localization.</param> | ||
| /// <param name="fromUnitAbbrev">The abbreviation of the unit in the given culture, such as "m".</param> | ||
| /// <param name="toUnitAbbrev">The abbreviation of the unit in the given culture, such as "m".</param> | ||
| /// <param name="formatProvider"> | ||
| /// The format provider to use for lookup. Defaults to <see cref="System.Globalization.CultureInfo.CurrentCulture" /> | ||
| /// if null. | ||
| /// </param> | ||
| /// <example>double centimeters = ConvertByName(5, "Length", "m", "cm"); // 500</example> | ||
| /// <returns>Output value as the result of converting to <paramref name="toUnitAbbrev" />.</returns> | ||
| /// <exception cref="QuantityNotFoundException"> | ||
| /// Thrown when no quantity information is found for the specified quantity name. | ||
| /// </exception> | ||
| /// <exception cref="UnitNotFoundException">No units match the abbreviation.</exception> | ||
| /// <exception cref="AmbiguousUnitParseException">More than one unit matches the abbreviation.</exception> | ||
| public static double ConvertByAbbreviation(double fromValue, string quantityName, string fromUnitAbbrev, string toUnitAbbrev, IFormatProvider? formatProvider) |
There was a problem hiding this comment.
I was about to comment on the same thing reading the above obsoletion of string-based culture names.
I'm not sure how I feel about string vs IFormatProvider, since we don't really use the culture formatting for anything, we just rely on the culture name.
If we end up just getting cached CultureInfo instances via CultureInfo.GetCultureInfo(name) from these strings anyway, I guess we could argue to pass in those instance to begin with.
And yes, it makes more sense to pass in CultureInfo here rather than IFormatProvider + safe-casting.
Just to mention, ResourceManager has some built-in logic for locale fallback: https://learn.microsoft.com/en-us/globalization/locale/fallback
| /// This key is particularly useful when using an enum-based unit in a hash-based collection, | ||
| /// as it avoids the boxing that would normally occur when casting the enum to <see cref="Enum" />. | ||
| /// </remarks> | ||
| public virtual UnitKey UnitKey => Value; |
I was hoping you would say that (regarding the |
In my latest version this has the following signature:
while the other overloads are marked as |
Yes, I think we're actually doing this all wrong (in terms of performance)- there is a huge benefit for re-using the /// <summary>
/// Initializes a new instance of the <see cref="QuantityInfo" /> class.
/// </summary>
/// <param name="name">The name of the quantity.</param>
/// <param name="quantityType">The type representing the quantity.</param>
/// <param name="baseDimensions">The base dimensions of the quantity.</param>
/// <param name="unitAbbreviations">
/// When provided, the resource manager used for localizing the quantity's unit abbreviations.
/// </param>
/// <exception cref="ArgumentNullException">
/// Thrown if <paramref name="name" />, <paramref name="quantityType" />, or <paramref name="baseDimensions" /> is
/// <c>null</c>.
/// </exception>
protected QuantityInfo(string name, Type quantityType, BaseDimensions baseDimensions, ResourceManager? unitAbbreviations)
{
Name = name ?? throw new ArgumentNullException(nameof(name));
QuantityType = quantityType ?? throw new ArgumentNullException(nameof(quantityType));
BaseDimensions = baseDimensions ?? throw new ArgumentNullException(nameof(baseDimensions));
UnitAbbreviations = unitAbbreviations;
}At the same time, this allows us to actually use custom resource dictionaries, defined by the client-app (both for customizing the existing stuff, as well as the public static readonly QuantityInfo<HowMuch, HowMuchUnit> Info = new(
HowMuchUnit.Some,
new UnitDefinition<HowMuchUnit>[]
{
new(HowMuchUnit.Some, "Some", BaseUnits.Undefined),
new(HowMuchUnit.ATon, "Tons", new BaseUnits(mass: MassUnit.Tonne), QuantityValue.FromTerms(1, 10)),
new(HowMuchUnit.AShitTon, "ShitTons", BaseUnits.Undefined, QuantityValue.FromTerms(1, 100))
},
new BaseDimensions(0, 1, 0, 0, 0, 0, 0),
// providing a resource manager for the unit abbreviations (optional)
Properties.CustomQuantities_HowMuch.ResourceManager); |
|
@angularsen Did you notice this issue? I'm pretty sure that this should also appear in
|
No, never seen that one in Rider. I haven't tried the example given in the issue though. |
|
@angularsen I'm merging this, so that it's not in the way.. My plan was to close #1463 next, by copy-pasting the tests from my other project (where the property is removed), leaving us just code changes to review- hopefully there won't be any differences in the tests, other than the removal of the property from the types that support it (99%). |
|
Alright! |
|
I see that the code coverage report is not happy, but no worries- here's what I've got from my latest build:
(with most of it being in the |
Enumboxing optimizations:UnitKeystruct as a replacement for the conversions fromTUnittoEnumUnitAbbreviationCacheusing theUnitKeyfor the concurrent dictionaryConvert.ToInt32(..)callsQuantityInfoLookupcollectionsQuantity/UnitConvertercode (using theUnitParser/QuantityInfoLookup)Breaking changes:
Quantity.Namesfromstring[]toIReadOnlyCollection<string>Quantity.InfosfromQuantityInfo[]toIReadOnlyList<QuantityInfo>UnitConverter.ConvertByAbbreviationmethod that takes astring?for theIFormatProvider?as[Obsolete](created an overload)UnitsNetSetup: replaced theICollection<QuantityInfo>constructor parameter withIEnumerable<QuantityInfo>QuantityInfo.ValueTypetoQuantityInfo.QuantityType(making the old property[Obsolete])UnitConverter.ConvertByName