Skip to content

Minor performance improvements to the BaseUnits class (v5) #1452

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

lipchev
Copy link
Collaborator

@lipchev lipchev commented Dec 11, 2024

  • avoid storing the IsFullyDefined value to a field (the property is only used when constructing a UnitSystem)
  • add a reference check to the Equals method (improves the == Undefined)
  • use the HashCode class for the GetHashCode method (note that the result would be different between Framework and NET projects)

- avoid storing the IsFullyDefined value to a field (the property is only used when constructing a UnitSystem)
- add a reference check to the Equals method (improves the == Undefined)
- use the HashCode class for the hashcode (note that the result would be different between Framework and NET projects)
@lipchev
Copy link
Collaborator Author

lipchev commented Dec 11, 2024

If you think the 3rd point is a "breaker", I could switch it over to v6 but I find it very unlikely that this would affect anyone (I'm frankly not even sure if the same code would produce the same result on both targets 🤷 ).
I have one more change I want to make- but that's more likely (although it shouldn't) to surprise somebody:

I'd like to change the ToString format:

        public override string ToString()
        {
            if (Equals(Undefined))
            {
                return "Undefined";
            }

            return string.Join(", ", GetUnitsDefined());

            IEnumerable<string> GetUnitsDefined()
            {
                if (Length is not null)
                {
                    yield return $"[Length]: {Length}";
                }
                if (Mass is not null)
                {
                    yield return $"[Mass]: {Mass}";
                }
                if (Time is not null)
                {
                    yield return $"[Time]: {Time}";
                }
                if (Current is not null)
                {
                    yield return $"[Current]: {Current}";
                }
                if (Temperature is not null)
                {
                    yield return $"[Temperature]: {Temperature}";
                }
                if (Amount is not null)
                {
                    yield return $"[Amount]: {Amount}";
                }
                if (LuminousIntensity is not null)
                {
                    yield return $"[LuminousIntensity]: {LuminousIntensity}";
                }
            }
        }

The difference is that we're not using the UnitAbbreviations - which removes the potential side effects that this could have when "configuring" the UnitsNetSetup.

@angularsen angularsen changed the title Minor performance improvements to the BaseUntis class (v5) Minor performance improvements to the BaseUnits class (v5) Dec 15, 2024
@angularsen
Copy link
Owner

Looks fine to me, I don't think this should break anyone or at least extremely few.

Regarding the ToString change, beware that yield return and enumerable are not really performant if that is the goal. No source right now, but if I recall correctly using StringBuilder is the way to go to construct strings performantly.

Of course, we're talking micro optimizations here, but also no real downside to that approach.

@angularsen angularsen enabled auto-merge (squash) December 15, 2024 11:59
@angularsen angularsen disabled auto-merge December 15, 2024 11:59
@angularsen angularsen merged commit 83063d3 into angularsen:master Dec 15, 2024
1 check failed
@angularsen
Copy link
Owner

angularsen pushed a commit that referenced this pull request Jan 2, 2025
- `BaseUnits`: no longer using the `AbbeviationsCache`, the new format
is `L=Meter, M=Kilogram, T=Second`
- `BaseDimensions`: the exponent moved inside the dimension-brackets:
`[Length][Time^-1]`
- `BaseDimensions`: minor performance improvements

As mentioned in #1452, the main motivation here is the removal of the
potential side effects of accessing/loading the unit abbreviations (e.g.
during the `QuantityInfo` construction)
@lipchev lipchev deleted the base-units-performance-optimizations-v5 branch April 11, 2025 20:25
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