Skip to content

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