<rdar://problem/53935772>
Created attachment 378148 [details] proposed patch.
Created attachment 378149 [details] proposed patch.
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
(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 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 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.
Created attachment 378191 [details] patch for landing.
Landed in r249578: <http://trac.webkit.org/r249578>.
Also landed a follow-up speculative test fix in r249599: <http://trac.webkit.org/r249599>.