-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-138122: Replace --interval with --sampling-rate #143085
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||||||||||||||||||||
| import argparse | ||||||||||||||||||||||||||||||
| import importlib.util | ||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||
| import selectors | ||||||||||||||||||||||||||||||
| import socket | ||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||
|
|
@@ -17,6 +18,7 @@ | |||||||||||||||||||||||||||||
| from .heatmap_collector import HeatmapCollector | ||||||||||||||||||||||||||||||
| from .gecko_collector import GeckoCollector | ||||||||||||||||||||||||||||||
| from .constants import ( | ||||||||||||||||||||||||||||||
| MICROSECONDS_PER_SECOND, | ||||||||||||||||||||||||||||||
| PROFILING_MODE_ALL, | ||||||||||||||||||||||||||||||
| PROFILING_MODE_WALL, | ||||||||||||||||||||||||||||||
| PROFILING_MODE_CPU, | ||||||||||||||||||||||||||||||
|
|
@@ -63,8 +65,8 @@ class CustomFormatter( | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Constants for socket synchronization | ||||||||||||||||||||||||||||||
| _SYNC_TIMEOUT = 5.0 | ||||||||||||||||||||||||||||||
| _PROCESS_KILL_TIMEOUT = 2.0 | ||||||||||||||||||||||||||||||
| _SYNC_TIMEOUT_SEC = 5.0 | ||||||||||||||||||||||||||||||
| _PROCESS_KILL_TIMEOUT_SEC = 2.0 | ||||||||||||||||||||||||||||||
| _READY_MESSAGE = b"ready" | ||||||||||||||||||||||||||||||
| _RECV_BUFFER_SIZE = 1024 | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
@@ -111,7 +113,8 @@ def _build_child_profiler_args(args): | |||||||||||||||||||||||||||||
| child_args = [] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Sampling options | ||||||||||||||||||||||||||||||
| child_args.extend(["-i", str(args.interval)]) | ||||||||||||||||||||||||||||||
| hz = MICROSECONDS_PER_SECOND // args.sampling_rate | ||||||||||||||||||||||||||||||
| child_args.extend(["-r", str(hz)]) | ||||||||||||||||||||||||||||||
| child_args.extend(["-d", str(args.duration)]) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if args.all_threads: | ||||||||||||||||||||||||||||||
|
|
@@ -234,7 +237,7 @@ def _run_with_sync(original_cmd, suppress_output=False): | |||||||||||||||||||||||||||||
| sync_sock.bind(("127.0.0.1", 0)) # Let OS choose a free port | ||||||||||||||||||||||||||||||
| sync_port = sync_sock.getsockname()[1] | ||||||||||||||||||||||||||||||
| sync_sock.listen(1) | ||||||||||||||||||||||||||||||
| sync_sock.settimeout(_SYNC_TIMEOUT) | ||||||||||||||||||||||||||||||
| sync_sock.settimeout(_SYNC_TIMEOUT_SEC) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Get current working directory to preserve it | ||||||||||||||||||||||||||||||
| cwd = os.getcwd() | ||||||||||||||||||||||||||||||
|
|
@@ -263,7 +266,7 @@ def _run_with_sync(original_cmd, suppress_output=False): | |||||||||||||||||||||||||||||
| process = subprocess.Popen(cmd, **popen_kwargs) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||
| _wait_for_ready_signal(sync_sock, process, _SYNC_TIMEOUT) | ||||||||||||||||||||||||||||||
| _wait_for_ready_signal(sync_sock, process, _SYNC_TIMEOUT_SEC) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Close stderr pipe if we were capturing it | ||||||||||||||||||||||||||||||
| if process.stderr: | ||||||||||||||||||||||||||||||
|
|
@@ -274,7 +277,7 @@ def _run_with_sync(original_cmd, suppress_output=False): | |||||||||||||||||||||||||||||
| if process.poll() is None: | ||||||||||||||||||||||||||||||
| process.terminate() | ||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||
| process.wait(timeout=_PROCESS_KILL_TIMEOUT) | ||||||||||||||||||||||||||||||
| process.wait(timeout=_PROCESS_KILL_TIMEOUT_SEC) | ||||||||||||||||||||||||||||||
| except subprocess.TimeoutExpired: | ||||||||||||||||||||||||||||||
| process.kill() | ||||||||||||||||||||||||||||||
| process.wait() | ||||||||||||||||||||||||||||||
|
|
@@ -285,16 +288,51 @@ def _run_with_sync(original_cmd, suppress_output=False): | |||||||||||||||||||||||||||||
| return process | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| _RATE_PATTERN = re.compile(r'^(\d+(?:\.\d+)?)(hz|khz|k)?$', re.IGNORECASE) | ||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha you know how much I like to comment these :) |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _parse_sampling_rate(rate_str: str) -> int: | ||||||||||||||||||||||||||||||
| """Parse sampling rate string to microseconds.""" | ||||||||||||||||||||||||||||||
| rate_str = rate_str.strip().lower() | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| match = _RATE_PATTERN.match(rate_str) | ||||||||||||||||||||||||||||||
| if not match: | ||||||||||||||||||||||||||||||
| raise argparse.ArgumentTypeError( | ||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Let's add a hint about spaces in the error message since "10 khz" (with space) is rejected but users might try it |
||||||||||||||||||||||||||||||
| f"Invalid sampling rate format: {rate_str}. " | ||||||||||||||||||||||||||||||
| "Expected: number followed by optional suffix (hz, khz, k)" | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| number_part = match.group(1) | ||||||||||||||||||||||||||||||
| suffix = match.group(2) or '' | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Determine multiplier based on suffix | ||||||||||||||||||||||||||||||
| suffix_map = { | ||||||||||||||||||||||||||||||
| 'hz': 1, | ||||||||||||||||||||||||||||||
| 'khz': 1000, | ||||||||||||||||||||||||||||||
| 'k': 1000, | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| multiplier = suffix_map.get(suffix, 1) | ||||||||||||||||||||||||||||||
| hz = float(number_part) * multiplier | ||||||||||||||||||||||||||||||
| if hz <= 0: | ||||||||||||||||||||||||||||||
| raise argparse.ArgumentTypeError(f"Sampling rate must be positive: {rate_str}") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| interval_usec = int(MICROSECONDS_PER_SECOND / hz) | ||||||||||||||||||||||||||||||
| if interval_usec < 1: | ||||||||||||||||||||||||||||||
| raise argparse.ArgumentTypeError(f"Sampling rate too high: {rate_str}") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return interval_usec | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _add_sampling_options(parser): | ||||||||||||||||||||||||||||||
| """Add sampling configuration options to a parser.""" | ||||||||||||||||||||||||||||||
| sampling_group = parser.add_argument_group("Sampling configuration") | ||||||||||||||||||||||||||||||
| sampling_group.add_argument( | ||||||||||||||||||||||||||||||
| "-i", | ||||||||||||||||||||||||||||||
| "--interval", | ||||||||||||||||||||||||||||||
| type=int, | ||||||||||||||||||||||||||||||
| default=100, | ||||||||||||||||||||||||||||||
| metavar="MICROSECONDS", | ||||||||||||||||||||||||||||||
| help="sampling interval", | ||||||||||||||||||||||||||||||
| "-r", | ||||||||||||||||||||||||||||||
| "--sampling-rate", | ||||||||||||||||||||||||||||||
| type=_parse_sampling_rate, | ||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The argument is named |
||||||||||||||||||||||||||||||
| default="1khz", | ||||||||||||||||||||||||||||||
| metavar="RATE", | ||||||||||||||||||||||||||||||
| help="sampling rate (e.g., 10000, 10khz, 10k)", | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| sampling_group.add_argument( | ||||||||||||||||||||||||||||||
| "-d", | ||||||||||||||||||||||||||||||
|
|
@@ -460,12 +498,12 @@ def _sort_to_mode(sort_choice): | |||||||||||||||||||||||||||||
| return sort_map.get(sort_choice, SORT_MODE_NSAMPLES) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _create_collector(format_type, interval, skip_idle, opcodes=False): | ||||||||||||||||||||||||||||||
| def _create_collector(format_type, sample_interval_usec, skip_idle, opcodes=False): | ||||||||||||||||||||||||||||||
| """Create the appropriate collector based on format type. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||
| format_type: The output format ('pstats', 'collapsed', 'flamegraph', 'gecko', 'heatmap') | ||||||||||||||||||||||||||||||
| interval: Sampling interval in microseconds | ||||||||||||||||||||||||||||||
| sample_interval_usec: Sampling interval in microseconds | ||||||||||||||||||||||||||||||
| skip_idle: Whether to skip idle samples | ||||||||||||||||||||||||||||||
| opcodes: Whether to collect opcode information (only used by gecko format | ||||||||||||||||||||||||||||||
| for creating interval markers in Firefox Profiler) | ||||||||||||||||||||||||||||||
|
|
@@ -481,9 +519,9 @@ def _create_collector(format_type, interval, skip_idle, opcodes=False): | |||||||||||||||||||||||||||||
| # and is the only format that uses opcodes for interval markers | ||||||||||||||||||||||||||||||
| if format_type == "gecko": | ||||||||||||||||||||||||||||||
| skip_idle = False | ||||||||||||||||||||||||||||||
| return collector_class(interval, skip_idle=skip_idle, opcodes=opcodes) | ||||||||||||||||||||||||||||||
| return collector_class(sample_interval_usec, skip_idle=skip_idle, opcodes=opcodes) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return collector_class(interval, skip_idle=skip_idle) | ||||||||||||||||||||||||||||||
| return collector_class(sample_interval_usec, skip_idle=skip_idle) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _generate_output_filename(format_type, pid): | ||||||||||||||||||||||||||||||
|
|
@@ -659,8 +697,8 @@ def main(): | |||||||||||||||||||||||||||||
| # Generate flamegraph from a script | ||||||||||||||||||||||||||||||
| `python -m profiling.sampling run --flamegraph -o output.html script.py` | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Profile with custom interval and duration | ||||||||||||||||||||||||||||||
| `python -m profiling.sampling run -i 50 -d 30 script.py` | ||||||||||||||||||||||||||||||
| # Profile with custom rate and duration | ||||||||||||||||||||||||||||||
| `python -m profiling.sampling run -r 5khz -d 30 script.py` | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Save collapsed stacks to file | ||||||||||||||||||||||||||||||
| `python -m profiling.sampling run --collapsed -o stacks.txt script.py` | ||||||||||||||||||||||||||||||
|
|
@@ -764,7 +802,7 @@ def _handle_attach(args): | |||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Create the appropriate collector | ||||||||||||||||||||||||||||||
| collector = _create_collector(args.format, args.interval, skip_idle, args.opcodes) | ||||||||||||||||||||||||||||||
| collector = _create_collector(args.format, args.sampling_rate, skip_idle, args.opcodes) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| with _get_child_monitor_context(args, args.pid): | ||||||||||||||||||||||||||||||
| collector = sample( | ||||||||||||||||||||||||||||||
|
|
@@ -833,7 +871,7 @@ def _handle_run(args): | |||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Create the appropriate collector | ||||||||||||||||||||||||||||||
| collector = _create_collector(args.format, args.interval, skip_idle, args.opcodes) | ||||||||||||||||||||||||||||||
| collector = _create_collector(args.format, args.sampling_rate, skip_idle, args.opcodes) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| with _get_child_monitor_context(args, process.pid): | ||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||
|
|
@@ -856,7 +894,7 @@ def _handle_run(args): | |||||||||||||||||||||||||||||
| if process.poll() is None: | ||||||||||||||||||||||||||||||
| process.terminate() | ||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||
| process.wait(timeout=_PROCESS_KILL_TIMEOUT) | ||||||||||||||||||||||||||||||
| process.wait(timeout=_PROCESS_KILL_TIMEOUT_SEC) | ||||||||||||||||||||||||||||||
| except subprocess.TimeoutExpired: | ||||||||||||||||||||||||||||||
| process.kill() | ||||||||||||||||||||||||||||||
| process.wait() | ||||||||||||||||||||||||||||||
|
|
@@ -871,7 +909,7 @@ def _handle_live_attach(args, pid): | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Create live collector with default settings | ||||||||||||||||||||||||||||||
| collector = LiveStatsCollector( | ||||||||||||||||||||||||||||||
| args.interval, | ||||||||||||||||||||||||||||||
| args.sampling_rate, | ||||||||||||||||||||||||||||||
| skip_idle=skip_idle, | ||||||||||||||||||||||||||||||
| sort_by="tottime", # Default initial sort | ||||||||||||||||||||||||||||||
| limit=20, # Default limit | ||||||||||||||||||||||||||||||
|
|
@@ -917,7 +955,7 @@ def _handle_live_run(args): | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Create live collector with default settings | ||||||||||||||||||||||||||||||
| collector = LiveStatsCollector( | ||||||||||||||||||||||||||||||
| args.interval, | ||||||||||||||||||||||||||||||
| args.sampling_rate, | ||||||||||||||||||||||||||||||
| skip_idle=skip_idle, | ||||||||||||||||||||||||||||||
| sort_by="tottime", # Default initial sort | ||||||||||||||||||||||||||||||
| limit=20, # Default limit | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the documentation to reflect the actual 1 kHz default