Skip to content

Commit c56afcd

Browse files
committed
gh-141592: remove all explanatory comments per review
Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>
1 parent 2f3e277 commit c56afcd

1 file changed

Lines changed: 0 additions & 56 deletions

File tree

Lib/test/test_perf_profiler.py

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -35,26 +35,16 @@ def supports_trampoline_profiling():
3535

3636

3737
class TestPerfTrampoline(unittest.TestCase):
38-
#
39-
# We've removed the buggy setUp and tearDown methods.
40-
# They caused race conditions by scanning /tmp for `perf-*.map` files.
41-
# Instead, we now use `addCleanup()` inside each test to delete
42-
# the *specific* files we know that test created.
43-
#
4438

4539
def _cleanup_perf_map(self, pid):
4640
"""
4741
Helper to safely delete a specific perf map file.
48-
We register this with addCleanup() to make sure it runs
49-
even if the test assertions fail.
5042
"""
5143
perf_map = pathlib.Path(f"/tmp/perf-{pid}.map")
5244
try:
5345
if perf_map.exists():
5446
perf_map.unlink()
5547
except OSError:
56-
# Surpress errors, e.g., file already removed or
57-
# another kind of race condition.
5848
pass
5949

6050
@unittest.skipIf(support.check_bolt_optimized(), "fails on BOLT instrumented binaries")
@@ -75,7 +65,6 @@ def baz():
7565
script = make_script(script_dir, "perftest", code)
7666
env = {**os.environ, "PYTHON_JIT": "0"}
7767

78-
# Start the subprocess...
7968
process = subprocess.Popen(
8069
[sys.executable, "-Xperf", script],
8170
text=True,
@@ -84,8 +73,6 @@ def baz():
8473
env=env,
8574
)
8675

87-
# ...and immediately register a cleanup task for its PID.
88-
# This is guaranteed to run after the test.
8976
self.addCleanup(self._cleanup_perf_map, process.pid)
9077

9178
with process:
@@ -134,11 +121,9 @@ def baz_fork():
134121
def foo():
135122
pid = os.fork()
136123
if pid == 0:
137-
# We're in the child process
138124
print(os.getpid())
139125
baz_fork()
140126
else:
141-
# We're in the parent process
142127
_, status = os.waitpid(-1, 0)
143128
sys.exit(status)
144129
@@ -160,17 +145,13 @@ def baz():
160145
stdout=subprocess.PIPE,
161146
env=env,
162147
) as process:
163-
# Register cleanup for the PARENT process
164148
self.addCleanup(self._cleanup_perf_map, process.pid)
165149
stdout, stderr = process.communicate()
166150

167151
self.assertEqual(process.returncode, 0)
168152
self.assertEqual(stderr, "")
169153

170-
# The child process printed its PID to stdout
171154
child_pid = int(stdout.strip())
172-
173-
# Register cleanup for the CHILD process
174155
self.addCleanup(self._cleanup_perf_map, child_pid)
175156

176157
perf_file = pathlib.Path(f"/tmp/perf-{process.pid}.map")
@@ -227,7 +208,6 @@ def baz():
227208
stdout=subprocess.PIPE,
228209
env=env,
229210
) as process:
230-
# Register cleanup inside the loop for each process created
231211
self.addCleanup(self._cleanup_perf_map, process.pid)
232212
stdout, stderr = process.communicate()
233213

@@ -301,12 +281,9 @@ def perf_command_works():
301281
except (subprocess.SubprocessError, OSError):
302282
return False
303283

304-
# perf version does not return a version number on Fedora. Use presence
305-
# of "perf.data" in help as indicator that it's perf from Linux tools.
306284
if "perf.data" not in stdout:
307285
return False
308286

309-
# Check that we can run a simple perf run
310287
with temp_dir() as script_dir:
311288
try:
312289
output_file = script_dir + "/perf_output.perf"
@@ -338,10 +315,6 @@ def perf_command_works():
338315

339316

340317
def run_perf(cwd, *args, use_jit=False, **env_vars):
341-
# This helper function runs perf and stores its output files *inside*
342-
# the 'cwd' (which is a temp_dir()). This is good! It means
343-
# those files (`perf_output.perf`, `jit_output.dump`)
344-
# are automatically cleaned up when the temp_dir() context manager exits.
345318
env = os.environ.copy()
346319
if env_vars:
347320
env.update(env_vars)
@@ -392,7 +365,6 @@ def run_perf(cwd, *args, use_jit=False, **env_vars):
392365
if proc.returncode:
393366
print(proc.stderr, file=sys.stderr)
394367
raise ValueError(f"Perf failed with return code {proc.returncode}")
395-
# Copy the jit_output_file to the output_file
396368
os.rename(jit_output_file, output_file)
397369

398370
base_cmd = ("perf", "script")
@@ -468,15 +440,10 @@ def baz(n):
468440
"Unwinding is unreliable with frame pointers",
469441
)
470442
class TestPerfProfiler(unittest.TestCase, TestPerfProfilerMixin):
471-
#
472-
# Just like in TestPerfTrampoline, we remove the racy
473-
# setUp/tearDown methods.
474-
#
475443

476444
def _cleanup_perf_map(self, pid):
477445
"""
478446
Helper to safely delete a specific perf map file.
479-
We need this for test_pre_fork_compile.
480447
"""
481448
perf_map = pathlib.Path(f"/tmp/perf-{pid}.map")
482449
try:
@@ -486,8 +453,6 @@ def _cleanup_perf_map(self, pid):
486453
pass
487454

488455
def run_perf(self, script_dir, script, activate_trampoline=True):
489-
# The run_perf helper handles its own temp files inside script_dir
490-
# so we don't need to add cleanups for it here.
491456
if activate_trampoline:
492457
return run_perf(script_dir, sys.executable, "-Xperf", script)
493458
return run_perf(script_dir, sys.executable, script)
@@ -540,8 +505,6 @@ def compile_trampolines_for_all_functions():
540505
stdout=subprocess.PIPE,
541506
env=env,
542507
) as process:
543-
# This test also creates map files, so we clean them up
544-
# just like in TestPerfTrampoline.
545508
self.addCleanup(self._cleanup_perf_map, process.pid)
546509
stdout, stderr = process.communicate()
547510

@@ -566,21 +529,13 @@ def compile_trampolines_for_all_functions():
566529
self.assertIn(f"py::foo_fork:{script}", child_perf_file_contents)
567530
self.assertIn(f"py::bar_fork:{script}", child_perf_file_contents)
568531

569-
# Pre-compiled perf-map entries of a forked process must be
570-
# identical in both the parent and child perf-map files.
571532
perf_file_lines = perf_file_contents.split("\n")
572533
for line in perf_file_lines:
573534
if f"py::foo_fork:{script}" in line or f"py::bar_fork:{script}" in line:
574535
self.assertIn(line, child_perf_file_contents)
575536

576537

577538
def _is_perf_version_at_least(major, minor):
578-
# The output of perf --version looks like "perf version 6.7-3" but
579-
# it can also be perf version "perf version 5.15.143", or even include
580-
# a commit hash in the version string, like "6.12.9.g242e6068fd5c"
581-
#
582-
# PermissionError is raised if perf does not exist on the Windows Subsystem
583-
# for Linux, see #134987
584539
try:
585540
output = subprocess.check_output(["perf", "--version"], text=True)
586541
except (subprocess.CalledProcessError, FileNotFoundError, PermissionError):
@@ -597,13 +552,6 @@ def _is_perf_version_at_least(major, minor):
597552
_is_perf_version_at_least(6, 6), "perf command may not work due to a perf bug"
598553
)
599554
class TestPerfProfilerWithDwarf(unittest.TestCase, TestPerfProfilerMixin):
600-
#
601-
# We've removed the racy setUp/tearDown methods from here, too.
602-
# The `run_perf` function (with use_jit=True) already creates its
603-
# .dump and .so files inside the `temp_dir()`, which gets
604-
# cleaned up automatically. The old setUp/tearDown were
605-
# trying to clean up /tmp, which was buggy and unreliable.
606-
#
607555

608556
def run_perf(self, script_dir, script, activate_trampoline=True):
609557
if activate_trampoline:
@@ -612,10 +560,6 @@ def run_perf(self, script_dir, script, activate_trampoline=True):
612560
)
613561
return run_perf(script_dir, sys.executable, script, use_jit=True)
614562

615-
#
616-
# No setUp or tearDown needed!
617-
#
618-
619563

620564
if __name__ == "__main__":
621565
unittest.main()

0 commit comments

Comments
 (0)