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: Allow Linux perf profiler to see Python calls by pablogsal · Pull Request #96123 · python/cpython

Conversation

@pablogsal

@pablogsal pablogsal commented Aug 19, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Allow the linux perf profiler to see Python calls #96143

Automerge-Triggered-By: GH:pablogsal

Sorry, something went wrong.

Uh oh!

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

Jongy reacted with thumbs up emoji tiran, adamchainz, kumaraditya303, jjerphan, erlend-aasland, dgiger42, holmanb, maxbrunet, dalehamel, GalaxySnail, and 2 more reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 12 reactions
@pablogsal pablogsal added the DO-NOT-MERGE label Aug 19, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Aug 19, 2022
pablogsal
pablogsal commented Aug 19, 2022
View reviewed changes
Comment thread Objects/codeobject.c Resolved

Uh oh!

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

pablogsal
pablogsal commented Aug 19, 2022
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

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

markshannon
markshannon reviewed Aug 19, 2022
View reviewed changes

@markshannon markshannon 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 have no idea if that assembly code does what you say, but I've reviewed the rest.

Definitely an intriguing idea.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Include/cpython/code.h Resolved

Uh oh!

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

Comment thread Include/internal/pycore_ceval.h Resolved

Uh oh!

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

Comment thread Objects/codeobject.c Resolved

Uh oh!

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

Comment thread Objects/codeobject.c Resolved

Uh oh!

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

Comment thread Objects/codeobject.c Resolved

Uh oh!

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

Comment thread Objects/codeobject.c Resolved

Uh oh!

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

Comment thread Objects/codeobject.c Resolved

Uh oh!

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

Comment thread Python/ceval.c Resolved

Uh oh!

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

@pablogsal
pablogsal force-pushed the perf branch 2 times, most recently from 9b009f2 to 439ef28 Compare August 19, 2022 16:51
@tiran

tiran commented Aug 19, 2022

Copy link
Copy Markdown
Member

You can use preprocesor macros if you name the file Objects/asm_trampoline.S or .sx instead of Objects/asm_trampoline.s. The .sx form needs a makefile rule.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Aug 19, 2022

Copy link
Copy Markdown
Member Author

You can use preprocesor macros if you name the file Objects/asm_trampoline.S or .sx instead of Objects/asm_trampoline.s. The .sx form needs a makefile rule.

Hummm, not sure I follow, could you maybe show me an example of what we can achieve with this?

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran

tiran commented Aug 19, 2022

Copy link
Copy Markdown
Member

You can have multiple implementations in the same file:

    .text
    .globl	_Py_trampoline_func_start
_Py_trampoline_func_start:
#ifdef __x86_64__
    push   %rbp
    mov    %rsp,%rbp
    mov    %rdi,%rax
    mov    %rsi,%rdi
    mov    %rdx,%rsi
    mov    %ecx,%edx
    call   *%rax
    pop    %rbp
    ret
#endif // __x86_64__
#ifdef __aarch64__
    TODO
#endif
    .globl	_Py_trampoline_func_end
_Py_trampoline_func_end:
pablogsal and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

gpshead
gpshead reviewed Aug 19, 2022
View reviewed changes
Comment thread Include/internal/pycore_ceval.h Resolved

Uh oh!

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

@gpshead gpshead added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 20, 2022
@pablogsal
pablogsal marked this pull request as ready for review August 20, 2022 17:10
@pablogsal
pablogsal requested a review from a team as a code owner August 20, 2022 17:10
@pablogsal
pablogsal force-pushed the perf branch 3 times, most recently from df2a40d to dda9e04 Compare August 20, 2022 17:26
@pablogsal pablogsal removed the DO-NOT-MERGE label Aug 20, 2022
@pablogsal pablogsal changed the title Allow Linux perf profiler to see Python calls gh-96143: Allow Linux perf profiler to see Python calls Aug 20, 2022
@pablogsal
pablogsal requested a review from markshannon August 20, 2022 17:36
@pablogsal
pablogsal force-pushed the perf branch 3 times, most recently from f38dfc2 to 22fc892 Compare August 20, 2022 17:46
kumaraditya303
kumaraditya303 reviewed Aug 20, 2022
View reviewed changes
Comment thread PCbuild/_freeze_module.vcxproj.filters Resolved

Uh oh!

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

@pablogsal
pablogsal force-pushed the perf branch 2 times, most recently from a2f4182 to 1b35ed3 Compare August 20, 2022 18:31
@tiran

