fixup! Merge pull request #117 from StanFromIreland/tachyon-subprocesses-atomic-speed

pablogsal · pablogsal · commit 63d63f0f892a · 2025-12-14T05:08:05.000Z
diff --git a/Doc/library/profiling.sampling.rst b/Doc/library/profiling.sampling.rst
@@ -461,8 +461,8 @@ applications that use :mod:`multiprocessing`, :mod:`subprocess`,
 or other process spawning mechanisms.
 
 .. code-block:: python
+   :caption: worker_pool.py
 
-   # worker_pool.py
    from concurrent.futures import ProcessPoolExecutor
    import math
 
@@ -483,7 +483,7 @@ or other process spawning mechanisms.
    python -m profiling.sampling run --subprocesses --flamegraph worker_pool.py
 
 This produces separate flame graphs for the main process and each worker
-process: ``flamegraph.<main_pid>.html``, ``flamegraph.<worker1_pid>.html``,
+process: ``flamegraph_<main_pid>.html``, ``flamegraph_<worker1_pid>.html``,
 and so on.
 
 Each subprocess receives its own output file. The filename is derived from
@@ -492,21 +492,20 @@ appended:
 
 - If you specify ``-o profile.html``, subprocesses produce ``profile_12345.html``,
   ``profile_12346.html``, and so on
-- With default output, subprocesses produce files like ``flamegraph.12345.html``
+- With default output, subprocesses produce files like ``flamegraph_12345.html``
   or directories like ``heatmap_12345``
 - For pstats format (which defaults to stdout), subprocesses produce files like
-  ``profile.12345.pstats``
+  ``profile_12345.pstats``
 
 The subprocess profilers inherit most sampling options from the parent (interval,
 duration, thread selection, native frames, GC frames, async-aware mode, and
 output format). All Python descendant processes are profiled recursively,
 including grandchildren and further descendants.
 
 Subprocess detection works by periodically scanning for new descendants of
-the target process and checking whether each new process is a Python process.
-On Linux, this uses a fast check of the executable name followed by a full
-probe of the process memory if needed. Non-Python subprocesses (such as
-shell commands or external tools) are ignored.
+the target process and checking whether each new process is a Python process
+by probing the process memory for Python runtime structures. Non-Python
+subprocesses (such as shell commands or external tools) are ignored.
 
 There is a limit of 100 concurrent subprocess profilers to prevent resource
 exhaustion in programs that spawn many processes. If this limit is reached,
diff --git a/Lib/profiling/sampling/_child_monitor.py b/Lib/profiling/sampling/_child_monitor.py
@@ -5,9 +5,8 @@
 separate profiler instances for each discovered child.
 """
 
-import os
-import sys
 import subprocess
+import sys
 import threading
 import time
 
@@ -42,30 +41,14 @@ def get_child_pids(pid, recursive=True):
 
 def is_python_process(pid):
     """
-    Quickly check if a process is a Python process.
-
-    This performs a two-stage check:
-    1. Fast path: Check /proc/{pid}/exe symlink for 'python' (Linux only)
-    2. Full probe: Attempt to locate Python runtime structures in memory
+    Check if a process is a Python process.
 
     Args:
         pid: Process ID to check
 
     Returns:
         bool: True if the process appears to be a Python process, False otherwise
     """
-    # Fast path: Check executable name on Linux (much faster than full probe)
-    if sys.platform == "linux":
-        try:
-            exe_path = os.readlink(f"/proc/{pid}/exe")
-            # Check if executable name contains 'python'
-            exe_name = os.path.basename(exe_path).lower()
-            if "python" not in exe_name:
-                return False
-        except (OSError, PermissionError):
-            # Can't read exe link, fall through to full probe
-            pass
-
     return _remote_debugging.is_python_process(pid)
 
 
@@ -282,7 +265,8 @@ def _build_child_cli_args(self, child_pid):
         args = list(self.cli_args)
 
         if self.output_pattern:
-            output_file = self.output_pattern.format(pid=child_pid)
+            # Use replace() instead of format() to handle user filenames with braces
+            output_file = self.output_pattern.replace("{pid}", str(child_pid))
             found_output = False
             for i, arg in enumerate(args):
                 if arg in ("-o", "--output") and i + 1 < len(args):
diff --git a/Lib/profiling/sampling/cli.py b/Lib/profiling/sampling/cli.py
@@ -144,6 +144,11 @@ def _build_child_profiler_args(args):
 
 
 def _build_output_pattern(args):
+    """Build output filename pattern for child profilers.
+
+    The pattern uses {pid} as a placeholder which will be replaced with the
+    actual child PID using str.replace(), so user filenames with braces are safe.
+    """
     if args.outfile:
         # User specified output - add PID to filename
         base, ext = os.path.splitext(args.outfile)
@@ -152,14 +157,14 @@ def _build_output_pattern(args):
         else:
             return f"{args.outfile}_{{pid}}"
     else:
-        # Use default pattern based on format
+        # Use default pattern based on format (consistent _ separator)
         extension = FORMAT_EXTENSIONS.get(args.format, "txt")
         if args.format == "heatmap":
             return "heatmap_{pid}"
         if args.format == "pstats":
             # pstats defaults to stdout, but for subprocesses we need files
-            return "profile.{pid}.pstats"
-        return f"{args.format}.{{pid}}.{extension}"
+            return "profile_{pid}.pstats"
+        return f"{args.format}_{{pid}}.{extension}"
 
 
 def _parse_mode(mode_string):
diff --git a/Lib/test/test_profiling/test_sampling_profiler/test_children.py b/Lib/test/test_profiling/test_sampling_profiler/test_children.py
@@ -24,6 +24,18 @@
 _SKIP_PERMISSION_MSG = "Insufficient permissions for remote profiling"
 
 
+def _check_remote_debugging_permissions():
+    """Check if we have permissions for remote debugging.
+
+    Returns True if we have permissions, False if we don't.
+    On macOS without proper entitlements, this will return False.
+    """
+    # If is_python_process returns False for the current process,
+    # we don't have permissions (since we ARE a Python process)
+    import _remote_debugging
+    return _remote_debugging.is_python_process(os.getpid())
+
+
 def _readline_with_timeout(file_obj, timeout):
     # Thread-based readline with timeout - works across all platforms
     # including Windows where select() doesn't work with pipes.
@@ -558,16 +570,23 @@ def test_is_python_process_current_process(self):
         """Test that current process is detected as Python."""
         from profiling.sampling._child_monitor import is_python_process
 
+        if not _check_remote_debugging_permissions():
+            self.skipTest(_SKIP_PERMISSION_MSG)
+
         # Current process should be Python
+        result = is_python_process(os.getpid())
         self.assertTrue(
-            is_python_process(os.getpid()),
+            result,
             f"Current process (PID {os.getpid()}) should be detected as Python",
         )
 
     def test_is_python_process_python_subprocess(self):
         """Test that a Python subprocess is detected as Python."""
         from profiling.sampling._child_monitor import is_python_process
 
+        if not _check_remote_debugging_permissions():
+            self.skipTest(_SKIP_PERMISSION_MSG)
+
         # Start a Python subprocess
         proc = subprocess.Popen(
             [sys.executable, "-c", "import time; time.sleep(10)"],
@@ -584,12 +603,9 @@ def test_is_python_process_python_subprocess(self):
             while time.time() < deadline:
                 if proc.poll() is not None:
                     self.fail(f"Process {proc.pid} exited unexpectedly")
-                try:
-                    if is_python_process(proc.pid):
-                        detected = True
-                        break
-                except PermissionError:
-                    self.skipTest(_SKIP_PERMISSION_MSG)
+                if is_python_process(proc.pid):
+                    detected = True
+                    break
                 time.sleep(0.05)
 
             self.assertTrue(
@@ -632,8 +648,9 @@ def test_is_python_process_nonexistent_pid(self):
         from profiling.sampling._child_monitor import is_python_process
 
         # Use a very high PID that's unlikely to exist
+        result = is_python_process(999999999)
         self.assertFalse(
-            is_python_process(999999999),
+            result,
             "Nonexistent PID 999999999 should return False",
         )
 
diff --git a/Modules/_remote_debugging/subprocess.c b/Modules/_remote_debugging/subprocess.c
@@ -400,52 +400,54 @@ get_child_pids_platform(pid_t target_pid, int recursive, pid_array_t *result)
         return -1;
     }
 
-    /* First pass: count processes */
-    PROCESSENTRY32 pe;
-    pe.dwSize = sizeof(PROCESSENTRY32);
+    /* Use dynamic arrays for single-pass collection */
+    pid_array_t all_pids;
+    pid_array_t ppids;
 
-    size_t count = 0;
-    if (Process32First(snapshot, &pe)) {
-        do {
-            count++;
-        } while (Process32Next(snapshot, &pe));
+    if (pid_array_init(&all_pids) < 0) {
+        CloseHandle(snapshot);
+        return -1;
     }
 
-    /* Allocate arrays for PIDs and PPIDs */
-    pid_t *pid_list = (pid_t *)PyMem_Malloc(count * sizeof(pid_t));
-    pid_t *ppids = (pid_t *)PyMem_Malloc(count * sizeof(pid_t));
-    if (!pid_list || !ppids) {
+    if (pid_array_init(&ppids) < 0) {
         CloseHandle(snapshot);
-        PyMem_Free(pid_list);
-        PyMem_Free(ppids);
-        PyErr_NoMemory();
+        pid_array_cleanup(&all_pids);
         return -1;
     }
 
-    /* Second pass: collect PIDs and PPIDs */
+    /* Single pass: collect PIDs and PPIDs together */
+    PROCESSENTRY32 pe;
     pe.dwSize = sizeof(PROCESSENTRY32);
-    size_t idx = 0;
     if (Process32First(snapshot, &pe)) {
         do {
-            pid_list[idx] = (pid_t)pe.th32ProcessID;
-            ppids[idx] = (pid_t)pe.th32ParentProcessID;
-            idx++;
-        } while (Process32Next(snapshot, &pe) && idx < count);
+            if (pid_array_append(&all_pids, (pid_t)pe.th32ProcessID) < 0) {
+                CloseHandle(snapshot);
+                pid_array_cleanup(&all_pids);
+                pid_array_cleanup(&ppids);
+                return -1;
+            }
+            if (pid_array_append(&ppids, (pid_t)pe.th32ParentProcessID) < 0) {
+                CloseHandle(snapshot);
+                pid_array_cleanup(&all_pids);
+                pid_array_cleanup(&ppids);
+                return -1;
+            }
+        } while (Process32Next(snapshot, &pe));
     }
 
     CloseHandle(snapshot);
 
     /* Find children using BFS */
     pid_array_t to_process;
     if (pid_array_init(&to_process) < 0) {
-        PyMem_Free(pid_list);
-        PyMem_Free(ppids);
+        pid_array_cleanup(&all_pids);
+        pid_array_cleanup(&ppids);
         return -1;
     }
 
     if (pid_array_append(&to_process, target_pid) < 0) {
-        PyMem_Free(pid_list);
-        PyMem_Free(ppids);
+        pid_array_cleanup(&all_pids);
+        pid_array_cleanup(&ppids);
         pid_array_cleanup(&to_process);
         return -1;
     }
@@ -454,22 +456,22 @@ get_child_pids_platform(pid_t target_pid, int recursive, pid_array_t *result)
     while (process_idx < to_process.count) {
         pid_t current_pid = to_process.pids[process_idx++];
 
-        for (size_t i = 0; i < idx; i++) {
-            if (ppids[i] == current_pid) {
-                pid_t child_pid = pid_list[i];
+        for (size_t i = 0; i < all_pids.count; i++) {
+            if (ppids.pids[i] == current_pid) {
+                pid_t child_pid = all_pids.pids[i];
 
                 if (!pid_array_contains(result, child_pid)) {
                     if (pid_array_append(result, child_pid) < 0) {
-                        PyMem_Free(pid_list);
-                        PyMem_Free(ppids);
+                        pid_array_cleanup(&all_pids);
+                        pid_array_cleanup(&ppids);
                         pid_array_cleanup(&to_process);
                         return -1;
                     }
 
                     if (recursive) {
                         if (pid_array_append(&to_process, child_pid) < 0) {
-                            PyMem_Free(pid_list);
-                            PyMem_Free(ppids);
+                            pid_array_cleanup(&all_pids);
+                            pid_array_cleanup(&ppids);
                             pid_array_cleanup(&to_process);
                             return -1;
                         }
@@ -483,8 +485,8 @@ get_child_pids_platform(pid_t target_pid, int recursive, pid_array_t *result)
         }
     }
 
-    PyMem_Free(pid_list);
-    PyMem_Free(ppids);
+    pid_array_cleanup(&all_pids);
+    pid_array_cleanup(&ppids);
     pid_array_cleanup(&to_process);
     return 0;
 }
