Drive the readout on a cadence, and flush the tail - #3977
Closed
shr wants to merge 2 commits into
Closed
Conversation
Summary: Follow-ups to the two base diffs, which had already been shipped when these came back from review. Four fixes and one test, all small and independent. - `collStatsAllocDeviceBlock` bounded `numThresholds` against its array capacity but not `sizeClasses.n`. `sizeClassOf` walks `i < sc.n` over a fixed-capacity `edges[]`, so an oversized `n` reads past it on the host path that resolves a window's size-class labels. Checked in the same expression as its sibling. - The span helpers are all behind `__CUDA_ARCH__ >= 900`, so on a pre-Hopper device they compile to no-ops and every span assertion in the GPU test holds vacuously -- a green run that tested nothing. The fixture now skips on compute capability, not just on the presence of a device. - `-D__HIP_PLATFORM_AMD__` was set only in `preprocessor_flags`, which covers this target's own sources. `CollStatsDeviceBlock.h` and `CollStatsSpan.cuh` are exported and pull in `cuda_runtime.h`, so dependents compiled the same headers without the define; it is now propagated too, matching the pattern `nccl_build_config.bzl` uses for the same reason. - `collStatsReadWindow`'s failure path synchronized the reader stream twice when the flip had happened and once when it had not, which was backwards -- the unconditional drain exists precisely for the case where the copies are already in flight. One drain now covers both reasons to need it. The test is for the sentinel guard in the span finalizer, the one invariant in that file that had none. A finalize with no entry leaves `start` at `UINT64_MAX`, and `end - UINT64_MAX` wraps to `end + 1`: a sub-microsecond duration that lands in the underflow bucket and reads as a real, very fast collective. Dropping it is what makes a fence or pre-reset bug surface as a missing observation rather than a believable wrong one. Differential Revision: D118514769
Summary: Policy on top of the window readout mechanism: when to take a window, and how to get the last one. `CollStatsReadoutDriver` ticks once per instrumented collective from the enqueue thread, so the epoch flip needs no boundary lock -- no collective can enqueue mid-flip on that thread. Every `cadence` ticks it harvests the previous window if its copy event has completed (a non-blocking query) and issues the next on a dedicated reader stream. Neither step is meant to synchronize the training thread, and neither does on a devgpu; under remote execution one tick still measures ~2005ms with the driver enabled and the pinned staging allocated, so something in the issue path serializes with the instrumented stream on some hardware. Tracked as T282705070 and not asserted anywhere until it is identified. - Teardown exports the tail. A cadence boundary is only reached every N collectives, so up to `cadence - 1` collectives per communicator used to be freed unread. `flush()` cannot recover them -- it only harvests a window already issued -- so `flushFinal()` flushes and then issues one extra window for the remainder. Measured at cadence 32 across 8 ranks: 245 collectives per rank now report as seven windows of 32 plus a tail of 21, with uniform counts across all five size classes; previously the last two classes reported 48 and 40. - That extra window is ungated and follows a device synchronize, because a destructor cannot assume a stream it does not own still exists. The sync covers only the calling thread's current device, so the driver records its device at construction and selects it for the wait. - Exported and dropped partition the windows. A harvest that finds the copy in flight is a deferral, counted separately as a retry: the window still lands in exactly one of the two later, and charging it as a drop would double-count it and, since the collective counter is not cleared on a skipped issue, inflate once per collective for the length of a stall. - Each window carries wall-clock bounds stamped at the flip, not at the harvest, so consecutive windows abut. Reviewed By: rmahidhar Differential Revision: D113661123
meta-cla
Bot
added
the
CLA Signed
Contributor
|
@shr has exported this pull request. If you are a Meta employee, you can view the originating Diff in D113661123. |
Contributor
|
This pull request has been merged in 642749c. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary:
Policy on top of the window readout mechanism: when to take a window, and how to get the last one.
CollStatsReadoutDriverticks once per instrumented collective from the enqueue thread, so the epoch flip needs no boundary lock -- no collective can enqueue mid-flip on that thread. Everycadenceticks it harvests the previous window if its copy event has completed (a non-blocking query) and issues the next on a dedicated reader stream. Neither step is meant to synchronize the training thread, and neither does on a devgpu; under remote execution one tick still measures ~2005ms with the driver enabled and the pinned staging allocated, so something in the issue path serializes with the instrumented stream on some hardware. Tracked as T282705070 and not asserted anywhere until it is identified.cadence - 1collectives per communicator used to be freed unread.flush()cannot recover them -- it only harvests a window already issued -- soflushFinal()flushes and then issues one extra window for the remainder. Measured at cadence 32 across 8 ranks: 245 collectives per rank now report as seven windows of 32 plus a tail of 21, with uniform counts across all five size classes; previously the last two classes reported 48 and 40.Reviewed By: rmahidhar
Differential Revision: D113661123