gh-108634: Py_TRACE_REFS uses a hash table#108663
Conversation
|
Py_TRACE_REFS adds
These changes have no impact on the ABI. So the Py_TRACE_REFS is now ABI compatible with release and debug builds. |
Sorry, something went wrong.
|
!buildbot TraceRefs |
Sorry, something went wrong.
Oh, it seems like "AMD64 Arch Linux TraceRefs 3.x" buildbot https://buildbot.python.org/all/#/builders/484 is not used from this GitHub command, |
Sorry, something went wrong.
|
|
Sorry, something went wrong.
Sorry, something went wrong.
|
Failed builds:
test_zippath_from_non_installed_posix() and test_upgrade_dependencies() of test_venv failed: known issue #103224, unrelated to this PR. |
Sorry, something went wrong.
|
I'm not qualified to review this, but nevertheless, here's my unqualified opinion: the idea of using a hash map instead of patching the object layout seems good to me; making the trace ref build ABI compatible with ordinary build seems like a big win. I'll leave the proper review to someone who knows the GC better (for example, Pablo was already CC'd on this PR). The configure changes are fine. |
Sorry, something went wrong.
|
I removed the force parameter and rebased the PR on the main branch. |
Sorry, something went wrong.
|
How often is Py_TRACE_REFS used? Did you measure the overhead of your new implementation compared to the old one? |
Sorry, something went wrong.
|
For a debug build, I don't really care of the memory usage (if it's "reasonable"). For example, enabling tracemalloc to trace Python memory allocation almost doubles the Python memory usage! This PR increases the peak memory usage of a
As far as I know, the main user is @pablogsal's TraceRefs buildbot which makes sure that Python still builds successfully with Py_TRACE_REFS (and that the test suite pass, as well). On Python 3.7 and older, I expect that the main difference of my change is the memory usage. I measured it with: Python built with: Memoy usage of Py_REF_DEBUG + Py_TRACE_REFS:
I ran the command 3 times, the first run has a higher peak memory usage, maybe because the first one used more memory to build PYC files. By the way, For comparison, memory usage of Py_REF_DEBUG (without Py_TRACE_REFS):
On the main branch, Py_TRACE_REFS adds 1 172 kiB (+11%). |
Sorry, something went wrong.
|
Well, why do we care about improving Py_TRACE_REFS if we think nobody uses it? :-) |
Sorry, something went wrong.
I would like to convert some stdlib C extensions to the limited C API:
The practical problem is that we do have a TraceRefs buildbot, and I care of having only green buildbots. Also, since it's unclear to me who uses TraceRefs, I prefer to fix it rather than ask who use it, or propose to deprecate/remove the feature. In the past, I removed COUNT_ALLOCS build, but this one was used by no one, and there was no buildbot :-) Right now, trying to workaround TraceRefs build error with Py_LIMITED_API causes me headaches... I wrote a trivial PR to convert the In short, fixing Py_LIMITED_API support for Py_TRACE_REFS looks simpler to me than trying to workaround Py_TRACE_REFS build errors :-) Also, well, IMO it's nice to have the same ABI for all Python builds! It should make these builds usable in more cases. I always wanted to give a try to the hash table idea. As you can see, it's not that complicated. |
Sorry, something went wrong.
|
On my _stat PR, @AA-Turner seems to be worried of breaking TraceRefs buildbot: #108573 (comment) |
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Hmm, I don't think I have ever used Py_TRACE_REFS for CPython core development. |
Sorry, something went wrong.
I don't use Py_TRACE_REFS for that: https://vstinner.github.io/debug-python-refleak.html I'm not sure what are the use cases of Py_TRACE_REFS and the |
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
I checked that this change does not introduce new memory leak: There is no leak. For comparison, if I comment _PyObject_FiniState() in PyInterpreterState_Delete(), leak on purpose for the test: |
Sorry, something went wrong.
Commit 13a0007 (python#108663) made all Python builds compatible with the Limited API, and removed the LIMITED_API_AVAILABLE flag. However, some tests were still checking for that flag, so they were now being incorrectly skipped. Remove these checks to let these tests run again. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Commit 13a0007 (python#108663) made all Python builds compatible with the Limited API, and removed the LIMITED_API_AVAILABLE flag. However, some tests were still checking for that flag, so they were now being incorrectly skipped. Remove these checks to let these tests run again. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
…09046) Commit 13a0007 (#108663) made all Python builds compatible with the Limited API, and removed the LIMITED_API_AVAILABLE flag. However, some tests were still checking for that flag, so they were now being incorrectly skipped. Remove these checks to let these tests run again. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Python built with "configure --with-trace-refs" (tracing references) is now ABI compatible with Python release build and debug build. Moreover, it now also supports the Limited API.
Change Py_TRACE_REFS build:
Test changes for Py_TRACE_REFS:
📚 Documentation preview 📚: https://cpython-previews--108663.org.readthedocs.build/