This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Deep-freezed modules create inconsistency in sys.gettotalrefcount() (_Py_Reftotal)
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, eric.snow, gvanrossum, kumaraditya, vstinner
Priority: normal Keywords: patch

Created on 2022-01-21 03:02 by vstinner, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 30976 closed kumaraditya, 2022-01-28 04:31
PR 30984 closed christian.heimes, 2022-01-28 10:14
PR 30985 closed kumaraditya, 2022-01-28 11:20
PR 30987 merged kumaraditya, 2022-01-28 11:41
Messages (21)
msg411075 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-21 03:02
Using the C program below, I see that _Py_RefTotal is decreasing at each iteration:
---
#include <Python.h>
int main(int argc, char *argv[])
{
    for (int i=1; i <= 100; i++) {
        Py_SetProgramName(L"./_testembed");
        Py_Initialize();
        Py_Finalize();
        printf("Loop #%d: %zd refs\n", i, _Py_RefTotal);
    }
}
---

Example of output:
---
...
Loop #96: 9557 refs
Loop #97: 9544 refs
Loop #98: 9531 refs
Loop #99: 9518 refs
Loop #100: 9505 refs
---

It seems to be a regression caused by this change:

commit 1cbaa505d007e11c4a1f0d2073d72b6c02c7147c
Author: Guido van Rossum <guido@python.org>
Date:   Wed Nov 10 18:01:53 2021 -0800

    bpo-45696: Deep-freeze selected modules (GH-29118)
    
    This gains 10% or more in startup time for `python -c pass` on UNIX-ish systems.
    
    The Makefile.pre.in generating code builds on Eric's work for bpo-45020, but the .c file generator is new.
    
    Windows version TBD.


Before the change, _Py_RefTotal was stable: 
---
...
Loop #97: 10805 refs
Loop #98: 10805 refs
Loop #99: 10805 refs
Loop #100: 10805 refs
---

I found this issue while working on bpo-46417 which is related to bpo-1635741.
msg411083 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-01-21 05:01
Hm, the deep-frozen objects are statically initialized with a very large refcount that isn't accounted for (they are intended to be immortal). It seems that Py_Finalize() somehow decrefs those objects. I guess this means we need some kind of flag indicating certain objects are immortal (Eric has proposed several schemes), then we could just mark these objects as immortal.

This reminds me, since https://github.com/python/cpython/pull/30715 (which I merged yesterday) the deep-frozen objects also reference the small ints directly, as well as the singleton for b"". Is this even safe across Py_Finalize()/Py_Initialize()? If not, we'll need to roll that back as well.
msg411094 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-21 08:13
> Hm, the deep-frozen objects are statically initialized with a very large refcount that isn't accounted for (they are intended to be immortal). It seems that Py_Finalize() somehow decrefs those objects. I guess this means we need some kind of flag indicating certain objects are immortal (Eric has proposed several schemes), then we could just mark these objects as immortal.

The problem is only _Py_RefTotal. Maybe frozen_only_do_patchups() should increment _Py_RefTotal when Python it build with Py_DEBUG macro defined, so it can be safely decremented in Py_Finalize()?

Adding a flag in PyObject/PyTypeObject and modifying Py_DECREF() sounds more controversial. I suggest to do that later ;-) (I am not convinced that it's the best approach.) I would suggest to write a PEP for immortal objects.
msg411095 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-21 08:14
> This reminds me, since https://github.com/python/cpython/pull/30715 (which I merged yesterday) the deep-frozen objects also reference the small ints directly, as well as the singleton for b"". Is this even safe across Py_Finalize()/Py_Initialize()? If not, we'll need to roll that back as well.

I don't know.
msg411152 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-01-21 17:10
I was hoping @eric.snow could tell me.
msg411212 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2022-01-21 23:57
> the deep-frozen objects also reference the small ints directly, as well as the singleton for b"".
> Is this even safe across Py_Finalize()/Py_Initialize()? If not, we'll need to roll that back as well.

The small ints and the empty bytes object each have "immortal" refcounts too (999999999, just like you did in deepfreeze).  So they would cause a similar behavior to what Victor reported.  Otherwise I wouldn't expect any problems across Py_Finalize()/Py_Initialize().
msg411308 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-22 23:45
Is there a way to disable deepfreeze when building Python?

It makes the Python build way slower. For example, a full build (after "make clean") of Python 3.10 takes 14.9 seconds on my laptop, whereas Python 3.11 takes 24.6 seconds (1.6x slower). It makes my workflow (trial-and-error based ;-)) less efficient.

