Skip to content

gh-109587: Allow "precompiled" perf-trampolines to largely mitigate the cost of enabling perf-trampolines #109666

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 32 commits into from
Oct 27, 2023

Conversation

gsallam
Copy link
Contributor

@gsallam gsallam commented Sep 21, 2023

This pull request implements a proposal in #109587 to allow precompiled perf-trampolines. This would mitigate the costs of enabling perf-trampolines, such as the disk IO and memory overhead.

The proposal introduces two new C-API functions:

  • PyUnstable_PerfTrampoline_CompileCode(): Creates a new trampoline and registers it.
  • PyUnstable_PerfTrampoline_SetPersistAfterFork(): Controls whether to re-initialize trampolines in child processes.

These functions can be used by extension modules to initialize trampolines eagerly, after the application is "warmed up". This would make it possible to have perf-trampoline running in an always-enabled fashion.

Benefits for a forked multiprocess model:

  • Reduces the CPU overhead of enabling perf-trampolines.
  • Makes it possible to have perf-trampoline running in an always-enabled fashion.

@gsallam gsallam requested a review from encukou as a code owner September 22, 2023 18:00
@encukou encukou removed their request for review September 25, 2023 11:50
Comment on lines 808 to 809
"PyUnstable_PerfTrampoline_CompileCode",
"PyUnstable_PerfTrampoline_SetPersistAfterFork",
Copy link
Member

Choose a reason for hiding this comment

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

Here too?

@czardoz
Copy link
Contributor

czardoz commented Oct 5, 2023

@pablogsal , @gpshead Would love to hear your thoughts on this (whenever you get a chance to take a look) :)

@gpshead gpshead self-assigned this Oct 6, 2023
@gpshead gpshead self-requested a review October 6, 2023 12:11
@gpshead gpshead removed their assignment Oct 6, 2023
@czardoz
Copy link
Contributor

czardoz commented Oct 6, 2023

Ok I resolved the merge conflict, but I think the new MacOS failure is unrelated.

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

Seems that we have some conflicts that we need to address before landing, @gsallam can you take a look?

@pablogsal pablogsal enabled auto-merge (squash) October 19, 2023 18:41
auto-merge was automatically disabled October 19, 2023 19:06

Head branch was pushed to by a user without write access

@pablogsal
Copy link
Member

@gsallam We still have some failures in the Windows and macOS CI. I think this is because the symbol doesn't exist in those cases:

_testinternalcapi.obj : error LNK2019: unresolved external symbol __imp_PyUnstable_PerfTrampoline_CompileCode referenced in function compile_perf_trampoline_entry [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
D:\a\cpython\cpython\PCbuild\amd64\_testinternalcapi_d.pyd : fatal error LNK1120: 1 unresolved externals [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
    0 Warning(s)
    2 Error(s)

facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request Oct 25, 2023
Summary: As titled. To be on sync with python/cpython#109666.

Reviewed By: czardoz

Differential Revision: D50329585

fbshipit-source-id: d4c52143216a1d9970196ad71ef656098be50120
@pablogsal
Copy link
Member

We still have some failing tests it seems

@czardoz
Copy link
Contributor

czardoz commented Oct 27, 2023

This approach is likely quite silly, but I don't have a Windows/Mac machine handy, so I'm kicking off these builds to test/debug.

@czardoz
Copy link
Contributor

czardoz commented Oct 27, 2023

I will look at this over the weekend on my Windows box, this trial-and-error is not an optimal way :)

@pablogsal
Copy link
Member

I will look at this over the weekend on my Windows box, this trial-and-error is not an optimal way :)

Don't worry, I pushed a commit that should fix it

@pablogsal pablogsal enabled auto-merge (squash) October 27, 2023 03:54
@pablogsal pablogsal merged commit 21f068d into python:main Oct 27, 2023
@czardoz
Copy link
Contributor

czardoz commented Oct 27, 2023

Thanks a lot @pablogsal! 😄

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
facebook-github-bot pushed a commit to facebookincubator/cinderx that referenced this pull request Dec 17, 2024
Summary: Straight up port of python/cpython#109666

Reviewed By: jbower-fb

Differential Revision: D66118319

fbshipit-source-id: bd7d4181c16d35526f57500cebb6e16fc2df58aa
facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request Dec 17, 2024
Summary: Straight up port of python/cpython#109666

Reviewed By: jbower-fb

Differential Revision: D66118319

fbshipit-source-id: bd7d4181c16d35526f57500cebb6e16fc2df58aa
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.

7 participants