Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

bpo-42536: GC track recycled tuples#23623

Merged
brandtbucher merged 23 commits into
python:masterpython/cpython:masterfrom
brandtbucher:untracked-zip-resultbrandtbucher/cpython:untracked-zip-resultCopy head branch name to clipboard
Dec 5, 2020
Merged

bpo-42536: GC track recycled tuples#23623
brandtbucher merged 23 commits into
python:masterpython/cpython:masterfrom
brandtbucher:untracked-zip-resultbrandtbucher/cpython:untracked-zip-resultCopy head branch name to clipboard

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Dec 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

This fixes possibly untracked reference cycles in:

  • collections.OrderedDict.items
  • dict.items
  • enumerate
  • functools.reduce
  • itertools.combinations
  • itertools.combinations_with_replacement
  • itertools.permutations
  • itertools.product
  • itertools.zip_longest
  • zip

https://bugs.python.org/issue42536

@brandtbucher brandtbucher added type-bug An unexpected behavior, bug, or error needs backport to 3.8 labels Dec 3, 2020
@brandtbucher brandtbucher requested a review from vstinner December 3, 2020 04:32
@brandtbucher brandtbucher self-assigned this Dec 3, 2020
vstinner
vstinner reviewed Dec 3, 2020
View reviewed changes
Comment thread Lib/test/test_builtin.py Outdated
Comment thread Lib/test/test_builtin.py Outdated
Comment thread Lib/test/test_builtin.py Outdated
Comment thread Lib/test/test_builtin.py
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst Outdated
Comment thread Python/bltinmodule.c Outdated
@brandtbucher brandtbucher requested a review from vstinner December 3, 2020 16:58
pablogsal
pablogsal previously requested changes Dec 3, 2020
View reviewed changes

@pablogsal pablogsal 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.

Thanks @brandtbucher for giving this a go. I think that there is a better approach here: force the result to be tracked on the visiting function. This has the advantage of adding only an overhead when the GC calls the visiting function over the zip object as opposed to pay per call to next(). To be clear, this is what I mean:

 a/Python/bltinmodule.c b/Python/bltinmodule.c
index a73b8cb320..b05239c037 100644
--- a/Python/bltinmodule.c
+++ b/Python/bltinmodule.c
@@ -2604,6 +2604,9 @@ static int
 zip_traverse(zipobject *lz, visitproc visit, void *arg)
 {
     Py_VISIT(lz->ittuple);
+    if (!_PyObject_GC_IS_TRACKED(lz->result)) {
+        _PyObject_GC_TRACK(Zz->result);
+    }
     Py_VISIT(lz->result);
     return 0;
 }

@bedevere-bot

bedevere-bot commented Dec 3, 2020

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal pablogsal dismissed their stale review December 3, 2020 20:12

Actually, I am not that sure that this is the better way, as the current approach is formally correct as this should be done when the tuple is mutated (as we do in other containers).

@pablogsal

pablogsal commented Dec 3, 2020

Copy link
Copy Markdown
Member

The only reason I think we should consider doing this on the visiting function is due to these results:

https://bugs.python.org/msg382365

@brandtbucher

brandtbucher commented Dec 3, 2020

Copy link
Copy Markdown
Member Author

Thanks Pablo. See my thoughts here:

https://bugs.python.org/msg382455

If that doesn't worry you at all, I'm fine moving this to the traverse function.

@brandtbucher

brandtbucher commented Dec 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Oh, wait. I see now that GitHub says you "dismissed" your "stale review". I'm not sure what that means... are we in agreement here to keep this as-is?

@pablogsal

pablogsal commented Dec 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Oh, wait. I see now that GitHub says you "dismissed" your "stale review". I'm not sure what that means... are we in agreement here to keep this as-is?

Sorry, I commented it here: https://bugs.python.org/msg382458. I think that unless we see a horrendous performance regression, I prefer this approach than the one I proposed using tp_traverse.

vstinner
vstinner reviewed Dec 4, 2020
View reviewed changes
Comment thread Lib/test/test_dict.py Outdated
Comment thread Lib/test/test_itertools.py Outdated
vstinner
vstinner approved these changes Dec 4, 2020
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.

LGTM, but I left some minor coding style remarks (you're free to ignore them).

I also suggest you to copy your NEWS entry as the commit message.

Comment thread Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst Outdated
pablogsal
pablogsal approved these changes Dec 4, 2020
View reviewed changes

@pablogsal pablogsal 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.

🚀

vstinner
vstinner approved these changes Dec 4, 2020
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.

LGTM. Thanks for the latest round of updates ;-)