tiran commented Aug 20, 2022

Copy link
Copy Markdown
Member

Why did you add a Windows build file? How about we do not define _PyPerfTrampoline_Init unless HAVE_PERF_TRAMPOLINE is defined?

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Aug 20, 2022

Copy link
Copy Markdown
Member Author

Why did you add a Windows build file? How about we do not define _PyPerfTrampoline_Init unless HAVE_PERF_TRAMPOLINE is defined?

Then we need to add more ifdef every place is used but that works as well for sure

All reactions

Sorry, something went wrong.

Uh oh!

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

tiran
tiran reviewed Aug 20, 2022
View reviewed changes
Comment thread Objects/perf_trampoline.c Resolved

Uh oh!

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

Comment thread Objects/perf_trampoline.c Resolved

Uh oh!

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

Comment thread Objects/perf_trampoline.c Resolved

Uh oh!

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

Comment thread Python/sysmodule.c Resolved

Uh oh!

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

tomytsai1
tomytsai1 reviewed Aug 27, 2022
View reviewed changes
Comment thread Objects/asm_trampoline.S Resolved

Uh oh!

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

@gpshead
Update sys API names in the NEWS entry.
ef0650b
they now match the current code.
gpshead
gpshead reviewed Aug 29, 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.

Two thoughts have been running around in my mind:

(1) We've got the command line flag and sys API, but what about just enabling this system wide or at least task/job wide within a container? An environment variable that turns it on would make that easy without plumbing options through. PYTHONPERFSUPPORT=1 as another equivalent to -Xperf? That's usually done in initconfig.c alongside the -X processing I believe.

Second: multiprocessing spawn start method. If the -Xperf flag was passed to the parent process I assume the "spawn" method should also pass that to its children.

Sorry, something went wrong.

Uh oh!

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

All reactions
@pablogsal

pablogsal commented Aug 29, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Two thoughts have been running around in my mind:

(1) We've got the command line flag and sys API, but what about just enabling this system wide or at least task/job wide within a container? An environment variable that turns it on would make that easy without plumbing options through. PYTHONPERFSUPPORT=1 as another equivalent to -Xperf? That's usually done in initconfig.c alongside the -X processing I believe.

Second: multiprocessing spawn start method. If the -Xperf flag was passed to the parent process I assume the "spawn" method should also pass that to its children.

I think (1) makes sense and is indeed coherent with how we handle some of the other options like tracemalloc so I will add an environment variable alongside the -X option.

Regarding (2) I am not that sure. We don't do this for the rest of the flags that we pass around and you can achieve the same with the environment variable if you want. In any case, as that would require some tests I would prefer to do that in a separate PR as this is already gigantic

gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@pablogsal
Add environment variable
d8932d2
@pablogsal

pablogsal commented Aug 29, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I implemented the environment variable and solved a bunch of conflicts. Please check it out when you have time.

I also had to solve a bunch of conflicts. @gpshead if you are ok with the current status I would like us to land if everything looks good as the size of the PR is already attracting a bunch of merge conflicts in the build system, clinic and other files.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal
Merge branch 'main' into perf
0f303ff
@pablogsal

pablogsal commented Aug 29, 2022

Copy link
Copy Markdown
Member Author

Ah wait, I need to document the environment variable. Will push a commit for that soon.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal
Document the env var and the -X option
e3f846e
@pablogsal

pablogsal commented Aug 29, 2022

Copy link
Copy Markdown
Member Author

Ah wait, I need to document the environment variable. Will push a commit for that soon.

Done 👍

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Aug 29, 2022

Copy link
Copy Markdown
Member Author

@erlend-aasland has mentioned that he was a bunch of docs improvements that he will do in a separate PR.

gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@pablogsal
pablogsal requested a review from gpshead August 29, 2022 21:09
gpshead
gpshead approved these changes Aug 29, 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.

My comments are easy to address things that don't need further review - minor edits or added note/todo comments to our future selves.

Agreed that the docs can use some polishing up but all the important bits are there to seed that future work, thanks for writing them!

Thanks for taking on adding this feature! I expect we'll see how a backport fares internally.

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 Lib/test/test_perf_profiler.py Resolved

Uh oh!

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

Comment thread Lib/test/test_perf_profiler.py Resolved

Uh oh!

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

Comment thread Objects/perf_trampoline.c Resolved

Uh oh!

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

