[PATCH v2 0/4] MR2800: ntdll/heap: Reduce memory footprint by better matching Windows subheap sizes
Turns out Windows is more conservative both in initial allocation when creating new heap and also in growing the heap. Comments: - Without patch 1 the related test in test_block_layout fails on x64 for me ("unexpected padding") after the last patch. I don't think the last patches really changes much in this regard, looks like the test just depends on the uninitialized data; - Without patch 2 test_HeapCreate fails (line 601 where it checks the last block pointer). I think it is also only triggered by my patch, I think that last block detection only works correctly now when commit size is smaller than the subheap size (which happens to be not the case in the test after the last patch); - The last patch extends a bit a todo in test_block_layout(). The actual reason for (extended) todo is that we are probably not handling pending free blocks quite like on Windows. I watched the pointers on Windows in the same test and they are always withing the same subheap allocation which suggests that Windows might be reusing pending free block once it can't find normal free block and before extending the heap. So the last patch changes subheap sizes and that triggers different subheaps for the allocated pointers in Wine. So it looks like not an actual issue with my patch. Reusing pending free pointers is apparently possible to implement but it seems to me that it doesn't worth the trouble until anything depends on that (since those pending free blocks are only there when heap debug flags are enabled which is rarely the case outside of debugging). -- v2: ntdll: Better match Windows subheap sizes. kernel32/tests: Add tests for subheap sizes. ntdll: Fix last block detection in heap_walk_blocks(). ntdll: Fix tail padding in mark_block_tail(). https://gitlab.winehq.org/wine/wine/-/merge_requests/2800
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 1 - dlls/ntdll/heap.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index d2742b55495..8c101e1c47d 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -3127,7 +3127,6 @@ static void test_block_layout( HANDLE heap, DWORD global_flags, DWORD heap_flags ok( !memcmp( ptr1 + alloc_size, tail_buf, tail_size ), "missing block tail\n" ); ok( !memcmp( ptr2 + alloc_size, tail_buf, tail_size ), "missing block tail\n" ); - todo_wine_if( global_flags & FLG_HEAP_ENABLE_FREE_CHECK ) ok( !memcmp( ptr0 + alloc_size + tail_size, padd_buf, 2 * sizeof(void *) ), "unexpected padding\n" ); tmp_ptr = (void *)0xdeadbeef; diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index ef1a9c3f3b1..54aa1a8960d 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -500,8 +500,8 @@ static inline void mark_block_tail( struct block *block, DWORD flags ) if (flags & HEAP_ADD_USER_INFO) { if (flags & HEAP_TAIL_CHECKING_ENABLED || RUNNING_ON_VALGRIND) tail += BLOCK_ALIGN; - valgrind_make_writable( tail + sizeof(void *), sizeof(void *) ); - memset( tail + sizeof(void *), 0, sizeof(void *) ); + valgrind_make_writable( tail, BLOCK_ALIGN ); + memset( tail, 0, BLOCK_ALIGN ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2800
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/heap.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 54aa1a8960d..ce3419734ee 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -2399,7 +2399,6 @@ BOOLEAN WINAPI RtlValidateHeap( HANDLE handle, ULONG flags, const void *ptr ) return ret; } - static NTSTATUS heap_walk_blocks( const struct heap *heap, const SUBHEAP *subheap, const struct block *block, struct rtl_heap_entry *entry ) { @@ -2423,8 +2422,8 @@ static NTSTATUS heap_walk_blocks( const struct heap *heap, const SUBHEAP *subhea entry->lpData = (char *)block + block_get_overhead( block ); entry->cbData = block_get_size( block ) - block_get_overhead( block ); /* FIXME: last free block should not include uncommitted range, which also has its own overhead */ - if (!contains( blocks, commit_end - (char *)blocks, block, block_get_size( block ) )) - entry->cbData = commit_end - (char *)entry->lpData - 4 * BLOCK_ALIGN; + if (!contains( blocks, commit_end - 4 * BLOCK_ALIGN - (char *)blocks, block, block_get_size( block ) )) + entry->cbData = commit_end - 4 * BLOCK_ALIGN - (char *)entry->lpData; entry->cbOverhead = 2 * BLOCK_ALIGN; entry->iRegionIndex = 0; entry->wFlags = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2800
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 90 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 8c101e1c47d..47c6a060885 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -3621,6 +3621,95 @@ static void test_GlobalMemoryStatus(void) #undef IS_WITHIN_RANGE } +static void get_valloc_info( void *mem, char **base, SIZE_T *alloc_size, SIZE_T *commit_size ) +{ + MEMORY_BASIC_INFORMATION info, info2; + SIZE_T size; + char *p; + + size = VirtualQuery( mem, &info, sizeof(info) ); + ok( size == sizeof(info), "got %Iu.\n", size ); + + info2 = info; + p = info.AllocationBase; + *commit_size = 0; + while (1) + { + size = VirtualQuery( p, &info2, sizeof(info2) ); + ok( size == sizeof(info), "got %Iu.\n", size ); + if (info2.AllocationBase != info.AllocationBase) + break; + ok( info2.State == MEM_RESERVE || info2.State == MEM_COMMIT, "got %#lx.\n", info2.State ); + if (info.State == MEM_COMMIT) + *commit_size += info2.RegionSize; + p += info2.RegionSize; + } + + *base = info.AllocationBase; + *alloc_size = p - *base; +} + +static void test_heap_size( SIZE_T initial_size ) +{ + static const SIZE_T default_heap_size = 0x10000, init_grow_size = 0x100000, max_grow_size = 0xfd0000; + + SIZE_T alloc_size, commit_size, current_subheap_size; + BOOL initial_subheap = TRUE, max_size_reached = FALSE; + char *base, *current_base; + unsigned int i; + HANDLE heap; + void *p; + + winetest_push_context( "init size %#Ix", initial_size ); + heap = HeapCreate( HEAP_NO_SERIALIZE, initial_size, 0 ); + get_valloc_info( heap, ¤t_base, &alloc_size, &commit_size ); + + todo_wine + ok( alloc_size == initial_size + default_heap_size || broken( (initial_size && alloc_size == initial_size) + || (!initial_size && (alloc_size == default_heap_size * sizeof(void*))) ) /* Win7 */, + "got %#Ix.\n", alloc_size ); + ok( commit_size == alloc_size, "got %#Ix.\n", commit_size ); + + current_subheap_size = alloc_size; + for (i = 0; i < 100; ++i) + { + winetest_push_context( "i %u, current_subheap_size %#Ix", i, current_subheap_size ); + p = HeapAlloc( heap, 0, 0x60000 ); + get_valloc_info( p, &base, &alloc_size, &commit_size ); + ok( alloc_size == commit_size, "got alloc_size %#Ix, commit_size %#Ix.\n", alloc_size, commit_size ); + if (base != current_base) + { + current_base = base; + if (initial_subheap) + { + current_subheap_size = init_grow_size; + initial_subheap = FALSE; + } + else + { + current_subheap_size = min( current_subheap_size * 2, max_grow_size ); + if (current_subheap_size == max_grow_size) + max_size_reached = TRUE; + } + } + todo_wine_if( !initial_subheap ) + ok( alloc_size == current_subheap_size, "got %#Ix.\n", alloc_size ); + winetest_pop_context(); + } + todo_wine_if( sizeof(void *) == 8 ) + ok( max_size_reached, "Did not reach maximum subheap size.\n" ); + + HeapDestroy( heap ); + winetest_pop_context(); +} + +static void test_heap_sizes(void) +{ + test_heap_size( 0 ); + test_heap_size( 0x80000 ); + test_heap_size( 0x150000 ); +} + START_TEST(heap) { int argc; @@ -3657,4 +3746,5 @@ START_TEST(heap) test_debug_heap( argv[0], 0xdeadbeef ); } else win_skip( "RtlGetNtGlobalFlags not found, skipping heap debug tests\n" ); + test_heap_sizes(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2800
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 8 ++------ dlls/ntdll/heap.c | 13 +++++++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 47c6a060885..cdd7d7e552a 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -578,7 +578,6 @@ static void test_HeapCreate(void) todo_wine ok( entries[0].Region.dwCommittedSize == 0x400 * sizeof(void *), "got Region.dwCommittedSize %#lx\n", entries[0].Region.dwCommittedSize ); - todo_wine ok( entries[0].Region.dwUnCommittedSize == 0x10000 - entries[0].Region.dwCommittedSize || entries[0].Region.dwUnCommittedSize == 0x10000 * sizeof(void *) - entries[0].Region.dwCommittedSize /* win7 */, "got Region.dwUnCommittedSize %#lx\n", entries[0].Region.dwUnCommittedSize ); @@ -3019,9 +3018,9 @@ static void test_block_layout( HANDLE heap, DWORD global_flags, DWORD heap_flags expect_size = max( alloc_size, 2 * sizeof(void *) ); expect_size = ALIGN_BLOCK_SIZE( expect_size + extra_size ); diff = min( llabs( ptr2 - ptr1 ), llabs( ptr1 - ptr0 ) ); - todo_wine_if( (!(global_flags & ~FLG_HEAP_ENABLE_FREE_CHECK) && alloc_size < 2 * sizeof(void *)) ) + todo_wine_if( (!global_flags && alloc_size < 2 * sizeof(void *)) || + ((heap_flags & HEAP_FREE_CHECKING_ENABLED) && diff >= 0x100000) ) ok( diff == expect_size, "got diff %#Ix exp %#Ix\n", diff, expect_size ); - ok( !memcmp( ptr0 + alloc_size, tail_buf, tail_size ), "missing block tail\n" ); ok( !memcmp( ptr1 + alloc_size, tail_buf, tail_size ), "missing block tail\n" ); ok( !memcmp( ptr2 + alloc_size, tail_buf, tail_size ), "missing block tail\n" ); @@ -3664,7 +3663,6 @@ static void test_heap_size( SIZE_T initial_size ) heap = HeapCreate( HEAP_NO_SERIALIZE, initial_size, 0 ); get_valloc_info( heap, ¤t_base, &alloc_size, &commit_size ); - todo_wine ok( alloc_size == initial_size + default_heap_size || broken( (initial_size && alloc_size == initial_size) || (!initial_size && (alloc_size == default_heap_size * sizeof(void*))) ) /* Win7 */, "got %#Ix.\n", alloc_size ); @@ -3692,11 +3690,9 @@ static void test_heap_size( SIZE_T initial_size ) max_size_reached = TRUE; } } - todo_wine_if( !initial_subheap ) ok( alloc_size == current_subheap_size, "got %#Ix.\n", alloc_size ); winetest_pop_context(); } - todo_wine_if( sizeof(void *) == 8 ) ok( max_size_reached, "Did not reach maximum subheap size.\n" ); HeapDestroy( heap ); diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index ce3419734ee..bbb4cbf8a99 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -311,7 +311,12 @@ C_ASSERT( offsetof(struct heap, subheap) <= REGION_ALIGN - 1 ); #define HEAP_MAGIC ((DWORD)('H' | ('E'<<8) | ('A'<<16) | ('P'<<24))) -#define HEAP_DEF_SIZE (0x40000 * BLOCK_ALIGN) +#define HEAP_INITIAL_SIZE 0x10000 +#define HEAP_INITIAL_GROW_SIZE 0x100000 +#define HEAP_MAX_GROW_SIZE 0xfd0000 + +C_ASSERT( HEAP_MIN_LARGE_BLOCK_SIZE <= HEAP_INITIAL_GROW_SIZE ); + #define MAX_FREE_PENDING 1024 /* max number of free requests to delay */ /* some undocumented flags (names are made up) */ @@ -1133,7 +1138,7 @@ static struct block *find_free_block( struct heap *heap, ULONG flags, SIZE_T blo if ((subheap = create_subheap( heap, flags, max( heap->grow_size, total_size ), total_size ))) { - if (heap->grow_size <= HEAP_MAX_FREE_BLOCK_SIZE / 2) heap->grow_size *= 2; + heap->grow_size = min( heap->grow_size * 2, HEAP_MAX_GROW_SIZE ); } else while (!subheap) /* shrink the grow size again if we are running out of space */ { @@ -1517,7 +1522,7 @@ HANDLE WINAPI RtlCreateHeap( ULONG flags, void *addr, SIZE_T total_size, SIZE_T flags &= ~(HEAP_TAIL_CHECKING_ENABLED|HEAP_FREE_CHECKING_ENABLED); if (process_heap) flags |= HEAP_PRIVATE; if (!process_heap || !total_size || (flags & HEAP_SHARED)) flags |= HEAP_GROWABLE; - if (!total_size) total_size = HEAP_DEF_SIZE; + if (!total_size) total_size = commit_size + HEAP_INITIAL_SIZE; if (!(heap = addr)) { @@ -1532,7 +1537,7 @@ HANDLE WINAPI RtlCreateHeap( ULONG flags, void *addr, SIZE_T total_size, SIZE_T heap->flags = (flags & ~HEAP_SHARED); heap->compat_info = HEAP_STD; heap->magic = HEAP_MAGIC; - heap->grow_size = max( HEAP_DEF_SIZE, total_size ); + heap->grow_size = HEAP_INITIAL_GROW_SIZE; heap->min_size = commit_size; list_init( &heap->subheap_list ); list_init( &heap->large_list ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2800
v2: - update valgrind call; - change last block expression; - simplify todo_wine condition in test_HeapCreate (applied Rémi's suggestion); - fully pass test_heap_size(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32660
participants (2)
-
Paul Gofman -
Paul Gofman (＠gofman)