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

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

[sergey-miryanov]

Maybe just create empty list or dict here? Importing module seems too heavy for testing purpose.

[eendebakpt]

I agree a module seems heavy, but we cannot add a list or dict to a set. A custom class or namedtuple would also do, but they require more code to setup. But any suggestion for a simple-to-create hashable object tracked by the GC is welcome.

[sergey-miryanov]

Ough, I forgot about the requirement to be a hashable. Thanks!
WDYT about exposing function for PySet_Add to _testcapi and write tests in python like I did this https://github.com/python/cpython/pull/140132/files#diff-70eaebed435342e02ba8f7f5a84e4eebd552438ce6ac2765e80abb5514bdea03R134?

Then you can write test like:

class Test:
    pass

fs = pyset_add(Test())

[eendebakpt]

See the reply to victor below. Just adding pyset_add will not work. We could add a _testcapi.test_pyset_add(tracked_item), but then we have the test functionality spread over both the python and c side.

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

[efimov-mikhail]

Maybe we should use faster _PyType_IS_GC(Py_TYPE(entry->key)) as in maybe_tracked from Objects/tupleobject.c?

[eendebakpt]

Not sure performance matters a lot, but I would prefer to have it consistent with what is used in tupleobject.c. Unless there are objections, I will change the implementation to use the maybe_tracked.

[vstinner]

Please add a comment to explain the purpose of this function with a link to the GitHub issue.

[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