[Python] Don't exploit garbage collection hooks for memory regulator#21111
Open
guitargeek wants to merge 2 commits intoroot-project:masterfrom
Open
[Python] Don't exploit garbage collection hooks for memory regulator#21111guitargeek wants to merge 2 commits intoroot-project:masterfrom
guitargeek wants to merge 2 commits intoroot-project:masterfrom
Conversation
guitargeek
force-pushed
the
test_templates
branch
from
e307f1d to
7f21de6
Compare
guitargeek
changed the title
Contributor
Author
|
All platforms were green once. Retrying to see possible spurious failures. |
Test Results 22 files 22 suites 3d 13h 2m 5s ⏱️ For more details on these failures, see this check. Results for commit c118bde. ♻️ This comment has been updated with latest results. |
Contributor
Author
|
Another CI run without any Python test failures on any platform has passed. |
guitargeek
force-pushed
the
test_templates
branch
from
7f21de6 to
f9c614b
Compare
Contributor
Author
|
Unfortunately, we saw a crash in ./../../../../../src/bindings/pyroot/cppyy/cppyy/test/test_templates.py::TestTEMPLATES::test33_using_template_argument *** Break *** segmentation violation
Generating stack trace...
0x0000ffff839912e4 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8392cf84 in _PyEval_EvalFrameDefault + 0x3abc from /lib64/libpython3.12.so.1.0
0x0000ffff839aa770 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a78d14 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8396faa0 in PyObject_Str + 0x200 from /lib64/libpython3.12.so.1.0
0x0000ffff8392dba4 in _PyEval_EvalFrameDefault + 0x46dc from /lib64/libpython3.12.so.1.0
0x0000ffff839c5f20 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8395c59c in PyObject_RichCompare + 0x8c from /lib64/libpython3.12.so.1.0
0x0000ffff8392b52c in _PyEval_EvalFrameDefault + 0x2064 from /lib64/libpython3.12.so.1.0
0x0000ffff83981a30 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8392defc in _PyEval_EvalFrameDefault + 0x4a34 from /lib64/libpython3.12.so.1.0
0x0000ffff83981a30 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8392defc in _PyEval_EvalFrameDefault + 0x4a34 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff83a04448 in PyEval_EvalCode + 0xa8 from /lib64/libpython3.12.so.1.0
0x0000ffff83a5b9a8 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a54bc8 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a4e748 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a4dc9c in _PyRun_SimpleFileObject + 0x1b4 from /lib64/libpython3.12.so.1.0
0x0000ffff83a4d7d8 in _PyRun_AnyFileObject + 0x54 from /lib64/libpython3.12.so.1.0
0x0000ffff83a486d8 in Py_RunMain + 0x3d4 from /lib64/libpython3.12.so.1.0
0x0000ffff839e7200 in Py_BytesMain + 0x40 from /lib64/libpython3.12.so.1.0
0x0000ffff836760dc in <unknown> from /lib64/libc.so.6
0x0000ffff836761bc in __libc_start_main at :? from /lib64/libc.so.6
0x0000aaaad3a90930 in _start + 0x30 from /py-venv/ROOT-CI/bin/python3.12
*** Break *** segmentation violation
Generating stack trace...
0x0000ffff839912e4 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8392cf84 in _PyEval_EvalFrameDefault + 0x3abc from /lib64/libpython3.12.so.1.0
0x0000ffff839aa770 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a78d14 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8396faa0 in PyObject_Str + 0x200 from /lib64/libpython3.12.so.1.0
0x0000ffff8392dba4 in _PyEval_EvalFrameDefault + 0x46dc from /lib64/libpython3.12.so.1.0
0x0000ffff839c5f20 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8395c59c in PyObject_RichCompare + 0x8c from /lib64/libpython3.12.so.1.0
0x0000ffff8392b52c in _PyEval_EvalFrameDefault + 0x2064 from /lib64/libpython3.12.so.1.0
0x0000ffff83981a30 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8392defc in _PyEval_EvalFrameDefault + 0x4a34 from /lib64/libpython3.12.so.1.0
0x0000ffff83981a30 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8392defc in _PyEval_EvalFrameDefault + 0x4a34 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff8391f924 in _PyObject_FastCallDictTstate + 0x360 from /lib64/libpython3.12.so.1.0
0x0000ffff83962b04 in _PyObject_Call_Prepend + 0x84 from /lib64/libpython3.12.so.1.0
0x0000ffff83a6a718 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff8391ade8 in _PyObject_MakeTpCall + 0x2e8 from /lib64/libpython3.12.so.1.0
0x0000ffff83929b8c in _PyEval_EvalFrameDefault + 0x6c4 from /lib64/libpython3.12.so.1.0
0x0000ffff83a04448 in PyEval_EvalCode + 0xa8 from /lib64/libpython3.12.so.1.0
0x0000ffff83a5b9a8 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a54bc8 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a4e748 in <unknown> from /lib64/libpython3.12.so.1.0
0x0000ffff83a4dc9c in _PyRun_SimpleFileObject + 0x1b4 from /lib64/libpython3.12.so.1.0
0x0000ffff83a4d7d8 in _PyRun_AnyFileObject + 0x54 from /lib64/libpython3.12.so.1.0
0x0000ffff83a486d8 in Py_RunMain + 0x3d4 from /lib64/libpython3.12.so.1.0
0x0000ffff839e7200 in Py_BytesMain + 0x40 from /lib64/libpython3.12.so.1.0
0x0000ffff836760dc in <unknown> from /lib64/libc.so.6
0x0000ffff836761bc in __libc_start_main at :? from /lib64/libc.so.6
0x0000aaaad3a90930 in _start + 0x30 from /py-venv/ROOT-CI/bin/python3.12
Make Error at /github/home/ROOT-CI/src/cmake/modules/RootTestDriver.cmake:253 (message):
error code: 129 |
guitargeek
force-pushed
the
test_templates
branch
from
4564ec4 to
b1caeaa
Compare
Contributor
Vipul-Cariappa
left a comment
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
CPPInstance should participate in reference cycle garbage collection.
Why?
It has only 2 datamembers:
// CPPInstance.h
void* fObject;
uint32_t fFlags;where fObject is pointer to the c++ object. Unless fFlag is set to kIsExtended. In that case fObject is pointer to ExtendedData. And ExtendedData can potentially hold PyObject* that points to itself.
But we don't have an implementation of tp_traverse for CPPInstance, which should cause memory leaks (unfreed memory).
Also, I am not sure op_clear is doing what it is supposed to do.
Comment on lines
-450
to
-452
| PyObject_GC_UnTrack((PyObject*)pyobj); | ||
| op_dealloc_nofree(pyobj); | ||
| PyObject_GC_Del((PyObject*)pyobj); |
Contributor
There was a problem hiding this comment.
I believe we should have PyObject_GC_UnTrack and the flag Py_TPFLAGS_HAVE_GC. One thing I can think of is a LinkedList kind of a case where the object as reference to itself.
I experimented with a few use cases, but the required destructor ran even without the Py_TPFLAGS_HAVE_GC flag.
But PyObject_GC_Del should be changed to Py_TYPE(pyobj)->tp_free((PyObject*)pyobj);
Contributor
Author
|
Very good comments, I didn't think about the extended object case! But it's good we both came to the conclusion that the cyclic garbage collection needs to be fixed here |
Looking at the CPPInstance, I noticed that is claims to support cycling garbage collection via the `Py_TPFLAGS_HAVE_GC`, but it doesn't stick completely to the contract explained in the docs: https://docs.python.org/3/c-api/gcsupport.html#c.PyObject_GC_Track In particular, it breaks the constructor rules: * The memory for the object must be allocated using PyObject_GC_New or PyObject_GC_NewVar. * Once all the fields which may contain references to other containers are initialized, it must call PyObject_GC_Track(). The `tp_traverse` and `tp_clear` implementations actually do nothing with the C Python API, so from the point of view of Python, there is no reason to support cyclic garbage collection. I think the CPPInstance tries to exploit the Python garbage collector to trigger its own memory regulator code path early in `tp_clear`, but breaking some of the Python C API contracts on the way. And the C++ object is also unregisterd from the memory regulator in `tp_dealloc` anyway. This might be the reason for the spurious failures in the CI, which happen *between* two tests, hinting to garbage collector problems. I think it's particularly problematic that the CPPInstance never registers itself to the GC with `PyObject_GC_Track`, but then uses `PyObject_GC_UnTrack`. That probably brings the garbage collector in a bad state. This reverts parts of the following CPyCppy commits from 2019: * wlav/CPyCppyy@e43759a * wlav/CPyCppyy@29eb42e
guitargeek
force-pushed
the
test_templates
branch
from
b1caeaa to
c118bde
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Looking at the CPPInstance, I noticed that is claims to support cycling
garbage collection via the
Py_TPFLAGS_HAVE_GC, but it doesn't stickcompletely to the contract explained in the docs:
https://docs.python.org/3/c-api/gcsupport.html#c.PyObject_GC_Track
In particular, it breaks the constructor rules:
The memory for the object must be allocated using PyObject_GC_New or
PyObject_GC_NewVar.
Once all the fields which may contain references to other containers
are initialized, it must call PyObject_GC_Track().
The
tp_traverseandtp_clearimplementations actually do nothingwith the C Python API, so from the point of view of Python, there is no
reason to support cyclic garbage collection.
I think the CPPInstance tries to exploit the Python garbage collector to
trigger its own memory regulator code path early in
tp_clear, butbreaking some of the Python C API contracts on the way. And the C++
object is also unregisterd from the memory regulator in
tp_deallocanyway.
This might be the reason for the spurious failures in the CI, which
happen between two tests, hinting to garbage collector problems.
I think it's particularly problematic that the CPPInstance never
registers itself to the GC with
PyObject_GC_Track, but then usesPyObject_GC_UnTrack. That probably brings the garbage collector in abad state.
This reverts parts of the following CPyCppy commits from 2019: