Move state machine from profiler into async handler#1352
Open
ryanzhang22 wants to merge 2 commits intopytorch:mainfrom
Open
Move state machine from profiler into async handler#1352ryanzhang22 wants to merge 2 commits intopytorch:mainfrom
ryanzhang22 wants to merge 2 commits intopytorch:mainfrom
Conversation
…h#1269) Summary: Part 1 of a refactor to improve safety and readability around sync/async profiling. This is a straightforward (no-op) split that creates Sync/AsyncActivityProfilerHandler, which contains the methods which belonged to the ActivityProfilerController but are only used for one type of tracing. The original methods in the controller become thin wrappers that forward the request to the appropriate handler. Differential Revision: D92550422
Summary: Part 2 of a refactor to improve safety and readability around sync/async profiling. Sync profiling currently only uses the profiler state machine to check whether `isActive()` is true. However that doesn't tell us if sync or async is the one that actually owns the profiler at that point, which can lead to edge cases when we make decisions about whether or not to proceed based on the profiler state. See the example below. Since the state machine behavior is only relevant for async tracing, we move the related functions from the `GenericActivityProfiler` into the Async handler. The `GenericActivityProfiler` just becomes a set of functions used to drive profiling, without needing to worry about transitions. Each handler owns their own `isActive()` state, and `ActivityProfilerController` makes decisions about how to handle scheduling between sync and async. In particular, we add a `cancel()` function both handlers so the controller can make sure both handlers are in a good state before preempting. For the async trace, `cancel()` will also clear the async config and move the state back to `WaitForRequest`. This change makes ownership explicit, which should hopefully make reasoning about preemptions from the controller perspective easier. Differential Revision: D93475197
|
@ryanzhang22 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D93475197. |
ryanzhang22
added a commit
to ryanzhang22/kineto
that referenced
this pull request
Apr 9, 2026
Summary: Part 2 of a refactor to improve safety and readability around sync/async profiling. Sync profiling currently only uses the profiler state machine to check whether `isActive()` is true. However that doesn't tell us if sync or async is the one that actually owns the profiler at that point, which can lead to edge cases when we make decisions about whether or not to proceed based on the profiler state. See the example below. Since the state machine behavior is only relevant for async tracing, we move the related functions from the `GenericActivityProfiler` into the Async handler. The `GenericActivityProfiler` just becomes a set of functions used to drive profiling, without needing to worry about transitions. Each handler owns their own `isActive()` state, and `ActivityProfilerController` makes decisions about how to handle scheduling between sync and async. In particular, we add a `cancel()` function both handlers so the controller can make sure both handlers are in a good state before preempting. For the async trace, `cancel()` will also clear the async config and move the state back to `WaitForRequest`. This change makes ownership explicit, which should hopefully make reasoning about preemptions from the controller perspective easier. Differential Revision: D93475197
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:
Part 2 of a refactor to improve safety and readability around sync/async profiling.
Sync profiling currently only uses the profiler state machine to check whether
isActive()is true. However that doesn't tell us if sync or async is the one that actually owns the profiler at that point, which can lead to edge cases when we make decisions about whether or not to proceed based on the profiler state. See the example below.Since the state machine behavior is only relevant for async tracing, we move the related functions from the
GenericActivityProfilerinto the Async handler. TheGenericActivityProfilerjust becomes a set of functions used to drive profiling, without needing to worry about transitions. Each handler owns their ownisActive()state, andActivityProfilerControllermakes decisions about how to handle scheduling between sync and async. In particular, we add acancel()function both handlers so the controller can make sure both handlers are in a good state before preempting. For the async trace,cancel()will also clear the async config and move the state back toWaitForRequest.This change makes ownership explicit, which should hopefully make reasoning about preemptions from the controller perspective easier.
Differential Revision: D93475197