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

Port to WebAssembly #32

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Port to WebAssembly #32

merged 1 commit into from
Jul 15, 2019

Conversation

jedisct1
Copy link
Contributor

This makes mimalloc compatible with the WebAssembly Standard Interface.

I'm considering it as a dlmalloc replacement for wasi-libc, and have been successfully running it in this scenario.

@jedisct1 jedisct1 changed the base branch from master to dev June 24, 2019 16:27
@jedisct1
Copy link
Contributor Author

Rebased against the dev branch.

Copy link
Collaborator

@daanx daanx left a comment

Choose a reason for hiding this comment

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

This is awesome you got this port done so quickly! Yet another advantage of a small codebase.
Some remarks:

  • can you remove the whitespace differences is the pull request? (only if not too much work)
  • is arc4random_buf available by default in WASI? don't we need a header file?
  • in os.c, the best is to only modify the lowlevel mi_mmap and mi_munmap functions and not os_alloc and os_free; can you move your modifications there? If you feel unsure, I can also accept the pull request as is, and do it for you now that I see how it works. Just let me know.
    Thanks again!

@jedisct1
Copy link
Contributor Author

Hi!

And thanks for your feedback!

The whitespace diffs have been removed.

arc4random_buf() is defined in stdlib.h, along with malloc, etc. So it's already assumed to be available.

I'm a bit reluctant to adding the changes to mi_mmap() because it would be misleading. Addresses are very predictable, page protection doesn't exist. Code calling a function named mmap() can assume to get the mmap() semantics, whereas on some platforms, it wouldn't be the case at all.

This can be revisited once mmap() gets eventually implemented in WebAssembly. Protection can also be implemented without this, if the host provides a way to protect specific pages of the linear memory (something we may add support for in Lucet), but once again, the way to allocate memory doesn't really match the mmap() semantics. It's really the allocation that we do differently for WebAssembly.

@daanx daanx mentioned this pull request Jun 25, 2019
src/init.c Outdated Show resolved Hide resolved
src/init.c Outdated Show resolved Hide resolved
src/os.c Outdated Show resolved Hide resolved
@daanx
Copy link
Collaborator

daanx commented Jun 26, 2019

@jedisct1 : I see what you mean about mi_mmap.. but I never intended it to be like mmap :-) I will perhaps rename that. My plan is to pull this in and then to a small rewrite if that is ok with you -- I hope you can then try to compile it again to ensure it is working as intended.

Generally, I would like this code to be reused for any sbrk like platform. However, currently we never free any memory; I guess eventually we need some kind of mini-allocator for the segments -- i.e. allocate in 4MiB parts from sbrk and maintain a bitmap of freed parts so we can reuse them; or a buddy allocator perhaps? Not sure yet.

@jedisct1
Copy link
Contributor Author

Sounds good. I moved the WebAssembly-specific allocation to mi_mmap(). But yes, having that function renamed may not be a bad idea for clarity.

@santagada
Copy link

IIUC this implementation leaks memory everytime mimalloc decides to free a page? Seems weird that is the intended behaviour... maybe free should put the page on a freelist?

@daanx
Copy link
Collaborator

daanx commented Jul 4, 2019

@santagada: yes, that is why I haven't merged yet. I am working in the dev-exp branch on a solution that will do the right thing on sbrk/WASM like systems (and would also allow for allocating from a fixed area) (see src/memory.c). The same solution can potentially also enable NUMA aware allocation in the future.

@jedisct1 jedisct1 changed the base branch from dev to dev-exp July 10, 2019 18:25
@jedisct1
Copy link
Contributor Author

jedisct1 commented Jul 10, 2019

That PR to support WebAssembly was adjusted to the dev-exp branch.

Can you give it a new look?

@daanx
Copy link
Collaborator

daanx commented Jul 11, 2019

Ah great -- the dev-exp branch is created specifically for this use case. I have no way to test the wasm build but it seems to work for you right? I will look into the pr in more detail soon but wanted to say thanks already :-)

@jedisct1
Copy link
Contributor Author

Yes, it seems to work perfectly fine here. I'm using it in a branch of the wasi libc, with MI_SECURE turned on, and haven't seen any regressions.

@daanx daanx merged commit ad45dbf into microsoft:dev-exp Jul 15, 2019
@daanx
Copy link
Collaborator

daanx commented Jul 15, 2019

Awesome. Thanks for contributing this and let me know how it works for you.

@jedisct1
Copy link
Contributor Author

Thanks you!

errno = ENOMEM;
return NULL;
}
return (void*) aligned_base;

Choose a reason for hiding this comment

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

There is a subtle race condition here: between the __builtin_wasm_memory_size call and the __builtin_wasm_memory_grow call, other threads could do __builtin_wasm_memory_grow. We should return something based on the return value of __builtin_wasm_memory_grow.

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.

5 participants