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

get_cache_key: replace sorted() with immutable Map #133

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

matthiasdiener
Copy link
Contributor

@matthiasdiener matthiasdiener commented Aug 24, 2023

sorted() is currently the function with the highest impact on compile performance in a mirgecom-y3prediction rhs compile on M1 (i.e., it is the top entry in cProfile when sorting by tottime, just above islpy's casting wrapper). This PR reduces the number of calls to sorted by 80%.

Other options might be:

  • use frozenset
  • use hash(immutables.Map(...))
  • use frozendict

@inducer
Copy link
Owner

inducer commented Aug 24, 2023

Wait... this change reduces compile time by 80%?

@matthiasdiener
Copy link
Contributor Author

matthiasdiener commented Aug 24, 2023

Wait... this change reduces compile time by 80%?

No, it reduces the number of calls to sorted by 80% (7.5 million to 1 Million). Overall compile time is improved by ~5%.

@inducer inducer merged commit 82220fe into inducer:main Aug 24, 2023
10 checks passed
@inducer
Copy link
Owner

inducer commented Aug 24, 2023

Sounds good. Every little bit helps! 🙂

@inducer
Copy link
Owner

inducer commented Aug 24, 2023

(And thanks!)

@inducer
Copy link
Owner

inducer commented Aug 24, 2023

One additional item is that, in many situations, there won't be a **kwargs component to deal with, and @optimize_mapper could specialize that away. For expensive mappers, this may be a good idea.

@matthiasdiener matthiasdiener deleted the sorted-immutables branch September 11, 2023 16:29
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