Moreover, I would like to disable it to investigate why _Py_RefTotal is now negative at Python exit:
https://bugs.python.org/issue46417#msg411307

Note: I pushed many changes in bpo-46417 to clear static types and a few "static" objects at Python exit (in Py_Finalize()).
msg411316 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-23 00:30
See also bpo-46476: "Not all memory allocated by _Py_Quicken() is released at Python exit".
msg411327 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-01-23 01:17
> Is there a way to disable deepfreeze when building Python?

It looks like this isn't easy, sorry. :-( Adding Christian Heimes in case he has a suggestion.
msg411401 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-01-23 18:54
If you modify the freeze_modules script to generate code like https://github.com/python/cpython/compare/main...tiran:split_frozen?expand=1 , then I can add a build option to compile Python with only required frozen modules.
msg411419 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-01-23 21:32
@Kumar do you want to tackle this?
msg411467 - (view) Author: Kumar Aditya (kumaraditya) * (Python triager) Date: 2022-01-24 12:08
> @Kumar do you want to tackle this?

I don't like this approach as it is opposite to what we did to reduce the size of deep-frozen modules to merge them in one file and this approach requires to split it again.
msg411468 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-01-24 12:15
Do you have an alternative suggestion how to build Python with minimal set of required deepfrozen modules or without any deepfrozen modules at all? A minimal set is not only helpful for Victor's use case. I would also like to use it in WebAssembly builds to reduce the overall file size.
msg411477 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-24 15:08
I don't want to change the default. Keeping fast startup time is a nice goal!

I'm asking to make it configurable for my own development workflow: build Python as fast as possible.

It is easy to hack Makefile.am.in and Python/frozen.c to freeze less modules. If you want, I can try to work on a patch to make it configurable. Maybe some people want to freeze... *more* modules, rather than less ;-)
msg411496 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-01-24 16:57
I tried to make the 'FROZEN' variable in freeze_modules.py empty, but it has a bunch of places where this is unexpected. Maybe someone can fix that?
msg411868 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-27 13:15
The bpo-46476 added _Py_Deepfreeze_Fini() and _PyStaticCode_Dealloc() functions: commit c7f810b34d91a5c2fbe0a8385562015d2dd961f2. If we need to ajust _Py_RefTotal manually, *maybe* it can be done there?

I don't understand well how static/immortal code object lead to negative _Py_RefTotal. For me, Py_INCREF() and Py_DECREF() should still be used on these objects, no?
msg411884 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-01-27 14:47
The problem is in PyImport_ImportFrozenModuleObject -> unmarshal_frozen_code() -> frozen_info.get_code() -> _Py_get_importlib__bootstrap_external_toplevel() call chain.

PyImport_ImportFrozenModuleObject() expects unmarshal_frozen_code() to return a strong reference to the code object. However a frozen_info struct with a get_code() function returns a borrowed reference from deepfreeze.c's toplevel code object.

# --- test.c
#include <Python.h>
int main(int argc, char *argv[])
{
    for (int i=1; i <= 100; i++) {
        Py_SetProgramName(L"./_testembed");
        Py_Initialize();
        Py_Finalize();
        printf("Loop #%d: %zd refs, bootstrap refs: %zd\n", i, _Py_RefTotal, Py_REFCNT(_Py_get_importlib__bootstrap_external_toplevel()));
    }
}
# ---

$ gcc -IInclude -I. -o test test.c libpython3.11d.a -lm && ./test
Loop #1: -3 refs, bootstrap refs: 999999998
Loop #2: -8 refs, bootstrap refs: 999999997
Loop #3: -13 refs, bootstrap refs: 999999996
Loop #4: -18 refs, bootstrap refs: 999999995
Loop #5: -23 refs, bootstrap refs: 999999994
Loop #6: -28 refs, bootstrap refs: 999999993
Loop #7: -33 refs, bootstrap refs: 999999992
Loop #8: -38 refs, bootstrap refs: 999999991
Loop #9: -43 refs, bootstrap refs: 999999990
Loop #10: -48 refs, bootstrap refs: 999999989


After I changed unmarshal_frozen_code() to "return Py_NewRef(code);", the reference count of the frozen bootstrap module stays stable, but total refcount increases over time:

Loop #1: 10 refs, bootstrap refs: 999999999
Loop #2: 18 refs, bootstrap refs: 999999999
Loop #3: 26 refs, bootstrap refs: 999999999
Loop #4: 34 refs, bootstrap refs: 999999999
Loop #5: 42 refs, bootstrap refs: 999999999
Loop #6: 50 refs, bootstrap refs: 999999999
Loop #7: 58 refs, bootstrap refs: 999999999
Loop #8: 66 refs, bootstrap refs: 999999999
Loop #9: 74 refs, bootstrap refs: 999999999
msg411974 - (view) Author: Kumar Aditya (kumaraditya) * (Python triager) Date: 2022-01-28 05:05
I created a PR https://github.com/python/cpython/pull/30976 which adjusts _Py_RefTotal and refcnt of immortal codeobjects to account for Py_INCREF/Py_DECREF on codeobjects. With that patch refcnt is 8 and increases by 8 with each initialization of Python.
msg411975 - (view) Author: Kumar Aditya (kumaraditya) * (Python triager) Date: 2022-01-28 05:29
Christian's solution seems better to me so I'll close my PR. Christian would you like to create a PR for it ?
msg411991 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-28 12:41
New changeset 5a9e423473bf2c4eb32a0982e8d73420875db1da by Kumar Aditya in branch 'main':
bpo-46449: deepfreeze get_code() now returns strong ref  (GH-30987)
https://github.com/python/cpython/commit/5a9e423473bf2c4eb32a0982e8d73420875db1da
msg411993 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2022-01-28 13:02
With my additional GH-30988 fix, msg411075 example no longer leaks :-)
---
Loop #1: 2 refs
Loop #2: 2 refs
Loop #3: 2 refs
...
Loop #98: 2 refs
Loop #99: 2 refs
Loop #100: 2 refs
---

I close the issue.

Thanks Christian and Kumar for the fix!
History
Date User Action Args
2022-04-11 14:59:55adminsetgithub: 90607
2022-01-28 13:02:46vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg411993

stage: patch review -> resolved
2022-01-28 12:41:43vstinnersetmessages: + msg411991
2022-01-28 11:41:40kumaradityasetpull_requests: + pull_request29166
2022-01-28 11:20:17kumaradityasetpull_requests: + pull_request29164
2022-01-28 10:14:22christian.heimessetpull_requests: + pull_request29163
2022-01-28 05:29:37kumaradityasetmessages: + msg411975
2022-01-28 05:05:36kumaradityasetmessages: + msg411974
2022-01-28 04:31:29kumaradityasetkeywords: + patch
stage: patch review
pull_requests: + pull_request29155
2022-01-27 14:47:05christian.heimessetmessages: + msg411884
2022-01-27 13:15:54vstinnersetmessages: + msg411868
2022-01-24 16:57:44gvanrossumsetmessages: + msg411496
2022-01-24 15:08:20vstinnersetmessages: + msg411477
2022-01-24 12:15:31christian.heimessetmessages: + msg411468
2022-01-24 12:08:18kumaradityasetmessages: + msg411467
2022-01-23 21:32:37gvanrossumsetnosy: + kumaraditya
messages: + msg411419
2022-01-23 18:54:23christian.heimessetmessages: + msg411401
2022-01-23 01:17:02gvanrossumsetnosy: + christian.heimes
messages: + msg411327
2022-01-23 00:30:11vstinnersetmessages: + msg411316
2022-01-22 23:45:49vstinnersetmessages: + msg411308
2022-01-21 23:57:05eric.snowsetmessages: + msg411212
2022-01-21 17:10:04gvanrossumsetmessages: + msg411152
2022-01-21 08:14:33vstinnersetmessages: + msg411095
2022-01-21 08:13:36vstinnersetmessages: + msg411094
2022-01-21 05:01:26gvanrossumsetmessages: + msg411083
2022-01-21 03:05:32vstinnersetnosy: + gvanrossum, eric.snow
2022-01-21 03:02:06vstinnercreate