Fill newly created tuple with Py_None instead of NULL

[sergey-miryanov]

Feature or enhancement

Proposal:

Eventually we can remove NULL checks inside _PyTuple_MaybeUntrack if all created tuples will be well-constructed and contain NULL only if the user is doing something wrong.

cc @markshannon

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

• GH-145914: Fill newly created tuple with Py_None instead of NULL #145913

[picnixz]

I would be extra careful here. This is part of the stable ABI and while not explicitly documented, we should be careful that this change won't break downstream. I suggest closing the PR for now and wait for more feedback.

Regarding performances, I don't think it will matter much but I see that for growing tuples, a memset has been replaced by a loop. This may not necessarily be more efficient if we create lots of tuples that are directly discarded or unfilled (though I wonder whether this happens a lot).

[picnixz]

cc @encukou @vstinner @serhiy-storchaka

[encukou]

Yes, this looks like it can break users.
I don't see the reason behind the change; is it worth looking for what might break?

[sergey-miryanov]

I can only think of one example where the user's code could be broken:

PyObject *tuple = PyTuple_New(n);
// ... skip lines
if (PyTuple_GET_ITEMS(tuple, i) != NULL) {
  PyTuple_SET_ITEM(tuple, i, object);
}

is it worth looking for what might break?

Do you mean look at the top of the pypi packages?

[serhiy-storchaka]

Even if it does not break user code, the benefit of this change is questionable. Setting to Py_None is slower then setting to NULL. Checking for Py_None is slower then checking for NULL. If you need to discard the tuple item, you will need to do with refcount and destructor instead of simply comparing the pointer with NULL.

I do not it is worth to change this.

[sergey-miryanov]

If you need to discard the tuple item, you will need to do with refcount and destructor instead of simply comparing the pointer with NULL.

It is about the check on 149 line:

cpython/Objects/tupleobject.c

Lines 147 to 155 in 9d7621b

	for (i = 0; i < n; i++) {
	PyObject *elt = PyTuple_GET_ITEM(t, i);
	/* Tuple with NULL elements aren't
	fully constructed, don't untrack
	them yet. */
	if (!elt ||
	_PyObject_GC_MAY_BE_TRACKED(elt))
	return;
	}

[picnixz]

Do you mean look at the top of the pypi packages?

Unfortunately, even if we just look at the top-level packages, changing implementation details of the stable API can break private code or hide nasty bugs. What I found questionable is the memset being replaced by the loop when growing tuples. memset is very fast, but going through a loop is not necessarily the case (I'm actually surprised that don't use memset to initialize the tuple actually).

[sergey-miryanov]

Thinking twice this change will break _PyTuple_MaybeUntrack's assumptions and could lead to memory leaks. The only way to remove NULL check is to deprecate PyTuple_New and remove it.

Thanks all for your feedback and sorry for bother.