jhawthorn / vernier

📏 next generation CRuby profiler
https://vernier.prof/
MIT License
718 stars 15 forks source link

Pretty thread names based on best-effort parsing of Thread#inspect output #54

Closed dalehamel closed 4 months ago

dalehamel commented 4 months ago

Problem

It looks like nothing sets the thread name in the output anymore, so the tracks in firefox-profiler will not show any name:

Screenshot 2024-02-23 at 1 24 39 PM

Checking out the json file that is going into this, the thread name is "". If no name was ever set, this is expected. However if a name was set, it is still not making it to the gecko output.

I originally suspected https://github.com/jhawthorn/vernier/pull/46, but this is happening for me (on ruby 3.3.0) even with older gems (0.3.0, 0.3.1) that tried to get it from the pthread. In any case, the code to get this from the native extension is currently commented out and nothing seems to set it anywhere.

Proposed solution

As @casperisfine has pointed out, calling "inspect" on a Thread object gives us a lot of useful information, including the thread name if it is set. So, even though we cannot directly get the name from the native extension, we should be able to inspect the thread objects we already have access to.

However, we need to be careful about this as calling rb_inspect requires the GVL is acquired. Within the native extension then, after our callback is fired for RUBY_INTERNAL_THREAD_EVENT_RESUMED, we should be able to safely call it. We will just directly store the result as the thread's "name" if we are able to do so.

If we were never able to set this within the native code before we attempt to get the gecko output, we'll try looking up the thread via its object id and calling inspect from ruby land. Either way, the output will be fed into a pretty_name function to try and display the name as nicely as possible:

With this proposed change, here is what the output looks like:

Screenshot 2024-02-26 at 3 36 41 PM
dalehamel commented 4 months ago

Set this back to draft while I update the description and address the great comments so far.

dalehamel commented 4 months ago

Ok, i think this is ready for another look.

I think I've got a way to safely get the thread name from native code via rb_inspect. I can remove that native code if it is contentious, as ultimately the same value is attempted to be grabbed in ruby land and is parsed in the same way. The native code just covers getting us a name in the case where the thread was GC'd by the time we got to ruby.

I made the tests more explicit to cover the different cases of the inspect output having a name, file path and line number, etc.

jhawthorn commented 4 months ago

Merged. Thank you!

jhawthorn commented 4 months ago

@dalehamel released as part of 0.5.0

dalehamel commented 4 months ago

cheers, thanks @jhawthorn 🎉