Fix bmalloc::Allocator:tryAllocate() to return null on failure to allocate.

• Component: bmalloc
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/53935772>

[Mark Lam]

Created attachment 378148 [details]
proposed patch.

[Mark Lam]

Created attachment 378149 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 378149 [details]
proposed patch.

Attachment 378149 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13003250

New failing tests:
stress/test-out-of-memory.js.ftl-no-cjit-b3o0
stress/test-out-of-memory.js.no-ftl
stress/test-out-of-memory.js.no-llint
stress/test-out-of-memory.js.default
stress/test-out-of-memory.js.ftl-no-cjit-no-put-stack-validate
stress/test-out-of-memory.js.mini-mode
stress/test-out-of-memory.js.dfg-eager-no-cjit-validate
stress/test-out-of-memory.js.ftl-eager-no-cjit
stress/test-out-of-memory.js.ftl-eager-no-cjit-b3o1
stress/test-out-of-memory.js.ftl-eager
stress/test-out-of-memory.js.ftl-no-cjit-no-inline-validate
stress/test-out-of-memory.js.no-cjit-collect-continuously
stress/test-out-of-memory.js.bytecode-cache
stress/test-out-of-memory.js.dfg-eager
stress/test-out-of-memory.js.ftl-no-cjit-validate-sampling-profiler
stress/test-out-of-memory.js.ftl-no-cjit-small-pool
stress/test-out-of-memory.js.eager-jettison-no-cjit
stress/test-out-of-memory.js.no-cjit-validate-phases

[Mark Lam]

(In reply to Build Bot from comment #3)
> Comment on attachment 378149 [details]
> proposed patch.
> 
> Attachment 378149 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/13003250
> 
> New failing tests:
> stress/test-out-of-memory.js.ftl-no-cjit-b3o0
> stress/test-out-of-memory.js.no-ftl
> stress/test-out-of-memory.js.no-llint
> stress/test-out-of-memory.js.default
> stress/test-out-of-memory.js.ftl-no-cjit-no-put-stack-validate
> stress/test-out-of-memory.js.mini-mode
> stress/test-out-of-memory.js.dfg-eager-no-cjit-validate
> stress/test-out-of-memory.js.ftl-eager-no-cjit
> stress/test-out-of-memory.js.ftl-eager-no-cjit-b3o1
> stress/test-out-of-memory.js.ftl-eager
> stress/test-out-of-memory.js.ftl-no-cjit-no-inline-validate
> stress/test-out-of-memory.js.no-cjit-collect-continuously
> stress/test-out-of-memory.js.bytecode-cache
> stress/test-out-of-memory.js.dfg-eager
> stress/test-out-of-memory.js.ftl-no-cjit-validate-sampling-profiler
> stress/test-out-of-memory.js.ftl-no-cjit-small-pool
> stress/test-out-of-memory.js.eager-jettison-no-cjit
> stress/test-out-of-memory.js.no-cjit-validate-phases

I think these test failures are bogus.  The jsc EWS bot has not been building bmalloc patches properly.  The test passes locally both with a release and debug build.

[Yusuke Suzuki]

Comment on attachment 378149 [details]
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.

[Mark Lam]

Comment on attachment 378149 [details]
proposed patch.

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

Thanks for the review.

>> 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.

I agree, which is why I opted to thread the FailureAction into bmalloc::Heap instead of just removing a lot of the asserts in there.  That said, I'm not going to try to do this exhaustively in this patch.  I'll change what I can.

>> Source/bmalloc/bmalloc/Allocator.cpp:147
>>      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.

I'll see what I can do.

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

I had thought about this, and opted for the basic enum because:
1. this FailureAction is only used in bmalloc's internal implementation, and the chance of a symbol collision between CrashOnFailure and ReturnNullOnFailure with something else is low to none.
2. it's a pain to have to type (and read) FailureAction::CrashOnFailure and FailureAction::ReturnNullOnFailure.

On the other hand, I can change this to FailureAction::Crash and FailureAction::ReturnNull, and that's not a pain.  I'll apply that.

[Mark Lam]

Created attachment 378191 [details]
patch for landing.

[Mark Lam]

Landed in r249578: <http://trac.webkit.org/r249578>.

[Mark Lam]

Also landed a follow-up speculative test fix in r249599: <http://trac.webkit.org/r249599>.