Skip to content

Fix profiling with C API - #710

Merged
snnn merged 2 commits into
microsoft:masterfrom
mika-fischer:fix-profiling-with-c-api
Apr 4, 2019
Merged

Fix profiling with C API#710
snnn merged 2 commits into
microsoft:masterfrom
mika-fischer:fix-profiling-with-c-api

Conversation

@mika-fischer

Copy link
Copy Markdown
Contributor

Currently, when using OrtEnableProfiling to enable profiling using the C API,
the profile output file is created but is always empty.

The reason is that InferenceSession::EndProfiling() needs to be called to
write the profiling data to the output file.

However there's currently no way to call this function via the C API.

This adds a call to EndProfiling() to the descructor of the session if
profiling is enabled in the session options.

@mika-fischer
mika-fischer requested a review from a team as a code owner March 26, 2019 18:15
Currently, when using OrtEnableProfiling to enable profiling using the C API,
the profile output file is created but is always empty.

The reason is that InferenceSession::EndProfiling() needs to be called to
write the profiling data to the output file.

However there's currently no way to call this function via the C API.

This adds a call to EndProfiling() to the descructor of the session if
profiling is enabled in the session options.
@mika-fischer
mika-fischer force-pushed the fix-profiling-with-c-api branch from a883739 to 3280697 Compare March 26, 2019 18:21
@raymondxyang

Copy link
Copy Markdown

/AzurePipelines run

@azure-pipelines

Copy link
Copy Markdown

Azure Pipelines successfully started running 7 pipeline(s).

@snnn

snnn commented Mar 26, 2019

Copy link
Copy Markdown
Contributor

Is it better to add an end profiling function in the C API?

@mika-fischer

Copy link
Copy Markdown
Contributor Author

@snnn There is also no way to call StartProfiling from the C API. It's always done implicitly when creating the session. So I would find it strange to support only implicit start but explicit end. Either both should be implicit or both should be explicit.

I also think this implicit EndProfiling covers the vast majority of use-cases. And allowing explicit control of the profiling state would necessitate larger changes in the C API (removing the profiling option and introducing new functions).

So all in all I think this is fine as is.

@snnn

snnn commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

Destructor shouldn't do any file io, including closing file.

@mika-fischer

Copy link
Copy Markdown
Contributor Author
  1. Why is that?
  2. What alternative do you suggest? Do the call in OrtReleaseSession? I fail to see how that's any better.

@snnn
snnn requested a review from RyanUnderhill March 27, 2019 23:58
@snnn

snnn commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

Because destructor can't report error.

@mika-fischer

Copy link
Copy Markdown
Contributor Author

True, but in this case an error is unlikely and logging would suffice IMO. It would be good to catch exceptions in the desrtuctor though. I'll update the pull request.

@mika-fischer

Copy link
Copy Markdown
Contributor Author

Alright, I catch the (unlikely) exception now and log it. I also added a TODO that this is not optimal and maybe the C API should be refactored, so that profiling can be started and stopped explicitly.

But this pull request takes profiling via the C API from completetly broken to working as expected in most cases.

@RyanUnderhill RyanUnderhill left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is a good compromise for now.

@snnn
snnn merged commit 2674d9b into microsoft:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants