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

emmalloc wastes memory with high alignments #20645

Closed
kripken opened this issue Nov 7, 2023 · 7 comments
Closed

emmalloc wastes memory with high alignments #20645

kripken opened this issue Nov 7, 2023 · 7 comments

Comments

@kripken
Copy link
Member

kripken commented Nov 7, 2023

#include <emscripten/emmalloc.h>
#include <stdio.h>

int main() {
  // Allocate 32MB with 1MB alignment.
  printf("%p\n", emmalloc_memalign(1024 * 1024, 32 * 1024 * 1024));
}

Build with

./emcc a.c -sMALLOC=emmalloc -sWASM=0 -sINITIAL_MEMORY=128mb --profiling

and then edit a.out.js to add some logging at the top of function sbrk:

 function sbrk($0) {
  console.log('sbrk', $0);

Running the program then shows

sbrk 48
sbrk 33554480
sbrk 33554480
0x100000

The initial sbrk is for emmalloc's internal data. Then we sbrk around 32MB, but then do so a second time for a total of 64MB.

This seems related to the high alignment, 1MB, as this does not happen without it.

This affects mimalloc when layered on emmalloc, since mimalloc does such large allocations with high alignment.

kripken added a commit that referenced this issue Nov 16, 2023
…20651)

The new allocator can be used with -sMALLOC=mimalloc.

On the benchmark added in this PR, dlmalloc does quite poorly here (getting
actually slower with each additional core, because the lock contention is much
larger than the actual work in the artificial benchmark). mimalloc, in
comparison, scales the same as natively: more cores keeps helping. So mimalloc
can be a significant speedup in codebases that have lock contention on malloc.

mimalloc is significantly larger than dlmalloc, however, so we do not want it
on by default. It also uses more memory, because of how mimalloc works and also
due to #20645.

Design-wise, this layers mimalloc on top of emmalloc. emmalloc functions as the
"system allocator", which is more powerful than just using raw sbrk - sbrk can't
free holes in the middle, for example.

Code-wise, all of system/lib/mimalloc is unchanged from upstream (see
README.emscripten) except for an ifdef or two, and then the new backend which
is in system/lib/mimalloc/src/prim/emscripten/prim.c. That file has more
comments explaining the design of the port.

A new test is added which is also usable as a benchmark,
test/other/test_mimalloc.cpp, which is where the numbers above come from.

Fixes #18369
kripken added a commit that referenced this issue Nov 22, 2023
Take the alignment into account when requesting new memory from the OS. If the
alignment is high then we need to ask for enough to ensure we end up aligned, or
else we can end up allocating double the memory we need (see #20645).

This only changes the amount we allocate from the OS if the alignment is non-
standard, that is, this is NFC for normal calls to malloc.

Also remove an assertion that limited the maximum alignment. mimalloc wants
4 MB alignment.

Fixes #20654
br0nk pushed a commit to br0nk/emscripten that referenced this issue Nov 29, 2023
Take the alignment into account when requesting new memory from the OS. If the
alignment is high then we need to ask for enough to ensure we end up aligned, or
else we can end up allocating double the memory we need (see emscripten-core#20645).

This only changes the amount we allocate from the OS if the alignment is non-
standard, that is, this is NFC for normal calls to malloc.

Also remove an assertion that limited the maximum alignment. mimalloc wants
4 MB alignment.

Fixes emscripten-core#20654
@englercj
Copy link

englercj commented Jan 15, 2024

Was this fixed by #20704?

This comment is still in main so I wasn't entirely sure.

@kripken
Copy link
Member Author

kripken commented Jan 16, 2024

Good catch, thanks @englercj ! Yes, this is basically fixed by that PR, aside from that one comment you noticed (unfortunately I don't think that remaining issue is fixable, as I think that high alignment is a design constraint of mimalloc).

In fact it looks like I meant to close this by that PR, by had a typo in the issue number to be closed by it 😄 (20645/20654...)

@kripken kripken closed this as completed Jan 16, 2024
@englercj
Copy link

Awesome, thanks for the info!

@kleisauke
Copy link
Collaborator

This comment is still in main so I wasn't entirely sure.

It looks like MI_SEGMENT_ALIGN will affect this. I was able to halve that alignment by reducing the page size from 32KiB to 16KiB on wasm32, see for example:
kleisauke@20c3153
microsoft/mimalloc#647 (comment)

(that commit also seems to fix a suspicious OOM error on wasm-vips' benchmarks when switching the memory allocator from dlmalloc to mimalloc)

@kripken
Copy link
Member Author

kripken commented Feb 26, 2024

Interesting, thanks @kleisauke ! Good find.

Do you have a sense of the tradeoffs there? I wonder if even smaller might make sense...

@kleisauke
Copy link
Collaborator

I'm not sure if there are any trade-offs, but it feels a bit dangerous to change these definitions.

Upstream mentions that the dev-slice branch has improved this, so perhaps we could sync with that. I just opened (draft) PR #21548 for this.

@kleisauke
Copy link
Collaborator

This comment is still in main so I wasn't entirely sure.

It looks like MI_SEGMENT_ALIGN will affect this. I was able to halve that alignment by reducing the page size from 32KiB to 16KiB on wasm32, see for example:
kleisauke@20c3153
microsoft/mimalloc#647 (comment)

FWIW, I just opened PR #23037 for this.

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

No branches or pull requests

3 participants