GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-126703: Add PyCFunction freelist by eendebakpt · Pull Request #128692 · python/cpython

Conversation

eendebakpt commented Jan 9, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

See #128368 and the corresponding issue for details.

Issue: Use freelist for range object, iterator objects and other often used objects #126703

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          add pycfunction freelist

20cc6b9

eendebakpt requested a review from ericsnowcurrently as a code owner

January 9, 2025 20:40

bedevere-app Bot mentioned this pull request

Jan 9, 2025

Use freelist for range object, iterator objects and other often used objects #126703

Closed

bedevere-app Bot added the awaiting review label

Jan 9, 2025

blurb-it Bot and others added 2 commits

January 9, 2025 22:12


          📜🤖 Added by blurb_it.

e86ccad


          Merge branch 'main' into pycfunctionobject_freelist

a76224b

ZeroIntensity approved these changes

Jan 10, 2025

View reviewed changes

ZeroIntensity left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/internal/pycore_freelist_state.h

		# define Py_futureiters_MAXFREELIST 255
		# define Py_object_stack_chunks_MAXFREELIST 4
		# define Py_unicode_writers_MAXFREELIST 1
		# define Py_pycfunctionobject_MAXFREELIST 16

ZeroIntensity Jan 10, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these lengths being chosen? 16 seems fine for PyCFunction, but I would think that methods are created much more frequently.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eendebakpt Jan 14, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choosing the number is a bit of an art (and a good number will depend what kind of code is executed).

How well the freelist works depends on the dynamics of the allocation and deallocation. For example the size of Py_unicode_writers_MAXFREELIST is only 1. I suspect this is because the typical use case is: create a writer, write some data, release it again. And during the writing of the data no new unicode writers are constructed.

With the diff below you can see whether a PyCMethodObject is obtained from the freelist (if the size > 0), or whether a new one is allocated (when size = 0).

diff --git a/Include/internal/pycore_freelist.h b/Include/internal/pycore_freelist.h
index 84a5ab30f3e..90d6616cbfd 100644
--- a/Include/internal/pycore_freelist.h
+++ b/Include/internal/pycore_freelist.h
@@ -28,6 +28,10 @@ _Py_freelists_GET(void)
 #endif
 }

