GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-96143: Improve perf profiler docs by erlend-aasland · Pull Request #96445 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Aug 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • 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
  • Issue: Allow the linux perf profiler to see Python calls #96143

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@erlend-aasland
pythongh-96143: Improve perf profiler docs
41bab32
@erlend-aasland
erlend-aasland requested review from ezio-melotti, gpshead and pablogsal August 30, 2022 22:05
@bedevere-bot bedevere-bot added the awaiting core review label Aug 30, 2022
erlend-aasland
erlend-aasland commented Aug 30, 2022
View reviewed changes
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

pablogsal
pablogsal reviewed Aug 30, 2022
View reviewed changes
Comment thread Doc/library/sys.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland marked this pull request as ready for review August 30, 2022 22:10
erlend-aasland
erlend-aasland commented Aug 30, 2022
View reviewed changes
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland
you => your
574b39a
pablogsal
pablogsal reviewed Aug 30, 2022
View reviewed changes
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland @pablogsal
Fixup What's New entry
9388964
Co-authored-by: Pablo Galindo Salgado <[email protected]>
pablogsal
pablogsal reviewed Aug 30, 2022
View reviewed changes
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Address review: add links to perf wiki, and use bullet points
31c0eb8
@erlend-aasland erlend-aasland mentioned this pull request Aug 30, 2022
gh-96143: Allow Linux perf profiler to see Python calls #96123
Merged
pablogsal
pablogsal approved these changes Aug 30, 2022
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

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.

LGTM

Great work! 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 30, 2022
vstinner
vstinner reviewed Aug 30, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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'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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Address Victor's review
9c72023
@erlend-aasland

erlend-aasland commented Aug 31, 2022

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland requested a review from vstinner August 31, 2022 08:40
ezio-melotti
ezio-melotti reviewed Aug 31, 2022
View reviewed changes
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/clinic/sysmodule.c.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@gpshead
https
3756252
gpshead
gpshead requested changes Aug 31, 2022
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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.

thanks for the edits!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Aug 31, 2022
@bedevere-bot

bedevere-bot commented Aug 31, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti
ezio-melotti reviewed Sep 1, 2022
View reviewed changes
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Sep 1, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 2 commits September 4, 2022 23:07
@erlend-aasland
Address reviews
cbe8b16
@erlend-aasland
Merge branch 'main' into perf-docs
9ce60ea
@erlend-aasland

erlend-aasland commented Sep 4, 2022

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti
ezio-melotti reviewed Sep 17, 2022
View reviewed changes
Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/howto/perf_profiling.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/sys.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/clinic/sysmodule.c.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti ezio-melotti mentioned this pull request Sep 17, 2022
Default role does not detect default roles starting with a single quote sphinx-contrib/sphinx-lint#40
Closed
Erlend E. Aasland and others added 2 commits October 4, 2022 12:27
Erlend E. Aasland @ezio-melotti
Apply suggestions from code review
db4aaa2
Co-authored-by: Ezio Melotti <[email protected]>
@erlend-aasland
Merge branch 'main' into perf-docs
d68c77e
@pablogsal

pablogsal commented Oct 25, 2022

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal pablogsal self-assigned this Oct 25, 2022
@erlend-aasland

erlend-aasland commented Oct 25, 2022

Copy link
Copy Markdown
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 :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
Merge branch 'main' into perf-docs
0d41b95
@pablogsal pablogsal added the skip news label Oct 26, 2022
pablogsal
pablogsal approved these changes Oct 26, 2022
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Oct 26, 2022
gpshead
gpshead approved these changes Oct 26, 2022
View reviewed changes
@pablogsal
pablogsal merged commit 723ebe7 into python:main Oct 27, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label 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
@erlend-aasland @gvanrossum
pythongh-96143: Improve perf profiler docs (python#96445)
18ac789
dependabot Bot added a commit to ronaldoussoren/cpython that referenced this pull request Nov 14, 2022
@dependabot
pythongh-96143: Improve perf profiler docs (python#96445)
63b6c66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@ezio-melotti ezio-melotti ezio-melotti left review comments
@gpshead gpshead gpshead approved these changes
@pablogsal pablogsal pablogsal approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@pablogsal pablogsal

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@erlend-aasland @bedevere-bot @pablogsal @gpshead @vstinner @ezio-melotti
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.