@brandtbucher brandtbucher merged commit 226a012 into python:master Dec 5, 2020
@miss-islington

miss-islington commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

Sorry, @brandtbucher, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 226a012d1cd61f42ecd3056c554922f359a1a35d 3.9

@miss-islington

miss-islington commented Dec 5, 2020

Copy link
Copy Markdown
Contributor

Sorry @brandtbucher, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 226a012d1cd61f42ecd3056c554922f359a1a35d 3.8

brandtbucher added a commit to brandtbucher/cpython that referenced this pull request Dec 5, 2020
@brandtbucher
bpo-42536: GC track recycled tuples (pythonGH-23623)
b77bae9 
Several built-in and standard library types now ensure that their internal result tuples are always tracked by the garbage collector:

- collections.OrderedDict.items
- dict.items
- enumerate
- functools.reduce
- itertools.combinations
- itertools.combinations_with_replacement
- itertools.permutations
- itertools.product
- itertools.zip_longest
- zip

Previously, they could have become untracked by a prior garbage collection.
(cherry picked from commit 226a012)
brandtbucher added a commit to brandtbucher/cpython that referenced this pull request Dec 5, 2020
@brandtbucher
bpo-42536: GC track recycled tuples (pythonGH-23623)
db2788a 
Several built-in and standard library types now ensure that their internal result tuples are always tracked by the garbage collector:

- collections.OrderedDict.items
- dict.items
- enumerate
- functools.reduce
- itertools.combinations
- itertools.combinations_with_replacement
- itertools.permutations
- itertools.product
- itertools.zip_longest
- zip

Previously, they could have become untracked by a prior garbage collection.
(cherry picked from commit 226a012)
@bedevere-bot

bedevere-bot commented Dec 5, 2020

Copy link
Copy Markdown

GH-23651 is a backport of this pull request to the 3.9 branch.

@bedevere-bot

bedevere-bot commented Dec 5, 2020

Copy link
Copy Markdown

GH-23652 is a backport of this pull request to the 3.8 branch.

pablogsal pushed a commit that referenced this pull request Dec 7, 2020
@brandtbucher
bpo-42536: GC track recycled tuples (GH-23623) (GH-23651)
60463e8 
Several built-in and standard library types now ensure that their internal result tuples are always tracked by the garbage collector:

- collections.OrderedDict.items
- dict.items
- enumerate
- functools.reduce
- itertools.combinations
- itertools.combinations_with_replacement
- itertools.permutations
- itertools.product
- itertools.zip_longest
- zip

Previously, they could have become untracked by a prior garbage collection.
(cherry picked from commit 226a012)
pablogsal pushed a commit that referenced this pull request Dec 7, 2020
@brandtbucher
bpo-42536: GC track recycled tuples (GH-23623) (GH-23652)
7c79798 
Several built-in and standard library types now ensure that their internal result tuples are always tracked by the garbage collector:

- collections.OrderedDict.items
- dict.items
- enumerate
- functools.reduce
- itertools.combinations
- itertools.combinations_with_replacement
- itertools.permutations
- itertools.product
- itertools.zip_longest
- zip

Previously, they could have become untracked by a prior garbage collection.
(cherry picked from commit 226a012)
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@brandtbucher @adorilson
bpo-42536: GC track recycled tuples (pythonGH-23623)
3c0e72e 
Several built-in and standard library types now ensure that their internal result tuples are always tracked by the garbage collector:

- collections.OrderedDict.items
- dict.items
- enumerate
- functools.reduce
- itertools.combinations
- itertools.combinations_with_replacement
- itertools.permutations
- itertools.product
- itertools.zip_longest
- zip

Previously, they could have become untracked by a prior garbage collection.
@brandtbucher brandtbucher deleted the untracked-zip-result branch July 21, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@methane methane methane approved these changes

@rhettinger rhettinger rhettinger approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@pablogsal pablogsal pablogsal approved these changes

@ethanfurman ethanfurman Awaiting requested review from ethanfurman

@markshannon markshannon Awaiting requested review from markshannon

Assignees

@brandtbucher brandtbucher

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@brandtbucher @bedevere-bot @pablogsal @miss-islington @vstinner @methane @rhettinger @serhiy-storchaka @the-knights-who-say-ni
Morty Proxy This is a proxified and sanitized view of the page, visit original site.