Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Debugger] Sorting op-time breakdown for quicker analysis. #4352

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

anijain2305
Copy link
Contributor

Profiler output is not sorted by time currently. So, a developer has to eyeball and find out the expensive ops. This PR sorts and add total time in the log for quicker analysis.

@tqchen @soiferj @yzhliu @icemelon9

Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@soiferj soiferj left a comment

Choose a reason for hiding this comment

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

Awesome, thanks lot for the change. LGTM.

@zhiics
Copy link
Member

zhiics commented Nov 16, 2019

BTW, if I remember correctly, the correct output is sorted by the occurrence of the ops. If so, should we make this configurable, i.e. passing a sort_by_time=False? For the VM profiler, I had a local change to do this, but I forgot to send the PR.

@anijain2305
Copy link
Contributor Author

@zhiics Makes sense. Added a flag.

@zhiics zhiics merged commit 560280d into apache:master Nov 16, 2019
@zhiics
Copy link
Member

zhiics commented Nov 16, 2019

Thanks @anijain2305 @icemelon9 @soiferj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants