Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-93728: fix memory leak in deepfrozen code objects #93729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jun 11, 2022

Closes #93728

@kumaraditya303
Copy link
Contributor Author

@kumaraditya303 kumaraditya303 commented Jun 11, 2022

Skipping news as no release is affected.

@kumaraditya303
Copy link
Contributor Author

@kumaraditya303 kumaraditya303 commented Jun 11, 2022

Valgrind report with fix:

==27367== HEAP SUMMARY:
==27367==     in use at exit: 0 bytes in 0 blocks
==27367==   total heap usage: 19,728 allocs, 19,728 frees, 2,629,532 bytes allocated
==27367== 
==27367== All heap blocks were freed -- no leaks are possible
==27367== 
==27367== Use --track-origins=yes to see where uninitialised values come from
==27367== For lists of detected and suppressed errors, rerun with: -s
==27367== ERROR SUMMARY: 40 errors from 12 contexts (suppressed: 0 from 0)

@kumaraditya303 kumaraditya303 requested a review from gvanrossum Jun 11, 2022
@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jun 11, 2022

I had an inkling that something was off about my PR (things were going too well :), but I'm really glad you caught this before any releases went out. Thanks again Kumar.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

From my very rudimentary understanding of deepfreeze, this LGTM.

@kumaraditya303
Copy link
Contributor Author

@kumaraditya303 kumaraditya303 commented Jun 11, 2022

I'm really glad you caught this before any releases went out. Thanks again Kumar.

Yeah, its nice that no release has this leak and I caught this early in 3.12 dev cycle.

Copy link
Member

@gvanrossum gvanrossum left a comment

LGTM.

The co_code field is a cache for the co_code attribute, initialized to NULL in init_code(), set to a bytes object (caching the deoptimized code) in _PyCode_GetCode() and decref'ed in code_dealloc(). It is also (implicitly) initialized to NULL in deepfreeze (since the real bytecode lives in co_code_adaptive). So it makes sense that clearing it is the right thing to do when restoring the initial state.

(Using .replace() sets co_code to the deoptimized code if it wasn't passed in, I think that is due to the way that API works.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants