Skip to content

gh-141592: test_perf_profiler: fix race in perf map cleanup using addCleanup #141593

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.

Sign up for GitHub

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

Closed
pareshjoshij wants to merge 5 commits into from
Closed

gh-141592: test_perf_profiler: fix race in perf map cleanup using addCleanup #141593

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f3e277
gh-141592: test_perf_profiler: fix race in perf map cleanup using add…
pareshjoshij Nov 15, 2025
c56afcd
gh-141592: remove all explanatory comments per review
pareshjoshij Nov 15, 2025
7fbbfde
gh-141592: final polish per review (docstring, unlink, with, comments)
pareshjoshij Nov 15, 2025
d14cac1
gh-141592: final: setUp cleanup, pathlib, no extra lines
pareshjoshij Nov 15, 2025
339209b
Merge branch 'main' into fix-perf-profiler-cleanup
pareshjoshij Nov 15, 2025
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
90 changes: 40 additions & 50 deletions Lib/test/test_perf_profiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,17 @@ def supports_trampoline_profiling():


class TestPerfTrampoline(unittest.TestCase):
def setUp(self):
super().setUp()
self.perf_files = set(pathlib.Path("/tmp/").glob("perf-*.map"))

def tearDown(self) -> None:
super().tearDown()
files_to_delete = (
set(pathlib.Path("/tmp/").glob("perf-*.map")) - self.perf_files
)
for file in files_to_delete:
file.unlink()

def _cleanup_perf_map(self, pid):
"""
Helper to safely delete a specific perf map file.
"""
Comment thread
pareshjoshij marked this conversation as resolved.
Outdated
perf_map = pathlib.Path(f"/tmp/perf-{pid}.map")
try:
if perf_map.exists():
perf_map.unlink()
except OSError:
pass
Comment thread
pareshjoshij marked this conversation as resolved.
Outdated

@unittest.skipIf(support.check_bolt_optimized(), "fails on BOLT instrumented binaries")
def test_trampoline_works(self):
Expand All @@ -64,13 +64,18 @@ def baz():
with temp_dir() as script_dir:
script = make_script(script_dir, "perftest", code)
env = {**os.environ, "PYTHON_JIT": "0"}
with subprocess.Popen(

Comment thread
picnixz marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

@picnixz picnixz Nov 15, 2025

Choose a reason for hiding this comment

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

You can now remove the extra blank line.

process = subprocess.Popen(
[sys.executable, "-Xperf", script],
text=True,
stderr=subprocess.PIPE,
stdout=subprocess.PIPE,
env=env,
) as process:
)

self.addCleanup(self._cleanup_perf_map, process.pid)

with process:
stdout, stderr = process.communicate()

self.assertEqual(stderr, "")
Expand Down Expand Up @@ -140,11 +145,15 @@ def baz():
stdout=subprocess.PIPE,
env=env,
) as process:
self.addCleanup(self._cleanup_perf_map, process.pid)
stdout, stderr = process.communicate()

self.assertEqual(process.returncode, 0)
self.assertEqual(stderr, "")

child_pid = int(stdout.strip())
self.addCleanup(self._cleanup_perf_map, child_pid)

perf_file = pathlib.Path(f"/tmp/perf-{process.pid}.map")
perf_child_file = pathlib.Path(f"/tmp/perf-{child_pid}.map")
self.assertTrue(perf_file.exists())
Expand Down Expand Up @@ -199,6 +208,7 @@ def baz():
stdout=subprocess.PIPE,
env=env,
) as process:
self.addCleanup(self._cleanup_perf_map, process.pid)
stdout, stderr = process.communicate()

self.assertEqual(stderr, "")
Expand Down Expand Up @@ -271,12 +281,9 @@ def perf_command_works():
except (subprocess.SubprocessError, OSError):
return False

# perf version does not return a version number on Fedora. Use presence
Comment thread
pareshjoshij marked this conversation as resolved.
# of "perf.data" in help as indicator that it's perf from Linux tools.
if "perf.data" not in stdout:
return False

# Check that we can run a simple perf run
Copy link
Copy Markdown
Member

@picnixz picnixz Nov 15, 2025

Choose a reason for hiding this comment

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

Keep existing comments since they are unrelated to the PR.

with temp_dir() as script_dir:
try:
output_file = script_dir + "/perf_output.perf"
Expand Down Expand Up @@ -358,7 +365,6 @@ def run_perf(cwd, *args, use_jit=False, **env_vars):
if proc.returncode:
print(proc.stderr, file=sys.stderr)
raise ValueError(f"Perf failed with return code {proc.returncode}")
# Copy the jit_output_file to the output_file
os.rename(jit_output_file, output_file)

base_cmd = ("perf", "script")
Expand Down Expand Up @@ -434,23 +440,23 @@ def baz(n):
"Unwinding is unreliable with frame pointers",
)
class TestPerfProfiler(unittest.TestCase, TestPerfProfilerMixin):

def _cleanup_perf_map(self, pid):
"""
Helper to safely delete a specific perf map file.
"""
perf_map = pathlib.Path(f"/tmp/perf-{pid}.map")
try:
if perf_map.exists():
perf_map.unlink()
except OSError:
pass

def run_perf(self, script_dir, script, activate_trampoline=True):
if activate_trampoline:
return run_perf(script_dir, sys.executable, "-Xperf", script)
return run_perf(script_dir, sys.executable, script)

def setUp(self):
super().setUp()
self.perf_files = set(pathlib.Path("/tmp/").glob("perf-*.map"))

def tearDown(self) -> None:
super().tearDown()
files_to_delete = (
set(pathlib.Path("/tmp/").glob("perf-*.map")) - self.perf_files
)
for file in files_to_delete:
file.unlink()

def test_pre_fork_compile(self):
code = """if 1:
import sys
Expand Down Expand Up @@ -499,11 +505,15 @@ def compile_trampolines_for_all_functions():
stdout=subprocess.PIPE,
env=env,
) as process:
self.addCleanup(self._cleanup_perf_map, process.pid)
stdout, stderr = process.communicate()

self.assertEqual(process.returncode, 0)
self.assertNotIn("Error:", stderr)

Copy link
Copy Markdown
Member

@picnixz picnixz Nov 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Remove this blank line (IOW, don't change blank lines)

child_pid = int(stdout.strip())
self.addCleanup(self._cleanup_perf_map, child_pid)

perf_file = pathlib.Path(f"/tmp/perf-{process.pid}.map")
perf_child_file = pathlib.Path(f"/tmp/perf-{child_pid}.map")
self.assertTrue(perf_file.exists())
Expand All @@ -519,21 +529,13 @@ def compile_trampolines_for_all_functions():
self.assertIn(f"py::foo_fork:{script}", child_perf_file_contents)
self.assertIn(f"py::bar_fork:{script}", child_perf_file_contents)

# Pre-compiled perf-map entries of a forked process must be
Comment thread
pareshjoshij marked this conversation as resolved.
# identical in both the parent and child perf-map files.
perf_file_lines = perf_file_contents.split("\n")
for line in perf_file_lines:
if f"py::foo_fork:{script}" in line or f"py::bar_fork:{script}" in line:
self.assertIn(line, child_perf_file_contents)


def _is_perf_version_at_least(major, minor):
# The output of perf --version looks like "perf version 6.7-3" but
Comment thread
pareshjoshij marked this conversation as resolved.
# it can also be perf version "perf version 5.15.143", or even include
# a commit hash in the version string, like "6.12.9.g242e6068fd5c"
#
# PermissionError is raised if perf does not exist on the Windows Subsystem
# for Linux, see #134987
try:
output = subprocess.check_output(["perf", "--version"], text=True)
except (subprocess.CalledProcessError, FileNotFoundError, PermissionError):
Expand All @@ -550,26 +552,14 @@ def _is_perf_version_at_least(major, minor):
_is_perf_version_at_least(6, 6), "perf command may not work due to a perf bug"
)
class TestPerfProfilerWithDwarf(unittest.TestCase, TestPerfProfilerMixin):

def run_perf(self, script_dir, script, activate_trampoline=True):
if activate_trampoline:
return run_perf(
script_dir, sys.executable, "-Xperf_jit", script, use_jit=True
)
return run_perf(script_dir, sys.executable, script, use_jit=True)

def setUp(self):
super().setUp()
self.perf_files = set(pathlib.Path("/tmp/").glob("jit*.dump"))
self.perf_files |= set(pathlib.Path("/tmp/").glob("jitted-*.so"))
Comment on lines -562 to -563
Copy link
Copy Markdown
Member

@picnixz picnixz Nov 15, 2025

Choose a reason for hiding this comment

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

Those files are no more deleted. So the same should be done for jit-related files.


def tearDown(self) -> None:
super().tearDown()
files_to_delete = set(pathlib.Path("/tmp/").glob("jit*.dump"))
files_to_delete |= set(pathlib.Path("/tmp/").glob("jitted-*.so"))
files_to_delete = files_to_delete - self.perf_files
for file in files_to_delete:
file.unlink()


if __name__ == "__main__":
unittest.main()
Loading