| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||||||||||
| """Thin Python wrapper around C binary writer for profiling data.""" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import _remote_debugging | ||||||||||||||||||||||||||||||||||
| Expand Down Expand Up | @@ -81,6 +82,7 @@ def __init__(self, filename, sample_interval_usec, *, skip_idle=False, | |||||||||||||||||||||||||||||||||
| self.filename = filename | ||||||||||||||||||||||||||||||||||
| self.sample_interval_usec = sample_interval_usec | ||||||||||||||||||||||||||||||||||
| self.skip_idle = skip_idle | ||||||||||||||||||||||||||||||||||
| self.running = True | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| compression_type = _resolve_compression(compression) | ||||||||||||||||||||||||||||||||||
| start_time_us = int(time.monotonic() * 1_000_000) | ||||||||||||||||||||||||||||||||||
| Expand All | @@ -102,9 +104,19 @@ def collect(self, stack_frames, timestamp_us=None): | |||||||||||||||||||||||||||||||||
| timestamp_us: Optional timestamp in microseconds. If not provided, | ||||||||||||||||||||||||||||||||||
| uses time.monotonic() to generate one. | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| if not self.running: | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| if timestamp_us is None: | ||||||||||||||||||||||||||||||||||
| timestamp_us = int(time.monotonic() * 1_000_000) | ||||||||||||||||||||||||||||||||||
| self._writer.write_sample(stack_frames, timestamp_us) | ||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||
| self._writer.write_sample(stack_frames, timestamp_us) | ||||||||||||||||||||||||||||||||||
| except OverflowError as e: | ||||||||||||||||||||||||||||||||||
|
Comment thread
Copy link
Copy Markdown
Contributor
Author
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality@pablogsal Truth be told, I'm not 100% sure what's the best layer for handling "finalizable" exceptions like OverflowError. The promise of "finalizing the file" is in the BinaryWriter: cpython/Modules/_remote_debugging/module.c Lines 1857 to 1864 in 3428959
This is not the case: cpython/Lib/profiling/sampling/binary_collector.py Lines 115 to 120 in 3428959
cpython/Modules/_remote_debugging/module.c Line 1874 in 3428959
Perhaps it's something as simple as weakening exc_type == Py_None, instead of doing a round-trip since the module should contain all the knowledge (ie: which exceptions are finalizable for it.) This issue presents itself there since we've got both the unwinder and the collector in the bloc : cpython/Lib/profiling/sampling/sample.py Line 180 in 3428959
...and the meaning of MemoryError, RuntimeError or OSError in the unwinder is non-fatal, but for the binary collector it might mean non-finalizable exception. Of course - the very proper way to approach this is to maintain the finalizable state is in the writer itself, instead of relying on the exceptions... but that's way too much for this PR.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI agree that the module should own this knowledge, but I don’t think weakening exc_type == Py_None is the right contract. What we really need to know is whether the writer can still produce a valid file. So the clean version is probably: the writer tracks “still finalizable” internally. Format-limit errors that are detected before changing writer state leave it finalizable; I/O/compression/partial-write failures mark it broken. Then __exit__ and the collector can both ask the writer state instead of reasoning from broad exception classes. For this PR I’m fine with the collector-level stop, but I’d avoid broadening __exit__ based only on OverflowError.
Sorry, something went wrong.
maurycy reacted with eyes emoji
All reactions
Copy link
Copy Markdown
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. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityActually I think for this PR, I’d prefer we make the C writer expose the real state instead of catching broad OverflowError. Te idea would be to:
Sorry, something went wrong.
maurycy reacted with eyes emoji
All reactions
|
||||||||||||||||||||||||||||||||||
| if not self._writer.limit_reached: | ||||||||||||||||||||||||||||||||||
| raise | ||||||||||||||||||||||||||||||||||
| self.running = False | ||||||||||||||||||||||||||||||||||
| print(f"Warning: {e}; stopping early and keeping the data " | ||||||||||||||||||||||||||||||||||
| "collected so far.", | ||||||||||||||||||||||||||||||||||
| file=sys.stderr) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def collect_failed_sample(self): | ||||||||||||||||||||||||||||||||||
| """Record a failed sample attempt (no-op for binary format).""" | ||||||||||||||||||||||||||||||||||
| Expand Down Expand Up | @@ -143,9 +155,5 @@ def __enter__(self): | |||||||||||||||||||||||||||||||||
| return self | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||||||||||||||||||||||||||||||||||
| """Context manager exit - finalize unless there was an error.""" | ||||||||||||||||||||||||||||||||||
| if exc_type is None: | ||||||||||||||||||||||||||||||||||
| self._writer.finalize() | ||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||
| self._writer.close() | ||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||
| """Finalize if the writer can still produce a valid file.""" | ||||||||||||||||||||||||||||||||||
| return self._writer.__exit__(exc_type, exc_val, exc_tb) | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Fix ``profiling.sampling --binary`` leaving unreadable profile files when | ||
| the binary format reaches a size limit. Preserve collected samples when the | ||
| writer can still finalize safely. Patch by Maurycy Pawłowski-Wieroński. |
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.