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.

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

fix: use vm_allocate instead of posix_memalign for Metal on macOS #7078

Merged
merged 5 commits into from
May 8, 2024

Conversation

giladgd
Copy link
Contributor

@giladgd giladgd commented May 4, 2024

Creating a context in an Electron app using node-llama-cpp crashes the process with some models (issue), so I've investigated what's happening and found that allocating a large memory block using posix_memalign is the culprit.

For some reason, it happens only on Electron and not on Nodejs, but I couldn't figure out why.

From my testings in Electron:

  • posix_memalign((void **) &data, 16384, 587218944) - works fine
  • posix_memalign((void **) &data, 16384, 1073741824) - crashes the process with SIGTRAP

Tested on an M1 Max machine with 32GB of RAM

I tried switching from posix_memalign to malloc in ggml-metal.m, and it seems that everything still works correctly, but maybe I'm missing something.
I assume that posix_memalign is used there for a reason, but since it seems to me that everything still works great with malloc, maybe the original reason for using posix_memalign is irrelevant by now?

I'm not sure whether the change I made in this PR is a good idea, so I opened it so someone more knowledgable in this area can take a look.

This may be a bug specific to Electron, so I shared my findings on the Electron repo, but since I haven't noticed any side effect of this workaround in llama.cpp, I think it may be a good idea to also solve this issue here.

@slaren
Copy link
Member

slaren commented May 4, 2024

Metal requires a page-aligned pointer, which is why posix_memalign is used.

@giladgd
Copy link
Contributor Author

giladgd commented May 4, 2024

I've switched to use vm_allocate instead since I found that this is what the Apple documentation recommends, and it seems to also fix the issue with Electron.

vm_allocate also allocates page-aligned memory.
Is the new change I made ok?

@giladgd giladgd changed the title fix: workaround to not crash when running in Electron fix: use vm_allocate instead of posix_memalign for Metal on macOS May 5, 2024
@giladgd
Copy link
Contributor Author

giladgd commented May 7, 2024

I've been using this for the past few days, and it seems to work great.
I've seen that all the tests passed, so I think this may be a good solution to the Electron issue without affecting other things.

@ggerganov ggerganov merged commit 26458af into ggml-org:master May 8, 2024
58 checks passed
@giladgd giladgd deleted the metalPosixMemalignWorkaround branch May 8, 2024 20:19
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.

3 participants