[PATCH 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). -- 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 | 2 +- 2 files changed, 1 insertion(+), 2 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..38c39368896 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -501,7 +501,7 @@ static inline void mark_block_tail( struct block *block, DWORD flags ) { 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 *) ); + 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 38c39368896..6a9ec90a444 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -2423,7 +2423,7 @@ 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 ) )) + if (!contains( blocks, commit_end - (char *)blocks - 4 * BLOCK_ALIGN, block, block_get_size( block ) )) entry->cbData = commit_end - (char *)entry->lpData - 4 * BLOCK_ALIGN; entry->cbOverhead = 2 * BLOCK_ALIGN; entry->iRegionIndex = 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 | 12 +++++------- dlls/ntdll/heap.c | 13 +++++++++---- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 47c6a060885..2329efcc9e8 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,10 @@ 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 *)) ) - ok( diff == expect_size, "got diff %#Ix exp %#Ix\n", diff, expect_size ); - + todo_wine_if( (!(global_flags & ~FLG_HEAP_ENABLE_FREE_CHECK) && alloc_size < 2 * sizeof(void *)) + || (global_flags & (FLG_HEAP_ENABLE_FREE_CHECK | FLG_HEAP_VALIDATE_PARAMETERS + | FLG_HEAP_VALIDATE_ALL) && diff > 0x100000)) + ok( diff == expect_size, "got diff %#Ix exp %#Ix, alloc_size %#Ix\n", diff, expect_size, alloc_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 +3664,7 @@ 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 + todo_wine_if( initial_size && alloc_size == initial_size ) 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 +3692,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 6a9ec90a444..da1e7cf3ea3 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 = 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
Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
{ if (flags & HEAP_TAIL_CHECKING_ENABLED || RUNNING_ON_VALGRIND) tail += BLOCK_ALIGN; valgrind_make_writable( tail + sizeof(void *), sizeof(void *) );
Please update the valgrind call too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32649
Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
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 ) )) + if (!contains( blocks, commit_end - (char *)blocks - 4 * BLOCK_ALIGN, block, block_get_size( block ) ))
Why is this needed? I think it could perhaps safely be HEAP_MIN_BLOCK_SIZE, but this 4*BLOCK_ALIGN looks very arbitrary. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32650
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/heap.c:
+ + 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( alloc_size == initial_size + default_heap_size ||
broken( alloc_size == (initial_size ? initial_size : 0x40000) ) /* Win7 */,
"got %#Ix.\n", alloc_size );
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32651
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/heap.c:
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 *)) ) - ok( diff == expect_size, "got diff %#Ix exp %#Ix\n", diff, expect_size ); - + todo_wine_if( (!(global_flags & ~FLG_HEAP_ENABLE_FREE_CHECK) && alloc_size < 2 * sizeof(void *)) + || (global_flags & (FLG_HEAP_ENABLE_FREE_CHECK | FLG_HEAP_VALIDATE_PARAMETERS + | FLG_HEAP_VALIDATE_ALL) && diff > 0x100000)) + ok( diff == expect_size, "got diff %#Ix exp %#Ix, alloc_size %#Ix\n", diff, expect_size, alloc_size );
```suggestion:-3+0 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 ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32652
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/heap.c:
heap = HeapCreate( HEAP_NO_SERIALIZE, initial_size, 0 ); get_valloc_info( heap, ¤t_base, &alloc_size, &commit_size );
- todo_wine + todo_wine_if( initial_size && alloc_size == initial_size )
Could we maybe fully pass this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32653
On Fri May 12 14:10:24 2023 +0000, Rémi Bernon wrote:
Why is this needed? I think it could perhaps safely be HEAP_MIN_BLOCK_SIZE, but this 4*BLOCK_ALIGN looks very arbitrary. Nevermind, I think I see now, it is consistent with the line below which is used to fake an uncommitted range.
Would you mind changing both line to be `commit_end - 4 * BLOCK_ALIGN - (char *)...`, so that they look more consistent? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2800#note_32654
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon