[PATCH v3 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). -- v3: ntdll: Better match Windows subheap sizes. kernel32/tests: Add tests for subheap sizes. 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 | 85 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 8c101e1c47d..fead409960c 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -3621,6 +3621,90 @@ static void test_GlobalMemoryStatus(void) #undef IS_WITHIN_RANGE } +static void get_valloc_info( void *mem, char **base, SIZE_T *alloc_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; + 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 ); + 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; + + BOOL initial_subheap = TRUE, max_size_reached = FALSE; + SIZE_T alloc_size, current_subheap_size; + 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 ); + + 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 ); + + 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 ); + 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 +3741,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 fead409960c..59a82c0c579 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" ); @@ -3661,7 +3660,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 ); - 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 ); @@ -3687,11 +3685,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
v3: - remote parts related to committed size from test. That wasn't testing anything right due to a typo in get_valloc_info(). The commit size is of course not equal to reserved size most of the time. There are the differences between Windows and Wine (e. g., Windows commits non-10k aligned ranges while Wine always align those), but the details of committing behaviour is not related neither to the purpose of the test nor the patches here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32662
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon