| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
I'm concerned that we now need to read memory three times with this approach: struct gc_stats {
uint32_t before_update_seq;
struct gc_young_stats_buffer young;
struct gc_old_stats_buffer old[2];
uint32_t after_update_seq;
};
If before_update_seq and after_update_seq don't match, we have a torn read. What do you think? |
Sorry, something went wrong.
|
@sergey-miryanov I'd push back. :-) successes=1966598 failures=191 is around ~200KHz. I'm not sure if there's a guarantee about copy-order and monotonicity of process_vm_readv etc. Also, assuming these guarantees, before_update_seq and after_update_seq might be racy? What if:
|
Sorry, something went wrong.
|
Yes, my variant will not work, I thought more about it - so drop it :) |
Sorry, something went wrong.
|
What if:
Reader:
|
Sorry, something went wrong.
|
Even better, we could pack both counters into a single 32-bit integer — using the lower 16 bits for before and the upper 16 bits for after. This would let us read both values with a single atomic load. What do you think? |
Sorry, something went wrong.
|
@sergey-miryanov The latter requires a full atomic snapshot read of the whole struct. Otherwise there's a race risk? S = { (before=0, after=0), stats }
1. Reader read (before=0, after=0)...
2. Writer incremented `before=1`
3. Writer changed the stats
4. Reader read ...stats
5. Writer incremented `after=1`
Atomicity is not guaranteed: The data transfers performed by process_vm_readv() and
process_vm_writev() are not guaranteed to be atomic in any way.
The former (ie: S = { update_seq, stats }) depends on a guarantee that update_seq is read before stats (copy-order / monotonicity, at least: target[0] is read before target[n > 0]), and I'm not sure if ReadProcessMemory or mach_vm_read_overwrite provide it. Windows does not document it, and macOS Mach VM seems to be sparsely documented. Perhaps we could do _Py_RemoteDebug_BatchedReadRemoteMemory, since iovec provides such guarantee:
That said, target[0] is read before target[n > 0] is likely how it works right now, but that's an implementation detail. I think that even memcpy on Linux/ARM breaks strong target[n] is read before target[n+1] and exactly-once read:
To be honest, I don't think that one additional syscall is that bad. Measured 200KHz is solid (way more than gcmon needs), it's not a hot path, and is trivial to wrap one's head around. In practice (implementation detail), I remember measuring it and small reads are much faster. I also added update_seq at the end to avoid breaking clients. Not sure if this matters but I remember this comment. The thing that I think is worth measuring - and I'm not an expert here - is the probability of collision. What's the maximum observable GC? The time.sleep(0.1) is 100Hz. Can we expect the rate of 100Khz GC collections? Sorry for so much text :) I keep posting essays here but wanted to explain myself, since maybe I'm making a fatal blunder here. Does it make sense to you? |
Sorry, something went wrong.
|
I tested on this script: import gc
import time
ts_start = time.perf_counter_ns()
stats_start = gc.get_stats()
while True:
try:
gc.collect(2)
except KeyboardInterrupt:
break
ts_stop = time.perf_counter_ns()
stats_stop = gc.get_stats()
dt = (ts_stop - ts_start) / 1e9
dc = (stats_stop[2]["collections"] - stats_start[2]["collections"])
dd = (stats_stop[2]["duration"] - stats_start[2]["duration"])
print("collections", dc)
print(f"durations: {dd:.4f}, rate: {dc/dd:.2f}, avg: {dd/dc:.6f}")
print(f"time: {dt:.4f}, rate: {dc/dt:.2f}, avg: {dt/dc:.6f}")
Average duration for gc.collect(2) is about 0.210 ms on my machine (11th Gen Intel(R) Core(TM) i5-11600K @ 3.90GHz, Windows 11) > python .\empty_gc.py collections 17523 durations: 3.7099, rate: 4723.37, avg: 0.000212 time: 3.7173, rate: 4713.90, avg: 0.000212 Average duration of _remote_debugging.get_gc_stats is about 0.560 ms. | PID:IID | Metric | Count | Sum | Avg | P50 | P90 | P95 | P99 | Cov | F | |---------|-------------|-----------|------------------|-------|-------|-------|-------|-------|--------|--------| | Total | GC Pause(0) | 3 | 0.286 | 0.095 | 0.094 | 0.102 | 0.103 | 0.104 | 100.0% | 1.000 | | | GC Pause(2) | 804/19676 | 181.871/4180.404 | 0.226 | 0.225 | 0.236 | 0.241 | 0.294 | 4.1% | 22.985 | |---------|-------------|-----------|------------------|-------|-------|-------|-------|-------|--------|--------| | | Read Time | 268 | 148.984 | 0.556 | 0.546 | 0.576 | 0.622 | 0.790 | | | I don't think we can achieve 100Khz rate for gc.collect :) |
Sorry, something went wrong.
|
@colesbury I know that we've just removed _PySeqLock but I'd love your sanity check here! Thank you. |
Sorry, something went wrong.
I mean one more try if we fail on the first attempt. @maurycy, would you mind adding a single retry pass? |
Sorry, something went wrong.
@sergey-miryanov It introduces some complexity. Do you think it could be avoided with gcmon retrying? |
Sorry, something went wrong.
|
@maurycy It seems cheaper to do the retry inside the call rather than from gcmon. A GCMonitor.get_gc_stats call costs about 30μs on my machine right now. But with the new GC, we can have pauses as short as ~200ns with ~100μs intervals between them. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
|
That was faster than light. Thank you, @pablogsal. 🖤 |
Sorry, something went wrong.
|
Thanks @maurycy for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15. |
Sorry, something went wrong.
|
Sorry, @maurycy and @pablogsal, I could not cleanly backport this to 3.15 due to a conflict. Please backport manually with cherry_picker, see the devguide for more information. cherry_picker 5fecd448bb120378978a37dde65dfce233d88c0d 3.15 |
Sorry, something went wrong.
|
Thanks all! |
Sorry, something went wrong.
|
@maurycy Do you want to take a look at the backport? If not, I can handle it myself. |
Sorry, something went wrong.
|
GH-158829 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
Sorry, something went wrong.
|
GH-158829 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
…nt torn reads (pythonGH-155828) (python#158829)" This reverts commit 0281240.
…58882) * [3.15] gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded (GH-153365) (#158813) gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded (GH-153365) * let me declare single limit * use our new limit in process_frame_chain() * add it in parse_async_frame_chain() * parse_coro_chain() * NEWS * async in the message? * test * no race * process_task_awaited_by * process_task_awaited_by limit test * NEWS * MAX_TASK_WAITER_CHAIN_DEPTH * TASK_WAITER_CHAIN_DEPTH in test * TASK_WAITER_CHAIN_DEPTH 256 * prevent the drift with the comment * better naming, better style * MAX_TASK_WAITER_CHAIN_DEPTH comment * task-waiter iterative bfs walk * iterative coro-walk * nicer news * 1 << 14 * comment * unused read_Py_ssize_t * fix tombstones * simplify * correct msg * better test * news for tombstones * left-over from when testing buggy version * redundant new line (cherry picked from commit e0861c6) Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com> (cherry picked from commit 9e401cf) * [3.15] gh-155811: Add a seqcount to `gc_stats` to prevent torn reads (GH-155828) (#158829) * update_seq * no need for XCHGL, MOVL is enough? * gh-155811: Retry an inconsistent GC snapshot once --------- (cherry picked from commit 5fecd44) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 0281240) * [3.15] gh-151292: `_remote_debugging`: Do not corrupt the binary file when hitting `OverflowError` (GH-152892) (#158830) gh-151292: `_remote_debugging`: Do not corrupt the binary file when hitting `OverflowError` (GH-152892) * the kolektor * test * better test * news * =Q, move const to the base, not self.running * gh-151292: Track binary writer finalization state --------- (cherry picked from commit f839c06) Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com> Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com> (cherry picked from commit 8a7c23f) * [3.15] gh-158583: Fix uninitialized memory read in bytes.fromhex() (GH-158584) (#158691) gh-158583: Fix uninitialized memory read in bytes.fromhex() (GH-158584) (cherry picked from commit 9d22a53) Co-authored-by: Victor Stinner <vstinner@python.org> (cherry picked from commit 4f7af46) * [3.15] gh-154194: Degrade frames in Tachyon instead of failing the sample (GH-154195) (#158831) * gh-154194: Degrade frames in Tachyon instead of failing the sample (#154195) * degrade gracefully * news * better NEWS wording * do not raise on MAX_REMOTE_STR_READ * bye MAX_REMOTE_STR_READ * fix -m asyncio ps|pstree * test truncation and linetable sentinel * simpler * simpler * redundant now * respect #157790 in the news --------- Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com> (cherry picked from commit 7d25916) * Preserve the stable ABI when creating fallback frame names --------- Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com> (cherry picked from commit c27f494) * [3.15] Add MSan to CI (GH-158625) (#158832) Run a job that the test suite with MSan to the CI (#158625) * Run the test suite with MSan in CI * Additional fixes * Add `_Py_MSAN_UNPOISON_STRING` * Apply Victor's suggestions * Apply Victor's suggestions --------- (cherry picked from commit b93fb19) Co-authored-by: Stan Ulbrych <stan@python.org> Co-authored-by: Victor Stinner <victor.stinner@gmail.com> (cherry picked from commit 1bc78de) * [3.15] gh-156810: Write the profiler's collapsed-stack export as UTF-8 (GH-156811) (#156814) gh-156810: Write the profiler's collapsed-stack export as UTF-8 (GH-156811) (cherry picked from commit c3706f4) Co-authored-by: tonghuaroot (童话) <tonghuaroot@gmail.com> (cherry picked from commit fce28da) * [3.15] gh-158552: Wait for Windows threads to suspend before blocking sampling (GH-158802) (#158845) gh-158552: Wait for Windows threads to suspend before blocking sampling (GH-158802) * gh-158552: Wait for Windows threads to suspend before blocking sampling * Use a named Windows thread enumeration status constant (cherry picked from commit 1643525) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com> (cherry picked from commit 31288da) * [3.15] gh-152721: Fix quadratic RLE replay time in the profiling binary reader (GH-152722) (#158850) Backport of GH-152722. Co-authored-by: tonghuaroot <tonghuaroot@gmail.com> (cherry picked from commit ebaca2a) * [3.15] gh-156545: Fix flamegraph export RecursionError on deeply recursive programs (GH-156546) (#158851) Backport of GH-156546. Co-authored-by: tonghuaroot (童话) <tonghuaroot@gmail.com> (cherry picked from commit 0ac7217) * [3.15] gh-158540: Add the profiled script's directory to sys.path (GH-158548) (#158844) gh-158540: Add the profiled script's directory to sys.path (GH-158548) * gh-158540: Add the profiled script's directory to sys.path When a script is profiled with ``python -m profiling.sampling run script.py`` from another directory, the script cannot import modules placed next to it, because ``_sync_coordinator._execute_script()`` executes it with the working directory (added by ``_setup_environment()`` for the module case) as ``sys.path[0]`` instead of the script's own directory. Make the script's directory importable in ``_execute_script()``, matching the behavior of ``python script.py``. Add a regression test that runs the coordinator on a script importing a sibling module. * Update _sync_coordinator.py Comment simplified * gh-158540: Resolve symlinks when adding the script directory to sys.path ``python script.py`` resolves symlinks when computing ``sys.path[0]``, so a script reached through a symlink (``link.py -> sub/where.py``) imports modules from the real script's directory. Apply ``os.path.realpath()`` before taking the directory name, and make sure the result is placed at the front of ``sys.path`` even if it was already listed. Add a regression test for a symlinked script. --------- (cherry picked from commit 3f02aab) Co-authored-by: he_tao <53343436+hetaozdh@users.noreply.github.com> Co-authored-by: Eduardo Villalpando Mello <eduardo.villalpando.mello@gmail.com> (cherry picked from commit f52d831) * [3.15] gh-153838: Skip non-regular source files in the heatmap exporter (GH-153839) (#158853) Backport of GH-153839. Co-authored-by: tonghuaroot <tonghuaroot@gmail.com> (cherry picked from commit 48998df) * [3.15] gh-158539: Fix exception mode missing handlers in generators/coroutines (GH-158581) (#158852) * [3.15] gh-158539: Fix exception mode missing handlers in generators/coroutines (GH-158581) Backport of GH-158581. Co-authored-by: LucasZhou <donghao.zhou@outlook.com> * [3.15] gh-158539: Use portable static assertion messages * [3.15] gh-158539: Keep layout assertions with debug-offset validation * [3.15] gh-158539: Use the platform guard for in-process inspection tests --------- Co-authored-by: LucasZhou <donghao.zhou@outlook.com> (cherry picked from commit d625ecb) * [3.15] gh-158522: Fix truncated stack for a task whose coroutine recurses (GH-158526) (#158870) Co-authored-by: Timofei Ivankov <128279579+deadlovelll@users.noreply.github.com> (cherry picked from commit a904b39) * gh-156545, gh-158539: Fix deep flamegraph export on small C stacks and macOS runtime lookup (#158874) (cherry picked from commit 114de19) Include the C-stack test helper from main, introduced by ce5ae29, which the regression test requires but 3.15 does not yet provide. --------- Co-authored-by: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com> Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Stan Ulbrych <stan@python.org> Co-authored-by: Victor Stinner <victor.stinner@gmail.com> Co-authored-by: tonghuaroot (童话) <tonghuaroot@gmail.com> Co-authored-by: he_tao <53343436+hetaozdh@users.noreply.github.com> Co-authored-by: Eduardo Villalpando Mello <eduardo.villalpando.mello@gmail.com> Co-authored-by: LucasZhou <donghao.zhou@outlook.com> Co-authored-by: Timofei Ivankov <128279579+deadlovelll@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
See #155811 for the context.
tl;dr get_gc_stats reads the stats without pausing the target (by design), so it can return torn data.
The PR adds a seqcount to gc_stats atomically increased by the writer around the stats update on every collection, where odd means it's in progress.
gcmon is the main consumer.
There's no retry, as per #155811 (comment). It's not needed. gc.collect() does not happen that often:
For:
I'm not sure what's optimal non-hacky test. We haven't had one in #152448. To be honest, it's implicitly tested by the current happy-path tests in test_gc_stats.