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

Merged
merged 3 commits into from
Mar 1, 2025
Merged

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")?

@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)

@bruchar1 bruchar1 added the options Meson configuration options label Feb 17, 2025
@bruchar1 bruchar1 added the perf label Feb 28, 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 mesonbuild#14245
This removes a few comparisons, and one function call.
It makes object initialisation slower...
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -103,8 +102,9 @@ class ArgparseKWs(TypedDict, total=False):
}

_BAD_VALUE = 'Qwert Zuiopü'
_optionkey_cache: T.Dict[T.Tuple[str, str, MachineChoice], OptionKey] = {}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a class object rather than a module level variable?

Copy link
Member

Choose a reason for hiding this comment

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

It cant be because the class is mucking with __new__

@jpakkane jpakkane merged commit a9a37de into mesonbuild:master Mar 1, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
options Meson configuration options perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

options refactor: OptionKey initialisation is too slow
4 participants