Skip to content

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

Closed
zacharyfmarion wants to merge 5 commits intojlfwong:mainfrom
zacharyfmarion:zac/frame-name
Closed

Default frame name to the name of the function call#451
zacharyfmarion wants to merge 5 commits intojlfwong:mainfrom
zacharyfmarion:zac/frame-name

Conversation

@zacharyfmarion
Copy link
Contributor

@zacharyfmarion zacharyfmarion commented Dec 16, 2023

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
Copy link
Contributor Author

@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
Copy link
Owner

jlfwong commented Dec 16, 2023

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: #201 (comment)

@coveralls
Copy link

coveralls commented Dec 17, 2023

Coverage Status

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

@zacharyfmarion
Copy link
Contributor Author

@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
Copy link
Owner

jlfwong commented Dec 18, 2023

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
Copy link
Contributor Author

@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.

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.

4 participants