Skip to content

Conversation

gbaraldi
Copy link
Member

This distinguishes malloced allocations from pool allocations in arrays. It also changes the printing of DataType to print the actual type. Same for module, where it gives the module name instead of Module

{
size_t name_or_idx = g_snapshot->names.find_or_create_string_id("<native>");

auto from_node_idx = record_node_to_gc_snapshot(from);
auto to_node_idx = record_pointer_to_gc_snapshot(to, bytes, "<malloc>");
auto alloc_type = pooled ? "<pooled>" : "<malloc>";
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 pooled is the right term. I think we have inline, malloc, foreign. And I think in the case of inline we should increase the size reported here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pooled was the name of the flag in the jl_array_t struct. Why would it report the wrong size in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Ah we actually have 4 different classes.

  1. small arrays are inline allocated
  2. medium sized array -> pooled
  3. large array -> malloced
  4. foreign: Who knows.

Now I remember that when were looking together at some snapshots one some arrays contained a pointer to a data object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Foreign arrays are treated exactly like malloced arrays. I found that out while playing with mimalloc, that's why I needed an extra bit in the array because to make it work one can't assume the same allocator for the foreign arrays as the local ones. We currently ignore the inlined arrays in the snapshot. I will see if I can add them however.

Copy link
Member Author

Choose a reason for hiding this comment

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

image
I think I found a couple megabytes sneaking around

Copy link
Member Author

Choose a reason for hiding this comment

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

This actually made it a lot more useful because we can now see the size of the inferred code.

Copy link
Member

Choose a reason for hiding this comment

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

nice. Is this RTM?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, yes.

Copy link
Member

Choose a reason for hiding this comment

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

@vchuravy RTM?

@vchuravy vchuravy merged commit 523c52f into JuliaLang:master Nov 8, 2022
Comment on lines +217 to +221
else if (jl_is_datatype(a)) {
type_name = string("Type{") + string(jl_symbol_name_(((_jl_datatype_t*)a)->name->name)) + string("}");
name = StringRef(type_name);
self_size = sizeof(jl_task_t);
}
Copy link
Member

Choose a reason for hiding this comment

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

What was the output for datatypes before this PR? Was it just DataType?
I think that this should be improved even further after #47503.

Thanks! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, before this PR it was DataType, with it we got what "kind" of DataType and with that one we should get the specific type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants