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

Remove get_id() hotspot #14108

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

Conversation

bonzini
Copy link
Collaborator

@bonzini bonzini commented Jan 10, 2025

Use lazy_property for get_id(). To avoid touching code all over the place, keep get_id() but make it access the actual lazy property (which will be simply an entry in self.dict)

mesonbuild/build.py Outdated Show resolved Hide resolved
These are given a name that is different from __func.__name__, so
store the name when the descriptor is placed in the class.

Signed-off-by: Paolo Bonzini <[email protected]>
@bonzini bonzini changed the title Remove a couple weird/unexpected hotspots Remove get_id() hotspot Jan 10, 2025
@dcbaker
Copy link
Member

dcbaker commented Jan 10, 2025

I’d honestly love to see many of the get_* functions go. They either need a better name, or end up being a useless layer of indirection to return a property, which comes with a performance cost.

incidentally I had been looking at this and the. Got bogged down adding type annotations to the vs backend…

@bonzini
Copy link
Collaborator Author

bonzini commented Jan 10, 2025

They either need a better name, or end up being a useless layer of indirection to return a property, which comes with a performance cost.

Absolutely. I'd also love to have a lazy_property variant that stores the outcome of a method in the __dict__ of a parameter, thus being a cheaper and cleaner variant of lru_cache(max_size=None) (for example if you have N>1 parameters but N-1 depend on the remaining 1). But it seemed to me that this was not the right place to do a full refactoring of get_*.

If you'd like me to introduce an id property then that's good, though it's probably a good idea anyway to fix lazy_property vs. __.

@dcbaker
Copy link
Member

dcbaker commented Jan 10, 2025

I think i'd rather have id exposed as a public property, even if we don't start using it yet.

I'm fine with the __set_name__ change, this code predates __set_name__ by a lot.

@bonzini bonzini added this to the 1.8 milestone Jan 19, 2025
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.

3 participants