Skip to content

Conversation

@Eiji7
Copy link
Contributor

@Eiji7 Eiji7 commented Jun 9, 2025

Fixes: #14564

This fix implements Erlang/OTP version check. The alternative solution could be:

# extract results
[_warmup, _profile_results, _columns, _total, first, second, _profile_done, ""] =
  String.split(result, ~r/\n+/)

# split columns
list = Enum.map([first, second], &String.split(&1, ~r/\s+/))

sort_func = fn result_colls ->
  result_colls
  |> List.last()
  |> String.to_float()
end

list == Enum.sort_by(list, sort_func)

The alternative solution additionally does not relies on 3.\d{2} value pattern of PER CALL column.

Fixes the sort issue that happen on `Erlang 28.0` where both columns have equal value.
I've used `[^\n]+3\.\d{2}\n` to match `3.00` value of `PER CALL` column at the end of the line.
@josevalim
Copy link
Member

I like your alternative solution, relying less on regexes, but we should also check on the calls. Perhaps we build {per_call, mfa} tuples and assert on the names after sorting?

Improvements:
* Does not require `Erlang/OTP` version check
* Independent of `JIT` optimisations and architecture
* Properly validates sorted results
* Asserts mfa string as column value
@Eiji7
Copy link
Contributor Author

Eiji7 commented Jun 9, 2025

@josevalim I have send an alternative version. Unfortunately we cannot guarantee the order of mfa strings since they may not be sorted if both PER CALL column values are same (in my case 3.00). No problem to change it with a different expression than @expr that works on every Erlang/OTP release.

Here are my results for this task:

$ mix profile.tprof --type memory --sort per_call -e "Enum.each(1..5, &String.Chars.Integer.to_string/1)"
Warmup...


Profile results of #PID<0.155.0>
#                           CALLS      % WORDS PER CALL
Total                           6 100.00    18     3.00
Enum.each/2                     1  16.67     3     3.00
:erlang.integer_to_binary/1     5  83.33    15     3.00

Profile done over 2 matching functions

@Eiji7
Copy link
Contributor Author

Eiji7 commented Jun 9, 2025

Oh, wait - I have just realised something something … This is the only test that uses --sort argument with per_call value. We can just sort by calls and then verify the sorted result. For some reason my mind was fixed on not changing this task call at all. 😅

@Eiji7
Copy link
Contributor Author

Eiji7 commented Jun 9, 2025

@josevalim I have reverted back to old regex solution, because it's shorter, but I kept the change in --sort argument value, so now we are sure that they are always properly sorted regardless of JIT, architecture or Erlang/OTP version. This resulted in swapping the order of expected lines, so now Enum.each/2 is expected first. I have also added patterns to verify the calls column value (1 and 5 surrounded by whitespaces and right after mfa).

Eiji7 added 3 commits June 9, 2025 22:57
Signed-off-by: Tomasz Marek Sulima <[email protected]>
Added assertion for the result sort order.

Signed-off-by: Tomasz Marek Sulima <[email protected]>
…er needed as we sort by calls column

Signed-off-by: Tomasz Marek Sulima <[email protected]>
@josevalim josevalim merged commit 7ef6905 into elixir-lang:main Jun 9, 2025
12 of 13 checks passed
@Eiji7 Eiji7 deleted the tprof-memory-sort-test branch June 10, 2025 00:24
ggVGc pushed a commit to ggVGc/elixir-verbatim that referenced this pull request Sep 12, 2025
ggVGc pushed a commit to ggVGc/elixir-verbatim that referenced this pull request Sep 19, 2025
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.

Same "PER CALL" column value for profile.tprof mix task on Erlang/OTP 28 when testing mix application

2 participants