Repository navigation
Conversation
Shamzik
added
runtime
| kphp::stl::vector<k2::LogTaggedEntry, kphp::memory::script_allocator> tagged_entries{}; | ||
| tagged_entries.reserve(tagged_entries_size); | ||
| size_t tagged_entries_size{}; | ||
| static constexpr size_t TAGGED_ENTRIES_BUFFER_SIZE = 8UZ; |
There was a problem hiding this comment.
Let's reserve more space for tags. Something like 20 should be ok for now
| extra_mem_size = std::bit_ceil(extra_mem_size); | ||
|
|
||
| auto* extra_mem{kphp::memory::platform::alloc(extra_mem_size)}; | ||
| kphp::log::assertion(extra_mem != nullptr); |
There was a problem hiding this comment.
There are also other assertions we need to convert to error
| auto* extra_mem{kphp::memory::platform::alloc(extra_mem_size)}; | ||
| kphp::log::assertion(extra_mem != nullptr); | ||
| if (extra_mem == nullptr) [[unlikely]] { | ||
| kphp::log::error("pool_allocator: failed to get extra memory from platform"); |
There was a problem hiding this comment.
Let's print memory usage statistics so we understand what's happened
Replace OOM assertions in pool_allocator::alloc/calloc/realloc_script_memory with raise_out_of_memory() that logs MemoryStats (current/peak usage, arena limit, total allocations, defragmentation calls, free pieces) before exiting, so OOM failures can be diagnosed from logs.
| kphp::stl::vector<k2::LogTaggedEntry, kphp::memory::script_allocator> tagged_entries{}; | ||
| tagged_entries.reserve(tagged_entries_size); | ||
| size_t tagged_entries_size{}; | ||
| static constexpr size_t TAGGED_ENTRIES_BUFFER_SIZE = 24UZ; |
| return 0; | ||
| }); | ||
| if (trace.has_value()) { | ||
| if (trace.has_value() && tagged_entries_size != TAGGED_ENTRIES_BUFFER_SIZE) { |
There was a problem hiding this comment.
сould we reserve one slot for the trace when it's present ?
There was a problem hiding this comment.
Done — tags are capped at TAGGED_ENTRIES_BUFFER_SIZE - 1 when a trace is present, so the trace slot is always reserved and the extra bounds check in the trace branch is gone.
|
|
||
| auto pool_allocator::raise_out_of_memory(size_t requested_size) const noexcept -> void { | ||
| const auto& stats{memory_resource.get_memory_stats()}; | ||
| kphp::log::error("pool_allocator: out of memory: failed to allocate {} bytes even after requesting extra memory, " |
There was a problem hiding this comment.
do we need to mention the extra-memory request here ? It exposes an implementation detail and couples the message to the current allocation strategy
| requested_size, extra_mem_size, stats.memory_used, stats.memory_limit, stats.max_memory_used, stats.real_memory_used, | ||
| stats.max_real_memory_used, stats.total_allocations, stats.total_memory_allocated, stats.defragmentation_calls, stats.huge_memory_pieces, | ||
| stats.small_memory_pieces); | ||
| } |
There was a problem hiding this comment.
this duplicates the logic in raise_out_of_memory(). Could we reuse or adapt it here ?
There was a problem hiding this comment.
Done — raise_out_of_memory() now takes a context string and is reused for the platform allocation failure too; the stats are formatted in a single place.
When a trace is present, contextual tags are capped at TAGGED_ENTRIES_BUFFER_SIZE - 1 so the trace is never dropped. Co-Authored-By: Claude Code <noreply@anthropic.com>
raise_out_of_memory() now takes a context string and is reused for the platform allocation failure in request_extra_memory(), so memory usage statistics are logged in a single place. The message no longer mentions the extra-memory request, decoupling it from the allocation strategy. Co-Authored-By: Claude Code <noreply@anthropic.com>
This branch has not been deployed
4 participants
This PR replaces
log::assertionwithlog::errorhandling OOM in pool allocator.It is also rewrites
kphp::log::impl::logfunction so it does not call pool allocator recursivly