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 ); ```