[webkit-reviews] review granted: [Bug 224472] [JSC] Do not copy SimpleJumpTable : [Attachment 425942] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 224472: [JSC] Do not copy SimpleJumpTable
https://bugs.webkit.org/show_bug.cgi?id=224472

Attachment 425942: Patch

https://bugs.webkit.org/attachment.cgi?id=425942&action=review




--- Comment #6 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 425942
  --> https://bugs.webkit.org/attachment.cgi?id=425942
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_swi
tchJumpTables.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->unlinkedS
witchJumpTable(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.