Bug 178934 – [DFG][FTL] Introduce StringSlice

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[DFG][FTL] Introduce StringSlice

[Yusuke Suzuki]

Created attachment 325150 [details]
Patch

[Saam Barati]

Comment on attachment 325150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325150&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2919
> +        if (argumentCountIncludingThis != 2)

Nit: even though they’re equivalent in this context, I think > 2 is more intuitive

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4309
> +        // end would be nullptr.

would => can

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4320
> +        LValue startIndex = m_out.select(m_out.greaterThanOrEqual(start, m_out.int32Zero),

These two boundary selections are equivalent if parameterized on start/end. Might be worth making a lambda

[Saam Barati]

Comment on attachment 325150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325150&action=review

> Source/JavaScriptCore/ChangeLog:13
> +        1. Empty string generation is accelerated. It is fully executed inlinely.

inlinely =>inline

[Yusuke Suzuki]

Comment on attachment 325150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325150&action=review

Thanks!

>> Source/JavaScriptCore/ChangeLog:13
>> +        1. Empty string generation is accelerated. It is fully executed inlinely.
> 
> inlinely =>inline

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2919
>> +        if (argumentCountIncludingThis != 2)
> 
> Nit: even though they’re equivalent in this context, I think > 2 is more intuitive

Changed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4309
>> +        // end would be nullptr.
> 
> would => can

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4320
>> +        LValue startIndex = m_out.select(m_out.greaterThanOrEqual(start, m_out.int32Zero),
> 
> These two boundary selections are equivalent if parameterized on start/end. Might be worth making a lambda

Yeah, nice. Fixed.

[Yusuke Suzuki]

Committed r224276: <https://trac.webkit.org/changeset/224276>

[Ryan Haddad]

This change introduced an assertion failure with LayoutTest inspector/formatting/formatting-javascript.html:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r224277%20(4602)/results.html

ASSERTION FAILED: graph.m_plan.weakReferences.contains(cell)
./ftl/FTLOutput.h(112) : LValue JSC::FTL::Output::weakPointer(DFG::Graph &, JSC::JSCell *)
1   0x10f31c9f0 WTFCrash
2   0x10e9abd12 JSC::FTL::Output::weakPointer(JSC::DFG::Graph&, JSC::JSCell*)
3   0x10e9288e9 JSC::FTL::(anonymous namespace)::LowerDFGToB3::compileStringSlice()
4   0x10e8ece68 JSC::FTL::(anonymous namespace)::LowerDFGToB3::compileNode(unsigned int)
5   0x10e8e7bf0 JSC::FTL::(anonymous namespace)::LowerDFGToB3::compileBlock(JSC::DFG::BasicBlock*)
6   0x10e8e3a56 JSC::FTL::(anonymous namespace)::LowerDFGToB3::lower()
7   0x10e8e274e JSC::FTL::lowerDFGToB3(JSC::FTL::State&)
8   0x10e728b37 JSC::DFG::Plan::compileInThreadImpl()
9   0x10e72367d JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*)
10  0x10e8b755e JSC::DFG::Worklist::ThreadBody::work()
11  0x10f32a3fe WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const
12  0x10f32a0dc WTF::Function<void ()>::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>::call()
13  0x10f35a8fe WTF::Function<void ()>::operator()() const
14  0x10f3a96ea WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*)
15  0x10f3af0d5 WTF::wtfThreadEntryPoint(void*)
16  0x7fff8536b99d _pthread_body
17  0x7fff8536b91a _pthread_body
18  0x7fff85369351 thread_start

The same assertion is also seen with 121 tests on the Debug JSC bot: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/1479

[Yusuke Suzuki]

Committed r224285: <https://trac.webkit.org/changeset/224285>

[Saam Barati]

(In reply to Yusuke Suzuki from comment #7)
> Committed r224285: <https://trac.webkit.org/changeset/224285>

Maybe it’s worth making FTLOutput’s weakPointer private and just friend lower’s weakPointer? Or maybe we can use naming to help prevent this mistake in the future in a case where it actually matters.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #8)
> (In reply to Yusuke Suzuki from comment #7)
> > Committed r224285: <https://trac.webkit.org/changeset/224285>
> 
> Maybe it’s worth making FTLOutput’s weakPointer private and just friend
> lower’s weakPointer? Or maybe we can use naming to help prevent this mistake
> in the future in a case where it actually matters.

It sound nice. I'll open a bug for that.

[Radar WebKit Bug Importer]

<rdar://problem/35567861>