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

Cache hash of ConnectionElement #2384

Merged
merged 7 commits into from
Dec 21, 2023
Merged

Conversation

chriselrod
Copy link
Contributor

Before:

julia> using ModelingToolkit: ConnectionSet

julia> connectionsets = ConnectionSet[]
ConnectionSet[]

julia> domain_csets = ConnectionSet[]
ConnectionSet[]

julia> ModelingToolkit.generate_connection_set!(connectionsets, domain_csets, sys, nothing, nothing);

julia> length(connectionsets)
40008

julia> @time merge(connectionsets);
 36.909123 seconds (628.06 M allocations: 9.941 GiB, 1.82% gc time, 0.01% compilation time)

After:

julia> @time merge(connectionsets);
  4.970410 seconds (406.83 k allocations: 29.342 MiB, 0.10% compilation time)

@ChrisRackauckas
Copy link
Member

Is there some kind of test we can add for performance regressions on this?

@chriselrod
Copy link
Contributor Author

It'd be great if we could have something like nanosoldier for SciML, some performance benchmark test suite that could run regularly on the same computer to track changes over time.
As is, it's hard to notice when regressions creep in. It's all too easy for a type instability to appear somewhere.

In this case, though, most of merge's time was just calling hash so caching the hash was an obvious solution that seemed to really improve things.

However, I fear the validity of things like this. What is v? Is it allowed to change?

For now, I added

    @inbounds begin
        @boundscheck begin
            @assert e.h === _hash_impl(e.sys, e.v, e.isouter)
        end
    end

Maybe we should make our own @dassert (debug assert) macro that is only on when bounds checking is enforced, and hence is always enabled when running test suites.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 16, 2023

We have something setup on BoundaryValueDiffEq.jl: SciML/BoundaryValueDiffEq.jl#140 (comment)

We should start doing that more widely.

@chriselrod
Copy link
Contributor Author

Blocked by bifurcationkit/BifurcationKit.jl#127

@chriselrod
Copy link
Contributor Author

Can we move forward with this without BifurcationKit? Who uses it/what's our reason for supporting it?

@ChrisRackauckas
Copy link
Member

Rebase

@chriselrod
Copy link
Contributor Author

Rebase

The first problem is that we can't even run tests without BifurcationKit having a compatible version.

@ChrisRackauckas
Copy link
Member

That's what's fixed on master.

@chriselrod
Copy link
Contributor Author

That's what's fixed on master.

Okay, the changes here should be unrelated to any SymbolicIndexingInterface issues. So if some other changes caused issues, I'll remove them.
Was mostly annoyed by not being able to run tests on CI.

@ChrisRackauckas ChrisRackauckas merged commit 9f0433b into SciML:master Dec 21, 2023
13 of 22 checks passed
@ChrisRackauckas
Copy link
Member

Having multiple very low level packages do major bumps in the same month is a nightmare for many reasons. We'll get through this rough patch though.

@chriselrod chriselrod deleted the cachehash branch December 21, 2023 02:38
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