Bug 224472 – [JSC] Do not copy SimpleJumpTable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Do not copy SimpleJumpTable

[Yusuke Suzuki]

Created attachment 425834 [details]
Patch

[Yusuke Suzuki]

Created attachment 425836 [details]
Patch

[Yusuke Suzuki]

Created attachment 425845 [details]
Patch

[Yusuke Suzuki]

Created attachment 425942 [details]
Patch

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Yusuke Suzuki]

Crashing randomly via WebCore::RenderLayerCompositor::computeCompositingRequirements, and this is known issue.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 426037 [details]
Patch

[EWS]

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

[Yusuke Suzuki]

Committed r275995 (236547@main): <https://commits.webkit.org/236547@main>

[Radar WebKit Bug Importer]

<rdar://problem/76681916>