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

PERF: dispatch on UnitSystem value in _get_multiplier #1216

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

GabrielKS
Copy link
Collaborator

@GabrielKS GabrielKS commented Nov 23, 2024

Before:
Screenshot 2024-11-22 at 4 20 21 PM

After:
Screenshot 2024-11-22 at 5 28 35 PM

On the current @scoped_enum implementation, UnitSystem.NATURAL_UNITS isn't a constant, it's a dictionary lookup — one that incurs 80 bytes of memory allocation. Thus you can see the performance impact piling up in the "before" case in the order in which the unit systems are checked — DEVICE_BASE, then SYSTEM_BASE, then NATURAL_UNITS. There are a few ways one might fix this:

  1. Alter @scoped_enum to make UnitSystem.NATURAL_UNITS behave more like an immutable struct lookup, either by actually making it one or doing something to clue the compiler into the fact that it will never change. This would be the best, most holistic solution, but I couldn't immediately figure out how we might do it without completely overhauling @scoped_enum. I'll keep it in the back of my head though.
  2. Instead of testing setting.unit_system == IS.UnitSystem.NATURAL_UNITS, do setting.unit_system.value == 2, etc. No dictionary lookup, but too hardcoded for me.
  3. The multiple dispatch approach given here.

Most of the cost savings are from dispatching on the UnitSystem value. Adding another layer of dispatch on the nothing-ness of get_internal(c).units_info seemed to make it a bit faster on top of that, so I've done that too.

@GabrielKS GabrielKS self-assigned this Nov 23, 2024
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.35%. Comparing base (a603b75) to head (91bc0ab).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/models/components.jl 42.85% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
- Coverage   84.37%   84.35%   -0.02%     
==========================================
  Files         181      181              
  Lines        8382     8375       -7     
==========================================
- Hits         7072     7065       -7     
  Misses       1310     1310              
Flag Coverage Δ
unittests 84.35% <42.85%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/models/components.jl 61.53% <42.85%> (-4.57%) ⬇️

::Val{IS.UnitSystem.NATURAL_UNITS},
) where {T <: Component} =
get_base_power(c)
_get_multiplier(::T, ::IS.SystemUnitsSettings, ::Val) where {T <: Component} =
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this one or can let Julia do its normal behavior?

Copy link
Collaborator Author

@GabrielKS GabrielKS Nov 25, 2024

Choose a reason for hiding this comment

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

I'm good with letting Julia do its normal behavior on this one.

@jd-lara jd-lara merged commit 92c7f27 into main Nov 25, 2024
10 of 11 checks passed
@jd-lara jd-lara deleted the gks/perf_get_multiplier branch November 25, 2024 20:43
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