From 73b1edab194efed1b1ab771ca1db18bb53c0ddc0 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 13 Nov 2023 15:57:43 -0800 Subject: [PATCH 1/9] fix --- system/lib/emmalloc.c | 7 ++++++ test/other/test_emmalloc_high_align.c | 32 +++++++++++++++++++++++++ test/other/test_emmalloc_high_align.out | 1 + test/test_other.py | 4 ++++ 4 files changed, 44 insertions(+) create mode 100644 test/other/test_emmalloc_high_align.c create mode 100644 test/other/test_emmalloc_high_align.out diff --git a/system/lib/emmalloc.c b/system/lib/emmalloc.c index 835dfe95cbc00..f067a117c592d 100644 --- a/system/lib/emmalloc.c +++ b/system/lib/emmalloc.c @@ -791,6 +791,13 @@ static void *allocate_memory(size_t alignment, size_t size) // We were unable to find a free memory region. Must sbrk() in more memory! size_t numBytesToClaim = size+sizeof(Region)*3; + // Take into account the alignment as well. For typical alignment we don't + // need to add anything here (so we do nothing if the alignment is equal to + // MALLOC_ALIGNMENT), but it can matter if the alignment is very high. In that + // case, not adding the alignment can lead to this sbrk not giving us enough + // (in which case, the next attempt fails and will sbrk the same amount again, + // potentially allocating a lot more memory than necessary). + numBytesToClaim += alignment - MALLOC_ALIGNMENT; assert(numBytesToClaim > size); // 32-bit wraparound should not happen here, allocation size has been validated above! bool success = claim_more_memory(numBytesToClaim); if (success) diff --git a/test/other/test_emmalloc_high_align.c b/test/other/test_emmalloc_high_align.c new file mode 100644 index 0000000000000..40cd0e26d35f0 --- /dev/null +++ b/test/other/test_emmalloc_high_align.c @@ -0,0 +1,32 @@ +#include +#include +#include +#include + +size_t sizeT(void* p) { + return (size_t)p; +} + +int main() { + const size_t MB = 1024 * 1024; + const size_t ALIGN = 4 * MB; + const size_t SIZE = 32 * MB; + + // Allocate a very large chunk of memory (32MB) with very high alignment (4 + // MB). This is similar to what mimalloc does in practice. + void* before = sbrk(0); + void* p = aligned_alloc(ALIGN, SIZE); + void* after = sbrk(0); + emscripten_console_logf("before: %p after: %p p: %p\n", before, after, p); + + // The allocation must be properly aligned. + assert(sizeT(p) % ALIGN == 0); + + // We should only have sbrk'd a reasonable amount (this is a regression test + // for #20645 where we sbrk'd double the necessary amount). We expect at most + // 36 MB (the size of the allocation plus the alignment) plus a few bytes of + // general overhead. + assert(sizeT(after) - sizeT(before) < ALIGN + SIZE + 100); + + emscripten_console_log("success"); +} diff --git a/test/other/test_emmalloc_high_align.out b/test/other/test_emmalloc_high_align.out new file mode 100644 index 0000000000000..2e9ba477f89e8 --- /dev/null +++ b/test/other/test_emmalloc_high_align.out @@ -0,0 +1 @@ +success diff --git a/test/test_other.py b/test/test_other.py index 99b5df6235333..bc531c4009224 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -7466,6 +7466,10 @@ def test(args, text=None): test(['-sALLOW_MEMORY_GROWTH', '-sMAXIMUM_MEMORY=1GB']) test(['-sALLOW_MEMORY_GROWTH', '-sMAXIMUM_MEMORY=4GB']) + def test_emmalloc_high_align(self): + self.do_other_test('test_emmalloc_high_align.c', + emcc_args=['-sMALLOC=emmalloc', '-sINITIAL_MEMORY=128MB']) + def test_2GB_plus(self): # when the heap size can be over 2GB, we rewrite pointers to be unsigned def test(page_diff): From cf5cb0984653c4af5a25a2d404b07cc520e7eef1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 13 Nov 2023 16:01:31 -0800 Subject: [PATCH 2/9] fix --- system/lib/emmalloc.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/system/lib/emmalloc.c b/system/lib/emmalloc.c index f067a117c592d..ebd775d51155a 100644 --- a/system/lib/emmalloc.c +++ b/system/lib/emmalloc.c @@ -657,8 +657,6 @@ static size_t validate_alloc_alignment(size_t alignment) // Cannot perform allocations that are less than 4 byte aligned, because the Region // control structures need to be aligned. Also round up to minimum outputted alignment. alignment = MAX(alignment, MALLOC_ALIGNMENT); - // Arbitrary upper limit on alignment - very likely a programming bug if alignment is higher than this. - assert(alignment <= 1024*1024); return alignment; } From 87b24d75338bc80796325f5267c25d41dc90d39c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 13 Nov 2023 16:30:31 -0800 Subject: [PATCH 3/9] code size improvements --- test/code_size/embind_val_wasm.json | 8 ++++---- test/code_size/hello_wasm_worker_wasm.json | 4 ++-- test/code_size/hello_webgl2_wasm.json | 8 ++++---- test/code_size/hello_webgl2_wasm2js.json | 8 ++++---- test/code_size/hello_webgl_wasm.json | 8 ++++---- test/code_size/hello_webgl_wasm2js.json | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/test/code_size/embind_val_wasm.json b/test/code_size/embind_val_wasm.json index 7a277ef441cf2..6f088e8874981 100644 --- a/test/code_size/embind_val_wasm.json +++ b/test/code_size/embind_val_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 431, "a.js": 7498, "a.js.gz": 3142, - "a.wasm": 11450, - "a.wasm.gz": 5732, - "total": 19621, - "total_gz": 9305 + "a.wasm": 11425, + "a.wasm.gz": 5725, + "total": 19596, + "total_gz": 9298 } diff --git a/test/code_size/hello_wasm_worker_wasm.json b/test/code_size/hello_wasm_worker_wasm.json index a5fa7e4de79d2..ed17e3cb20e89 100644 --- a/test/code_size/hello_wasm_worker_wasm.json +++ b/test/code_size/hello_wasm_worker_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 433, "a.js": 667, "a.js.gz": 458, - "a.wasm": 1855, + "a.wasm": 1843, "a.wasm.gz": 1049, - "total": 3259, + "total": 3247, "total_gz": 1940 } diff --git a/test/code_size/hello_webgl2_wasm.json b/test/code_size/hello_webgl2_wasm.json index cd1ffb3fde123..e2f97d7ee09ef 100644 --- a/test/code_size/hello_webgl2_wasm.json +++ b/test/code_size/hello_webgl2_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 379, "a.js": 4699, "a.js.gz": 2419, - "a.wasm": 10467, - "a.wasm.gz": 6706, - "total": 15735, - "total_gz": 9504 + "a.wasm": 10453, + "a.wasm.gz": 6708, + "total": 15721, + "total_gz": 9506 } diff --git a/test/code_size/hello_webgl2_wasm2js.json b/test/code_size/hello_webgl2_wasm2js.json index 7ce0e009131a5..f38d591c4da88 100644 --- a/test/code_size/hello_webgl2_wasm2js.json +++ b/test/code_size/hello_webgl2_wasm2js.json @@ -1,10 +1,10 @@ { "a.html": 567, "a.html.gz": 379, - "a.js": 17912, - "a.js.gz": 8067, + "a.js": 17820, + "a.js.gz": 8058, "a.mem": 3171, "a.mem.gz": 2713, - "total": 21650, - "total_gz": 11159 + "total": 21558, + "total_gz": 11150 } diff --git a/test/code_size/hello_webgl_wasm.json b/test/code_size/hello_webgl_wasm.json index 6f3b878fb0c96..90e650a79a128 100644 --- a/test/code_size/hello_webgl_wasm.json +++ b/test/code_size/hello_webgl_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 379, "a.js": 4185, "a.js.gz": 2243, - "a.wasm": 10467, - "a.wasm.gz": 6706, - "total": 15221, - "total_gz": 9328 + "a.wasm": 10453, + "a.wasm.gz": 6708, + "total": 15207, + "total_gz": 9330 } diff --git a/test/code_size/hello_webgl_wasm2js.json b/test/code_size/hello_webgl_wasm2js.json index add71d6b43685..0faa5114ab3ee 100644 --- a/test/code_size/hello_webgl_wasm2js.json +++ b/test/code_size/hello_webgl_wasm2js.json @@ -1,10 +1,10 @@ { "a.html": 567, "a.html.gz": 379, - "a.js": 17390, - "a.js.gz": 7895, + "a.js": 17298, + "a.js.gz": 7874, "a.mem": 3171, "a.mem.gz": 2713, - "total": 21128, - "total_gz": 10987 + "total": 21036, + "total_gz": 10966 } From dfa2019e26965473c17dcd92e1f37f424d3e936d Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 13 Nov 2023 16:38:25 -0800 Subject: [PATCH 4/9] simplify --- system/lib/emmalloc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/system/lib/emmalloc.c b/system/lib/emmalloc.c index 658aad8e2bca9..2bcf4e14f883f 100644 --- a/system/lib/emmalloc.c +++ b/system/lib/emmalloc.c @@ -656,8 +656,7 @@ static size_t validate_alloc_alignment(size_t alignment) { // Cannot perform allocations that are less than 4 byte aligned, because the Region // control structures need to be aligned. Also round up to minimum outputted alignment. - alignment = MAX(alignment, MALLOC_ALIGNMENT); - return alignment; + return MAX(alignment, MALLOC_ALIGNMENT); } static size_t validate_alloc_size(size_t size) From a6c39ad956e8dc57bace2f6c25b1936863d35c70 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 14 Nov 2023 09:17:21 -0800 Subject: [PATCH 5/9] feedback --- system/lib/emmalloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/lib/emmalloc.c b/system/lib/emmalloc.c index 2bcf4e14f883f..342193dc9ebf7 100644 --- a/system/lib/emmalloc.c +++ b/system/lib/emmalloc.c @@ -654,8 +654,8 @@ static void *attempt_allocate(Region *freeRegion, size_t alignment, size_t size) static size_t validate_alloc_alignment(size_t alignment) { - // Cannot perform allocations that are less than 4 byte aligned, because the Region - // control structures need to be aligned. Also round up to minimum outputted alignment. + // Cannot perform allocations that are less our minimal alignment, because + // the Region control structures need to be aligned themselves. return MAX(alignment, MALLOC_ALIGNMENT); } From c6aa7dcaa9a6c32e5ef6c6e4e9c85304e5078594 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 14 Nov 2023 09:29:32 -0800 Subject: [PATCH 6/9] feedback --- system/lib/emmalloc.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/system/lib/emmalloc.c b/system/lib/emmalloc.c index 342193dc9ebf7..55c931577a1b2 100644 --- a/system/lib/emmalloc.c +++ b/system/lib/emmalloc.c @@ -794,7 +794,16 @@ static void *allocate_memory(size_t alignment, size_t size) // case, not adding the alignment can lead to this sbrk not giving us enough // (in which case, the next attempt fails and will sbrk the same amount again, // potentially allocating a lot more memory than necessary). - numBytesToClaim += alignment - MALLOC_ALIGNMENT; + // + // Note that this is not necessarily optimal, as the extra allocation size for + // the alignment might not be needed (if we are lucky and already aligned), + // and even if it helps us allocate it will not immediately be ready for reuse + // (as it will be added to the currently-in-use region before us, so it is not + // in a free list). As a compromise however it seems reasonable in practice as + // a way to handle large aligned regions to avoid even worse waste. + if (alignment > MALLOC_ALIGNMENT) { + numBytesToClaim += MALLOC_ALIGNMENT; + } assert(numBytesToClaim > size); // 32-bit wraparound should not happen here, allocation size has been validated above! bool success = claim_more_memory(numBytesToClaim); if (success) From c295958130a306af7e92bf6cf3bd9bb0b8ee076c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 20 Nov 2023 13:49:12 -0800 Subject: [PATCH 7/9] oops --- system/lib/emmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/lib/emmalloc.c b/system/lib/emmalloc.c index 55c931577a1b2..6352771d1bec0 100644 --- a/system/lib/emmalloc.c +++ b/system/lib/emmalloc.c @@ -802,7 +802,7 @@ static void *allocate_memory(size_t alignment, size_t size) // in a free list). As a compromise however it seems reasonable in practice as // a way to handle large aligned regions to avoid even worse waste. if (alignment > MALLOC_ALIGNMENT) { - numBytesToClaim += MALLOC_ALIGNMENT; + numBytesToClaim += alignment; } assert(numBytesToClaim > size); // 32-bit wraparound should not happen here, allocation size has been validated above! bool success = claim_more_memory(numBytesToClaim); From 3c800ab29495d41efdbf6aa4ed1a64cdce5e6e7b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 20 Nov 2023 14:27:38 -0800 Subject: [PATCH 8/9] fix --- test/other/metadce/test_metadce_cxx_ctors1.size | 1 - test/other/metadce/test_metadce_cxx_except.size | 1 - test/other/metadce/test_metadce_cxx_except_wasm.size | 1 - test/other/metadce/test_metadce_cxx_mangle.size | 1 - test/other/metadce/test_metadce_cxx_noexcept.size | 1 - test/other/metadce/test_metadce_cxx_wasmfs.size | 1 - 6 files changed, 6 deletions(-) diff --git a/test/other/metadce/test_metadce_cxx_ctors1.size b/test/other/metadce/test_metadce_cxx_ctors1.size index 7619901675dae..078a8d5ed5a07 100644 --- a/test/other/metadce/test_metadce_cxx_ctors1.size +++ b/test/other/metadce/test_metadce_cxx_ctors1.size @@ -1,2 +1 @@ 125832 - diff --git a/test/other/metadce/test_metadce_cxx_except.size b/test/other/metadce/test_metadce_cxx_except.size index f132537fd9774..00a652b0c66e9 100644 --- a/test/other/metadce/test_metadce_cxx_except.size +++ b/test/other/metadce/test_metadce_cxx_except.size @@ -1,2 +1 @@ 168083 - diff --git a/test/other/metadce/test_metadce_cxx_except_wasm.size b/test/other/metadce/test_metadce_cxx_except_wasm.size index 3d110c7a457b9..f6a999fcb99f6 100644 --- a/test/other/metadce/test_metadce_cxx_except_wasm.size +++ b/test/other/metadce/test_metadce_cxx_except_wasm.size @@ -1,2 +1 @@ 139261 - diff --git a/test/other/metadce/test_metadce_cxx_mangle.size b/test/other/metadce/test_metadce_cxx_mangle.size index 642f6ece7055f..c44fe194b09e8 100644 --- a/test/other/metadce/test_metadce_cxx_mangle.size +++ b/test/other/metadce/test_metadce_cxx_mangle.size @@ -1,2 +1 @@ 223560 - diff --git a/test/other/metadce/test_metadce_cxx_noexcept.size b/test/other/metadce/test_metadce_cxx_noexcept.size index 478cdfb944b3d..fcd956bbabc87 100644 --- a/test/other/metadce/test_metadce_cxx_noexcept.size +++ b/test/other/metadce/test_metadce_cxx_noexcept.size @@ -1,2 +1 @@ 128620 - diff --git a/test/other/metadce/test_metadce_cxx_wasmfs.size b/test/other/metadce/test_metadce_cxx_wasmfs.size index b0a9ac77350ce..50d022fa5d85d 100644 --- a/test/other/metadce/test_metadce_cxx_wasmfs.size +++ b/test/other/metadce/test_metadce_cxx_wasmfs.size @@ -1,2 +1 @@ 165965 - From 1caf4c15591aee76ec1c23939e0653039978a342 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 20 Nov 2023 14:53:36 -0800 Subject: [PATCH 9/9] rebaseline --- test/code_size/embind_val_wasm.json | 4 ++-- test/code_size/hello_wasm_worker_wasm.json | 4 ++-- test/code_size/hello_webgl2_wasm.json | 8 ++++---- test/code_size/hello_webgl2_wasm2js.json | 8 ++++---- test/code_size/hello_webgl_wasm.json | 8 ++++---- test/code_size/hello_webgl_wasm2js.json | 8 ++++---- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/test/code_size/embind_val_wasm.json b/test/code_size/embind_val_wasm.json index 96aea45e73a12..1f5956272c1ee 100644 --- a/test/code_size/embind_val_wasm.json +++ b/test/code_size/embind_val_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 431, "a.js": 7498, "a.js.gz": 3142, - "a.wasm": 11504, + "a.wasm": 11507, "a.wasm.gz": 5770, - "total": 19675, + "total": 19678, "total_gz": 9343 } diff --git a/test/code_size/hello_wasm_worker_wasm.json b/test/code_size/hello_wasm_worker_wasm.json index 36ac2d90811ea..7e2050f9cc5a6 100644 --- a/test/code_size/hello_wasm_worker_wasm.json +++ b/test/code_size/hello_wasm_worker_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 433, "a.js": 667, "a.js.gz": 458, - "a.wasm": 1847, + "a.wasm": 1850, "a.wasm.gz": 1054, - "total": 3251, + "total": 3254, "total_gz": 1945 } diff --git a/test/code_size/hello_webgl2_wasm.json b/test/code_size/hello_webgl2_wasm.json index a7194ff0ac0c5..4c77a39c71593 100644 --- a/test/code_size/hello_webgl2_wasm.json +++ b/test/code_size/hello_webgl2_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 379, "a.js": 4699, "a.js.gz": 2419, - "a.wasm": 10457, - "a.wasm.gz": 6712, - "total": 15725, - "total_gz": 9510 + "a.wasm": 10460, + "a.wasm.gz": 6713, + "total": 15728, + "total_gz": 9511 } diff --git a/test/code_size/hello_webgl2_wasm2js.json b/test/code_size/hello_webgl2_wasm2js.json index 1205a9fd72d88..fde27e988661d 100644 --- a/test/code_size/hello_webgl2_wasm2js.json +++ b/test/code_size/hello_webgl2_wasm2js.json @@ -1,10 +1,10 @@ { "a.html": 567, "a.html.gz": 379, - "a.js": 17828, - "a.js.gz": 8065, + "a.js": 17832, + "a.js.gz": 8070, "a.mem": 3171, "a.mem.gz": 2713, - "total": 21566, - "total_gz": 11157 + "total": 21570, + "total_gz": 11162 } diff --git a/test/code_size/hello_webgl_wasm.json b/test/code_size/hello_webgl_wasm.json index c7f077f115ef1..9f946ef1375ef 100644 --- a/test/code_size/hello_webgl_wasm.json +++ b/test/code_size/hello_webgl_wasm.json @@ -3,8 +3,8 @@ "a.html.gz": 379, "a.js": 4185, "a.js.gz": 2243, - "a.wasm": 10457, - "a.wasm.gz": 6712, - "total": 15211, - "total_gz": 9334 + "a.wasm": 10460, + "a.wasm.gz": 6713, + "total": 15214, + "total_gz": 9335 } diff --git a/test/code_size/hello_webgl_wasm2js.json b/test/code_size/hello_webgl_wasm2js.json index 7cc0a36485d8b..d21fa07a9df15 100644 --- a/test/code_size/hello_webgl_wasm2js.json +++ b/test/code_size/hello_webgl_wasm2js.json @@ -1,10 +1,10 @@ { "a.html": 567, "a.html.gz": 379, - "a.js": 17306, - "a.js.gz": 7885, + "a.js": 17310, + "a.js.gz": 7891, "a.mem": 3171, "a.mem.gz": 2713, - "total": 21044, - "total_gz": 10977 + "total": 21048, + "total_gz": 10983 }