Bug 201529 - Fix bmalloc::Allocator:tryAllocate() to return null on failure to allocate.
Summary: Fix bmalloc::Allocator:tryAllocate() to return null on failure to allocate.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-05 18:38 PDT by Mark Lam
Modified: 2019-09-06 16:10 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch. (25.73 KB, patch)
2019-09-05 18:48 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (26.21 KB, patch)
2019-09-05 18:52 PDT, Mark Lam
ysuzuki: review+
ews: commit-queue-
Details | Formatted Diff | Diff
patch for landing. (30.05 KB, patch)
2019-09-06 08:38 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-09-05 18:38:51 PDT
<rdar://problem/53935772>
Comment 1 Mark Lam 2019-09-05 18:48:08 PDT
Created attachment 378148 [details]
proposed patch.
Comment 2 Mark Lam 2019-09-05 18:52:36 PDT
Created attachment 378149 [details]
proposed patch.
Comment 3 Build Bot 2019-09-05 21:26:25 PDT
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
Comment 4 Mark Lam 2019-09-05 21:42:32 PDT
(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.
Comment 5 Yusuke Suzuki 2019-09-06 00:37:47 PDT
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.
Comment 6 Mark Lam 2019-09-06 07:47:59 PDT
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.
Comment 7 Mark Lam 2019-09-06 08:38:04 PDT
Created attachment 378191 [details]
patch for landing.
Comment 8 Mark Lam 2019-09-06 10:04:55 PDT
Landed in r249578: <http://trac.webkit.org/r249578>.
Comment 9 Mark Lam 2019-09-06 16:10:17 PDT
Also landed a follow-up speculative test fix in r249599: <http://trac.webkit.org/r249599>.