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 cache to OptionKey #14250

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

bruchar1
Copy link
Member

@bruchar1 bruchar1 commented Feb 13, 2025

OptionKey objects are used extensively. We want them with a simple API, but they also need to be optimized to not compromise meson performances.

Since this is an immutable object, it is possible to cache the OptionKey object creation. We need to do it using the new to make the caching mechanism transparent.

Fixes #14245

@bruchar1 bruchar1 requested a review from jpakkane as a code owner February 13, 2025 13:05
@bruchar1 bruchar1 force-pushed the cached-optionkey branch 3 times, most recently from f52d09e to 9fdbd2b Compare February 13, 2025 14:54
@bonzini bonzini added this to the 1.8 milestone Feb 13, 2025
@bonzini
Copy link
Collaborator

bonzini commented Feb 13, 2025

Do you even need to override equality if there's a cache? Wouldn't all equal objects be the same object (i.e. "a == b" becomes the same as "a is b")?

OptionKey objects are used extensively. We want them with a simple API,
but they also need to be optimized to not compromise meson performances.

Since this is an immutable object, it is possible to cache the
OptionKey object creation. We need to do it using the __new__
to make the caching mechanism transparent.

Fixes mesonbuild#14245
@bruchar1
Copy link
Member Author

Do you even need to override equality if there's a cache? Wouldn't all equal objects be the same object (i.e. "a == b" becomes the same as "a is b")?

That's a good point. However, I don't see any hit on comparison operators in my profile. Therefore, I suppose Python already recognize the identity of objects when performing hash map lookup.

@bruchar1 bruchar1 force-pushed the cached-optionkey branch 2 times, most recently from a4ade37 to a4fab0f Compare February 14, 2025 14:09
@bruchar1
Copy link
Member Author

With those optimisations, target generation is now even faster that before the merge of the options refactor (before options refactor: 73s, after options refactor: 77s, with those optimisations: 64s)

This removes a few comparisons, and one function call.
It makes object initialisation slower...
@bruchar1 bruchar1 added the options Meson configuration options label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
options Meson configuration options
Projects
None yet
Development

Successfully merging this pull request may close these issues.

options refactor: OptionKey initialisation is too slow
2 participants