Skip to content

Conversation

@noamcohen97
Copy link
Contributor

@noamcohen97noamcohen97 commented Jun 23, 2025

@noamcohen97noamcohen97force-pushed the refcount-call-tuple-1 branch from f99ffdd to c7c4873CompareJune 23, 2025 20:24
@Fidget-Spinner
Copy link
Member

Nice! That was fast. This PR LGTM, but we need to wait will Mark merges his register allocation PR to get the benefits of this work. See my other comment here #135818 (comment)

@Fidget-Spinner
Copy link
Member

Cool. I see an ~5% improvement in this code:

def foo(): for x in range(200000000): tuple(()) return None foo() 
Before: real 0m3.497s After: real 0m3.323s 

Considering only one reference count was eliminated, this is good news for everything else that will remove more!

@Fidget-SpinnerFidget-Spinner merged commit a78f43b into python:mainDec 11, 2025
74 checks passed
@Fidget-Spinner
Copy link
Member

@markshannon sorry can you please check if this is correct? I think the ERROR_IF might be misplaced, sorry!

@Fidget-Spinner
Copy link
Member

the generated code looks correct but I wonder if the C analyzer treats it any differently

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Dec 11, 2025

@markshannon sorry can you please check if this is correct? I think the ERROR_IF might be misplaced, sorry!

So this seems to be the case. If you look at the error case, it leaks a reference on exit, as it doesn't decref/close it. Sorry I have to revert this PR, or provide a fix. So sorry again!

Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Dec 11, 2025
Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Dec 11, 2025
@markshannon
Copy link
Member

I think the ERROR_IF might be misplaced, sorry!

Yes, this leaks arg

Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Dec 12, 2025
@vstinner
Copy link
Member

I confirm that this change introduced a memory leak in the following tests:

7 tests failed: test.test_multiprocessing_fork.test_processes test.test_multiprocessing_fork.test_threads test.test_multiprocessing_forkserver.test_processes test.test_multiprocessing_forkserver.test_threads test.test_multiprocessing_spawn.test_processes test.test_multiprocessing_spawn.test_threads test_dbm_dumb 

Example AMD64 Fedora Stable Refleaks 3.x: https://buildbot.python.org/#/builders/320/builds/3425

$ ./python -m test test.test_multiprocessing_fork.test_threads -R 3:3 -m test_map_handle_iterable_exception test.test_multiprocessing_fork.test_threads leaked [4, 4, 4] references, sum=12 test.test_multiprocessing_fork.test_threads leaked [2, 2, 2] memory blocks, sum=6 

@vstinner
Copy link
Member

@Fidget-Spinner wrote #142604 to fix the leak.

@noamcohen97
Copy link
ContributorAuthor

Sorry about that! Thanks @Fidget-Spinner 🙏

@vstinner
Copy link
Member

@Fidget-Spinner wrote #142604 to fix the leak.

He wrote another change to fix the leak: #142620.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@noamcohen97@Fidget-Spinner@markshannon@vstinner