gh-140232: Do not track frozenset objects with immutables

[eendebakpt]

In the PR we untrack frozen tuples for the normal constructors. There are a few methods shared between the set and frozenset (for example set_intersection in setobject.c) where we have not added the untracking. (this is possible, but I am not sure this is worthwhile to do).

Here is a small script to test the idea:

import gc
import time
from statistics import mean

number_of_iterations = 20
number_of_gc_iterations = 50

deltas = []

gc.disable()
gc.collect()
for kk in range(number_of_iterations):
    t0 = time.perf_counter()
    for jj in range(number_of_gc_iterations):
        gc.collect()
    dt = time.perf_counter() - t0
    deltas.append(dt)
print(f"time per collection: mean {1e3 * mean(deltas) / number_of_iterations:.3f} [ms], min {1e3 * min(deltas) / number_of_iterations:.3f} [ms]")

sets = [frozenset([ii]) for ii in range(10_000)]
deltas = []
print("---")
gc.disable()
gc.collect()
for kk in range(number_of_iterations):
    t0 = time.perf_counter()
    for jj in range(number_of_gc_iterations):
        gc.collect()
    dt = time.perf_counter() - t0
    deltas.append(dt)
print(f"time per collection: mean {1e3 * mean(deltas) / number_of_iterations:.3f} [ms], min {1e3 * min(deltas) / number_of_iterations:.3f} [ms]")

#%% Show statistics of frozen containers

gc.collect()

def candidate(obj):
    return all(not gc.is_tracked(x) for x in obj)

for immutable_type in (tuple, frozenset):
    number_of_objects_tracked = 0
    number_of_candidates = 0
    number_of_immutable_candidates = 0

    for obj in gc.get_objects():
        number_of_objects_tracked += 1
        if type(obj) is immutable_type:
            number_of_candidates += 1
            # print(f"{type(obj)} = {obj}")
            if candidate(obj):
                number_of_immutable_candidates += 1

    print(f"type {immutable_type}")
    print(f"  {number_of_objects_tracked=}")
    print(f"  {number_of_candidates=}")
    print(f"  {number_of_immutable_candidates=}")

It measures the performance of garbage collection, and outputs some statistics for the numbers of frozen containers.

Main:

time per collection: mean 1.311 [ms], min 1.301 [ms]
---
time per collection: mean 2.467 [ms], min 2.272 [ms]
type <class 'tuple'>
  number_of_objects_tracked=18330
  number_of_candidates=546
  number_of_immutable_candidates=1
type <class 'frozenset'>
  number_of_objects_tracked=18330
  number_of_candidates=10059
  number_of_immutable_candidates=10057

PR

time per collection: mean 1.285 [ms], min 1.251 [ms]
---
time per collection: mean 1.424 [ms], min 1.396 [ms]
type <class 'tuple'>
  number_of_objects_tracked=8273
  number_of_candidates=546
  number_of_immutable_candidates=6
type <class 'frozenset'>
  number_of_objects_tracked=8273
  number_of_candidates=2
  number_of_immutable_candidates=0

Note: generative ai was used in creating the PR

• Issue: Disable tracking of frozenset objects with immutables in the GC #140232

[sergey-miryanov]

Maybe it is worth to change tp_alloc for something like:

PyObject *
PyFrozenSet_Alloc(PyTypeObject *type, Py_ssize_t nitems)
{
    PyObject *obj = PyType_GenericAlloc(type, nitems);
    if (obj == NULL) {
        return NULL;
    }

    _PyFrozenSet_MaybeUntrack(obj);
    return obj;
}

[eendebakpt]

Maybe it is worth to change tp_alloc for something like:

The tp_alloc is used in make_new_set, which in turn is called by make_new_set. The last one is used set_intersection which modifies a frozenset. So adding _PyFrozenSet_MaybeUntrack to tp_alloc would mean we have to add a _PyFrozenSet_MaybeTrack to the end of set_intersection. This is a complication I do not want to tackle (certainly not in this PR).

[sergey-miryanov]

Code looks good to me.

[vstinner]

Would it be possible to write tests in Python rather than in C?

[eendebakpt]

Would it be possible to write tests in Python rather than in C?

I tried, but it is not easy. We have to expose PySet_Add (frozenset().add does not exist on the python side). I added pyset_add on the _testcapi module (with pyset_add just calling PySet_Add). But running this on a frozenset from the python side does not work: when calling _testcapi.pyset_add(frozen_set, item) there too many references to the frozen_set and PySet_Add will fail with an internal error here:

cpython/Objects/setobject.c

Line 2778 in d78d7a5

PyErr_BadInternalCall();

And when calling _testcapi.pyset_add(frozenset(), item) we do not have the frozenset available to test whether tracking has been enabled.

[sergey-miryanov]

And when calling _testcapi.pyset_add(frozenset(), item) we do not have the frozenset available to test whether tracking has been enabled.

IIUC, if you return the first argument from pyset_add then you can test it on the python side.

[eendebakpt]

And when calling _testcapi.pyset_add(frozenset(), item) we do not have the frozenset available to test whether tracking has been enabled.

IIUC, if you return the first argument from pyset_add then you can test it on the python side.

Ok, I gave it another try. The first attempt failed, but by using the vectorcall convention I can keep the reference count at 1 also from the Python side.

[vstinner]

Why not untracking frozenset subtypes?

[efimov-mikhail]

We can make a cycle using frozenset subtype:

>>> class F(frozenset):
...     pass
...     
>>> f = F([1,2,3])
>>> f.cycle = f

So, we need to track them.

[vstinner]

Oh ok. In this case, please add a comment explaining that :-)

[vstinner]

I don't understand why the second test fails, whereas the first succeed.

[eendebakpt]

The second test fails because the argument frozen_set is not uniquely referenced. The error is raised here:

cpython/Objects/setobject.c

Line 2777 in ce4b0ed

(!PyFrozenSet_Check(anyset) || !_PyObject_IsUniquelyReferenced(anyset))) {

I will add a comment to the test

[vstinner]

Please move this function to Modules/_testlimitedcapi/set.c.

[eendebakpt]

I moved the function and converted the calling convention to METH_VARARGS. This adds another reference to the set causing the test to fail.

I will convert back to METH_FASTCALL, or define the set at C level.

[vstinner]

It should return None on success.

[eendebakpt]

From #140234 (comment) I gathered we prefer to write the tests in Python (as much as possible). I can return None here, but then I have convert the code from test_set to C (possible, but more code and a bit harder to read)

[eendebakpt]

@vstinner A branch with the tests written in C is here:

main...eendebakpt:cpython:frozenset_immutable_tracking_c

Let me know if you prefer that approach, then I will merge the branch into this PR.

[vstinner]

I can return None here, but then I have convert the code from test_set to C (possible, but more code and a bit harder to read)

Why do you have to write tests in C if this function returns None? Are you talking about tests on frozenset? I see this code in PySet_Add():

    if (PyFrozenSet_Check(anyset) && _PyObject_IsUniquelyReferenced(anyset)) {
        // We can only change frozensets if they are uniquely referenced. The
        // API limits the usage of `PySet_Add` to "fill in the values of brand
        // new frozensets before they are exposed to other code". In this case,
        // this can be done without a lock.
        return set_add_key((PySetObject *)anyset, key);
    }

If you want to write tests for this code path, writing them in C would be more reliable to have a fine control on the reference count, right. But other tests on the set types can be written in Python, no?

[eendebakpt]

The reason for returning the set in pyset_add was indeed to be able test the code path you mention for the frozenset. Writing

x = frozenset()
_testinternalcapi.pyset_add(x, 1)

fails (because the frozenset is not uniquely referenced). For that reason I used:

x =_testinternalcapi.pyset_add(frozenset(), 1)
assert 1 in x

Doing that in C indeed seems better.

The other tests (for this PR the TestPySet_Add.test_set and TestPySet_Add.test_frozenset) can still be written in Python, although there are a couple already there for set and set subclasses, so there is not much left to test from the Python side.

[vstinner]

Please move these tests to Lib/test/test_capi/test_set.py.