[profiler] Rework GC roots reporting. #5710
Conversation
This change re-implement GC roots reporting in a way that allows users to correctly track the source of a GC root. The new design is based on emitting GC root information upfront and only report addresses during heap dumps. When a GC root is registered we emit an event that is an address range, the root kind, a key and a text description. A decoder can use this information to match a reported root address with all registration addresses to figure out what they mean. A GC Root key is used to further its meaning. For example, if kind is thread, key is a tid, if kind is static variables, key is the class pointer and so on. The build of the change is introducing this key argument across all root registration code and rework our root reporting code to encode their addresses. Some roots use pseudo-addresses when they are not registrable. This is the case of thread stacks/registers and the finalizer queue. Finally, root reporting was changed to happen only once per collection and at the end, leading to correct and useful data being produced.
luhenry
requested review from
alexrp,
BrzVlad,
DavidKarlas,
kumpera and
vargaz
as code owners
|
This is #5126 with master merged in to resolve conflicts. |
luhenry
force-pushed
the
gc_roots_experiments
branch
from
12bc60f
to
51dc3dd
Compare
|
build profiler stress |
1 similar comment
|
build profiler stress |
| Attributes = StreamHeader.FormatVersion == 13 ? (LogHeapRootAttributes) _reader.ReadByte () : (LogHeapRootAttributes) _reader.ReadULeb128 (), | ||
| ExtraInfo = (long) _reader.ReadULeb128 (), | ||
| AddressPointer = ReadPointer (), | ||
| ObjectPointer = ReadObject () |
There was a problem hiding this comment.
This is breaking support for v13, isn't it?
There was a problem hiding this comment.
This is something I missed from merging master, I will fix that.
|
@monojenkins build pkg |
|
@monojenkins build pkg |
|
build |
luhenry
force-pushed
the
gc_roots_experiments
branch
from
a87834c
to
1e4898d
Compare
|
build profiler stress |
|
build pkg |
|
Jetlag doesn't let me sleep...
Issue that I have... So my issue is... Which number is off here? New API registering too little stuff or |
|
I'm trying to make sense of all this data without much success... Would be nice if some unit tests could be added for example... profile app which creates object and assign it to static variable... In unit test verify that root is reported... |
|
build profiler stress |
|
@kumpera we still have the profiler stress test crash at https://jenkins.mono-project.com/job/test-mono-pull-request-profilerstress/20/testReport/MonoTests/profiler-stress/msbiology/ |
|
Ugg, the stress test actually caught a bug in thread registration. |
|
@kumpera could you please work on getting the profiler stress test fixed? I will rebase on master, fix the conflicts and push back here. Feel free to push anything you want to |
|
build profiler stress |
|
@monojenkins build pkg |
luhenry
force-pushed
the
gc_roots_experiments
branch
from
a7792a0
to
a12729e
Compare
|
@monojenkins build pkg |
thread_stopping occurs earlier than thread_stopped, before any of the detach code has run. thread_exited occurs after thread_stopped, once all detach logic has finished.
…events.
With the revamped GC root reporting work, some GC root unregister events will
arrive after the thread_stopped event. This will cause the thread to be
re-added to the thread list in the profiler, and never be removed until program
exit. This meant that if such a thread had actually exited and a new thread
would reuse its thread ID, that new thread would fail to add itself to the
thread list, leading to failures like this one:
init_thread: failed to insert thread 0x70000b761000 in log_profiler.profiler_thread_list, found = true
By using the new thread_exited event, we remove the thread from the thread list
after these GC root unregister events have arrived, thereby ensuring that the
thread won't be incorrectly 'resurrected'.
…ents. These checks are no longer necessary as these thread callbacks are not invoked for tools threads in the first place.
…esurrection case.
|
@monojenkins build profiler stress |
|
@monojenkins build pkg |
|
This can be closed since it was reopened and merged as another PR. |
This change re-implement GC roots reporting in a way that allows users to correctly track the source of a GC root.
The new design is based on emitting GC root information upfront and only report addresses during heap dumps.
When a GC root is registered we emit an event that is an address range, the root kind, a key and a text description.
A decoder can use this information to match a reported root address with all registration addresses to figure out what they mean.
A GC Root key is used to further its meaning. For example, if kind is thread, key is a tid, if kind is static variables, key is the class pointer and so on.
The build of the change is introducing this key argument across all root registration code and rework our root reporting code to encode their addresses.
Some roots use pseudo-addresses when they are not registrable. This is the case of thread stacks/registers and the finalizer queue.
Finally, root reporting was changed to happen only once per collection and at the end, leading to correct and useful data being produced.