Bug 171537

*** Bug 171454 has been marked as a duplicate of this bug. ***

While we're here, if We try to tier up a function but don't have X memory available for it then it should just stay at O1. Slow is better than dead. That's what we do for JS (stay in llint).

Created attachment 313772 [details] patch I'm wondering if this is worth testing, and how. I'm open to suggestions! I've been running a huge program with: JSC_webAssemblyBBQOptimizationLevel=1 JSC_useConcurrentGC=0 JSC_useConcurrentJIT=0 JSC_jitMemoryReservationSize=5000 to hit its import limit, and then higher for BBQ functions. Maybe I should force llint by disabling DFG and FTL? Also, I'd like to make sure we don't leak code that's associated with the Module and which was compiled before the failure! Not all the failure points are tested, because that's kind of dynamic (wasm->wasm entry points are tricky to hit). Tiering is also tricky to hit OoM with.

<rdar://problem/32963338>

Comment on attachment 313772 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313772&action=review > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:299 > - if (m_state == State::Compiled) { > + if (!failed() && m_state == State::Compiled) { Why not just return early if already failed? The only possible reason for not returning early is if you want to move the state to State::Complete. However, this disagrees with the allocation failure cases below where you return early without setting the state to State::Complete. So, which is behaving wrongly here? > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:65 > + m_errorMessage = "Out of executable memory"; Use ASCIILiteral here: m_errorMessage = ASCIILiteral("Out of executable memory"); > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:101 > + bool runnable() { return !m_errorMessage; } nit: I suggest: bool runnable() const { ... > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:109 > + CString cString = m_errorMessage.ascii(); > + return String(cString.data()); Why marshal thru CString? Why not just return m_errorMessage? > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:116 > m_codeBlock.set(vm, this, codeBlock); > + if (UNLIKELY(!codeBlock->runnable())) { Why do this check after setting m_codeBlock? Why not do it before?

Comment on attachment 313772 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313772&action=review > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63 > + switch (binding.error()) { Nit: I’m a fan of adding default: ASSERT_NOT_REACHED > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:105 > + String errorMessage() This comment is wrong. This is a JSCell. No races to worry about >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:116 >> + if (UNLIKELY(!codeBlock->runnable())) { > > Why do this check after setting m_codeBlock? Why not do it before? +1

(In reply to JF Bastien from comment #3) > Created attachment 313772 [details] > patch > > I'm wondering if this is worth testing, and how. I'm open to suggestions! Testing ARM64 shouldn’t be that hard. I’d just craft an artificial function that’s close to 1 MB in size. Compile in a loop and keep array of them to keep them all alive. > > I've been running a huge program with: JSC_webAssemblyBBQOptimizationLevel=1 > JSC_useConcurrentGC=0 JSC_useConcurrentJIT=0 > JSC_jitMemoryReservationSize=5000 to hit its import limit, and then higher > for BBQ functions. Maybe I should force llint by disabling DFG and FTL? > Also, I'd like to make sure we don't leak code that's associated with the > Module and which was compiled before the failure! Not all the failure points > are tested, because that's kind of dynamic (wasm->wasm entry points are > tricky to hit). Tiering is also tricky to hit OoM with.

Comment on attachment 313772 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313772&action=review > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:93 > + Base::fail(holdLock(m_lock), makeString("Out of executable memory while tiering up function at index ", String::number(m_functionIndex))); What will happen once we fail here? Will we shut down the compilation and never try again? I think this would be pretty easy to test on ARM64

(In reply to Saam Barati from comment #8) > Comment on attachment 313772 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313772&action=review > > > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:93 > > + Base::fail(holdLock(m_lock), makeString("Out of executable memory while tiering up function at index ", String::number(m_functionIndex))); > > What will happen once we fail here? Will we shut down the compilation and > never try again? I think this would be pretty easy to test on ARM64 Actually, there is an option to set the executable allocator pool size. You could probably write a test using that on x86 too. You just need to make sure that option actually does what it’s supposed to

Created attachment 313960 [details] patch (In reply to Mark Lam from comment #5) > Comment on attachment 313772 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313772&action=review > > > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:299 > > - if (m_state == State::Compiled) { > > + if (!failed() && m_state == State::Compiled) { > > Why not just return early if already failed? The only possible reason for > not returning early is if you want to move the state to State::Complete. > However, this disagrees with the allocation failure cases below where you > return early without setting the state to State::Complete. So, which is > behaving wrongly here? Because I also want to run completion tasks, as done below, which can call back into this function. (In reply to Saam Barati from comment #6) > Comment on attachment 313772 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313772&action=review > > > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63 > > + switch (binding.error()) { > > Nit: I’m a fan of adding default: ASSERT_NOT_REACHED That's a bad idea because right now it's a compile error if one of the cases isn't covered. With default it because a runtime assert.

Comment on attachment 313772 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313772&action=review >>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63 >>> + switch (binding.error()) { >> >> Nit: I’m a fan of adding default: ASSERT_NOT_REACHED > > That's a bad idea because right now it's a compile error if one of the cases isn't covered. With default it because a runtime assert. My preferred trick is to return at the end of the case rather than just breaking and then we can ASSERT_NOT_REACHED after the switch. That way we get both a compile error if a case isn’t covered and a runtime assert if an invalid value somehow gets into the enumeration.

Comment on attachment 313772 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313772&action=review >>>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63 >>>> + switch (binding.error()) { >>> >>> Nit: I’m a fan of adding default: ASSERT_NOT_REACHED >> >> That's a bad idea because right now it's a compile error if one of the cases isn't covered. With default it because a runtime assert. > > My preferred trick is to return at the end of the case rather than just breaking and then we can ASSERT_NOT_REACHED after the switch. That way we get both a compile error if a case isn’t covered and a runtime assert if an invalid value somehow gets into the enumeration. This makes sense JF, I wasn't thinking about the compilation error. I'm also a fan of Darin's suggestion, and it'd be trivial to apply to this code.

Comment on attachment 313960 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=313960&action=review r=me > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:320 > + return exec->vm().canUseJIT() && Options::useBaselineJIT(); nice > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:179 > + if (UNLIKELY(!binding)) { Do your tests trigger this? > Source/JavaScriptCore/wasm/WasmBinding.cpp:310 > + LinkBuffer linkBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail); > + if (UNLIKELY(linkBuffer.didFailToAllocate())) > + return makeUnexpected(BindingFailure::OutOfMemory); Do your tests stress this? > Source/JavaScriptCore/wasm/WasmBinding.cpp:606 > + LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail); > + if (UNLIKELY(patchBuffer.didFailToAllocate())) ditto > Source/JavaScriptCore/wasm/WasmBinding.cpp:664 > + LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail); > + if (UNLIKELY(patchBuffer.didFailToAllocate())) > + return makeUnexpected(BindingFailure::OutOfMemory); > + ditto > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:95 > + LinkBuffer linkBuffer(*context.wasmEntrypointJIT, nullptr, JITCompilationCanFail); > + if (UNLIKELY(linkBuffer.didFailToAllocate())) { > + Base::fail(holdLock(m_lock), makeString("Out of executable memory while tiering up function at index ", String::number(m_functionIndex))); > + return; > + } Do any of your tests trigger this? > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:62 > + if (UNLIKELY(!binding)) { Do your tests trigger this? > Tools/Scripts/run-jsc-stress-tests:1261 > + run("default-wasm", "--useConcurrentGC=0" , "--useConcurrentJIT=0", "--jitMemoryReservationSize=15000", "--useBaselineJIT=0", "--useDFGJIT=0", "--useFTLJIT=0", "-m") I see a potential bug where we don't honor this option. It won't matter in this case since your region is smaller than what's used, but since you're relying on this option, you might as well fix it. There is a direct use of fixedExecutableMemoryPoolSize instead of asking the allocator its reservation size. Look in JITStubRoutine.h. It should be trivial to fix just by loading it from the allocator instead.

Created attachment 313995 [details] patch Address all comments. On tests: yes, I believe I probabilistically hit all the checks. What I mean is that I disable all non-wasm JITting to raise the probability that wasm will hit a limit, then I remove concurrent things so it's still more likely. I then create functions of random size so that even if I hit one limit on one run it'll maybe hit another limit on another run (say, if our codegen changes). I skew the "imports" and "exports" test so they have more of those compared to regular wasm code. Finally, the execution test tries to cause tier up as well. You can see this by running the tests with verbose = true in each file, and JSC_logExecutableAllocation=1 on the command-line. Non-obvious, but locally it seems to do what I want! On the potential bug: Saam and I chatted on IRC and he submitted https://bugs.webkit.org/show_bug.cgi?id=173906 to handle the issue.

> On tests: yes, I believe I probabilistically hit all the checks. What I mean > is that I disable all non-wasm JITting to raise the probability that wasm > will hit a limit, then I remove concurrent things so it's still more likely. > I then create functions of random size so that even if I hit one limit on > one run it'll maybe hit another limit on another run (say, if our codegen > changes). I skew the "imports" and "exports" test so they have more of those > compared to regular wasm code. Finally, the execution test tries to cause > tier up as well. > > You can see this by running the tests with verbose = true in each file, and > JSC_logExecutableAllocation=1 on the command-line. > > Non-obvious, but locally it seems to do what I want! Clarifying "probability": by that I mean that I loop until *something* hits the executable limit. What that thing is I don't check as long as it's the right kind of WebAssembly exception. I can't check because I don't know for a fact that the current codegen will *ever* hit that limit! But with the randomness I throw in it's likely to hit the limits we want.

My only question is: have you verified locally all paths are hit at some point? It’s ok if this is tied to a particular platform.

(In reply to Saam Barati from comment #16) > My only question is: have you verified locally all paths are hit at some > point? It’s ok if this is tied to a particular platform. I believe I have, yes. My answer was trying to illustrate how the tests are written to stay relatively current even if some of our implementation changes.

Comment on attachment 313995 [details] patch Clearing flags on attachment: 313995 Committed r218868: <http://trac.webkit.org/changeset/218868>

All reviewed patches have been landed. Closing bug.