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-125900: Clean-up logic around immortalization in free-threading #125901

Merged
merged 5 commits into from
Oct 24, 2024
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
8 changes: 0 additions & 8 deletions Include/internal/pycore_gc.h
Original file line number Diff line number Diff line change
Expand Up @@ -342,14 +342,6 @@ struct _gc_runtime_state {
collections, and are awaiting to undergo a full collection for
the first time. */
Py_ssize_t long_lived_pending;

/* gh-117783: Deferred reference counting is not fully implemented yet, so
as a temporary measure we treat objects using deferred reference
counting as immortal. The value may be zero, one, or a negative number:
0: immortalize deferred RC objects once the first thread is created
1: immortalize all deferred RC objects immediately
<0: suppressed; don't immortalize objects */
int immortalize;
#endif
};

Expand Down
3 changes: 3 additions & 0 deletions Include/internal/pycore_tstate.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ typedef struct _PyThreadStateImpl {
// If set, don't use per-thread refcounts
int is_finalized;
} refcounts;

// When >1, code objects do not immortalize their non-string constants.
int suppress_co_const_immortalization;
#endif

#if defined(Py_REF_DEBUG) && defined(Py_GIL_DISABLED)
Expand Down
8 changes: 2 additions & 6 deletions Lib/test/libregrtest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
import time
import trace

from test.support import (os_helper, MS_WINDOWS, flush_std_streams,
suppress_immortalization)
from test.support import os_helper, MS_WINDOWS, flush_std_streams

from .cmdline import _parse_args, Namespace
from .findtests import findtests, split_test_packages, list_cases
Expand Down Expand Up @@ -535,10 +534,7 @@ def _run_tests(self, selected: TestTuple, tests: TestList | None) -> int:
if self.num_workers:
self._run_tests_mp(runtests, self.num_workers)
else:
# gh-117783: don't immortalize deferred objects when tracking
# refleaks. Only relevant for the free-threaded build.
with suppress_immortalization(runtests.hunt_refleak):
self.run_tests_sequentially(runtests)
self.run_tests_sequentially(runtests)

coverage = self.results.get_coverage_results()
self.display_result(runtests)
Expand Down
5 changes: 1 addition & 4 deletions Lib/test/libregrtest/single.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,10 +304,7 @@ def run_single_test(test_name: TestName, runtests: RunTests) -> TestResult:
result = TestResult(test_name)
pgo = runtests.pgo
try:
# gh-117783: don't immortalize deferred objects when tracking
# refleaks. Only relevant for the free-threaded build.
with support.suppress_immortalization(runtests.hunt_refleak):
_runtest(result, runtests)
_runtest(result, runtests)
except:
if not pgo:
msg = traceback.format_exc()
Expand Down
1 change: 0 additions & 1 deletion Lib/test/seq_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,6 @@ def test_pickle(self):
self.assertEqual(lst2, lst)
self.assertNotEqual(id(lst2), id(lst))

@support.suppress_immortalization()
def test_free_after_iterating(self):
support.check_free_after_iterating(self, iter, self.type2test)
support.check_free_after_iterating(self, reversed, self.type2test)
27 changes: 0 additions & 27 deletions Lib/test/support/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -512,33 +512,6 @@ def has_no_debug_ranges():
def requires_debug_ranges(reason='requires co_positions / debug_ranges'):
return unittest.skipIf(has_no_debug_ranges(), reason)

@contextlib.contextmanager
def suppress_immortalization(suppress=True):
"""Suppress immortalization of deferred objects."""
try:
import _testinternalcapi
except ImportError:
yield
return

if not suppress:
yield
return

_testinternalcapi.suppress_immortalization(True)
try:
yield
finally:
_testinternalcapi.suppress_immortalization(False)

def skip_if_suppress_immortalization():
try:
import _testinternalcapi
except ImportError:
return
return unittest.skipUnless(_testinternalcapi.get_immortalize_deferred(),
"requires immortalization of deferred objects")


