Skip to content

gh-96143: Improve perf profiler docs #96445

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

Merged
merged 11 commits into from
Oct 27, 2022
Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 30, 2022

  • Add What's New
  • Rewrite NEWS entry to match What's New
  • Document new sys APIs
  • Fix formatting here and there
  • Adjust wording a few places

@erlend-aasland erlend-aasland marked this pull request as ready for review August 30, 2022 22:10
Co-authored-by: Pablo Galindo Salgado <[email protected]>
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

Great work! 👍

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I'm confused by "compatibility mode" wording whereas the env var is called "perf support". I would prefer to say that -X perf enables support for the Linux perf tool.

"Enable" support or "Add" support, not sure which one makes more send. Technically, the option adds something ;-)

The PyConfig member, the env var and the -X option have 3 different names which also confuse me. Would it make sense to rename the -X option to -X perfsupport to make it more consistent? If yes, I would suggest doing that in a separated PR.

The name "perf" is very generic and can mean many things. A newcomer might read it as: "use -X perf to get best performance" :-)

For me, "compatibility mode" sounds like "backward compatibility mode".

@erlend-aasland
Copy link
Contributor Author

I'm confused by "compatibility mode" wording whereas the env var is called "perf support". I would prefer to say that -X perf enables support for the Linux perf tool.

+1

I've tried to address your review comments in 9c72023, @vstinner. Are you ok with those changes?

The PyConfig member, the env var and the -X option have 3 different names which also confuse me. Would it make sense to rename the -X option to -X perfsupport to make it more consistent? If yes, I would suggest doing that in a separated PR.

+1

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

thanks for the edits!

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Ezio made many interesting comments. I will wait until they are addressed to review again the PR ;-)

@erlend-aasland
Copy link
Contributor Author

Sorry for the delay. I finally got around to try to address the review. Please take a look at the changes.

Erlend E. Aasland and others added 2 commits October 4, 2022 12:27
@pablogsal
Copy link
Member

@erlend-aasland can you fix the conflicts and I will land this? Thanks!

@pablogsal pablogsal self-assigned this Oct 25, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland can you fix the conflicts and I will land this? Thanks!

Will do! Thanks for the heads-up. I forgot about this PR :)

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fixes! ❤️

I plan to land this this week as all the feedback has been addressed but I will give some time in case someone wants to request some further changes.

@pablogsal pablogsal merged commit 723ebe7 into python:main Oct 27, 2022
@erlend-aasland erlend-aasland deleted the perf-docs branch October 27, 2022 15:20
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Oct 28, 2022
dependabot bot added a commit to ronaldoussoren/cpython that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants