[webkit-reviews] review granted: [Bug 201529] Fix bmalloc::Allocator:tryAllocate() to return null on failure to allocate. : [Attachment 378149] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 201529: Fix bmalloc::Allocator:tryAllocate() to return null on failure to
allocate.
https://bugs.webkit.org/show_bug.cgi?id=201529

Attachment 378149: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=378149&action=review




--- Comment #5 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 378149
  --> https://bugs.webkit.org/attachment.cgi?id=378149
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=378149&action=review

r=me

> Source/bmalloc/ChangeLog:8
> +

Can we crash immediately if the flag is CrashOnFailure? After starting looking
into the collected crash traces, I prefer passing a flag (like this) instead of
using two functions `tryAllocate` and `allocate`.\
Here is the reason: typically we only provide `tryAllocate`, and we provide an
API `allocate` with `RELEASE_ASSERT(result)`.
But this is not so good in terms of crash trace analysis. We actually don't
know why this allocation failed from the crash trace. If we pass the flag like
crashOnFailure, we can crash immediately when something goes wrong. And the
crash trace says what is the reason of this failure by the stack trace itself.

> Source/bmalloc/bmalloc/Allocator.cpp:147
> +	   return m_heap.tryAllocateLarge(lock, alignment, size);
>      return m_heap.allocateLarge(lock, alignment, size);

Can we merge `Heap::tryAllocateLarge` and `Heap::allocateLarge`? Like, taking
an `action` flag, and crash in `allocateLarge` when action is saying we should
crash.

> Source/bmalloc/bmalloc/FailureAction.h:30
> +enum FailureAction { CrashOnFailure, ReturnNullOnFailure };

Use `enum class` instead.