Skip to content

gh-131525: Remove _HashedSeq wrapper from lru_cache #131922

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

Merged
merged 1 commit into from
Mar 31, 2025

Conversation

lgeiger
Copy link
Contributor

@lgeiger lgeiger commented Mar 31, 2025

As suggested in #131525 (comment) this PR removes _HashedSeq wrapper from the Python-only functools.lru_cache implementation. Since tuple hashes are now cached (#131529) this wrapper is not needed anymore.

I ran some very quick benchmarks with the following ipython script to check whether this change has an impact on the performance:

from functools import lru_cache

@lru_cache(maxsize=1)
def foo(a, b, c, d, e):
    pass

%%timeit
foo(1, 2, 3, 4, 5)
foo(1, 2, 3, 4, 6)

In this toy benchmark with trivial hashes this change removes some overhead

version pre #131529 main
C implementation 137 ns ± 2.51 ns 139 ns ± 0.682 ns
Python only 1.23 μs ± 35.2 ns 1.23 μs ± 6.91 ns
Python only no _HashedSeq (this PR) 788 ns ± 2.49 ns 777 ns ± 5.12 ns

In an example where hashes are artificially slow this change would've caused a regression before #131529 but now performs equally well (4.14 s without tuple hash cashing vs 1.03 s with hash caching).

class SlowInt:
    def __init__(self, val) -> None:
        self.val = val

    def __hash__(self):
        time.sleep(0.1)
        return hash(self.val)

%%timeit
foo(SlowInt(1), SlowInt(2), SlowInt(3), SlowInt(4), SlowInt(5))
foo(SlowInt(1), SlowInt(2), SlowInt(3), SlowInt(4), SlowInt(6))

I don't think a news entry is necessary since this only affects the Python only implementation, but let me know if I should still add one.

Lib/functools.py Outdated
@@ -690,7 +674,6 @@ def wrapper(*args, **kwds):
# still adjusting the links.
root = oldroot[NEXT]
oldkey = root[KEY]
oldresult = root[RESULT]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value was never used

Copy link
Contributor

Choose a reason for hiding this comment

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

This line needs to be restored. Its job is to hold on to the object reference until the function returns (after the cache is in a stable state).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Sorry about that, I wasn't aware. My IDE complained about it and I should've doubted it more. I reverted 5ebba28

@lgeiger lgeiger force-pushed the lru-cache-hashing branch from 685f8fe to 7c4dfe6 Compare March 31, 2025 09:07
@rhettinger rhettinger merged commit 0147be0 into python:main Mar 31, 2025
39 of 40 checks passed
@lgeiger lgeiger deleted the lru-cache-hashing branch March 31, 2025 13:43
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants