On Tue Feb 7 10:52:39 2023 +0000, Jinoh Kang wrote:
I've described some possible room for improvement below. Feel free to dismiss them if you believe them not to be significant.
- **State simplification**: The statements can be reordered 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 (release) ordering. 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).
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. */ WriteRelease( &bin->enabled, TRUE );
Sure, that probably makes sense. As long as it passes the tests wrt. LFH activation.