-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Implement mix profile.tprof #13605
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
Implement mix profile.tprof #13605
Conversation
scenario or focus on the main modules or processes. Another alternative is to use | ||
`Mix.Tasks.Profile.Cprof` that uses [`:cprof`](`:cprof`) and has a low performance degradation effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scenario or focus on the main modules or processes. Another alternative is to use | |
`Mix.Tasks.Profile.Cprof` that uses [`:cprof`](`:cprof`) and has a low performance degradation effect. | |
scenario or focus on the main modules or processes. |
This is pretty much the same as cprof (which will be deprecated in the future anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry as I mentioned in the description I should still rewrite the bulk of the moduledoc, it's still mostly eprof
's 😅
fun.() | ||
end | ||
|
||
tprof_module().start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to start and stop it, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right we don't 27e56dc
end | ||
|
||
defp get_filter_value!(type, time, _memory) when is_integer(time) and type != :time do | ||
Mix.raise("Incompatible use of time option with type `#{type}`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use backticks in Elixir error messages. Perhaps #{inspect(type)} is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds much better indeed. I needed some kind of visual distinction to keep readability but atom notation feels much more natural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautifully done!
OK I think we should now be good. Updated the moduledoc adf24b6. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole doc looked good to me before and it still looks good to me now :D
Close #13581
Sharing my draft for feedback as I think the implementation is now done - I still need to do the moduledocs.
I used
profile.eprof
as a base, since these are actually quite close.Reviewing this commit Actually implement tprof might help the review since it highlights the differences.
Sample outputs