Comment thread Objects/perf_trampoline.c Resolved

Uh oh!

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

Comment thread Objects/perf_trampoline.c Resolved

Uh oh!

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

@pablogsal pablogsal added the 🤖 automerge label Aug 30, 2022
@miss-islington
miss-islington merged commit 6d791a9 into python:main Aug 30, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 30, 2022
@pablogsal

pablogsal commented Aug 30, 2022

Copy link
Copy Markdown
Member Author

Damn, @miss-islington has landed the PR without waiting for the CI to build the last commit I pushed so I created #96433.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Aug 30, 2022

Copy link
Copy Markdown
Member Author

Thanks, everyone for the fantastic reviews and for helping to get this feature ready ❤️

You all rock 🤘

kumaraditya303, jjerphan, pradyunsg, and erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 4 reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Aug 30, 2022

Copy link
Copy Markdown
Member Author

@erlend-aasland You can make the doc improvement PR after #96433 lands.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jjerphan

jjerphan commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

Thanks for all for pushing for better interoperability with perf(1). I highly appreciate this contribution. 🙏

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Aug 30, 2022

Copy link
Copy Markdown
Member

Nice feature!

There is a typo in sys.deactivate_stack_trampoline() docstring: "Dectivate the perf profiler trampoline": Deactivate.


In the doc, python -m sysconfig | grep HAVE_PERF_TRAMPOLINE can be replaced with python -c "import sysconfig; print(bool(sysconfig.get_config_var('PY_HAVE_PERF_TRAMPOLINE')))" to not rely on the external grep command. At least, I suggest to add PY_ to fix the variable name:

python -m sysconfig | grep PY_HAVE_PERF_TRAMPOLINE

The second command can be replaced with python -c "import sysconfig; print('no-omit-frame-pointer' in sysconfig.get_config_var('PY_CORE_CFLAGS'))".

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

There is a typo in sys.deactivate_stack_trampoline() docstring: "Dectivate the perf profiler trampoline": Deactivate.

[...]

See #96445 :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@rajveerb

rajveerb commented Mar 7, 2023

Copy link
Copy Markdown

@pablogsal

Is it possible to port these changes, from python 3.12, without breaking to previous python versions, say 3.11, 3.10, 3.9?

Why?
Some libraries use specific versions of python and it will take time until they catch up to 3.12's release. This will defer the insights that can be gained till later on.

All reactions

Sorry, something went wrong.

Uh oh!

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

@pablogsal

pablogsal commented Mar 8, 2023

Copy link
Copy Markdown
Member Author

Is it possible to port these changes, from python 3.12, without breaking to previous python versions, say 3.11, 3.10, 3.9?

Is possible, but as these versions are only accepting security fixes or bugfixes we cannot backport new features. This means that if you want you can backport them yourself but you need to maintain a fork of the interpreter.

rajveerb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@carljm carljm mentioned this pull request Apr 5, 2023
expose API for writing perf map files #103295
Closed
@gsallam gsallam mentioned this pull request Apr 14, 2023
gh-103295: expose API for writing perf map files #103546
Merged
@art049 art049 mentioned this pull request Apr 20, 2023
perf maps address format #103650
Closed
facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request May 19, 2023
Gamal Sallam @facebook-github-bot
Backport perf trampoline
a88f497
Summary: Backport the perf-trampoline introduced in python/cpython#96123.  The perf trampoline doesn't work properly with the JIT, so we have submitted a PR to have a C-API to unify writing to the perf-map files python/cpython#103546.

Reviewed By: czardoz

Differential Revision: D45419843

fbshipit-source-id: 16bd13d7981e48c9eb7bc0e5eef1c1f4748965f6
@Zheaoli Zheaoli mentioned this pull request Oct 9, 2023
Missing DTrace probes #104280
Open
@Zheaoli Zheaoli mentioned this pull request Jan 3, 2024
GH-113464: A copy-and-patch JIT compiler #113465
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes
@zooba zooba zooba left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@markshannon markshannon Awaiting requested review from markshannon
+3 more reviewers
@tiran tiran tiran approved these changes
@tomytsai1 tomytsai1 tomytsai1 left review comments
@chalggg chalggg chalggg left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

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.

14 participants

@pablogsal @tiran @bedevere-bot @gpshead @jjerphan @vstinner @erlend-aasland @rajveerb @zooba @markshannon @tomytsai1 @chalggg @kumaraditya303 @miss-islington
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.