Skip to content

gh-134584: Eliminate redundant refcounting from _CALL_TUPLE_1 - #135860

Merged
Fidget-Spinner merged 6 commits into
python:mainfrom
noamcohen97:refcount-call-tuple-1
Dec 11, 2025
Merged

gh-134584: Eliminate redundant refcounting from _CALL_TUPLE_1#135860
Fidget-Spinner merged 6 commits into
python:mainfrom
noamcohen97:refcount-call-tuple-1

Conversation

@noamcohen97

@noamcohen97noamcohen97 commented Jun 23, 2025

Copy link
Copy Markdown
Contributor

@noamcohen97
noamcohen97force-pushed the refcount-call-tuple-1 branch from f99ffdd to c7c4873CompareJune 23, 2025 20:24
@Fidget-Spinner

Copy link
Copy Markdown
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)

Comment threadPython/bytecodes.c Outdated
Comment threadPython/bytecodes.c
@Fidget-Spinner

Copy link
Copy Markdown
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-Spinner
Fidget-Spinner merged commit a78f43b into python:mainDec 11, 2025
74 checks passed
@Fidget-Spinner

Copy link
Copy Markdown
Member

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

@Fidget-Spinner

Copy link
Copy Markdown
Member

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

@Fidget-Spinner

Fidget-Spinner commented Dec 11, 2025

Copy link
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@noamcohen97

Copy link
Copy Markdown
ContributorAuthor

Sorry about that! Thanks @Fidget-Spinner 🙏

@vstinner

Copy link
Copy Markdown
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