Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-104142: Fix _Py_RefcntAdd to respect immortality #104143

Merged
merged 4 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions Include/internal/pycore_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ extern void _Py_DecRefTotal(PyInterpreterState *);
// Increment reference count by n
static inline void _Py_RefcntAdd(PyObject* op, Py_ssize_t n)
{
if (_Py_IsImmortal(op)) {
return;
}
Comment on lines +61 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

One quick comment here is that if this were to be in a hot path, I would recommend using the same technique that we use for Py_INCREF to minimize the number of generated instructions to do the check and add: https://github.com/python/cpython/blob/main/Include/object.h#L620-L634

Now, given that it's only in use for sq_repeat in tuple and list, I don't think this is perf sensitive so keeping the code as you have it here should be good.

#ifdef Py_REF_DEBUG
_Py_AddRefTotal(_PyInterpreterState_GET(), n);
#endif
Expand Down
43 changes: 25 additions & 18 deletions Lib/test/test_builtin.py
Original file line number Diff line number Diff line change
Expand Up @@ -2372,24 +2372,31 @@ def __del__(self):

@cpython_only
class ImmortalTests(unittest.TestCase):
def test_immortal(self):
none_refcount = sys.getrefcount(None)
true_refcount = sys.getrefcount(True)
false_refcount = sys.getrefcount(False)
smallint_refcount = sys.getrefcount(100)

# Assert that all of these immortal instances have large ref counts.
self.assertGreater(none_refcount, 2 ** 15)
self.assertGreater(true_refcount, 2 ** 15)
self.assertGreater(false_refcount, 2 ** 15)
self.assertGreater(smallint_refcount, 2 ** 15)

# Confirm that the refcount doesn't change even with a new ref to them.
l = [None, True, False, 100]
self.assertEqual(sys.getrefcount(None), none_refcount)
self.assertEqual(sys.getrefcount(True), true_refcount)
self.assertEqual(sys.getrefcount(False), false_refcount)
self.assertEqual(sys.getrefcount(100), smallint_refcount)

if sys.maxsize < (1 << 32):
IMMORTAL_REFCOUNT = (1 << 30) - 1
else:
IMMORTAL_REFCOUNT = (1 << 32) - 1

IMMORTALS = (None, True, False, Ellipsis, NotImplemented, *range(-5, 257))

def assert_immortal(self, immortal):
with self.subTest(immortal):
self.assertEqual(sys.getrefcount(immortal), self.IMMORTAL_REFCOUNT)

def test_immortals(self):
for immortal in self.IMMORTALS:
Copy link
Contributor

@eduardo-elizondo eduardo-elizondo May 4, 2023

Choose a reason for hiding this comment

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

Small nit: I generally prefer rolling out assertions in unit tests to make it easier to isolate by line-number in the place where the test failed. But I don't think there's a standard in the code base so I'm fine either way

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'd like comprehensive testing of these immortals though, and I don't want to roll out ~250 different assertions for each test.

I figured making each one a subTest in assert_immortal is a pretty good compromise.

self.assert_immortal(immortal)

def test_list_repeat_respect_immortality(self):
refs = list(self.IMMORTALS) * 42
for immortal in self.IMMORTALS:
self.assert_immortal(immortal)

def test_tuple_repeat_respect_immortality(self):
refs = tuple(self.IMMORTALS) * 42
for immortal in self.IMMORTALS:
self.assert_immortal(immortal)


class TestType(unittest.TestCase):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix an issue where :class:`list` or :class:`tuple` repetition could fail to
respect :pep:`683`.