Skip to content

Commit 326600a

Browse files
authored
emmalloc: Ask sbrk for aligned sizes (#20759)
If we give it an unaligned size, then it will align it, and then memory will seem non-contiguous. That is, if we sbrk 4 bytes from address 0 then it returns 0 (the start of the allocation), and logically we reserved the range 0-4. Our next sbrk of any amount will return 8, because sbrk aligns up the address to multiples of 8. So it seems like we have a region 0-4 and another 8-. Because of this emmalloc would generate separate root regions for them, that is, they would not get merged because of the 4 bytes of dead space in the middle. With this PR, the amount of new regions created in test_emmalloc_memvalidate_verbose goes from 1311 to 1234. I'm not sure if it's worth testing that, as it would make updating the test difficult, and this is such an internal detail of emmalloc. Also add some verbose logging that helps debug this. Noticed in #20704 (comment)
1 parent bbb2e36 commit 326600a

7 files changed

+41
-24
lines changed

system/lib/emmalloc.c

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,12 @@ static bool claim_more_memory(size_t numBytes)
486486
validate_memory_regions();
487487
#endif
488488

489+
// Make sure we send sbrk requests of aligned sizes. If we do not do that then
490+
// it will not return contiguous regions, which leads to use creating more
491+
// root regions below, inefficiently. (Note that we assume our alignment is
492+
// identical to sbrk's, see the assumptions at the start of this file.)
493+
numBytes = (size_t)ALIGN_UP(numBytes, MALLOC_ALIGNMENT);
494+
489495
// Claim memory via sbrk
490496
uint8_t *startPtr = (uint8_t*)sbrk(numBytes);
491497
if ((intptr_t)startPtr == -1)
@@ -510,6 +516,9 @@ static bool claim_more_memory(size_t numBytes)
510516
uint8_t *previousSbrkEndAddress = listOfAllRegions ? listOfAllRegions->endPtr : 0;
511517
if (startPtr == previousSbrkEndAddress)
512518
{
519+
#ifdef EMMALLOC_VERBOSE
520+
MAIN_THREAD_ASYNC_EM_ASM(err('claim_more_memory: expanding previous'));
521+
#endif
513522
Region *prevEndSentinel = prev_region((Region*)startPtr);
514523
assert(debug_region_is_consistent(prevEndSentinel));
515524
assert(region_is_in_use(prevEndSentinel));
@@ -536,6 +545,9 @@ static bool claim_more_memory(size_t numBytes)
536545
else
537546
{
538547
// Create a root region at the start of the heap block
548+
#ifdef EMMALLOC_VERBOSE
549+
MAIN_THREAD_ASYNC_EM_ASM(err('claim_more_memory: creating new root region'));
550+
#endif
539551
create_used_region(startPtr, sizeof(Region));
540552

541553
// Dynamic heap start region:
@@ -583,6 +595,11 @@ void emmalloc_blank_slate_from_orbit()
583595
static void *attempt_allocate(Region *freeRegion, size_t alignment, size_t size)
584596
{
585597
ASSERT_MALLOC_IS_ACQUIRED();
598+
599+
#ifdef EMMALLOC_VERBOSE
600+
MAIN_THREAD_ASYNC_EM_ASM(out('attempt_allocate(freeRegion=' + ptrToString($0) + ',alignment=' + Number($1) + ',size=' + Number($2) + ')'), freeRegion, alignment, size);
601+
#endif
602+
586603
assert(freeRegion);
587604
// Look at the next potential free region to allocate into.
588605
// First, we should check if the free region has enough of payload bytes contained

test/code_size/embind_val_wasm.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
"a.html.gz": 431,
44
"a.js": 7498,
55
"a.js.gz": 3142,
6-
"a.wasm": 11525,
7-
"a.wasm.gz": 5766,
8-
"total": 19696,
9-
"total_gz": 9339
6+
"a.wasm": 11533,
7+
"a.wasm.gz": 5767,
8+
"total": 19704,
9+
"total_gz": 9340
1010
}

test/code_size/hello_wasm_worker_wasm.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
"a.html.gz": 433,
44
"a.js": 667,
55
"a.js.gz": 458,
6-
"a.wasm": 1855,
7-
"a.wasm.gz": 1049,
8-
"total": 3259,
9-
"total_gz": 1940
6+
"a.wasm": 1863,
7+
"a.wasm.gz": 1051,
8+
"total": 3267,
9+
"total_gz": 1942
1010
}

test/code_size/hello_webgl2_wasm.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
"a.html.gz": 379,
44
"a.js": 4699,
55
"a.js.gz": 2419,
6-
"a.wasm": 10467,
7-
"a.wasm.gz": 6706,
8-
"total": 15735,
9-
"total_gz": 9504
6+
"a.wasm": 10475,
7+
"a.wasm.gz": 6710,
8+
"total": 15743,
9+
"total_gz": 9508
1010
}
Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"a.html": 567,
33
"a.html.gz": 379,
4-
"a.js": 17912,
5-
"a.js.gz": 8067,
4+
"a.js": 17921,
5+
"a.js.gz": 8079,
66
"a.mem": 3171,
77
"a.mem.gz": 2713,
8-
"total": 21650,
9-
"total_gz": 11159
8+
"total": 21659,
9+
"total_gz": 11171
1010
}

test/code_size/hello_webgl_wasm.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
"a.html.gz": 379,
44
"a.js": 4185,
55
"a.js.gz": 2243,
6-
"a.wasm": 10467,
7-
"a.wasm.gz": 6706,
8-
"total": 15221,
9-
"total_gz": 9328
6+
"a.wasm": 10475,
7+
"a.wasm.gz": 6710,
8+
"total": 15229,
9+
"total_gz": 9332
1010
}
Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"a.html": 567,
33
"a.html.gz": 379,
4-
"a.js": 17390,
5-
"a.js.gz": 7895,
4+
"a.js": 17399,
5+
"a.js.gz": 7910,
66
"a.mem": 3171,
77
"a.mem.gz": 2713,
8-
"total": 21128,
9-
"total_gz": 10987
8+
"total": 21137,
9+
"total_gz": 11002
1010
}

0 commit comments

Comments
 (0)