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

Check if type attribute is actually modified before label it to improve some performance #113480

Closed
gaogaotiantian opened this issue Dec 26, 2023 · 1 comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement

Comments

@gaogaotiantian
Copy link
Member

gaogaotiantian commented Dec 26, 2023

Feature or enhancement

Proposal:

In type_setattro, if an assignment is successful, PyType_Modified will be called and all the specialization caches for the type will be invalidated. However, it's possible that the newly assigned value is the current value so the type is not modified at all.

So, what's the possibility of that? Of course it highly depends on the application itself, but imagine a class with a boolean or enum class member, it's normal to assign the same value to the member. I ran some subtests of the current CPython repo, the number is between 1% and 2%.

The number is so low, will it worth it? Yes, the number is not significant, but also not neglectable. It all comes to the cost/gain ratio.

The cost to check if the member is actually replaced is surprisingly low because all the logic is already there. Because it's a type_setattro, there can't be anything fancy like descriptor (even for metaclasses) when it comes to it. So any assignment must happen in a dict. We already have a dict watcher which will increment a global dict version if updated.

As a result, we don't need an expensive LOAD_ATTR like operation to check whether the attribute is actually changed, it's basically

    uint64_t prev_version = interp->dict_state.global_version;  // Get the prev version
    res = _PyObject_GenericSetAttrWithDict((PyObject *)type, name, value, NULL);
    if (res == 0 && prev_version != interp->dict_state.global_version) {  // Check the current version

It's super cheap! And it does not add code to any hot path like dict operations.

The reward for one cache miss save is significant. I have a micro-benchmark below, the optimization saved about 40% of the execution time.

import timeit

setup = """
class C:
    counter = 0  # Class variable
"""

stmt = """
for _ in range(100000):
    a = C.counter
    C.counter = a  # Increment class variable
"""

print(timeit.timeit(stmt, setup=setup, number=100))

It also has a nice side effect - slightly slowing down the speed of version increment which alleviates #113462. In some extreme cases, it can save a lot of version numbers.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

@gaogaotiantian gaogaotiantian added the type-feature A feature request or enhancement label Dec 26, 2023
@gaogaotiantian gaogaotiantian added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Dec 26, 2023
@brandtbucher
Copy link
Member

We decided not to do this, mostly since it relies on dictionary versions (deprecated in PEP 699 for removal in 3.14).

@brandtbucher brandtbucher closed this as not planned Won't fix, can't repro, duplicate, stale Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants