Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Lib/test/test_sys.py
Original file line number Diff line number Diff line change
Expand Up @@ -1876,7 +1876,10 @@ class S(set):
check(S(), set(), '3P')
class FS(frozenset):
__slots__ = 'a', 'b', 'c'
check(FS(), frozenset(), '3P')

class mytuple(tuple):
pass
check(FS([mytuple()]), frozenset([mytuple()]), '3P')
from collections import OrderedDict
class OD(OrderedDict):
__slots__ = 'a', 'b', 'c'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Frozenset objects with immutable elements are no longer tracked by the garbage collector.
63 changes: 62 additions & 1 deletion Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -2435,6 +2435,66 @@ test_critical_sections(PyObject *module, PyObject *Py_UNUSED(args))
}



static PyObject *
test_pyset_add(PyObject* self, PyObject *Py_UNUSED(args))
{

PyObject *set = NULL, *empty_tuple=NULL, *tracked_object;


tracked_object = PyImport_ImportModule("sys");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (tracked_object == NULL) {
goto failed;
}
if (!PyObject_GC_IsTracked(tracked_object)) {
PyErr_SetString(PyExc_ValueError, "Test item is not tracked by GC");
goto failed;
}


int return_value;
empty_tuple = PyTuple_New(0);
if (empty_tuple == NULL) {
goto failed;
}
set = PyFrozenSet_New(empty_tuple);
if (set == NULL) {
return NULL;
}

if (PyObject_GC_IsTracked(set)) {
PyErr_SetString(PyExc_ValueError, "Empty frozenset object is tracked by GC");
goto failed;
}
return_value = PySet_Add(set, empty_tuple);
if (return_value<0) {
goto failed;
}
if (PyObject_GC_IsTracked(set)) {
PyErr_SetString(PyExc_ValueError, "Frozenset object with immutable is tracked by GC");
goto failed;
}

PySet_Add(set, tracked_object);
if (return_value<0) {
goto failed;
}
if (!PyObject_GC_IsTracked(set)) {
PyErr_SetString(PyExc_ValueError, "Frozenset object with tracked objects is not tracked by GC");
goto failed;
}

Py_RETURN_NONE;

failed:
Py_XDECREF(tracked_object);
Py_XDECREF(empty_tuple);
Py_XDECREF(set);
return NULL;

}

// Used by `finalize_thread_hang`.
#if defined(_POSIX_THREADS) && !defined(__wasi__)
static void finalize_thread_hang_cleanup_callback(void *Py_UNUSED(arg)) {
Expand Down Expand Up @@ -2625,7 +2685,7 @@ static PyMethodDef TestMethods[] = {
{"return_null_without_error", return_null_without_error, METH_NOARGS},
{"return_result_with_error", return_result_with_error, METH_NOARGS},
{"getitem_with_error", getitem_with_error, METH_VARARGS},
{"Py_CompileString", pycompilestring, METH_O},
{"Py_CompileString", pycompilestring, METH_O},
{"raise_SIGINT_then_send_None", raise_SIGINT_then_send_None, METH_VARARGS},
{"stack_pointer", stack_pointer, METH_NOARGS},
#ifdef W_STOPCODE
Expand All @@ -2646,6 +2706,7 @@ static PyMethodDef TestMethods[] = {
{"gen_get_code", gen_get_code, METH_O, NULL},
{"get_feature_macros", get_feature_macros, METH_NOARGS, NULL},
{"test_code_api", test_code_api, METH_NOARGS, NULL},
{"test_pyset_add", test_pyset_add, METH_NOARGS, NULL},
{"settrace_to_error", settrace_to_error, METH_O, NULL},
{"settrace_to_record", settrace_to_record, METH_O, NULL},
{"test_macros", test_macros, METH_NOARGS, NULL},
Expand Down
28 changes: 26 additions & 2 deletions Objects/setobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1174,6 +1174,23 @@ make_new_set_basetype(PyTypeObject *type, PyObject *iterable)
return make_new_set(type, iterable);
}

void
Copy link
Member

Choose a reason for hiding this comment

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

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

_PyFrozenSet_MaybeUntrack(PyObject *op)
{
if ((op == NULL) || !(PyFrozenSet_CheckExact(op))) {
return;
}
// if all elements of a frozenset are not tracked, we untrack the object
Py_ssize_t pos = 0;
setentry *entry;
while (set_next((PySetObject *)op, &pos, &entry)) {
if (_PyObject_GC_MAY_BE_TRACKED(entry->key)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return;
}
}
_PyObject_GC_UNTRACK(op);
}

static PyObject *
make_new_frozenset(PyTypeObject *type, PyObject *iterable)
{
Expand All @@ -1185,7 +1202,9 @@ make_new_frozenset(PyTypeObject *type, PyObject *iterable)
/* frozenset(f) is idempotent */
return Py_NewRef(iterable);
}
return make_new_set(type, iterable);
PyObject *obj = make_new_set(type, iterable);
_PyFrozenSet_MaybeUntrack(obj);
return obj;
}

static PyObject *
Expand Down Expand Up @@ -2710,7 +2729,9 @@ PySet_New(PyObject *iterable)
PyObject *
PyFrozenSet_New(PyObject *iterable)
{
return make_new_set(&PyFrozenSet_Type, iterable);
PyObject *result = make_new_set(&PyFrozenSet_Type, iterable);
_PyFrozenSet_MaybeUntrack(result);
return result;
}

Py_ssize_t
Expand Down Expand Up @@ -2779,6 +2800,9 @@ PySet_Add(PyObject *anyset, PyObject *key)
return -1;
}

if (PyFrozenSet_Check(anyset) && PyObject_GC_IsTracked(key) && !PyObject_GC_IsTracked(anyset) ) {
_PyObject_GC_TRACK(anyset);
}
int rv;
Py_BEGIN_CRITICAL_SECTION(anyset);
rv = set_add_key((PySetObject *)anyset, key);
Expand Down
Loading