make CollectTrace for profiling by iteration async #966
Conversation
| if (!stopByIterThread) { | ||
| std::lock_guard<std::mutex> guard(mutex_); | ||
| if (!stopByIterThread) { | ||
| stopByIterThread = new std::thread([collection_done, this, now](){ |
| std::lock_guard<std::mutex> guard(mutex_); | ||
| if (stopByIterThread) { | ||
| stopByIterThread->join(); | ||
| delete stopByIterThread; |
There was a problem hiding this comment.
with unique ptr this woudl be stopByIterThread.reset()
|
Thank you for those helpful tips, I'll improve this pr as comments advised. |
staugust
force-pushed
the
main2
branch
2 times, most recently
from
2bfb538 to
1af59c6
Compare
|
@briancoutinho It seems this pr is ignored after last commit. I've rebased master branch, please take some time to re-review this pr, thank you. |
|
Hi, sorry am out for 1 1/2 week, will take a look once I'm back. you can
also ask @
sraikund16 for a review
Thx
…On Fri, Aug 30, 2024 at 12:38 AM Augusto Yao ***@***.***> wrote:
@briancoutinho <https://github.com/briancoutinho> It seems this pr is
ignored after last commit. I've rebased master branch, please take some
time to re-review this pr, thank you.
—
Reply to this email directly, view it on GitHub
<#966 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABUZ7ZCZM47WFP4YYUGFAELZUAOOLAVCNFSM6AAAAABLQGOI7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRQGM2TMMRUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@briancoutinho it's ok. @sraikund16 would you like to review this pr? |
|
@briancoutinho @sraikund16 Is there any update? |
|
@staugust sorry I didn't notice this. Let me take a look later today. Thanks for your patience! |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
| #if defined(HAS_CUPTI) || defined(HAS_ROCTRACER) | ||
| if (cupti_.stopCollection) { | ||
| ecs_.cupti_stopped_early = cupti_.stopCollection; | ||
| LOG(ERROR) |
There was a problem hiding this comment.
Any reason why we removed this LOG? Doesn't appear in the new function
There was a problem hiding this comment.
Those lines are added to the main branch after this pr. I'd rebase from master branch, and update this pr.
There was a problem hiding this comment.
Oh interesting, that diff was supposed to be only formatting hmm. I'll add it myself, thanks!
There was a problem hiding this comment.
main...staugust:kineto:main2_new , I have reorganized the code on a new branch based on Kineto's main branch, hoping this will help.
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| // currentIter > 0 means this is an iteration-based collection, | ||
| // triggered by pytorch main thread, it should be executed in another | ||
| // thread in case pytorch main thread is blocked | ||
| if (currentIter > 0) { |
There was a problem hiding this comment.
I think iteration based is >= 0
There was a problem hiding this comment.
Good catch! I think this didn't get caught because all it would do is make a single step trace still synchronous but we should make it async for all traces. Thanks!
I don't see any functional tests with on-demand so its hard to test it any other way than the unit tests that I changed in this PR |
|
@staugust has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@sraikund16 merged this pull request in 7a2a167. |
This fix issue #953. Makes
libkineto::api().client()->stop()andstopTraceInternalrun inprofilerThread_so that, the training process will not be blocked.