[JSC] Do not copy SimpleJumpTable
Created attachment 425834 [details] Patch
Created attachment 425836 [details] Patch
Created attachment 425845 [details] Patch
Created attachment 425942 [details] Patch
Comment on attachment 425942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425942&action=review > Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:-213 > - for (unsigned i = m_codeBlock->numberOfSwitchJumpTables(); i--;) { > - if (usedJumpTables.get(i)) > - continue; > - > - m_codeBlock->switchJumpTable(i).clear(); > - } We materialize SimpleJumpTable's content when ensureCTITable() is called. And when calling ensureCTITable, we put didUseJumpTable = true. So, this is not necessary. If the table is not used, it is not having contents (since, we are no longer copying these vectors at first). > Source/JavaScriptCore/ftl/FTLLink.cpp:-50 > - // B3 will create its own jump tables as needed. > - codeBlock->clearSwitchJumpTables(); > - We do not move the content to CodeBlock when compiling FTL. This means that it is not set. We do not need to clear here.
Comment on attachment 425942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425942&action=review r=me. Please update the copyright year in the files you modify if they aren't already showing "-2021". > Source/JavaScriptCore/bytecode/CodeBlock.cpp:419 > - if (unlinkedCodeBlock->numberOfExceptionHandlers() || unlinkedCodeBlock->numberOfSwitchJumpTables()) { > + if (unlinkedCodeBlock->numberOfExceptionHandlers()) { > createRareDataIfNecessary(); Nice. One less trigger for creating RareData. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:211 > + const UnlinkedSimpleJumpTable& unlinkedSwitchJumpTable(int tableIndex) { ASSERT(m_rareData); return m_rareData->m_unlinkedSwitchJumpTables[tableIndex]; } Make this a const function? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8560 > + byteCodeParser->m_graph.m_switchJumpTables.resize(byteCodeParser->m_graph.m_switchJumpTables.size() + codeBlock->numberOfUnlinkedSwitchJumpTables()); > + for (unsigned i = 0; i < codeBlock->numberOfUnlinkedSwitchJumpTables(); ++i) { > + m_switchRemap[i] = byteCodeParser->m_graph.m_unlinkedSwitchJumpTables.size(); > + byteCodeParser->m_graph.m_unlinkedSwitchJumpTables.append(&codeBlock->unlinkedSwitchJumpTable(i)); > + } This blob is now identical to the one for the "inline case" above. Would it be possible to refactor this out into the common section below? Also refactor out the m_switchRemap.resize(). > Source/JavaScriptCore/dfg/DFGGraph.h:1067 > + const UnlinkedSimpleJumpTable& unlinkedSwitchJumpTable(unsigned index) { return *m_unlinkedSwitchJumpTables[index]; } Make function const? >> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:-213 >> - } > > We materialize SimpleJumpTable's content when ensureCTITable() is called. And when calling ensureCTITable, we put didUseJumpTable = true. > So, this is not necessary. If the table is not used, it is not having contents (since, we are no longer copying these vectors at first). Did you mean when call emitSwitchIntJump()? I don't see ensureCTITable() setting didUseJumpTable. >> Source/JavaScriptCore/ftl/FTLLink.cpp:-50 >> - > > We do not move the content to CodeBlock when compiling FTL. This means that it is not set. We do not need to clear here. Can we ASSERT that it is not set? I suggest retaining the above B3 comment with the ASSERT.
Crashing randomly via WebCore::RenderLayerCompositor::computeCompositingRequirements, and this is known issue.
Comment on attachment 425942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425942&action=review Thanks! >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:419 >> createRareDataIfNecessary(); > > Nice. One less trigger for creating RareData. :D >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:211 >> + const UnlinkedSimpleJumpTable& unlinkedSwitchJumpTable(int tableIndex) { ASSERT(m_rareData); return m_rareData->m_unlinkedSwitchJumpTables[tableIndex]; } > > Make this a const function? Sure! Fixed. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8560 >> + } > > This blob is now identical to the one for the "inline case" above. Would it be possible to refactor this out into the common section below? Also refactor out the m_switchRemap.resize(). Fixed. >> Source/JavaScriptCore/dfg/DFGGraph.h:1067 >> + const UnlinkedSimpleJumpTable& unlinkedSwitchJumpTable(unsigned index) { return *m_unlinkedSwitchJumpTables[index]; } > > Make function const? Fixed. >>> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:-213 >>> - } >> >> We materialize SimpleJumpTable's content when ensureCTITable() is called. And when calling ensureCTITable, we put didUseJumpTable = true. >> So, this is not necessary. If the table is not used, it is not having contents (since, we are no longer copying these vectors at first). > > Did you mean when call emitSwitchIntJump()? I don't see ensureCTITable() setting didUseJumpTable. Yes. I mean emitSwitchIntJump. And when calling ensureCTITable, we also set didUseJumpTable in all the cases. >>> Source/JavaScriptCore/ftl/FTLLink.cpp:-50 >>> - >> >> We do not move the content to CodeBlock when compiling FTL. This means that it is not set. We do not need to clear here. > > Can we ASSERT that it is not set? I suggest retaining the above B3 comment with the ASSERT. Added.
Created attachment 426037 [details] Patch
ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.
Committed r275995 (236547@main): <https://commits.webkit.org/236547@main>
<rdar://problem/76681916>