Skip to content

Speed up caching #4383

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
merged 9 commits into from
Jul 7, 2025
Merged

Speed up caching #4383

merged 9 commits into from
Jul 7, 2025

Conversation

connorjward
Copy link
Contributor

@connorjward connorjward commented Jun 20, 2025

This is a bit of a refactor of parallel_cache to try and address the performance issues reported in #3983.

Fixes #3983

TODOs

  • Verify that this is parallel safe (esp. check with G-ADOPT)

The main changes are:

  • API changes comm_getter to get_comm and cache_factory to make_cache.
  • Do not use as_hexdigest every time we do a cache lookup. I think this was the main cause of the slowdown.
  • Identify caches by a local ID attribute not type(make_cache()).__class__.__name__ which was unsafe and slow (?).

Using the example from #3983 the performance improvement takes us (with a hot cache) from 31s to 14s, so I must be doing something right.

@connorjward connorjward force-pushed the connorjward/cache-fixes branch from de050ee to 3951383 Compare June 25, 2025 10:05
@connorjward connorjward requested a review from ksagiyam June 25, 2025 10:06
@connorjward connorjward marked this pull request as ready for review June 25, 2025 10:06
Copy link
Contributor

@ksagiyam ksagiyam left a comment

Choose a reason for hiding this comment

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

Otherwise, looks good to me.

@connorjward
Copy link
Contributor Author

I have run a big G-ADOPT example on 16 cores and it ran without issue. I think that this is safe to merge.

@Ig-dolci
Copy link
Contributor

Ig-dolci commented Jul 5, 2025

Should I consider re-running my performance experiments after this PR is merged?

@connorjward
Copy link
Contributor Author

Should I consider re-running my performance experiments after this PR is merged?

That might potentially be a good idea. Were your experiments done against release or master? (This is going into master)

@Ig-dolci
Copy link
Contributor

Ig-dolci commented Jul 5, 2025

Should I consider re-running my performance experiments after this PR is merged?

That might potentially be a good idea. Were your experiments done against release or master? (This is going into master)

Release

@connorjward
Copy link
Contributor Author

Should I consider re-running my performance experiments after this PR is merged?

That might potentially be a good idea. Were your experiments done against release or master? (This is going into master)

Release

I guess this doesn't actually matter. You might just need to reinstall.

The changes here only really make an impact if you are running with small problems at the strong scaling limit. Does that apply?

@connorjward connorjward merged commit d85339c into master Jul 7, 2025
11 of 14 checks passed
@connorjward connorjward deleted the connorjward/cache-fixes branch July 7, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Caching implementation is slow
3 participants