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

speed up adding sparsity pattern entries with coupling #1028

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

KristofferC
Copy link
Collaborator

Ref #993

As written then in check for j was performed on every iteration over i.

Before:
julia> @time allocate_matrix(dh; topology=top, interface_coupling = interface_coupling)
 25.123126 seconds (217.41 k allocations: 1.820 GiB, 0.87% gc time)


After:
julia> @time allocate_matrix(dh; topology=top, interface_coupling = interface_coupling)
 12.786278 seconds (217.84 k allocations: 1.821 GiB, 1.79% gc time)

I think the approach here of doing this in check like this has to be rethought do.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.61%. Comparing base (98e2b67) to head (18db56d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1028      +/-   ##
==========================================
+ Coverage   93.60%   93.61%   +0.01%     
==========================================
  Files          39       39              
  Lines        5883     5893      +10     
==========================================
+ Hits         5507     5517      +10     
  Misses        376      376              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KristofferC
Copy link
Collaborator Author

I see that the check for the coupling being non-zero is done inside _add_interface_entry. Lifting it out to avoid all these in for the zero case is probably worth while.

@fredrikekre
Copy link
Member

There is already a duplicate-check here

if k == length(x) + 1 || @inbounds(x[k]) != item
so possibly there shouldn't be one in the function modified here at all?

@KristofferC KristofferC force-pushed the kc/sparse_coupling_perf branch 2 times, most recently from 3f2ef5f to d77989c Compare July 29, 2024 14:31
@KristofferC KristofferC changed the title slightly speed up adding sparsity pattern entries with coupling speed up adding sparsity pattern entries with coupling Jul 29, 2024
@KristofferC
Copy link
Collaborator Author

I now have

julia> @time allocate_matrix(dh);
  2.430063 seconds (1.21 k allocations: 1.767 GiB, 14.28% gc time)

julia> @time allocate_matrix(dh; topology, interface_coupling)
  3.207378 seconds (1.04 M allocations: 1.880 GiB, 13.06% gc time)

In #993, the first only took 0.7 seconds. I'm on a pretty fast CPU so did something regress @fredrikekre?

@KristofferC
Copy link
Collaborator Author

so possibly there shouldn't be one in the function modified here at all?

If I just flat out comment out the in checks the tests fail at least.

@KristofferC
Copy link
Collaborator Author

@AbdAlazezAhmed maybe you could look over this.

@AbdAlazezAhmed
Copy link
Collaborator

LGTM :D

@KristofferC KristofferC merged commit 5d95f89 into master Jul 31, 2024
11 checks passed
@KristofferC KristofferC deleted the kc/sparse_coupling_perf branch July 31, 2024 10: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