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

Added support for Papyrus profiles (#427) #428

Merged
merged 10 commits into from
Jun 23, 2023
Merged

Added support for Papyrus profiles (#427) #428

merged 10 commits into from
Jun 23, 2023

Conversation

xieve
Copy link
Contributor

@xieve xieve commented Jun 18, 2023

Here's what I have so far. There are still a couple of issues:

  • I couldn't get the tests to run (sh: 1: ./scripts/test-setup.sh: not found)
  • I've specified Profile.totalWeight but they still always start at 0
  • I'm not sure why Profile.stack and Profile.frames are protected, this way I have to re-implement those

@xieve xieve marked this pull request as ready for review June 18, 2023 19:37
@xieve
Copy link
Contributor Author

xieve commented Jun 18, 2023

Everything works now. As I said, implementation could be more ideal if I could access Profile.stack and Profile.frames, but apart from that, I'd say this is mergeable.

Frames left on the stack were prolonged by <startValue> ms
@xieve
Copy link
Contributor Author

xieve commented Jun 18, 2023

I've noticed one more thing: In the time order view, you can't see the things that take under 1ms, because they count as 0ms, which makes them completely invisible. Calling those functions still takes time, though, as the calls sometimes can't execute instantly and are thus put on the queue instead. I will try to implement queues as frames that contain the function calls.

Refactored variable naming to be more consistent
src/import/index.ts Outdated Show resolved Hide resolved
Copy link
Owner

@jlfwong jlfwong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

A few small things that need revision, but this should be good to land afterwards

src/import/papyrus.ts Outdated Show resolved Hide resolved
src/import/papyrus.ts Outdated Show resolved Hide resolved
src/import/papyrus.ts Outdated Show resolved Hide resolved
src/import/papyrus.ts Outdated Show resolved Hide resolved
src/import/papyrus.ts Outdated Show resolved Hide resolved
@xieve
Copy link
Contributor Author

xieve commented Jun 19, 2023

Implemented all suggested changes :)

@xieve xieve requested a review from jlfwong June 19, 2023 11:39
@xieve
Copy link
Contributor Author

xieve commented Jun 20, 2023

I had to change more than I anticipated, everything seems to be working as intended now.

@jlfwong
Copy link
Owner

jlfwong commented Jun 21, 2023

Hey @xieve -- just opened a test run, and it looks like CI is failing at the moment. You can see the issue here: https://github.com/jlfwong/speedscope/actions/runs/5317035811/jobs/9678064431?pr=428

It seems like the snapshots don't match -- can you take a look, update the snapshots, and make sure the changes are as expected?

@xieve
Copy link
Contributor Author

xieve commented Jun 22, 2023

Damn, I forgot to commit the updated snapshots.

@jlfwong jlfwong merged commit 693545b into jlfwong:main Jun 23, 2023
8 checks passed
jackerghan pushed a commit to jackerghan/speedscope that referenced this pull request Jul 28, 2023
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.

None yet

2 participants