msg411075 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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) * |
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) * |
Date: 2022-01-21 17:10 |
I was hoping @eric.snow could tell me.
|
msg411212 - (view) |
Author: Eric Snow (eric.snow) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2022-01-23 21:32 |
@Kumar do you want to tackle this?
|
msg411467 - (view) |
Author: Kumar Aditya (kumaraditya) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:55 | admin | set | github: 90607 |
2022-01-28 13:02:46 | vstinner | set | status: open -> closed resolution: fixed messages:
+ msg411993
stage: patch review -> resolved |
2022-01-28 12:41:43 | vstinner | set | messages:
+ msg411991 |
2022-01-28 11:41:40 | kumaraditya | set | pull_requests:
+ pull_request29166 |
2022-01-28 11:20:17 | kumaraditya | set | pull_requests:
+ pull_request29164 |
2022-01-28 10:14:22 | christian.heimes | set | pull_requests:
+ pull_request29163 |
2022-01-28 05:29:37 | kumaraditya | set | messages:
+ msg411975 |
2022-01-28 05:05:36 | kumaraditya | set | messages:
+ msg411974 |
2022-01-28 04:31:29 | kumaraditya | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request29155 |
2022-01-27 14:47:05 | christian.heimes | set | messages:
+ msg411884 |
2022-01-27 13:15:54 | vstinner | set | messages:
+ msg411868 |
2022-01-24 16:57:44 | gvanrossum | set | messages:
+ msg411496 |
2022-01-24 15:08:20 | vstinner | set | messages:
+ msg411477 |
2022-01-24 12:15:31 | christian.heimes | set | messages:
+ msg411468 |
2022-01-24 12:08:18 | kumaraditya | set | messages:
+ msg411467 |
2022-01-23 21:32:37 | gvanrossum | set | nosy:
+ kumaraditya messages:
+ msg411419
|
2022-01-23 18:54:23 | christian.heimes | set | messages:
+ msg411401 |
2022-01-23 01:17:02 | gvanrossum | set | nosy:
+ christian.heimes messages:
+ msg411327
|
2022-01-23 00:30:11 | vstinner | set | messages:
+ msg411316 |
2022-01-22 23:45:49 | vstinner | set | messages:
+ msg411308 |
2022-01-21 23:57:05 | eric.snow | set | messages:
+ msg411212 |
2022-01-21 17:10:04 | gvanrossum | set | messages:
+ msg411152 |
2022-01-21 08:14:33 | vstinner | set | messages:
+ msg411095 |
2022-01-21 08:13:36 | vstinner | set | messages:
+ msg411094 |
2022-01-21 05:01:26 | gvanrossum | set | messages:
+ msg411083 |
2022-01-21 03:05:32 | vstinner | set | nosy:
+ gvanrossum, eric.snow
|
2022-01-21 03:02:06 | vstinner | create | |