-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Print the detailed type on heap snapshot #47503
Conversation
I think this is the right approach. Grouping can be handled by the viewer tooling. Though, what impact does this have on collection time? |
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. |
master
this PR
|
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. |
Seems okay. It is not expected to be fast |
src/gc-heap-snapshot.cpp
Outdated
jl_static_show(str, a); | ||
name = StringRef((const char*)str_.buf, str_.size); | ||
self_size = sizeof(jl_datatype_t); |
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.
Do you think we should just use static show for everything in this function, instead of having all these custom if-statements for printing??
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.
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
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.
Excelllllllent! :) Thanks @gbaraldi!
We might want to add more specific objects to |
@@ -446,7 +453,6 @@ static inline void _record_gc_edge(const char *node_type, const char *edge_type, | |||
auto to_node_idx = record_node_to_gc_snapshot(b); | |||
|
|||
auto &from_node = g_snapshot->nodes[from_node_idx]; | |||
from_node.type = g_snapshot->node_types.find_or_create_string_id(node_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.
🤔 what was in the node_type param here?
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.
It got passed into the function. It's an argument to it, I still need to remove it :)
? sizeof(jl_array_t) | ||
: (size_t)jl_datatype_size(type); | ||
|
||
self_size = (size_t)jl_datatype_size(type); | ||
// print full type into ios buffer and get StringRef to it. | ||
// The ios is cleaned up below. | ||
ios_need_close = 1; |
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.
so what do the else-case nodes get for node_type
? "object", still?
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, though i want to catch all the cases if possible.
Do we want to keep "normal" julia objects as "objects" or do we want to say they are for example |
yeah, interesting question. i'm not sure. I think i'd probably say that keeping normal julia objects as So i think keeping them as objects is right. 👍 |
Are you waiting for another review, @gbaraldi? Do you need more input anywhere? |
The changes are fine, I just need to do some cleanup and it should be good to merge |
d2ad5d2
to
f9de78e
Compare
If performance is too much of an issue, we could attempt replacing |
@gbaraldi were you planning to try Jameson's idea out, or should we merge this? |
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. |
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