MS_WINDOWS = (sys.platform == 'win32')

Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_ast/test_ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -2259,7 +2259,7 @@ def test_validation(self):
"got an invalid type in Constant: list")

def test_singletons(self):
for const in (None, False, True, Ellipsis, b'', frozenset()):
for const in (None, False, True, Ellipsis, b''):
mpage marked this conversation as resolved.
Show resolved Hide resolved
with self.subTest(const=const):
value = self.compile_constant(const)
self.assertIs(value, const)
Expand Down
3 changes: 0 additions & 3 deletions Lib/test/test_capi/test_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from test.support import threading_helper
from test.support import warnings_helper
from test.support import requires_limited_api
from test.support import suppress_immortalization
from test.support import expected_failure_if_gil_disabled
from test.support import Py_GIL_DISABLED
from test.support.script_helper import assert_python_failure, assert_python_ok, run_python_until_end
Expand Down Expand Up @@ -481,7 +480,6 @@ def test_heap_ctype_doc_and_text_signature(self):
def test_null_type_doc(self):
self.assertEqual(_testcapi.NullTpDocType.__doc__, None)

@suppress_immortalization()
def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self):
class HeapGcCTypeSubclass(_testcapi.HeapGcCType):
def __init__(self):
Expand All @@ -499,7 +497,6 @@ def __init__(self):
del subclass_instance
self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass))

@suppress_immortalization()
def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self):
class A(_testcapi.HeapGcCType):
def __init__(self):
Expand Down
4 changes: 1 addition & 3 deletions Lib/test/test_capi/test_watchers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from contextlib import contextmanager, ExitStack
from test.support import (
catch_unraisable_exception, import_helper,
gc_collect, suppress_immortalization)
gc_collect)


# Skip this test if the _testcapi module isn't available.
Expand Down Expand Up @@ -404,7 +404,6 @@ def assert_event_counts(self, exp_created_0, exp_destroyed_0,
self.assertEqual(
exp_destroyed_1, _testcapi.get_code_watcher_num_destroyed_events(1))

@suppress_immortalization()
def test_code_object_events_dispatched(self):
# verify that all counts are zero before any watchers are registered
self.assert_event_counts(0, 0, 0, 0)
Expand Down Expand Up @@ -451,7 +450,6 @@ def test_error(self):
self.assertIsNone(cm.unraisable.object)
self.assertEqual(str(cm.unraisable.exc_value), "boom!")

@suppress_immortalization()
def test_dealloc_error(self):
co = _testcapi.code_newempty("test_watchers", "dummy0", 0)
with self.code_watcher(2):
Expand Down
8 changes: 1 addition & 7 deletions Lib/test/test_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,7 @@
ctypes = None
from test.support import (cpython_only,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that there are too many imports in the current line. Maybe it can be split. See line 146.

This suggestion applies to all changes (maybe I don't like to import too much from the from statement, so it's better to use * directly

(In this way, this () can be removed. Yes, it's still)

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 think this is fine. I don't think the PR should do a broader import cleanup -- just undo the changes related @suppress_immortalization.

check_impl_detail, requires_debug_ranges,
gc_collect, Py_GIL_DISABLED,
suppress_immortalization,
skip_if_suppress_immortalization)
gc_collect, Py_GIL_DISABLED)
from test.support.script_helper import assert_python_ok
from test.support import threading_helper, import_helper
from test.support.bytecode_helper import instructions_with_positions
Expand Down Expand Up @@ -579,7 +577,6 @@ def test_interned_string_with_null(self):

@cpython_only
@unittest.skipUnless(Py_GIL_DISABLED, "does not intern all constants")
@skip_if_suppress_immortalization()
def test_interned_constants(self):
# compile separately to avoid compile time de-duping

Expand All @@ -599,7 +596,6 @@ def func2():

class CodeWeakRefTest(unittest.TestCase):

@suppress_immortalization()
def test_basic(self):
# Create a code object in a clean environment so that we know we have
# the only reference to it left.
Expand Down Expand Up @@ -850,7 +846,6 @@ def test_bad_index(self):
self.assertEqual(GetExtra(f.__code__, FREE_INDEX+100,
ctypes.c_voidp(100)), 0)

@suppress_immortalization()
def test_free_called(self):
# Verify that the provided free function gets invoked
# when the code object is cleaned up.
Expand Down Expand Up @@ -878,7 +873,6 @@ def test_get_set(self):
del f

@threading_helper.requires_working_threading()
@suppress_immortalization()
def test_free_different_thread(self):
# Freeing a code object on a different thread then
# where the co_extra was set should be safe.
Expand Down
1 change: 0 additions & 1 deletion Lib/test/test_descr.py
Original file line number Diff line number Diff line change
Expand Up @@ -5076,7 +5076,6 @@ def __new__(cls):
cls.lst = [2**i for i in range(10000)]
X.descr

@support.suppress_immortalization()
def test_remove_subclass(self):
# bpo-46417: when the last subclass of a type is deleted,
# remove_subclass() clears the internal dictionary of subclasses:
Expand Down
1 change: 0 additions & 1 deletion Lib/test/test_functools.py
Original file line number Diff line number Diff line change
Expand Up @@ -1992,7 +1992,6 @@ def f():
return 1
self.assertEqual(f.cache_parameters(), {'maxsize': 1000, "typed": True})

@support.suppress_immortalization()
def test_lru_cache_weakrefable(self):
@self.module.lru_cache
def test_function(x):
Expand Down
11 changes: 2 additions & 9 deletions Lib/test/test_gc.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from test import support
from test.support import (verbose, refcount_test,
cpython_only, requires_subprocess,
requires_gil_enabled, suppress_immortalization,
requires_gil_enabled,
Py_GIL_DISABLED)
from test.support.import_helper import import_module
from test.support.os_helper import temp_dir, TESTFN, unlink
Expand Down Expand Up @@ -110,7 +110,6 @@ def test_tuple(self):
del l
self.assertEqual(gc.collect(), 2)

@suppress_immortalization()
def test_class(self):
class A:
pass
Expand All @@ -119,7 +118,6 @@ class A:
del A
self.assertNotEqual(gc.collect(), 0)

@suppress_immortalization()
def test_newstyleclass(self):
class A(object):
pass
Expand All @@ -136,7 +134,6 @@ class A:
del a
self.assertNotEqual(gc.collect(), 0)

@suppress_immortalization()
def test_newinstance(self):
class A(object):
pass
Expand Down Expand Up @@ -223,7 +220,6 @@ class B(object):
self.fail("didn't find obj in garbage (finalizer)")
gc.garbage.remove(obj)

@suppress_immortalization()
def test_function(self):
# Tricky: f -> d -> f, code should call d.clear() after the exec to
# break the cycle.
Expand Down Expand Up @@ -566,7 +562,6 @@ def test_get_referents(self):

self.assertEqual(gc.get_referents(1, 'a', 4j), [])

@suppress_immortalization()
def test_is_tracked(self):
# Atomic built-in types are not tracked, user-defined objects and
# mutable containers are.
Expand Down Expand Up @@ -604,9 +599,7 @@ class UserFloatSlots(float):
class UserIntSlots(int):
__slots__ = ()

if not Py_GIL_DISABLED:
# gh-117783: modules may be immortalized in free-threaded build
self.assertTrue(gc.is_tracked(gc))
self.assertTrue(gc.is_tracked(gc))
self.assertTrue(gc.is_tracked(UserClass))
self.assertTrue(gc.is_tracked(UserClass()))
self.assertTrue(gc.is_tracked(UserInt()))
Expand Down
4 changes: 1 addition & 3 deletions Lib/test/test_inspect/test_inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
except ImportError:
ThreadPoolExecutor = None

from test.support import cpython_only, import_helper, suppress_immortalization
from test.support import cpython_only, import_helper
from test.support import MISSING_C_DOCSTRINGS, ALWAYS_EQ
from test.support.import_helper import DirsOnSysPath, ready_to_import
from test.support.os_helper import TESTFN, temp_cwd
Expand Down Expand Up @@ -771,7 +771,6 @@ def test_getfile_builtin_function_or_method(self):
inspect.getfile(list.append)
self.assertIn('expected, got', str(e_append.exception))

@suppress_immortalization()
def test_getfile_class_without_module(self):
class CM(type):
@property
Expand Down Expand Up @@ -2576,7 +2575,6 @@ def __getattribute__(self, attr):

self.assertFalse(test.called)

@suppress_immortalization()
def test_cache_does_not_cause_classes_to_persist(self):
# regression test for gh-118013:
# check that the internal _shadowed_dict cache does not cause
Expand Down
3 changes: 0 additions & 3 deletions Lib/test/test_module/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import weakref
from test.support import gc_collect
from test.support import import_helper
from test.support import suppress_immortalization
from test.support.script_helper import assert_python_ok

import sys
Expand Down Expand Up @@ -104,7 +103,6 @@ def f():
gc_collect()
self.assertEqual(f().__dict__["bar"], 4)

@suppress_immortalization()
def test_clear_dict_in_ref_cycle(self):
destroyed = []
m = ModuleType("foo")
Expand All @@ -120,7 +118,6 @@ def __del__(self):
gc_collect()
self.assertEqual(destroyed, [1])

@suppress_immortalization()
def test_weakref(self):
m = ModuleType("foo")
wr = weakref.ref(m)
Expand Down
3 changes: 1 addition & 2 deletions Lib/test/test_ordered_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import weakref
from collections.abc import MutableMapping
from test import mapping_tests, support
from test.support import import_helper, suppress_immortalization
from test.support import import_helper


py_coll = import_helper.import_fresh_module('collections',
Expand Down Expand Up @@ -669,7 +669,6 @@ def test_dict_update(self):
dict.update(od, [('spam', 1)])
self.assertNotIn('NULL', repr(od))

@suppress_immortalization()
def test_reference_loop(self):
# Issue 25935
OrderedDict = self.OrderedDict
Expand Down
3 changes: 1 addition & 2 deletions Lib/test/test_struct.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import weakref

from test import support
from test.support import import_helper, suppress_immortalization
from test.support import import_helper
from test.support.script_helper import assert_python_ok
from test.support.testcase import ComplexesAreIdenticalMixin

Expand Down Expand Up @@ -697,7 +697,6 @@ def __del__(self):
self.assertIn(b"Exception ignored in:", stderr)
self.assertIn(b"C.__del__", stderr)

@suppress_immortalization()
def test__struct_reference_cycle_cleaned_up(self):
# Regression test for python/cpython#94207.

Expand Down
4 changes: 1 addition & 3 deletions Lib/test/test_trace.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import os
from pickle import dump
import sys
from test.support import captured_stdout, requires_resource, requires_gil_enabled
from test.support import captured_stdout, requires_resource
from test.support.os_helper import (TESTFN, rmtree, unlink)
from test.support.script_helper import assert_python_ok, assert_python_failure
import textwrap
Expand Down Expand Up @@ -301,7 +301,6 @@ def test_loop_caller_importing(self):

@unittest.skipIf(hasattr(sys, 'gettrace') and sys.gettrace(),
'pre-existing trace function throws off measurements')
@requires_gil_enabled("gh-117783: immortalization of types affects traced method names")
def test_inst_method_calling(self):
obj = TracedClass(20)
self.tracer.runfunc(obj.inst_method_calling, 1)
Expand Down Expand Up @@ -335,7 +334,6 @@ def setUp(self):

@unittest.skipIf(hasattr(sys, 'gettrace') and sys.gettrace(),
'pre-existing trace function throws off measurements')
@requires_gil_enabled("gh-117783: immortalization of types affects traced method names")
def test_loop_caller_importing(self):
self.tracer.runfunc(traced_func_importing_caller, 1)

Expand Down
Loading
Loading