Jinoh Kang (@iamahuman) commented about dlls/ntdll/heap.c:
+{ + ULONG alloc = ReadNoFence( &bin->count_alloc ), freed = ReadNoFence( &bin->count_freed ); + SIZE_T block_size = BLOCK_BIN_SIZE( bin - heap->bins ); + BOOL enable = FALSE; + + if (bin == heap->bins && alloc > 0x10) enable = TRUE; + else if (bin - heap->bins < 0x30 && alloc > 0x800) enable = TRUE; + else if (bin - heap->bins < 0x30 && alloc - freed > 0x10) enable = TRUE; + else if (alloc - freed > 0x400000 / block_size) enable = TRUE; + + if (enable) InterlockedExchange( &bin->enabled, TRUE ); + if (enable && ReadNoFence( &heap->compat_info ) != HEAP_LFH) + { + ULONG info = HEAP_LFH; + RtlSetHeapInformation( heap, HeapCompatibilityInformation, &info, sizeof(info) ); + } I've described some possible room for improvement below. Feel free to dismiss them if you believe them not to be significant.
1. **State simplification**: This can be improved so that LFH activation comes before LFH bin activation. This lets us maintain the invariant that a LFH bin is enabled _only if_ LFH is activated for the entire heap. This invariant give us an option (not mandatory) to simplify the guard clauses in `heap_allocate_block_lfh` by removing the `compat_info == HEAP_LFH` check, which will make the LFH code path a little faster. Perhaps your intention was to abide by a "transactional" code pattern, where you first make changes to the bin _and then_ commit it by flipping the LFH enable switch. While I indeed favor this code style in general, I think it's more useful to keep the invariant above since it reduces the number of states to reason about (e.g. a standard heap cannot ever have any LFH-enabled bins) and simplifies checks. 2. **Performance**: This does not need the sequentially consistent memory ordering guarantee imposed by `InterlockedExchange`. Sequentially consistent memory access implies a full barrier, which may lead to nontrivial performance impact compared to weaker ordering especially in wmo architectures such as ARM. How about downgrading it to `WriteRelease`? 3. **Control flow simplification**: Two conditional control structues, one as an `if` statement and the other as a ternary expression, are controlled by the same predicate: `enable`. We can merge them (jump threading in compiler terms) to remove duplicate expressions. The suggestion below uses an early return statement. You can use nested `if`s as well, but I think guard clauses without visible side effects can help readability in C (which lacks many control structure syntactic sugars found in many functional programming languages). ```suggestion:-4+0 if (!enabled) return; if (ReadNoFence( &heap->compat_info ) != HEAP_LFH) { ULONG info = HEAP_LFH; /* NOTE: assume that failure means HEAP_LFH has been set concurrently */ RtlSetHeapInformation( heap, HeapCompatibilityInformation, &info, sizeof(info) ); } /* paired with ReadAcquire in heap_allocate_block_lfh. */ WrtieRelease( &bin->enabled, TRUE ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1628#note_23092