[wasm] stop using mmap/munmap #108512

pavelsavara · 2024-10-03T10:55:16Z

Contributes to #108510
Fixes #107215

Context

sgen GC is using mono_valloc -> mmap on all other platforms.
mmap emulator in emscripten and wasi-libc doens't implement partial munmap
- It's implemented on top of malloc rather than on top of sbrk.
- It has large alignment overhead and no added value.
We have our own implementation of custom mmap
- which can do partial munmap
- but it never releases pages back to malloc pool.
wasm could only grow, we can't return pages to OS/browser
wasm32 adress space is rather small and could get fragmented easily

Changes

set MS_BLOCK_ALLOC_NUM to 1 which prevents sgen from doing partial mono_vfree
implement mono_vfree using dlfree
implement mono_valloc_aligned and mono_valloc using posix_memalign
- emscripten sys_alloc implementation will stitch last segment to new memory allocated via sbrk
avoids emscripten mmap which always calls memset to zero the memory
- this PR brings optimization that new pages which are received from sbrk don't need that.
- also some calls to mono_valloc pass MONO_MMAP_NOZERO
removes mono-wasm-pagemgr.c and wasm-mmap option

TODO

add unit test
measure startup performance
try to fix LOS alignment overhead between 64K and 1MB
fix soft-heap-limit to understand dlmalloc overhead (vs zero overhead of mmap on other systems)

kg · 2024-10-08T16:18:47Z

src/mono/mono/sgen/sgen-marksweep.c

+#else
+// WASM doesn't support partial munmap, so we can't free individual blocks
+// we use posix_memalign and free the whole block at once
+#define MS_BLOCK_ALLOC_NUM	1


we should investigate whether this will meaningfully regress performance; mwpm caused a benchmark regression that was fairly sizable

kg · 2024-10-08T16:19:18Z

src/mono/mono/utils/lock-free-alloc.h

@@ -45,11 +45,7 @@ typedef struct {
 	MonoMemAccountType account_type;
 } MonoLockFreeAllocator;

-#ifdef HOST_WASM
-#define LOCK_FREE_ALLOC_SB_MAX_SIZE					(mono_opt_wasm_mmap ? 65536 : 16384)
-#else
 #define LOCK_FREE_ALLOC_SB_MAX_SIZE					16384


we need a solution for the 16 vs 64k problem here since the system page size is still 64kb right now

does sbrk() page size matter in any way ? Why ?

i think it might be fine with the combination of all the changes you made, but mono_pagesize not agreeing with other sources of page size information feels like a hazard down the road

what would be the other sources which could conflict ?

kg · 2024-10-08T16:19:34Z

src/mono/mono/utils/mono-mmap-wasm.c

-#endif
-
-	return saved_pagesize;
+	return 16384;


we should make sure it's actually okay to lie here

this will improve the state of things by making pagesize not > the max block size, but the design of sgen is optimized for a base page size in the 4-8K range so that it can start with smaller blocks and then step up to larger blocks. i think with a page size of 16K it will always allocate only large blocks, and that might be bad? i'm not sure.

We can change it down to 4K, I think

The natural wasm page size of 64k is spoiled by the fact there is dlmalloc header around it anyway.
So I think we could choose arbitrary page size and 16K is max on other platforms for sgen.

Right @BrzVlad ?

@kg not sure if you are talking about other blocks. GC major blocks (for small object allocation) are at least 16k.

I think 16k page size is just fine.

I'm referring to the comments in the lock free allocator, which is one of the places that was consuming the page size and interacting with mmap. I guess I was under the mistaken assumption that we used the lock free allocator in more places than we do.

src/mono/mono/utils/mono-mmap-wasm.c

pavelsavara · 2024-10-09T07:20:41Z

@radekdoulik could you please run startup benchmark on this PR ? Many thanks

BrzVlad · 2024-10-09T11:41:15Z

src/mono/mono/utils/mono-mmap-wasm.c

+	if ((flags & MONO_MMAP_NOZERO) == 0) {
+		memset (res, 0, size);
+	}
+#endif


this no longer does mono_account_mem tracking of allocated memory

BrzVlad · 2024-10-09T11:41:55Z

src/mono/mono/utils/mono-mmap-wasm.c

-
-	mono_account_mem (type, -(ssize_t)length);
-
+	// NOTE: this doesn't implement partial freeing like munmap does


as above, missing tracking of allocated memory.

pavelsavara added arch-wasm WebAssembly architecture area-GC-mono os-browser Browser variant of arch-wasm labels Oct 3, 2024

pavelsavara added this to the 10.0.0 milestone Oct 3, 2024

pavelsavara self-assigned this Oct 3, 2024

wip

779fbac

pavelsavara force-pushed the browser_page_leak_repro branch from 0d68ed9 to 779fbac Compare October 8, 2024 16:03

fix

9a2da84

kg reviewed Oct 8, 2024

View reviewed changes

src/mono/mono/utils/mono-mmap-wasm.c Show resolved Hide resolved

kg reviewed Oct 8, 2024

View reviewed changes

src/mono/mono/utils/mono-mmap-wasm.c Show resolved Hide resolved

kg reviewed Oct 8, 2024

View reviewed changes

src/mono/mono/utils/mono-mmap-wasm.c Show resolved Hide resolved

kg reviewed Oct 8, 2024

View reviewed changes

src/mono/mono/utils/mono-mmap-wasm.c Show resolved Hide resolved

more toxic test case

635c936

pavelsavara changed the title ~~[browser] memory leak repro~~ [wasm] stop using mmap/munmap Oct 8, 2024

pavelsavara marked this pull request as ready for review October 9, 2024 07:33

pavelsavara requested review from lewing, lambdageek, steveisok and BrzVlad as code owners October 9, 2024 07:33

This was referenced Oct 9, 2024

[wasm][mt] fail: System.TypeLoadException: Could not load type of field" #102615

Open

WasmTestOnChrome-MT-System.Runtime.InteropServices.JavaScript.Tests CI timeouts #104203

Open

build-analysis bot mentioned this pull request Oct 9, 2024

[browser][MT] System.Runtime.InteropServices.JavaScript.Tests CI timeouts #104492

Open

BrzVlad reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] stop using mmap/munmap #108512

[wasm] stop using mmap/munmap #108512

pavelsavara commented Oct 3, 2024 •

edited

Loading

kg Oct 8, 2024

kg Oct 8, 2024

pavelsavara Oct 8, 2024

kg Oct 8, 2024

pavelsavara Oct 8, 2024

kg Oct 8, 2024

kg Oct 8, 2024

pavelsavara Oct 8, 2024

pavelsavara Oct 9, 2024 •

edited

Loading

BrzVlad Oct 9, 2024

kg Oct 9, 2024

pavelsavara commented Oct 9, 2024

BrzVlad Oct 9, 2024

BrzVlad Oct 9, 2024


		mono_account_mem (type, -(ssize_t)length);

		// NOTE: this doesn't implement partial freeing like munmap does

[wasm] stop using mmap/munmap #108512

Are you sure you want to change the base?

[wasm] stop using mmap/munmap #108512

Conversation

pavelsavara commented Oct 3, 2024 • edited Loading

Context

Changes

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelsavara Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelsavara commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelsavara commented Oct 3, 2024 •

edited

Loading

pavelsavara Oct 9, 2024 •

edited

Loading