Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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`.