Skip to content

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

Merged
merged 7 commits into from
May 27, 2024
Merged

Implement mix profile.tprof #13605

merged 7 commits into from
May 27, 2024

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented May 27, 2024

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
mix profile.tprof -e "Enum.each(1..5, &String.Chars.Integer.to_string/1)"
Warmup...


Profile results of #PID<0.107.0>
#                                               CALLS      % TIME µS/CALL
Total                                              20 100.00    2    0.10
String.Chars.Integer.to_string/1                    5   0.00    0    0.00
anonymous fn/0 in :elixir_compiler_1.__FILE__/1     1   0.00    0    0.00
Enum.each/2                                         1   0.00    0    0.00
Enum.reduce_range/5                                 3   0.00    0    0.00
:erlang.integer_to_binary/1                         5  50.00    1    0.20
anonymous fn/3 in Enum.each/2                       5  50.00    1    0.20

Profile done over 6 matching functions
mix profile.tprof -e "Enum.each(1..5, &String.Chars.Integer.to_string/1)" --type memory
Warmup...


Profile results of #PID<0.107.0>
#                           CALLS      % WORDS PER CALL
Total                           6 100.00    19     3.17
Enum.each/2                     1  21.05     4     4.00
:erlang.integer_to_binary/1     5  78.95    15     3.00

Profile done over 2 matching functions
mix profile.tprof -e "Enum.each(1..5, &String.Chars.Integer.to_string/1)" --type calls
Warmup...


Profile results over all processes
#                                               CALLS      %
Total                                              20 100.00
anonymous fn/0 in :elixir_compiler_1.__FILE__/1     1   5.00
Enum.each/2                                         1   5.00
Enum.reduce_range/5                                 3  15.00
:erlang.integer_to_binary/1                         5  25.00
String.Chars.Integer.to_string/1                    5  25.00
anonymous fn/3 in Enum.each/2                       5  25.00

Profile done over 6 matching functions
mix profile.tprof -e "Enum.each(1..5, &String.Chars.Integer.to_string/1)" --type calls --sort time
Warmup...

** (Mix) Incompatible sort option `time` with type `calls`

@sabiwara sabiwara marked this pull request as draft May 27, 2024 00:49
Comment on lines 109 to 110
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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).

Copy link
Contributor Author

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()
Copy link
Member

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?

Copy link
Contributor Author

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}`")
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Beautifully done!

@sabiwara
Copy link
Contributor Author

sabiwara commented May 27, 2024

OK I think we should now be good. Updated the moduledoc adf24b6.
Reused some of the wording from the original doc:
Screenshot 2024-05-27 at 21 38 02

@sabiwara sabiwara marked this pull request as ready for review May 27, 2024 12:39
Copy link
Member

@josevalim josevalim left a 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

@sabiwara sabiwara merged commit 76fec7e into elixir-lang:main May 27, 2024
9 checks passed
@sabiwara sabiwara deleted the tprof branch May 27, 2024 12:51
josevalim pushed a commit that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add mix profile.tprof
2 participants