Skip to content
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

Jump to bottom

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+13 −1
Open

gh-126195: Use pthread_jit_write_protect_np on macOS #126196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e090bf
gh-126195: Use pthread_jit_write_protect_np on macOS
diegorusso Apr 23, 2024
a2e5f6c
📜🤖 Added by blurb_it.
blurb-it[bot] Oct 30, 2024
363a96c
Merge branch 'main' into pthread_jit_write_protect_np
Eclips4 Nov 2, 2024
4e5a0b5
Merge branch 'main' into pthread_jit_write_protect_np
diegorusso Nov 4, 2024
3d384fa
Merge branch 'main' into pthread_jit_write_protect_np
brandtbucher Nov 7, 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.
13 changes: 12 additions & 1 deletion Python/jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,16 @@ jit_alloc(size_t size)
int flags = MEM_COMMIT | MEM_RESERVE;
unsigned char *memory = VirtualAlloc(NULL, size, flags, PAGE_READWRITE);
int failed = memory == NULL;
#elif defined(__APPLE__) && defined(__aarch64__)
Copy link
Member

@brandtbucher brandtbucher Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does this work? If so, it seems a bit less fragile.

Suggested change
#elif defined(__APPLE__) && defined(__aarch64__)
#elif defined(MAP_JIT)

I'd also like to reduce the duplication of code here. So maybe we can just add the flags in this case and use the same code path as before:

#ifdef MAP_JIT
    flags |= MAP_JIT;
    prot |= PROT_EXEC;
#endif

int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_JIT;
int prot = PROT_READ | PROT_WRITE | PROT_EXEC;
unsigned char *memory = mmap(NULL, size, prot, flags, -1, 0);
int failed = memory == MAP_FAILED;
pthread_jit_write_protect_np(0);
Copy link
Member

Choose a reason for hiding this comment

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

This will leave the memory protection off for the entire thread if mapping the memory fails.

Even though pthread_jit_write_protect_np(0) is logically part of jit_alloc, and pthread_jit_write_protect_np(1) is logically part of mark_executable, I feel like it will be less error-prone and easier to reason about error paths if we make these calls in _PyJIT_Compile instead. It might also make porting to the new callback API easier, since the callback will just be all of the code between the two pthread_jit_write_protect_np calls.

What do you think?

#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;
unsigned char *memory = mmap(NULL, size, prot, flags, -1, 0);
int failed = memory == MAP_FAILED;
#endif
if (failed) {
Expand Down Expand Up @@ -101,6 +108,10 @@ mark_executable(unsigned char *memory, size_t size)
}
int old;
int failed = !VirtualProtect(memory, size, PAGE_EXECUTE_READ, &old);
#elif defined(__APPLE__) && defined(__aarch64__)
int failed = 0;
__builtin___clear_cache((char *)memory, (char *)memory + size);
pthread_jit_write_protect_np(1);
#else
__builtin___clear_cache((char *)memory, (char *)memory + size);
int failed = mprotect(memory, size, PROT_EXEC | PROT_READ);
Expand Down
Loading