Skip to content
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

Heap Snapshot has messed up array edge indexes #51576

Closed
NHDaly opened this issue Oct 3, 2023 · 5 comments · Fixed by #51592
Closed

Heap Snapshot has messed up array edge indexes #51576

NHDaly opened this issue Oct 3, 2023 · 5 comments · Fixed by #51592
Labels
GC Garbage collector tooling

Comments

@NHDaly
Copy link
Member

NHDaly commented Oct 3, 2023

If you look at a heap snapshot, all the edges coming from an array all report an index of [0]:
Screenshot 2023-10-03 at 4 46 44 PM

Looking at the .heapsnapshot file produced by the current C++ code, it looks like we are constructing edges with garbage values. Here is a snippet from one .heapsnapshot file I produced:

0,2,1
0,3,1
0,5,2
0,7,3
1,8,5
2,18446744073709551615,6
2,18446744073709551615,7
2,18446744073709551615,8
2,18446744073709551615,7
2,18446744073709551615,9
2,18446744073709551615,7
2,18446744073709551615,10
2,18446744073709551615,7
2,18446744073709551615,11
2,18446744073709551615,7

The first field is the edge type, and 2 is "element", meaning this is an array item.

The second field is supposed to be the array index, but all of them have the same value: 18446744073709551615, which is -1 in Int64:

julia> Unsigned(18446744073709551615)
0x0000000000000000ffffffffffffffff

julia> Unsigned(18446744073709551615) == typemax(UInt64)
true

This value is coming from gc_slot_to_arrayidx(from, to), here:

_gc_heap_snapshot_record_array_edge(from, *to, gc_slot_to_arrayidx(from, to));

So somehow the gc_slot_to_arrayidx value is wrong for all of the edges:

cat /Users/nathandaly/src/julia/37833_1019974912726708.heapsnapshot.edges | grep '^2,' | grep -v '18446744073709551615' | wc -l
       0

Is it valid to call gc_slot_to_arrayidx(from, to) on the parent/child objects? The callsites look like this, for example:

julia/src/gc.c

Lines 2069 to 2076 in e5c6340

verify_parent2("obj array", obj_parent, obj_begin, "elem(%d)",
gc_slot_to_arrayidx(obj_parent, obj_begin));
jl_taggedvalue_t *o = jl_astaggedvalue(new_obj);
if (!gc_old(o->header))
nptr |= 1;
if (!gc_marked(o->header))
break;
gc_heap_snapshot_record_array_edge(obj_parent, &new_obj);

Asking for support here from @gbaraldi, @apaz-cli, @d-netto. Thanks in advance!

@NHDaly NHDaly added the GC Garbage collector label Oct 3, 2023
@NHDaly
Copy link
Member Author

NHDaly commented Oct 3, 2023

Should we be passing in the index from the callsite instead? Does the callsite know the index?

@NHDaly
Copy link
Member Author

NHDaly commented Oct 3, 2023

Just checked, and this is reproducible on 1.9, so probably this was never working correctly (thanks for the suggestion @d-netto):

$ cat 1.9.heapsnapshot | grep '^,2,' | grep -v ',0,0$' | grep -v '18446744073709551615' | wc -l
       0
$ cat 1.9.heapsnapshot | grep '^,2,' | grep -v ',0,0$' | grep '18446744073709551615' | wc -l
 1771550

@NHDaly NHDaly added the tooling label Oct 4, 2023
@gbaraldi
Copy link
Member

gbaraldi commented Oct 4, 2023

-1 is the "default" result for the gc_slot_to_arrayidx. Though It's odd that we do get the default.

@vchuravy
Copy link
Member

vchuravy commented Oct 4, 2023

It means the parent wasn't an array....

@d-netto
Copy link
Member

d-netto commented Oct 4, 2023

Couldn't reproduce this on 1.9.

We're likely just passing a garbage pointer to the snapshot functions.

@d-netto d-netto closed this as completed in 5bdc1b3 Oct 5, 2023
KristofferC pushed a commit that referenced this issue Oct 9, 2023
Fixes #51576 on a simple
snapshot I collected on my machine.

(cherry picked from commit 5bdc1b3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants