-
-
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
Merged
+154
−100
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| import importlib.util | ||
| import locale | ||
| import os | ||
| import re | ||
| import selectors | ||
| import socket | ||
| import subprocess | ||
|
|
@@ -20,6 +21,7 @@ | |
| from .binary_collector import BinaryCollector | ||
| from .binary_reader import BinaryReader | ||
| from .constants import ( | ||
| MICROSECONDS_PER_SECOND, | ||
| PROFILING_MODE_ALL, | ||
| PROFILING_MODE_WALL, | ||
| PROFILING_MODE_CPU, | ||
|
|
@@ -66,8 +68,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 | ||
|
|
||
|
|
@@ -116,7 +118,8 @@ def _build_child_profiler_args(args): | |
| child_args = [] | ||
|
|
||
| # Sampling options | ||
| child_args.extend(["-i", str(args.interval)]) | ||
| hz = MICROSECONDS_PER_SECOND // args.sample_interval_usec | ||
| child_args.extend(["-r", str(hz)]) | ||
| child_args.extend(["-d", str(args.duration)]) | ||
|
|
||
| if args.all_threads: | ||
|
|
@@ -239,7 +242,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() | ||
|
|
@@ -268,7 +271,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: | ||
|
|
@@ -279,7 +282,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() | ||
|
|
@@ -290,16 +293,64 @@ def _run_with_sync(original_cmd, suppress_output=False): | |
| return process | ||
|
|
||
|
|
||
| _RATE_PATTERN = re.compile(r''' | ||
| ^ # Start of string | ||
| ( # Group 1: The numeric value | ||
| \d+ # One or more digits (integer part) | ||
| (?:\.\d+)? # Optional: decimal point followed by digits | ||
| ) # Examples: "10", "0.5", "100.25" | ||
| ( # Group 2: Optional unit suffix | ||
| hz # "hz" - hertz | ||
| | khz # "khz" - kilohertz | ||
| | k # "k" - shorthand for kilohertz | ||
| )? # Suffix is optional (bare number = Hz) | ||
| $ # End of string | ||
| ''', re.VERBOSE | re.IGNORECASE) | ||
|
|
||
|
|
||
| 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( | ||
| f"Invalid sampling rate format: {rate_str}. " | ||
| "Expected: number followed by optional suffix (hz, khz, k) with no spaces (e.g., 10khz)" | ||
| ) | ||
|
|
||
| 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", | ||
| dest="sample_interval_usec", | ||
| help="sampling rate (e.g., 10000, 10khz, 10k)", | ||
| ) | ||
| sampling_group.add_argument( | ||
| "-d", | ||
|
|
@@ -487,14 +538,13 @@ 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, | ||
| output_file=None, compression='auto'): | ||
| """Create the appropriate collector based on format type. | ||
|
|
||
| Args: | ||
| format_type: The output format ('pstats', 'collapsed', 'flamegraph', 'gecko', 'heatmap', 'binary') | ||
| 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) | ||
|
|
@@ -519,9 +569,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): | ||
|
|
@@ -725,8 +775,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` | ||
|
|
@@ -860,7 +910,7 @@ def _handle_attach(args): | |
|
|
||
| # Create the appropriate collector | ||
| collector = _create_collector( | ||
| args.format, args.interval, skip_idle, args.opcodes, | ||
| args.format, args.sample_interval_usec, skip_idle, args.opcodes, | ||
| output_file=output_file, | ||
| compression=getattr(args, 'compression', 'auto') | ||
| ) | ||
|
|
@@ -938,7 +988,7 @@ def _handle_run(args): | |
|
|
||
| # Create the appropriate collector | ||
| collector = _create_collector( | ||
| args.format, args.interval, skip_idle, args.opcodes, | ||
| args.format, args.sample_interval_usec, skip_idle, args.opcodes, | ||
| output_file=output_file, | ||
| compression=getattr(args, 'compression', 'auto') | ||
| ) | ||
|
|
@@ -965,7 +1015,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() | ||
|
|
@@ -980,7 +1030,7 @@ def _handle_live_attach(args, pid): | |
|
|
||
| # Create live collector with default settings | ||
| collector = LiveStatsCollector( | ||
| args.interval, | ||
| args.sample_interval_usec, | ||
| skip_idle=skip_idle, | ||
| sort_by="tottime", # Default initial sort | ||
| limit=20, # Default limit | ||
|
|
@@ -1027,7 +1077,7 @@ def _handle_live_run(args): | |
|
|
||
| # Create live collector with default settings | ||
| collector = LiveStatsCollector( | ||
| args.interval, | ||
| args.sample_interval_usec, | ||
| skip_idle=skip_idle, | ||
| sort_by="tottime", # Default initial sort | ||
| limit=20, # Default limit | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nit: Let's add a hint about spaces in the error message since "10 khz" (with space) is rejected but users might try it