media: Add page alignment and lockless decommit#11550
Conversation
…erBufferAllocator Add support for the allocate_with_page_alignment flag to the configurable decommit strategy. This flag is bound to bit 25 (the second bit of the highest byte) of the setting and controls whether the fallback block allocator forces page alignment and page-rounded size. Additionally, implement a lockless suspend decommit optimization by checking the decommit_on_suspend_enabled_ atomic flag before acquiring the mutex. This prevents UI thread blocking on suspend when decommits are disabled. Also simplify StarboardMemoryAllocator pointer arithmetic inside Decommit() using uint8_t* and standardize parameter documentation. Bug: 454441375 TAG=agy CONV=92545d5a-4b8a-4031-bbd7-3775e458e496
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
xiaomings
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable page alignment option (allocate_with_page_alignment) to the DecoderBufferAllocator and StarboardMemoryAllocator strategies, and updates H5vccSettings to allow configuring this option via a bitmask. The review feedback highlights a backward compatibility issue where existing configurations would silently disable page alignment, suggesting to treat the new bit as a disable flag instead. Additionally, the reviewer recommends using uintptr_t instead of uint8_t* for alignment calculations in StarboardMemoryAllocator to prevent potential compilation issues, and updating the corresponding documentation comments.
| bool aggressive_decommit_on_suspend = ((value >> 24) & 0x01) != 0; | ||
| bool allocate_with_page_alignment = ((value >> 24) & 0x02) != 0; |
There was a problem hiding this comment.
Extracting allocate_with_page_alignment directly as ((value >> 24) & 0x02) != 0 breaks backward compatibility with existing configurations (e.g., 0x01040402). Previously, page alignment was always enabled by default. With this change, existing configurations that do not explicitly set bit 25 will have page alignment silently disabled, which could lead to performance regressions or memory fragmentation.
To maintain backward compatibility, we should treat bit 25 as a disable_page_alignment flag (where 0 means enabled, and 1 means disabled).
| bool aggressive_decommit_on_suspend = ((value >> 24) & 0x01) != 0; | |
| bool allocate_with_page_alignment = ((value >> 24) & 0x02) != 0; | |
| bool aggressive_decommit_on_suspend = ((value >> 24) & 0x01) != 0; | |
| bool allocate_with_page_alignment = ((value >> 24) & 0x02) == 0; |
| uint8_t* start = static_cast<uint8_t*>(memory); | ||
| uint8_t* end = start + size; | ||
| uint8_t* aligned_start = AlignUp(start, page_size_); | ||
| uint8_t* aligned_end = AlignDown(end, page_size_); | ||
|
|
||
| if (aligned_size > 0) { | ||
| if (aligned_start < aligned_end) { | ||
| size_t aligned_size = aligned_end - aligned_start; | ||
| // MADV_FREE is not supported on all kernel versions/configurations. If it | ||
| // fails, fallback to MADV_DONTNEED to ensure memory is still decommitted. | ||
| if (conservative && madvise(memory, aligned_size, MADV_FREE) == 0) { | ||
| if (conservative && | ||
| madvise(aligned_start, aligned_size, MADV_FREE) == 0) { | ||
| return; | ||
| } | ||
| madvise(memory, aligned_size, MADV_DONTNEED); | ||
| madvise(aligned_start, aligned_size, MADV_DONTNEED); | ||
| } |
There was a problem hiding this comment.
Using uint8_t* pointers directly with AlignUp and AlignDown can cause compilation failures depending on how these templates/functions are defined in the codebase (e.g., if they do not support pointer types or if they cause template deduction conflicts). Performing alignment calculations using uintptr_t is much safer, more standard, and avoids any potential compilation or pointer arithmetic issues.
uintptr_t start = reinterpret_cast<uintptr_t>(memory);
uintptr_t end = start + size;
uintptr_t aligned_start = AlignUp(start, page_size_);
uintptr_t aligned_end = AlignDown(end, page_size_);
if (aligned_start < aligned_end) {
size_t aligned_size = aligned_end - aligned_start;
void* aligned_memory = reinterpret_cast<void*>(aligned_start);
// MADV_FREE is not supported on all kernel versions/configurations. If it
// fails, fallback to MADV_DONTNEED to ensure memory is still decommitted.
if (conservative &&
madvise(aligned_memory, aligned_size, MADV_FREE) == 0) {
return;
}
madvise(aligned_memory, aligned_size, MADV_DONTNEED);
}| // - allocate_with_page_alignment (bit 25): When set (second bit is | ||
| // non-zero), forces new OS block allocations to be page-aligned and | ||
| // rounded up to page sizes. Default is true. |
There was a problem hiding this comment.
Update the comment to reflect that bit 25 acts as a disable flag to maintain backward compatibility.
| // - allocate_with_page_alignment (bit 25): When set (second bit is | |
| // non-zero), forces new OS block allocations to be page-aligned and | |
| // rounded up to page sizes. Default is true. | |
| // - disable_page_alignment (bit 25): When set (second bit is | |
| // non-zero), disables forcing new OS block allocations to be page-aligned and | |
| // rounded up to page sizes. Default is false (meaning page alignment is enabled). |
1 participant
Add support for a page alignment flag in DecoderBufferAllocator to
control OS block allocation alignment. This allows for more granular
memory management when using the configurable decommit strategy.
Implement a lockless optimization during suspend by checking an atomic
flag before acquiring the allocator mutex. This prevents unnecessary
blocking of the UI thread when decommit on suspend is not enabled.
Additionally, simplify pointer arithmetic in StarboardMemoryAllocator
using uint8_t pointers and update H5vcc settings to expose the new
alignment configuration bit.
Bug: 454441375