gh-126195: Use pthread_jit_write_protect_np on macOS by diegorusso · Pull Request #126196 · python/cpython
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.
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.
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.
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.
Merging main to get rid of failures in test_embed
(failures on aarch64macos runners as you might know are expected thing 🙂 )
I've updated the branch to check if errors disappeared...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue with an error path, and a couple of suggestions.
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.
Replace mprotect with pthread_jit_write_protect_np on MacOS Apple Silicon. Improve JIT performance by ~1.4% on this platform.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a couple of cosmetic suggestions, then I'll go ahead and merge.
Thanks for tackling this!
Comment on lines +543 to +547
| int status = mark_executable(memory, total_size); | ||
| #ifdef MAP_JIT | ||
| pthread_jit_write_protect_np(1); | ||
| #endif | ||
| if (status) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work, right? Then we don't need status:
| 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)) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