+#define _Py_FREELIST_FREE_PRINT(NAME, op, freefunc) \
+    _PyFreeList_Free_Print(&_Py_freelists_GET()->NAME, _PyObject_CAST(op), \
+                     Py_ ## NAME ## _MAXFREELIST, freefunc, #NAME)
+
 // Pushes `op` to the freelist, calls `freefunc` if the freelist is full
 #define _Py_FREELIST_FREE(NAME, op, freefunc) \
     _PyFreeList_Free(&_Py_freelists_GET()->NAME, _PyObject_CAST(op), \
@@ -69,6 +73,16 @@ _PyFreeList_Free(struct _Py_freelist *fl, void *obj, Py_ssize_t maxsize,
     }
 }

+static inline void
+_PyFreeList_Free_Print(struct _Py_freelist *fl, void *obj, Py_ssize_t maxsize,
+                 freefunc dofree, const char *name)
+{
+    if (!_PyFreeList_Push(fl, obj, maxsize)) {
+        printf("    %s: no space to store object\n", name);
+        dofree(obj);
+    }
+}
+
 static inline void *
 _PyFreeList_PopNoStats(struct _Py_freelist *fl)
 {
diff --git a/Objects/methodobject.c b/Objects/methodobject.c
index fc77055b0a2..58d928d78bf 100644
--- a/Objects/methodobject.c
+++ b/Objects/methodobject.c
@@ -86,6 +86,7 @@ PyCMethod_New(PyMethodDef *ml, PyObject *self, PyObject *module, PyTypeObject *c
                             "flag but no class");
             return NULL;
         }
+        printf("PyCMethodObject: try allocation (freelist size %d)\n", _Py_FREELIST_SIZE(pycmethodobject));
         PyCMethodObject *om = _Py_FREELIST_POP(PyCMethodObject, pycmethodobject);
         if (om == NULL) {
             om = PyObject_GC_New(PyCMethodObject, &PyCMethod_Type);
@@ -102,6 +103,7 @@ PyCMethod_New(PyMethodDef *ml, PyObject *self, PyObject *module, PyTypeObject *c
                             "but no METH_METHOD flag");
             return NULL;
         }
+        printf("PyCFunctionObject: try allocation (freelist size %d)\n", _Py_FREELIST_SIZE(pycfunctionobject));
         op = _Py_FREELIST_POP(PyCFunctionObject, pycfunctionobject);
         if (op == NULL) {
             op = PyObject_GC_New(PyCFunctionObject, &PyCFunction_Type);
@@ -180,11 +182,11 @@ meth_dealloc(PyObject *self)
     Py_XDECREF(m->m_module);
     if (m->m_ml->ml_flags & METH_METHOD) {
         assert(Py_IS_TYPE(self, &PyCMethod_Type));
-        _Py_FREELIST_FREE(pycmethodobject, m, PyObject_GC_Del);
+        _Py_FREELIST_FREE_PRINT(pycmethodobject, m, PyObject_GC_Del);
     }
     else {
         assert(Py_IS_TYPE(self, &PyCFunction_Type));
-        _Py_FREELIST_FREE(pycfunctionobject, m, PyObject_GC_Del);
+        _Py_FREELIST_FREE_PRINT(pycfunctionobject, m, PyObject_GC_Del);
     }
     Py_TRASHCAN_END;
 }

When I use this on python -m test test_operator:

there are many allocations at the start of the program (because nothing has been deallocated, so there is nothing on the freelist)
there is a dynamic section where often objects are allocated and deallocated, the freelist size is changing a lot, but it is not reaching the maximum size of 16
at the end the python interpreter is closing down, so many objects are deallocated

Based on this there would be no need to increase the freelist size. On the other hand, it is almost free to increase the size (the only memory is the objects on the freelist).

I am fine with changing the size (anything between 4 and 400 would be fine with me), but I am not sure it matters or how to make a more informed decision.

Full output is at:

https://gist.github.com/eendebakpt/7a867587450dca4689bb46271fb01ec2

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting core review and removed awaiting review labels

Jan 10, 2025

eendebakpt mentioned this pull request

Jan 14, 2025

gh-126703: Add freelists for iterators and range, method and builtin_function_or_method objects #128368

Closed

vstinner commented Jan 15, 2025

Copy link

Copy Markdown

Member

Would you mind to rerun the benchmark on this PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'main' into pycfunctionobject_freelist

6c9d056

eendebakpt commented Jan 15, 2025

Copy link

Copy Markdown

Contributor Author

Here are benchmarks against current main (Linux, PGO). The first benchmark tests the freelist from this PR, the other two are control benchmarks (they should not be affected):

bench_builtin_or_method: Mean +- std dev: [main0] 5.51 us +- 0.11 us -> [pr] 4.03 us +- 0.07 us: 1.37x faster
bench_property: Mean +- std dev: [main0] 1.68 us +- 0.02 us -> [pr] 1.70 us +- 0.03 us: 1.01x slower
bench_class_method: Mean +- std dev: [main0] 1.90 us +- 0.05 us -> [pr] 1.88 us +- 0.04 us: 1.01x faster

Geometric mean: 1.11x faster

Script

# Quick benchmark for cpython freelists

import pyperf

def bench_builtin_or_method(loops):
    range_it = iter(range(loops))
    tpl = tuple(range(50))

    lst = []
    it = iter(set([2, 3, 4]))
    t0 = pyperf.perf_counter()
    for ii in range_it:
        for ii in tpl:
            lst.append
            it.__length_hint__
    return pyperf.perf_counter() - t0


class A:
    def __init__(self, value):
        self.value = value

    def x(self):
        return self.value

    @property
    def v(self):
        return self.value


def bench_property(loops):
    range_it = iter(range(loops))
    tpl = tuple(range(50))

    t0 = pyperf.perf_counter()
    for ii in range_it:
        a = A(ii)
        for ii in tpl:
            _ = a.v
    return pyperf.perf_counter() - t0


def bench_class_method(loops):
    range_it = iter(range(loops))
    tpl = tuple(range(50))

    t0 = pyperf.perf_counter()
    for ii in range_it:
        a = A(ii)
        for ii in tpl:
            _ = a.x()
    return pyperf.perf_counter() - t0


if __name__ == "__main__":
    runner = pyperf.Runner()
    runner.bench_time_func("bench_builtin_or_method", bench_builtin_or_method)
    runner.bench_time_func("bench_property", bench_property)
    runner.bench_time_func("bench_class_method", bench_class_method)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner approved these changes

Jan 15, 2025

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Jan 15, 2025


          Merge branch 'main' into pycfunctionobject_freelist

7a165db

mdboom commented Jan 27, 2025

Copy link

Copy Markdown

Contributor

I'm kicking off a run of pyperformance on this PR on our benchmarking infrastructure and will report back here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdboom commented Jan 27, 2025

Copy link

Copy Markdown

Contributor

Results are here. No change overall, but the mako benchmark has a meaningful improvement. https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20250127-3.14.0a4%2B-7a165db/bm-20250127-linux-x86_64-eendebakpt-pycfunctionobject_fr-3.14.0a4%2B-7a165db-vs-base.svg

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eendebakpt mentioned this pull request

Jan 28, 2025

gh-126703: Add freelists for list and tuple iterators #128592

Merged

Fidget-Spinner approved these changes

Apr 5, 2025

View reviewed changes

Fidget-Spinner merged commit 29772b0 into python:main

Apr 5, 2025

bedevere-app Bot removed the awaiting merge label

Apr 5, 2025

hawkinsp commented Apr 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

I think this change introduced a use-after-free we're seeing in our Python 3.14 free-threading CI.

TSAN reports:

WARNING: ThreadSanitizer: heap-use-after-free (pid=45389)
  Read of size 4 at 0x7208000b2110 by thread T3 (mutexes: write M0, write M1):
    #0 meth_dealloc /__w/jax/jax/cpython/Objects/methodobject.c:181:18 (python3.14+0x29bb61) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #1 _Py_Dealloc /__w/jax/jax/cpython/Objects/object.c:3023:5 (python3.14+0x2a2452) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #2 _Py_MergeZeroLocalRefcount /__w/jax/jax/cpython/Objects/object.c (python3.14+0x2a2452)
    #3 Py_DECREF /__w/jax/jax/cpython/./Include/refcount.h:387:13 (python3.14+0x2744af) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #4 Py_XDECREF /__w/jax/jax/cpython/./Include/refcount.h:526:9 (python3.14+0x2744af)
    #5 insertdict /__w/jax/jax/cpython/Objects/dictobject.c:1875:5 (python3.14+0x2744af)
    #6 setitem_take2_lock_held /__w/jax/jax/cpython/Objects/dictobject.c:2605:12 (python3.14+0x2737df) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
...

  Previous write of size 8 at 0x7208000b2110 by thread T3 (mutexes: write M0, write M1):
    #0 free  (python3.14+0xdff13) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #1 pybind11::cpp_function::destruct(pybind11::detail::function_record*, bool) /proc/self/cwd/bazel-out/k8-opt/bin/external/pybind11/_virtual_includes/pybind11/pybind11/pybind11.h:708:17 (libjax_common.so+0xe846323) (BuildId: a01db73a470e92a14abe4105cd23bb895c264bff)
    #2 pybind11::cpp_function::initialize_generic(std::unique_ptr&&, char const*, std::type_info const* const*, unsigned long)::'lambda'(void*)::operator()(void*) const /proc/self/cwd/bazel-out/k8-opt/bin/external/pybind11/_virtual_includes/pybind11/pybind11/pybind11.h:553:49 (libjax_common.so+0xe8460ee) (BuildId: a01db73a470e92a14abe4105cd23bb895c264bff)
    #3 pybind11::cpp_function::initialize_generic(std::unique_ptr&&, char const*, std::type_info const* const*, unsigned long)::'lambda'(void*)::__invoke(void*) /proc/self/cwd/bazel-out/k8-opt/bin/external/pybind11/_virtual_includes/pybind11/pybind11/pybind11.h:553:33 (libjax_common.so+0xe8460ee)
    #4 pybind11::capsule::initialize_with_void_ptr_destructor(void const*, char const*, void (*)(void*))::'lambda'(_object*)::operator()(_object*) const /proc/self/cwd/bazel-out/k8-opt/bin/external/pybind11/_virtual_includes/pybind11/pybind11/detail/../pytypes.h:2067:17 (libjax_common.so+0xe846412) (BuildId: a01db73a470e92a14abe4105cd23bb895c264bff)
    #5 pybind11::capsule::initialize_with_void_ptr_destructor(void const*, char const*, void (*)(void*))::'lambda'(_object*)::__invoke(_object*) /proc/self/cwd/bazel-out/k8-opt/bin/external/pybind11/_virtual_includes/pybind11/pybind11/detail/../pytypes.h:2053:64 (libjax_common.so+0xe84637d) (BuildId: a01db73a470e92a14abe4105cd23bb895c264bff)
    #6 capsule_dealloc /__w/jax/jax/cpython/Objects/capsule.c:292:9 (python3.14+0x1f49ac) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #7 _Py_Dealloc /__w/jax/jax/cpython/Objects/object.c:3023:5 (python3.14+0x2a2452) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #8 _Py_MergeZeroLocalRefcount /__w/jax/jax/cpython/Objects/object.c (python3.14+0x2a2452)
    #9 Py_DECREF /__w/jax/jax/cpython/./Include/refcount.h:387:13 (python3.14+0x29bad3) (BuildId: 48c7ba326d9f22d081e76424a9d52fc8476e98f5)
    #10 Py_XDECREF /__w/jax/jax/cpython/./Include/refcount.h:526:9 (python3.14+0x29bad3)
    #11 meth_dealloc /__w/jax/jax/cpython/Objects/methodobject.c:179:5 (python3.14+0x29bad3)
...

pybind11 keeps its PyMethodDef alive via the self object:

https://github.com/pybind/pybind11/blob/a28ea8ccc5b98af73a2306ca0e43bcf0c82d39d8/include/pybind11/pybind11.h#L633

But after this change, m->ml is accessed after self has already been freed and the PyMethodDef no longer exists.

cpython/Objects/methodobject.c

Line 179 in 3feac7a

Py_XDECREF(m->m_self);

Please fix?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner commented Apr 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

But after this change, m->ml is accessed after self has already been freed and the PyMethodDef no longer exists.

I don't get it. The current code that decrefs the m->ml has always been there.

pybind11 keeps its PyMethodDef alive via the self object:

That's a bug. See the documentation here https://docs.python.org/3/c-api/structures.html#c.PyCMethod_New.

The caller must ensure that ml outlives the callable. Typically, ml is defined as a static variable.

I think you should report this to PyBind11 instead.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hawkinsp commented Apr 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

But after this change, m->ml is accessed after self has already been freed and the PyMethodDef no longer exists.

I don't get it. The current code that decrefs the m->ml has always been there.

The problem is you are reading if (m->m_ml->ml_flags & METH_METHOD) {

after decrefing m->m_self.

If you simply move the read of m->m_ml->ml_flags up a few lines the problem will be fixed.

pybind11 keeps its PyMethodDef alive via the self object:

That's a bug. See the documentation here https://docs.python.org/3/c-api/structures.html#c.PyCMethod_New.

The caller must ensure that ml outlives the callable. Typically, ml is defined as a static variable.

I think you should report this to PyBind11 instead.

I don't speak for the pybind11 authors, but I think they would argue that they are expecting you to keep the self object alive long enough here.

They cannot use a static variable because this is a dynamically allocated method. By what other mechanism can they keep the PyMethodDef alive as long as the result of PyCFunction_NewEx? The only other mechanism I can think is that they leak these and never deallocate them.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner commented Apr 9, 2025

Copy link

Copy Markdown

Member

But after this change, m->ml is accessed after self has already been freed and the PyMethodDef no longer exists.

I don't get it. The current code that decrefs the m->ml has always been there.

The problem is you are reading if (m->m_ml->ml_flags & METH_METHOD) {

after decrefing m->m_self.

If you simply move the read of m->m_ml->ml_flags up a few lines the problem will be fixed.

Ok that makes more sense now. Thank you. @eendebakpt can you fix this? If not I can put up a fix.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vfdev-5 added a commit to vfdev-5/jax that referenced this pull request

Apr 9, 2025


          Added race_top:meth_dealloc to tsan-suppressions_3.14.txt

df0f0d0

Description:
- According to investigations by Peter: python/cpython#128692 (comment) added race_top:meth_dealloc to tsan-suppressions_3.14.txt

vfdev-5 mentioned this pull request

Apr 9, 2025

Added few known races tsan-suppressions_3.14.txt jax-ml/jax#27872

Closed

eendebakpt mentioned this pull request

Feb 1, 2026

Performance regression for making bound methods #83298

Open

Reviewers

vstinner

vstinner approved these changes

Fidget-Spinner

Fidget-Spinner approved these changes

ZeroIntensity

ZeroIntensity approved these changes

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

eendebakpt commented Jan 9, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

eendebakpt Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 15, 2025

Uh oh!

eendebakpt commented Jan 15, 2025

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

mdboom commented Jan 27, 2025

Uh oh!

mdboom commented Jan 27, 2025

Uh oh!

hawkinsp commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fidget-Spinner commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkinsp commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fidget-Spinner commented Apr 9, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eendebakpt commented Jan 9, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

hawkinsp commented Apr 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner commented Apr 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

hawkinsp commented Apr 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.