Disable profiling when getGraphExecutorOptimize is unset
#46479
Conversation
facebook-github-bot
added
the
oncall: jit
💊 CI failures summary and remediationsAs of commit 117d097 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 4 times. |
Krovatkin
force-pushed
the
krovatkin/no_profile
branch
from
4e2ba91
to
117d097
Compare
| @@ -736,7 +736,13 @@ c10::intrusive_ptr<Future> GraphExecutor::runAsync(Stack& stack) { | |||
| } | |||
|
|
|||
| size_t GraphExecutor::getDefaultNumBailOuts() { | |||
| return getProfilingMode() ? getBailoutDepth().load() : 0; | |||
There was a problem hiding this comment.
Could we consider not even querying GraphExecutor if ! getGraphExecutorOptimize() ? This feels like the wrong place to add the check to me
There was a problem hiding this comment.
Or dispatching to the SimpleExecutor if !getGraphExecutorOptimize()
There was a problem hiding this comment.
From looking around maybe there's not a great place to do this... LGTM
There was a problem hiding this comment.
That was my impression as well tbh
💊 CI failures summary and remediationsAs of commit 263807b (more details on the Dr. CI page):
3 failures not recognized by patterns:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 20 times. |
There was a problem hiding this comment.
@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -466,6 +447,15 @@ ExecutionPlan ProfilingGraphExecutorImpl::getPlanFor( | |||
| return *optimized_plan_; | |||
| } | |||
|
|
|||
| // no opt mode | |||
| if (!getGraphExecutorOptimize()) { | |||
| auto copy = graph->copy(); | |||
There was a problem hiding this comment.
This has the opposite problem now - if you reenable optimization after running it with disabling, you will never optimize.
There was a problem hiding this comment.
err i'm kinda thinking it should be okay. If the user changes the number of profiled runs midway, we won't update it for models in flight either.
There was a problem hiding this comment.
Alternatively, we could try disregarding getGraphExecutorOptimize if we already started profiling runs but this behaviour seems complicated conceptually
| @@ -96,12 +96,7 @@ void runNooptPassPipeline(std::shared_ptr<Graph>& graph) { | |||
| LowerGradOf(*graph); | |||
| GRAPH_DEBUG("After LowerGradOf, before RemoveExpands\n", *graph); | |||
| RemoveExpands(graph); | |||
| GRAPH_DEBUG("After RemoveExpands, before CanonicalizeOps\n", *graph); | |||
There was a problem hiding this comment.
Why was this changed as part of the PR?
There was a problem hiding this comment.
@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@Krovatkin merged this pull request in c3466da. |
getGraphExecutorOptimizemandates we don't do any optimizations beyond what's required to run graphs. In this scenario, we don't want to do any profiling as profiling information will not be used.