...
Created attachment 392317 [details] WIP It begins
Created attachment 392359 [details] WIP
Created attachment 392370 [details] WIP
Created attachment 392529 [details] WIP
Created attachment 392608 [details] WIP
Created attachment 392663 [details] WIP
Created attachment 392822 [details] WIP Seems to work. Now, I need to implement freeing islands and pick the correct region sizes.
Created attachment 392836 [details] WIP
Created attachment 392983 [details] WIP
Created attachment 392984 [details] WIP
Created attachment 392985 [details] WIP
Created attachment 393107 [details] WIP I think it might be complete. Time to clean up the code
Created attachment 393118 [details] WIP I need to make jump islands work with live repatching wasm code. Currently, we might load the wrong thing from the icache :-(
Created attachment 393184 [details] WIP
(In reply to Saam Barati from comment #14) > Created attachment 393184 [details] > WIP I solved my wasm bug. Turns out, when hot patching code concurrently, you need to do a few things to not SIGILL (surprise surprise...). I need to clean up the code again after my wasm debugging session, and then it should be ready for reviews. Running JS2, and using the random allocation algorithm, our max jump island utilization rate is about 1%. So if we use 16MB, we should have a lot of leeway.
Created attachment 393403 [details] patch
Created attachment 393406 [details] patch
Created attachment 393412 [details] patch try to fix testwtf build error
Created attachment 393413 [details] patch testwtf build fix take 2
Created attachment 393415 [details] patch testwtf build fix take 3
Created attachment 393440 [details] patch
Created attachment 393444 [details] patch
Created attachment 393449 [details] patch
Comment on attachment 393449 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=393449&action=review r=me if you make the requested changes and fix Win build. > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:113 > +#if USE(JUMP_ISLANDS) > +static constexpr size_t fixedExecutableMemoryPoolSize = 1 * 1024 * 1024 * 1024; > +// These sizes guarantee that any jump within an island can jump forwards or backwards > +// to the adjacent island in a single instruction. > +static constexpr size_t regionSize = 112 * 1024 * 1024; > +static constexpr size_t islandRegionSize = 16 * 1024 * 1024; > +static constexpr size_t numberOfRegions = fixedExecutableMemoryPoolSize / regionSize; > +static constexpr size_t islandSizeInBytes = 4; > +static constexpr size_t maxIslandsPerRegion = islandRegionSize / islandSizeInBytes; > +#else > static constexpr size_t fixedExecutableMemoryPoolSize = 128 * 1024 * 1024; > +#endif > #elif CPU(X86_64) > -static constexpr size_t fixedExecutableMemoryPoolSize = 1024 * 1024 * 1024; > +static constexpr size_t fixedExecutableMemoryPoolSize = 1 * 1024 * 1024 * 1024; Can we switch to using the MB/GB constants here? > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:815 > + CodeLocationLabel<ExecutableMemoryPtrTag> rootJumpLocation; Can we call this jumpSourceLocation? rootJumpLocation doesn't really make it clear what this is to me. I thought it was the destination for a while, which clearly would have been borked. > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:824 > + RedBlackTree<Islands, void*> islandsForJumpLocation; ditto on changing this to m_islandsForJumpSourceLocation. You also don't have an m_ prefix which is weird. > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:173 > + for (unsigned i = 0; i < m_codeBlock->m_wasmToWasmCallsites.size(); ++i) { > + stageRepatch(m_codeBlock->m_wasmToWasmCallsites[i]); > + if (m_codeBlock->m_llintCallees) { > + LLIntCallee& llintCallee = m_codeBlock->m_llintCallees->at(i).get(); > + if (JITCallee* replacementCallee = llintCallee.replacement()) > + stageRepatch(replacementCallee->wasmToWasmCallsites()); > + if (OMGForOSREntryCallee* osrEntryCallee = llintCallee.osrEntryCallee()) > + stageRepatch(osrEntryCallee->wasmToWasmCallsites()); > + } > + if (BBQCallee* bbqCallee = m_codeBlock->m_bbqCallees[i].get()) { > + if (OMGCallee* replacementCallee = bbqCallee->replacement()) > + stageRepatch(replacementCallee->wasmToWasmCallsites()); > + if (OMGForOSREntryCallee* osrEntryCallee = bbqCallee->osrEntryCallee()) > + stageRepatch(osrEntryCallee->wasmToWasmCallsites()); > + } > + } Seems like we could have some code deduplication between the different plans... Can you do that? > Tools/ChangeLog:8 > + * Scripts/run-jsc-stress-tests: I think you need to update this for the TestWTF changes? Can you also add a TestWTF for RedBlackTree::iterate?
<rdar://problem/60797127>
I'm addressing Keith's comments.
Created attachment 394534 [details] patch for landing
Created attachment 394639 [details] patch for landing
Committed r259582: <https://trac.webkit.org/changeset/259582> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394639 [details].