Skip to content

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

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.

Sign up for GitHub

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 5 commits into from
Nov 12, 2024
Merged

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71cef5e
gh-126195: Use pthread_jit_write_protect_np on macOS
diegorusso Apr 23, 2024
24719d7
📜🤖 Added by blurb_it.
blurb-it[bot] Oct 30, 2024
507e662
Address Brandt feedback
diegorusso Nov 8, 2024
138525d
Address Brandt's feedback
diegorusso Nov 9, 2024
7974378
Address feedback
diegorusso Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2024-10-30-18-16-10.gh-issue-126195.6ezBpr.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve JIT performance by 1.4% on macOS Apple Silicon. Patch by Diego Russo.
21 changes: 18 additions & 3 deletions Python/jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,12 @@ jit_alloc(size_t size)
int failed = memory == NULL;
#else
int flags = MAP_ANONYMOUS | MAP_PRIVATE;
unsigned char *memory = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -1, 0);
int prot = PROT_READ | PROT_WRITE;
# ifdef MAP_JIT
flags |= MAP_JIT;
prot |= PROT_EXEC;
# endif
unsigned char *memory = mmap(NULL, size, prot, flags, -1, 0);
int failed = memory == MAP_FAILED;
#endif
if (failed) {
Expand Down Expand Up @@ -102,8 +107,11 @@ mark_executable(unsigned char *memory, size_t size)
int old;
int failed = !VirtualProtect(memory, size, PAGE_EXECUTE_READ, &old);
#else
int failed = 0;
__builtin___clear_cache((char *)memory, (char *)memory + size);
int failed = mprotect(memory, size, PROT_EXEC | PROT_READ);
# ifndef MAP_JIT
failed = mprotect(memory, size, PROT_EXEC | PROT_READ);
# endif
#endif
if (failed) {
jit_error("unable to protect executable memory");
Expand Down Expand Up @@ -499,6 +507,9 @@ _PyJIT_Compile(_PyExecutorObject *executor, const _PyUOpInstruction trace[], siz
if (memory == NULL) {
return -1;
}
#ifdef MAP_JIT
pthread_jit_write_protect_np(0);
#endif
// Update the offsets of each instruction:
for (size_t i = 0; i < length; i++) {
state.instruction_starts[i] += (uintptr_t)memory;
Expand Down Expand Up @@ -529,7 +540,11 @@ _PyJIT_Compile(_PyExecutorObject *executor, const _PyUOpInstruction trace[], siz
data += group->data_size;
assert(code == memory + code_size);
assert(data == memory + code_size + data_size);
if (mark_executable(memory, total_size)) {
int status = mark_executable(memory, total_size);
#ifdef MAP_JIT
pthread_jit_write_protect_np(1);
#endif
if (status) {
Copy link
Member

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:

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)) {

Copy link
Contributor Author

@diegorusso diegorusso Nov 9, 2024 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

jit_free(memory, total_size);
return -1;
}
Expand Down
Loading