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.

Sign up for GitHub

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

Optimize the OptionKey class more #14248

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

Optimize the OptionKey class more #14248

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d2286f
options: don't use total_ordering for OptionKey
dcbaker Feb 12, 2025
b10ff1f
options: pre-calculate the tuple for for comparisons
dcbaker Feb 12, 2025
383584f
options: cache OptionKey.from_string
dcbaker Feb 12, 2025
9945026
options: Add a cached constructor to OptionKey
dcbaker Feb 12, 2025
d821fd3
Use the OptionKey factory method
dcbaker Feb 12, 2025
b4b1f81
options: require passing hash to the Optioney
dcbaker Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions mesonbuild/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ class ArgparseKWs(TypedDict, total=False):
'vsenv',
}

_OPTIONS_CACHE: T.Dict[int, OptionKey] = {}

class OptionKey:

"""Represents an option key in the various option dictionaries.
Expand All @@ -116,15 +118,16 @@ class OptionKey:
_as_tuple: T.Tuple[str, MachineChoice, str]

def __init__(self, name: str, subproject: str = '',
machine: MachineChoice = MachineChoice.HOST):
machine: MachineChoice = MachineChoice.HOST,
hash_: T.Optional[int] = None):
# the _type option to the constructor is kinda private. We want to be
# able to save the state and avoid the lookup function when
# pickling/unpickling, but we need to be able to calculate it when
# constructing a new OptionKey
object.__setattr__(self, 'name', name)
object.__setattr__(self, 'subproject', subproject)
object.__setattr__(self, 'machine', machine)
object.__setattr__(self, '_hash', hash((name, subproject, machine)))
object.__setattr__(self, '_hash', hash_ if hash_ is not None else hash((name, subproject, machine)))
object.__setattr__(self, '_as_tuple', (self.subproject, self.machine, self.name))
Copy link
Member

Choose a reason for hiding this comment

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

This starts to look very very very similar to a named tuple 🙂


def __setattr__(self, key: str, value: T.Any) -> None:
Expand Down Expand Up @@ -192,6 +195,25 @@ def __str__(self) -> str:
def __repr__(self) -> str:
return f'OptionKey({self.name!r}, {self.subproject!r}, {self.machine!r})'

@classmethod
def factory(cls, name: str, subproject: str = '', machine: MachineChoice = MachineChoice.HOST) -> OptionKey:
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tried to implement it, but I think you can obtain the same effect without forcing the users to call the .factory() class method by implementing __new__() instead.

"""A cached initializer for OptionKeys

:param name: The name of the option
:param subproject: the subproject the option belongs to, defaults to ''
:param machine: the machine the subproject belongs to, defaults to MachineChoice.HOST
:return: An OptionKey
"""
# This doesn't use lru_cache so that we can re-use the calculated hash
# in a cache miss situation.
_hash = hash((name, subproject, machine))
try:
return _OPTIONS_CACHE[_hash]
Copy link
Member

Choose a reason for hiding this comment

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

It is unlikely, but two objects can have the same hash but not be equal. I'm not sure that the cache lookup based on the hash only is a good idea. If this is done so many times that computing the hash becomes the bottleneck, maybe it is better to simply cache the has.

except KeyError:
inst = cls(name, subproject, machine, _hash)
_OPTIONS_CACHE[_hash] = inst
return inst

@classmethod
@lru_cache(None)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this has a big impact. It is probably only used for parsing options given by the command line.

Copy link
Member Author

Choose a reason for hiding this comment

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

This gets used in a surprising number of places, many of them because we still are doing user input validation below the interpreter.

def from_string(cls, raw: str) -> 'OptionKey':
Expand Down