Print the detailed type on heap snapshot

[gbaraldi]

This adds even more details to the node. It might even be too much.
It would be cool to be able to still group them, but keep the extra details.
Fixes #47502

[IanButterworth]

I think this is the right approach. Grouping can be handled by the viewer tooling.

Though, what impact does this have on collection time?

[gbaraldi]

I haven't checked, if it's too much I can just yank the part of the code that specifically does the datatype printing. Though since this function already used to fallback to static_show I don't expect much of a difference.

[IanButterworth]

master

$ ./julia --startup-file=no
...
julia> @time Profile.take_heap_snapshot()
  2.125155 seconds (37 allocations: 3.156 KiB, 45.84% gc time)
"/home/ian/Documents/GitHub/julia2/julia/114986_40155041450905.heapsnapshot"

julia> @time Profile.take_heap_snapshot()
  2.156030 seconds (37 allocations: 3.156 KiB, 46.53% gc time)
"/home/ian/Documents/GitHub/julia2/julia/114986_40159572227999.heapsnapshot"

julia> @time Profile.take_heap_snapshot()
  2.110962 seconds (37 allocations: 3.156 KiB, 45.86% gc time)
"/home/ian/Documents/GitHub/julia2/julia/114986_40163564691475.heapsnapshot"

this PR

$ ./julia --startup-file=no
...
julia> @time Profile.take_heap_snapshot()
  2.907817 seconds (37 allocations: 3.117 KiB, 56.23% gc time)
"/home/ian/Documents/GitHub/julia/109127_37985089277755.heapsnapshot"

julia> @time Profile.take_heap_snapshot()
  2.911053 seconds (37 allocations: 3.117 KiB, 56.54% gc time)
"/home/ian/Documents/GitHub/julia/109127_37989526095958.heapsnapshot"

julia> @time Profile.take_heap_snapshot()
  2.872300 seconds (37 allocations: 3.117 KiB, 56.00% gc time)
"/home/ian/Documents/GitHub/julia/109127_37993550442678.heapsnapshot"

[gbaraldi]

That's quite a bit. I guess yanking the code might be worth it then. Though it's also adding a lot more information so I guess it also makes sense that it takes longer.

[vtjnash]

Seems okay. It is not expected to be fast

[NHDaly]

Do you think we should just use static show for everything in this function, instead of having all these custom if-statements for printing??

[gbaraldi]

static_show is a bit slow, and for some of these objects there isn't that much to be gained. Also static_show might be too specific in some cases, which might actually hinder usability

[NHDaly]

Excelllllllent! :) Thanks @gbaraldi!

[gbaraldi]

We might want to add more specific objects to record_node_to_gc_snapshot so that they are grouped by type not also by name.

[NHDaly]

🤔 what was in the node_type param here?

[gbaraldi]

It got passed into the function. It's an argument to it, I still need to remove it :)

[NHDaly]

so what do the else-case nodes get for node_type? "object", still?

[gbaraldi]

Yes, though i want to catch all the cases if possible.

[gbaraldi]

Do we want to keep "normal" julia objects as "objects" or do we want to say they are for example jl_value_t so they can be grouped up? Or more generally, what level of granularity do we want. I think types, symbols, strings, arrays and the rest might be fine. Since we are static_showing most things now they aren't as grouped as before, so adding ways of grouping them up might be interesting.

[NHDaly]

yeah, interesting question. i'm not sure.

I think i'd probably say that keeping normal julia objects as "object" is the right call. For users who aren't using this tool to profile Julia itself, but are rather using this tool to profile their applications, i think it would be annoying to group all objects into one jl_value_t bucket, since that's where all of their valuable data is.

So i think keeping them as objects is right. 👍

[NHDaly]

Are you waiting for another review, @gbaraldi? Do you need more input anywhere?

[gbaraldi]

The changes are fine, I just need to do some cleanup and it should be good to merge

[vtjnash]

If performance is too much of an issue, we could attempt replacing find_or_create_string_id with something that instead maps a jl_value_t to a more-lazy-printed string, so that we only make the string once for each type

[IanButterworth]

@gbaraldi were you planning to try Jameson's idea out, or should we merge this?

[gbaraldi]

I don't think there is much benefit into memoizing the strings here, we have repeated types, but they are also the simpler ones to print.