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

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

Closed
wants to merge 5 commits into from

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