emmalloc: Fix allocations of high alignment (emscripten-core#20704)

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

system/lib/emmalloc.c

@@ -671,12 +671,9 @@ 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.
-  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;
+  // Cannot perform allocations that are less our minimal alignment, because
+  // the Region control structures need to be aligned themselves.
+  return MAX(alignment, MALLOC_ALIGNMENT);
 }
 
 static size_t validate_alloc_size(size_t size)
@@ -808,6 +805,22 @@ 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).
+  //
+  // 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 += 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)

test/code_size/embind_val_wasm.json

@@ -3,8 +3,8 @@
   "a.html.gz": 431,
   "a.js": 7498,
   "a.js.gz": 3142,
-  "a.wasm": 11533,
-  "a.wasm.gz": 5767,
-  "total": 19704,
-  "total_gz": 9340
+  "a.wasm": 11515,
+  "a.wasm.gz": 5771,
+  "total": 19686,
+  "total_gz": 9344
 }

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": 1863,
-  "a.wasm.gz": 1051,
-  "total": 3267,
-  "total_gz": 1942
+  "a.wasm": 1858,
+  "a.wasm.gz": 1058,
+  "total": 3262,
+  "total_gz": 1949
 }

test/code_size/hello_webgl2_wasm.json

@@ -3,8 +3,8 @@
   "a.html.gz": 379,
   "a.js": 4699,
   "a.js.gz": 2419,
-  "a.wasm": 10475,
-  "a.wasm.gz": 6710,
-  "total": 15743,
-  "total_gz": 9508
+  "a.wasm": 10468,
+  "a.wasm.gz": 6719,
+  "total": 15736,
+  "total_gz": 9517
 }

test/code_size/hello_webgl2_wasm2js.json

@@ -1,10 +1,10 @@
 {
   "a.html": 567,
   "a.html.gz": 379,
-  "a.js": 17921,
-  "a.js.gz": 8079,
+  "a.js": 17841,
+  "a.js.gz": 8088,
   "a.mem": 3171,
   "a.mem.gz": 2713,
-  "total": 21659,
-  "total_gz": 11171
+  "total": 21579,
+  "total_gz": 11180
 }

test/code_size/hello_webgl_wasm.json

@@ -3,8 +3,8 @@
   "a.html.gz": 379,
   "a.js": 4185,
   "a.js.gz": 2243,
-  "a.wasm": 10475,
-  "a.wasm.gz": 6710,
-  "total": 15229,
-  "total_gz": 9332
+  "a.wasm": 10468,
+  "a.wasm.gz": 6719,
+  "total": 15222,
+  "total_gz": 9341
 }

test/code_size/hello_webgl_wasm2js.json

@@ -1,10 +1,10 @@
 {
   "a.html": 567,
   "a.html.gz": 379,
-  "a.js": 17399,
-  "a.js.gz": 7910,
+  "a.js": 17319,
+  "a.js.gz": 7908,
   "a.mem": 3171,
   "a.mem.gz": 2713,
-  "total": 21137,
-  "total_gz": 11002
+  "total": 21057,
+  "total_gz": 11000
 }

test/other/test_emmalloc_high_align.c

@@ -0,0 +1,32 @@
+#include <assert.h>
+#include <emscripten/console.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+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");
+}

test/other/test_emmalloc_high_align.out

@@ -0,0 +1 @@
+success

test/test_other.py

@@ -7494,6 +7494,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):