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

Add generated, performant snake_case for NodeID. #1982

Merged
merged 2 commits into from
Dec 21, 2024
Merged

Conversation

evetion
Copy link
Member

@evetion evetion commented Dec 20, 2024

Fixes #1981

This precalculates all snakecases for NodeType and compiles it. @visr could you benchmark this?

@evetion evetion requested a review from visr December 20, 2024 16:19
Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

Ah yes with this I don't see this line anymore in the profile of the LHM model run. I did see a bunch of allocations, so I pushed some views onto this branch to remove them.

With this the concentration callback takes around 20% of runtime. Most of that remaining time is spent iterating over a namedtuple here:

Ribasim/core/src/util.jl

Lines 1101 to 1105 in ae2589c

for (comp, range) in pairs(NT)
if comp == component_name
return range[id.idx]
end
end

@evetion
Copy link
Member Author

evetion commented Dec 20, 2024

I assumed this code was also used in non-concentration parts? So we might see a speedup in normal runs too?

Good catch with those views. Unless you have ideas on how to further optimize that namedtuple iteration, let's merge this.

@visr visr merged commit de9b527 into main Dec 21, 2024
28 checks passed
@visr visr deleted the fix/snake_case_nodetype branch December 21, 2024 07:55
@visr visr mentioned this pull request Dec 24, 2024
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.

Speed up snake_case for concentrations
2 participants