-
Notifications
You must be signed in to change notification settings - Fork 701
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 a performance measuring top-level user guide page #10539
Conversation
Thanks a bunch for starting this. Several things:
ghc-prof-options: -fprof-auto -fno-prof-count-entries -fprof-auto-calls
|
Thanks for the quick review @Kleidukos and @geekosaur. I've added all your review remarks. Yes, the json output can be loaded into speedscope directly. This https://github.com/mpickering/hs-speedscope only deals with |
9839ef1
to
0370522
Compare
0370522
to
8988bee
Compare
@malteneuss Much better, only two more changes to make and we should be good to go! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that eventlog profiling is far superior than just -pj
, and it is (for recent GHCs) just a matter of passing -l -p
instead (or if a smaller eventlog is wanted -l-agu -p
). I have personally never used the json profiling report.
77341b0
to
d326c39
Compare
@jasagredo Do you have a link to a post or blog that discusses the differences? I have never used it. Would you like to add a second section showing how to use it in another MR? I think we should add another section for profiling memory, where eventlog should be discussed after all. |
Thank you for taking this on! It's amazing to see efforts in expanding the guide section! I have certain critique below but, please, realize that documentation is a matter where reasonable people can disagree. I can survive the current version, especially given that you already got the mandatory two approvals. IMO the text uses a
I propose solving most (all) of these with the following reformatting.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I temporarily block it to make sure you have enough time to respond.
Re-reading the document again I am unsure this describes Cabal's job on profiling. I think the section should be more about what options does cabal need to enable profiling than how to produce a profiling report, therefore I think the title is misleading and we are stepping into GHC's User Guide territory. My suggestion would be to leave most of the description and RTS options to the GHC User guide and describe only the following in the text:
This way the choice of p vs pj, or what files are produced or where to load them is deferred to the User Guide, as it is GHC-specific business. This way we don't mix "How to profile" and "How to configure for profiling". The latter is Cabal's job, but the former is GHC or general Haskell information which should not live in Cabal's docs I think. It will always be too specific or too general what you can say there and it might depend on GHC versions and so on. |
OTOH, for some users a full example session of profiling would be immensely valuable and precisely what they are looking for, in despair reaching even for the cabal documentation. So maybe at least link to the original discourse post, saying it contains practical examples, including how to tweak GHC to profile best, which is out of scope of the cabal guide? |
I fixed the typos. Further changes, especially to other pages, should go to separate MRs. |
I've left some comments for accuracy/brevity. But this is good work! |
@AndreasPK I don't see your comments. |
Nor do I. Did you actually submit them, or leave them pending? |
Seems they were pending :) |
Thanks! I remove the merge label until the comments are resolved. |
That's not really necessary; since they're conversations on specific lines, they count as unresolved and GitHub won't allow merging (see the big red "Unresolved conversations" box). It's comments like this one that don't count because they're not considered review comments. |
@geekosaur i thought it’s only respected by GitHub, not Mergify. But good to know, thanks. |
It's part of the branch protection rules, which Mergify copies into its own rules. See the first checkbox in the "merge+no rebase" ruleset in the Mergify summary check. (ETA: whoops, wrong PR, it's the "squash+merge me" ruleset in this case.) |
Co-authored-by: AndreasPK <[email protected]>
Co-authored-by: AndreasPK <[email protected]>
Seems all my points have been addressed. It seems I can't resolve the conversation as it's outdated perhaps? Anyway from my POV this looks good now :) |
Another step of the user guide improvement initiative #9214:
Add a simple profiling user-guide page based on https://discourse.haskell.org/t/ghc-profiling-a-cabal-project-with-an-interactive-application/10465/2?u=malteneuss, which the author allowed us to use.
Feel free to modify yourself if that is faster.