GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-42972: Fully implement GC protocol for functools LRU cache by erlend-aasland · Pull Request #26423 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Erlend E. Aasland
bpo-42972: Fully implement GC protocol for functools LRU cache
3573c4d
@erlend-aasland
erlend-aasland requested a review from rhettinger as a code owner May 28, 2021 08:25
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 28, 2021
@bedevere-bot bedevere-bot added the awaiting review label May 28, 2021
@erlend-aasland
erlend-aasland requested review from methane, shihai1991 and vstinner May 28, 2021 08:25
@erlend-aasland erlend-aasland added needs backport to 3.10 skip news labels May 28, 2021
methane
methane approved these changes May 28, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels May 28, 2021
@erlend-aasland erlend-aasland mentioned this pull request May 28, 2021
bpo-42972: Fully implement GC protocol for functools keywrapper and partial types #26363
Merged
vstinner
vstinner reviewed May 28, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_functoolsmodule.c
Py_VISIT(Py_TYPE(self));
lru_list_elem *link = self->root.next;
while (link != &self->root) {
lru_list_elem *next = link->next;

@vstinner vstinner May 28, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add a comment (with a reference to bpo-42972) somewhere (here may be good place) to explain why the link type doesn't implement the GC protocol.

I understand that the code works as if the GC protocol is implemented, but the code is inlined.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_functoolsmodule.c
lru_list_elem *next = link->next;
Py_VISIT(link->key);
Py_VISIT(link->result);
Py_VISIT(Py_TYPE(link));

@vstinner vstinner May 28, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nitpick: I suggest to visit the type first, to visit object members in their definition order.

I'm not sure that I get the rationale why the link type doesn't implement the GC protocol but is "exposed" by this Py_VISIT() call in gc.get_objects(). I'm not sure that it's a good idea to visit the type.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland May 28, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nitpick: I suggest to visit the type first, to visit object members in their definition order.

Sorry, I was a little bit too fast there. We can fix it later if needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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:

  • https://bugs.python.org/issue32422
  • https://github.com/python/cpython/pull/5008/files

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label May 28, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2021
Erlend Egeberg Aasland @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 <[email protected]>
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.10 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request May 28, 2021
@miss-islington Erlend Egeberg Aasland
bpo-42972: Fully implement GC protocol for functools LRU cache (GH-26423
1c0106c 
)

(cherry picked from commit 3f8d332)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal pablogsal mentioned this pull request Jul 18, 2021
[3.10] Correct the order of regen-abidump #27228
Closed
@vstinner vstinner mentioned this pull request Jun 9, 2022
[C API] Heap types (PyType_FromSpec) must fully implement the GC protocol #87138
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@erlend-aasland @vstinner @miss-islington @bedevere-bot @pablogsal @methane @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.