search

jlfwong / speedscope

🔬 A fast, interactive web-based viewer for performance profiles.
https://www.speedscope.app
MIT License
5.45k stars 237 forks source link

Default frame name to the name of the function call #451

Closed zacharyfmarion closed 8 months ago

zacharyfmarion commented 8 months ago

Because the displayed frame name is the entire key, the display name becomes very hard to parse if there are args that get stringified into the key (this is the case for Hermes profiles). Defaults to just showing the name for trace profiles.

Before After
Before 1 After 1
Before 2 After 2
Before 3 After 3
zacharyfmarion commented 8 months ago

@jlfwong happy to iterate on this, no rush but want to make sure you agree this is a good approach. Also would it be possible to release a new version of speedscope with this commit and my previous one one they are good to go? Would like my company's engineers to switch over from using Perfetto since this UI is way better.

jlfwong commented 8 months ago

Hey -- unfortunately if this information is hidden, then the args information isn't visible anywhere at all in speedscope. There's no place to display metadata unrelated to the file & line information.

Others have, in the past, indicated that the display of this information is helpful. For example: https://github.com/jlfwong/speedscope/issues/201#issuecomment-458019528

coveralls commented 8 months ago

Coverage Status

coverage: 43.048% (+0.007%) from 43.041% when pulling fbb0104e3215bece4cfbebb099ece3ed0fc62cfd on zacharyfmarion:zac/frame-name into dfd3a0dfb3b35d73b75023a1447604ebd721af12 on jlfwong:main.

zacharyfmarion commented 8 months ago

@jlfwong gotcha, makes sense, for now I can just fork. Do you agree that the profile afterward is easier to read though? Maybe we can iterate on an approach. What if we just showed all the args in the tooltip and footer panel?

jlfwong commented 8 months ago

Hm, I don't have a strong enough intuition for how people use args, especially given that the chrome trace format is spit out by a really wide variety of profiling tools. I have general gripes with the chrome trace format because it leaves so much behavior unspecified.

I also don't know how anything about the Hermes ecosystem, but is it possible to submit a patch to export directly into speedscope's own format to avoid a bunch of these annoying ambiguities? Or, alternatively, I'd accept a patch which ingests Hermes own internal profiling format (which would hopefully obviate the need for using https://github.com/react-native-community/hermes-profile-transformer?tab=readme-ov-file, which I'm assuming you're using?)

zacharyfmarion commented 8 months ago

@jlfwong thanks for the response - I have opened a PR to support the hermes format in speedscope, since I think it is good to have. I still think that the ideal case is that hermes-profile-transformer exports into a format that perfetto, chrome devtools, and speedscope can all read. I agree the spec is kind of bad but given that it is what we already have and widely supported, are you strongly opposed to me just check if all the args match a hermes profile, and having custom logic to format Hermes profiles well if that is the case? I talk a bit more about it in my hermes support PR.