◐ Shell
reader mode source ↗
Skip to content

bpo-42972: Fully implement GC protocol for functools LRU cache#26423

Merged
vstinner merged 1 commit into
python:mainfrom
erlend-aasland:gc-lru-cache
May 28, 2021
Merged

bpo-42972: Fully implement GC protocol for functools LRU cache#26423
vstinner merged 1 commit into
python:mainfrom
erlend-aasland:gc-lru-cache

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented May 28, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor
  • clear and visit members in correct order
  • visit LRU cache elem type via lru_cache_tp_traverse

https://bugs.python.org/issue42972

@erlend-aasland erlend-aasland mentioned this pull request May 28, 2021
Merged
vstinner
vstinner reviewed May 28, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I'm not sure if it's a good idea to "expose" the link type in this traverse function. The only purpose would be to be able to break reference cycles involving multiple "hidden" objects: the LRU cache itself (which is not directly accessible in Python, you need to dig into gc.get_objects()) and the link type (again, you need gc.get_objects() since @methane removed it from the module dict, which is a good thing).

I'm not strongly against it, technically, it respects the GC: the clear function clears the linked list and so removes references to the link type.

By the way, making the two LRU heap types (cache and link) immutable can make these reference cycles even less likely. But it should be done in a separated PR.

@vstinner

vstinner commented May 28, 2021

Copy link
Copy Markdown
Member

Oh. lru_list_elem_type was modified to not implemented the GC to optimize the code, I missed that:

Please explain it in a comment, it's non-obvious.

https://bugs.python.org/issue32422 seems to conflict with https://bugs.python.org/issue42972

Maybe it's acceptable to not implement the GC protocol if the link type is hidden well and it is immutable. In my experience, reference cycles are created in very surprising ways. I don't know about this one. I don't know if it's acceptable for now, if it's a trade-off, or if there is a risk of leaks.

An alternative would be to avoid completely Python objects in the cache. Replace lru_cache_object.cache Python dictionary with a _Py_hashtable_t. So the linked list items don't have to be Python objects anymore.

But this change would be a major change and should only be done in the main branch.

The question is what to do in the 3.10 branch:

  • Implement again the GC protocol in the link type: revert https://bugs.python.org/issue32422
  • Don't implement the GC protocol in the link type, make it immutable, and hide it to reduce the risk of reference cycles involing this type

@vstinner vstinner merged commit 3f8d332 into python:main May 28, 2021
@miss-islington

miss-islington commented May 28, 2021

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2021
@miss-islington
bpo-42972: Fully implement GC protocol for functools LRU cache (pytho… …
2e1b3b7 
…nGH-26423)

(cherry picked from commit 3f8d332)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@bedevere-bot

bedevere-bot commented May 28, 2021

Copy link
Copy Markdown

GH-26425 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 28, 2021
@vstinner

vstinner commented May 28, 2021

Copy link
Copy Markdown
Member

I dislike the change, but I merged it anyway to unblock https://bugs.python.org/issue42972 since @pablogsal marked it as a release blocker for the beta2. Moreover, at least, the change makes the code less broken, since it now visits the LRU cache type which is a bugfix. We can decide what to do with the link type after the beta2.

@erlend-aasland erlend-aasland deleted the gc-lru-cache branch May 28, 2021 09:44
@erlend-aasland

erlend-aasland commented May 28, 2021

Copy link
Copy Markdown
Contributor Author

@vstinner: I can add explanatory comments in a new PR, if you want. Also, let me know if you want a PR for making the cache and cache link types immutable.

@pablogsal

pablogsal commented May 28, 2021

Copy link
Copy Markdown
Member

I am a bit confused about what happened here, the PR mentions that "fully implement the GC protocol" but the change just moves some calls and visits the type....

@erlend-aasland

erlend-aasland commented May 28, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor Author

I am a bit confused about what happened here, the PR mentions that "fully implement the GC protocol" but the change just moves some calls and visits the type....

The LRU cache type already had the Py_TPFLAGS_HAVE_GC flag, and already had a traverse method. The only thing missing was visiting the type. According to bpo-42972, the LRU cache now fully implements the GC protocol, no?

miss-islington added a commit that referenced this pull request May 28, 2021
@miss-islington
bpo-42972: Fully implement GC protocol for functools LRU cache (GH-26423 …
1c0106c 
)

(cherry picked from commit 3f8d332)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@pablogsal

pablogsal commented May 28, 2021

Copy link
Copy Markdown
Member

I am a bit confused about what happened here, the PR mentions that "fully implement the GC protocol" but the change just moves some calls and visits the type....

The LRU cache type already had the Py_TPFLAGS_HAVE_GC flag, and already had a traverse method. The only thing missing was visiting the type. According to bpo-42972, the LRU cache now fully implements the GC protocol, no?

Is there any regression in performance with this or other related changes for the cache? I am asking basically based on @methane previous - efforts to optimize this.

@erlend-aasland

erlend-aasland commented May 28, 2021

Copy link
Copy Markdown
Contributor Author

Is there any regression in performance with this or other related changes for the cache? I am asking basically based on @methane previous - efforts to optimize this.

@methane's optimisations were in the end applied (lru cache link type is visited via the lru cache type traversal slot). I haven't done any benchmarking yet.

@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
@vstinner vstinner mentioned this pull request Jun 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

@methane methane methane approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

@shihai1991 shihai1991 Awaiting requested review from shihai1991

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@erlend-aasland @vstinner @miss-islington @bedevere-bot @pablogsal @methane @the-knights-who-say-ni