gh-126195: Use pthread_jit_write_protect_np on macOS #126196

diegorusso · 2024-10-30T18:11:35Z

Replace mprotect with pthread_jit_write_protect_np on MacOS Apple Silicon.
Improve JIT performance by ~1.4% on this platform.

Issue: JIT: Use pthread_jit_write_protect_np on macOS #126195

mdboom · 2024-10-30T18:23:59Z

We should probably do one last benchmarking run on macOS for this PR. Unfortunately our Mac benchmark machine is offline at the moment -- I'm waiting for someone to physically go and see what's up. Hopefully less than a couple days or so.

Fidget-Spinner · 2024-10-31T01:17:01Z

JIT tests are failing and I have a partial fix at #126166 (comment). But it doesn't work on Windows because I'm not sure how to detect the JIT is running on there.

mdboom · 2024-10-31T13:24:17Z

We should probably do one last benchmarking run on macOS for this PR. Unfortunately our Mac benchmark machine is offline at the moment -- I'm waiting for someone to physically go and see what's up. Hopefully less than a couple days or so.

The machine is back up and I'm running the benchmarks now.

mdboom · 2024-10-31T17:09:38Z

We should probably do one last benchmarking run on macOS for this PR. Unfortunately our Mac benchmark machine is offline at the moment -- I'm waiting for someone to physically go and see what's up. Hopefully less than a couple days or so.

The machine is back up and I'm running the benchmarks now.

I think the benchmarks are hitting the tests being broken on main -- failing in the test_embed module when building for PGO. We'll probably need to wait for that and then merge that here and re-run the benchmarks.

Eclips4 · 2024-11-02T22:17:33Z

Merging main to get rid of failures in test_embed
(failures on aarch64macos runners as you might know are expected thing 🙂 )

diegorusso · 2024-11-04T15:13:53Z

I've updated the branch to check if errors disappeared...

brandtbucher · 2024-11-07T19:29:25Z

When looking this up, it seems that the documented way to do this now is the pthread_jit_write_with_callback_np API, which is a bit trickier to retrofit onto our JIT. It looks like pthread_jit_write_protect_np probably still works, but it's not well documented anymore and it seems like the new API might work in more places: https://developer.apple.com/documentation/apple-silicon/porting-just-in-time-compilers-to-apple-silicon

Probably makes sense to land this now, then explore the other API after.

brandtbucher

One issue with an error path, and a couple of suggestions.

Python/jit.c

bedevere-app · 2024-11-07T19:40:09Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

brandtbucher · 2024-11-07T20:02:44Z

BTW, benchmarking results for anyone curious: https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20241018-3.14.0a1%2B-f74cd79-JIT/bm-20241018-darwin-arm64-diegorusso-pthread-3.14.0a1%2B-f74cd79-vs-base.svg

Replace mprotect with pthread_jit_write_protect_np on MacOS Apple Silicon. Improve JIT performance by ~1.4% on this platform.

brandtbucher

Looks great! Just a couple of cosmetic suggestions, then I'll go ahead and merge.

Thanks for tackling this!

Misc/NEWS.d/next/Core_and_Builtins/2024-10-30-18-16-10.gh-issue-126195.6ezBpr.rst

Python/jit.c

brandtbucher · 2024-11-09T00:46:14Z

Python/jit.c

+    int status = mark_executable(memory, total_size);
+#ifdef MAP_JIT
+    pthread_jit_write_protect_np(1);
+#endif
+    if (status) {


This should work, right? Then we don't need status:

Suggested change

int status = mark_executable(memory, total_size);

#ifdef MAP_JIT

pthread_jit_write_protect_np(1);

#endif

if (status) {

#ifdef MAP_JIT

pthread_jit_write_protect_np(1);

#endif

if (mark_executable(memory, total_size)) {

I don't think this is the same thing. if we place pthread_jit_write_protect_np(1) in mark_executable, this would be just after the clear cache. The solution you suggested would place it before the clear cache and It could raise an error as it enable the write protection and then the clear cache.

I don't think it matters which order they occur in (it's not like the cache invalidation tries to execute the code or anything). I just tried it locally and didn't experience any issues.

I tried not to change the previous code logic, but anyway you are right, it works in our internal CI as well. Unless there are other requests, I think this was the last request for change.

brandtbucher

Thanks!

diegorusso requested a review from brandtbucher as a code owner October 30, 2024 18:11

bedevere-app bot added the awaiting review label Oct 30, 2024

bedevere-app bot mentioned this pull request Oct 30, 2024

JIT: Use pthread_jit_write_protect_np on macOS #126195

Open

brandtbucher self-assigned this Oct 30, 2024

brandtbucher added performance Performance or resource usage skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.14 new features, bugs and security fixes topic-JIT and removed skip news labels Oct 30, 2024

brandtbucher requested changes Nov 7, 2024

View reviewed changes

Python/jit.c Outdated Show resolved Hide resolved

Python/jit.c Outdated Show resolved Hide resolved

bedevere-app bot removed the awaiting review label Nov 7, 2024

bedevere-app bot added the awaiting changes label Nov 7, 2024

diegorusso and others added 3 commits November 8, 2024 14:00

pythongh-126195: Use pthread_jit_write_protect_np on macOS

71cef5e

Replace mprotect with pthread_jit_write_protect_np on MacOS Apple Silicon. Improve JIT performance by ~1.4% on this platform.

📜🤖 Added by blurb_it.

24719d7

Address Brandt feedback

507e662

diegorusso force-pushed the pthread_jit_write_protect_np branch from 3d384fa to 507e662 Compare November 8, 2024 16:35

diegorusso requested a review from brandtbucher November 8, 2024 16:38

brandtbucher approved these changes Nov 9, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Nov 9, 2024

Address Brandt's feedback

138525d

diegorusso requested a review from brandtbucher November 9, 2024 09:28

LeoniClemente mentioned this pull request Nov 11, 2024

<spam> #126656

Closed

Address feedback

7974378

brandtbucher approved these changes Nov 12, 2024

View reviewed changes

brandtbucher merged commit c45be8a into python:main Nov 12, 2024
59 checks passed

bedevere-app bot removed the awaiting merge label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

diegorusso commented Oct 30, 2024 •

edited by bedevere-app bot

Loading

mdboom commented Oct 30, 2024

Fidget-Spinner commented Oct 31, 2024

mdboom commented Oct 31, 2024

mdboom commented Oct 31, 2024

Eclips4 commented Nov 2, 2024 •

edited

Loading

diegorusso commented Nov 4, 2024

brandtbucher commented Nov 7, 2024

brandtbucher left a comment

bedevere-app bot commented Nov 7, 2024

brandtbucher commented Nov 7, 2024

brandtbucher left a comment

brandtbucher Nov 9, 2024

diegorusso Nov 9, 2024 •

edited

Loading

brandtbucher Nov 10, 2024

diegorusso Nov 11, 2024

brandtbucher left a comment

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

Conversation

diegorusso commented Oct 30, 2024 • edited by bedevere-app bot Loading

mdboom commented Oct 30, 2024

Fidget-Spinner commented Oct 31, 2024

mdboom commented Oct 31, 2024

mdboom commented Oct 31, 2024

Eclips4 commented Nov 2, 2024 • edited Loading

diegorusso commented Nov 4, 2024

brandtbucher commented Nov 7, 2024

brandtbucher left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Nov 7, 2024

brandtbucher commented Nov 7, 2024

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher Nov 9, 2024

Choose a reason for hiding this comment

diegorusso Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

brandtbucher Nov 10, 2024

Choose a reason for hiding this comment

diegorusso Nov 11, 2024

Choose a reason for hiding this comment

brandtbucher left a comment

Choose a reason for hiding this comment

diegorusso commented Oct 30, 2024 •

edited by bedevere-app bot

Loading

Eclips4 commented Nov 2, 2024 •

edited

Loading

diegorusso Nov 9, 2024 •

edited

Loading