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

add inferno-collapse-ghcprof to handle GHC's prof files #300

Merged
merged 14 commits into from Sep 16, 2023

Conversation

dten
Copy link
Contributor

@dten dten commented Aug 13, 2023

Because this format is not directly rust related I'm not sure if you would prefer this be part of this repo or be a separate crate listing

Basically GHC (the Haskell compiler) has its own format for profiling traces and this adds collapsing for it.

https://downloads.haskell.org/ghc/latest/docs/users_guide/profiling.html

@dten dten force-pushed the ghcprof branch 3 times, most recently from 0c63239 to 1f1d104 Compare August 13, 2023 11:26
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Left some comments, but I think this is pretty close! One thing we should definitely check out is GHC's -pj flag for JSON output if that can make this job easier (and more robust).

Also, could you run one of these all the way to an SVG just to give it a visual correctness check as well (if you haven't already that is).

src/bin/collapse-ghcprof.rs Show resolved Hide resolved
src/bin/collapse-ghcprof.rs Outdated Show resolved Hide resolved
src/bin/collapse-ghcprof.rs Outdated Show resolved Hide resolved
src/collapse/ghcprof.rs Show resolved Hide resolved
src/collapse/ghcprof.rs Outdated Show resolved Hide resolved
src/collapse/ghcprof.rs Show resolved Hide resolved
src/collapse/ghcprof.rs Show resolved Hide resolved
src/collapse/ghcprof.rs Outdated Show resolved Hide resolved
src/collapse/ghcprof.rs Outdated Show resolved Hide resolved
src/collapse/ghcprof.rs Show resolved Hide resolved
@dten
Copy link
Contributor Author

dten commented Aug 26, 2023

I think I've addressed all the comments with updates except for the utf8 one.
I will probably have a look at the json file option at a later date

@dten dten requested a review from jonhoo August 26, 2023 09:32
@dten
Copy link
Contributor Author

dten commented Aug 26, 2023

these are the svgs of the test outputs, would you like them committing?

percent
ticks
ticks_bytes
ticks_ticks
utf8
utf8_bytes
utf8_ticks

@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Patch coverage is 91.20% of modified lines.

Files Changed Coverage
src/collapse/mod.rs ø
src/collapse/ghcprof.rs 90.98%
src/collapse/guess.rs 100.00%

📢 Thoughts on this report? Let us know!.

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Nice! We're almost there. Just one more bit.

No need to commit the SVGs — just wanted a sanity-check that things looked reasonable.

// - BUT it has a max width of 9 whilst its values can exceed (but are always space separted)
// "%time %alloc %time %alloc ticks bytes"
let source = match self.opt.source {
Source::PercentTime => l.find("%time").unwrap(),
Copy link
Owner

Choose a reason for hiding this comment

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

This still panics, but should return an error instead (like the other two now do).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's one of those cases where we "know" it's there since otherwise we couldn't be in the block because we just matched it as part of START_LINE. I've added a nice return error anyway and made clippy happy

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, in that case you can use expect instead and give the rationale!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay swapped it

jonhoo
jonhoo previously approved these changes Sep 9, 2023
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

🎉

@jonhoo
Copy link
Owner

jonhoo commented Sep 9, 2023

Oh, looks like you may need to merge from main too

@dten
Copy link
Contributor Author

dten commented Sep 9, 2023

rebased

@jonhoo
Copy link
Owner

jonhoo commented Sep 10, 2023

Oh, sorry, one very last thing — can you add an entry to CHANGELOG.md too? That way I can do a release right after merging!

@dten
Copy link
Contributor Author

dten commented Sep 12, 2023

Message added to changelog

Copy link
Owner

@jonhoo jonhoo 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!

@jonhoo jonhoo merged commit 305be42 into jonhoo:main Sep 16, 2023
16 checks passed
@jonhoo
Copy link
Owner

jonhoo commented Sep 16, 2023

Released in 0.11.17 🎉

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

3 participants