Bug 178934 - [DFG][FTL] Introduce StringSlice
Summary: [DFG][FTL] Introduce StringSlice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-27 05:15 PDT by Yusuke Suzuki
Modified: 2017-11-15 12:36 PST (History)
9 users (show)

See Also:


Attachments
Patch (37.52 KB, patch)
2017-10-27 05:24 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-10-27 05:15:37 PDT
[DFG][FTL] Introduce StringSlice
Comment 1 Yusuke Suzuki 2017-10-27 05:24:28 PDT
Created attachment 325150 [details]
Patch
Comment 2 Saam Barati 2017-10-27 10:12:30 PDT
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
Comment 3 Saam Barati 2017-10-27 10:48:16 PDT
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
Comment 4 Yusuke Suzuki 2017-11-01 06:23:03 PDT
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.
Comment 5 Yusuke Suzuki 2017-11-01 06:25:39 PDT
Committed r224276: <https://trac.webkit.org/changeset/224276>
Comment 6 Ryan Haddad 2017-11-01 11:46:46 PDT
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
Comment 7 Yusuke Suzuki 2017-11-01 11:55:14 PDT
Committed r224285: <https://trac.webkit.org/changeset/224285>
Comment 8 Saam Barati 2017-11-01 13:01:31 PDT
(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.
Comment 9 Yusuke Suzuki 2017-11-08 06:11:37 PST
(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.
Comment 10 Radar WebKit Bug Importer 2017-11-15 12:36:31 PST
<rdar://problem/35567861>