Skip to content

Conversation

@noamcohen97
Copy link
Contributor

@noamcohen97 noamcohen97 commented Jun 23, 2025

@noamcohen97 noamcohen97 force-pushed the refcount-call-tuple-1 branch from f99ffdd to c7c4873 Compare June 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-Spinner Fidget-Spinner merged commit a78f43b into python:main Dec 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!

@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
Contributor Author

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 free to 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