Review addressed

Author: efimov-mikhail

Lib/test/test_capi/test_tuple.py

@@ -14,6 +14,17 @@ class TupleSubclass(tuple):
 
 
 class CAPITest(unittest.TestCase):
+    def _not_tracked(self, t):
+        # There is no need for gc.collect()
+        # since we don't track these tuples at all.
+        self.assertFalse(gc.is_tracked(t), t)
+
+    def _tracked(self, t):
+        self.assertTrue(gc.is_tracked(t), t)
+        gc.collect()
+        gc.collect()
+        self.assertTrue(gc.is_tracked(t), t)
+
     def test_check(self):
         # Test PyTuple_Check()
         check = _testlimitedcapi.tuple_check
@@ -52,11 +63,14 @@ def test_tuple_new(self):
         self.assertEqual(tup1, ())
         self.assertEqual(size(tup1), 0)
         self.assertIs(type(tup1), tuple)
+        self._not_tracked(tup1)
+
         tup2 = tuple_new(1)
         self.assertIs(type(tup2), tuple)
         self.assertEqual(size(tup2), 1)
         self.assertIsNot(tup2, tup1)
         self.assertTrue(checknull(tup2, 0))
+        self._tracked(tup2)
 
         self.assertRaises(SystemError, tuple_new, -1)
         self.assertRaises(SystemError, tuple_new, PY_SSIZE_T_MIN)
@@ -69,6 +83,12 @@ def test_tuple_fromarray(self):
         tup = tuple([i] for i in range(5))
         copy = tuple_fromarray(tup)
         self.assertEqual(copy, tup)
+        self._tracked(copy)
+
+        tup = tuple((42**i, 0) for i in range(5))
+        copy = tuple_fromarray(tup)
+        self.assertEqual(copy, tup)
+        self._not_tracked(copy)
 
         tup = ()
         copy = tuple_fromarray(tup)
@@ -92,11 +112,16 @@ def test_tuple_pack(self):
         self.assertEqual(pack(1, [1]), ([1],))
         self.assertEqual(pack(2, [1], [2]), ([1], [2]))
 
+        self._tracked(pack(1, [1]))
+        self._tracked(pack(2, [1], b'abc'))
+        self._not_tracked(pack(2, 42, b'abc'))
+
         self.assertRaises(SystemError, pack, PY_SSIZE_T_MIN)
         self.assertRaises(SystemError, pack, -1)
         self.assertRaises(MemoryError, pack, PY_SSIZE_T_MAX)
 
         # CRASHES pack(1, NULL)
+        # CRASHES pack(1, None)
         # CRASHES pack(2, [1])
 
     def test_tuple_size(self):

Lib/test/test_tuple.py

@@ -291,9 +291,8 @@ def test_repr(self):
         self.assertEqual(repr(a2), "(0, 1, 2)")
 
     def _not_tracked(self, t):
-        # Nested tuples can take several collections to untrack
-        gc.collect()
-        gc.collect()
+        # There is no need for gc.collect()
+        # since we don't track these tuples at all.
         self.assertFalse(gc.is_tracked(t), t)
 
     def _tracked(self, t):
@@ -323,6 +322,19 @@ def test_track_literals(self):
         self._tracked((set(),))
         self._tracked((x, y, z))
 
+    @support.cpython_only
+    def test_track_after_operations(self):
+        x, y, z = 1.5, "a", []
+
+        self._not_tracked((x, y) + (x, y))
+        self._tracked((x, y) + (z,))
+
+        self._not_tracked((x, y) * 5)
+        self._tracked((y, z) * 5)
+
+        self._not_tracked((x, y, z)[:2])
+        self._tracked((x, y, z)[2:])
+
     def check_track_dynamic(self, tp, always_track):
         x, y, z = 1.5, "a", []
 
@@ -343,6 +355,14 @@ def check_track_dynamic(self, tp, always_track):
         self._tracked(tp(tuple([obj]) for obj in [x, y, z]))
         self._tracked(tuple(tp([obj]) for obj in [x, y, z]))
 
+        t = tp([1, x, y, z])
+        self.assertEqual(type(t), tp)
+        self._tracked(t)
+        self.assertEqual(type(t[:]), tuple)
+        self._tracked(t[:])
+        self.assertEqual(type(t[:3]), tuple)
+        self._not_tracked(t[:3])
+
     @support.cpython_only
     def test_track_dynamic(self):
         # Test GC-optimization of dynamically constructed tuples.

Objects/tupleobject.c

@@ -156,6 +156,8 @@ _PyTuple_MaybeUntrack(PyObject *op)
     _PyObject_GC_UNTRACK(op);
 }
 
+/* This function helps to avoids performance issue in the GC,
+ * we don't track immutable tuples. */
 static bool
 tuple_need_tracking(PyTupleObject *self)
 {