Bug 262623 – Reduce indirection in Path / PathStream

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Dumez]

Reduce indirection in Path / PathStream. PathStream now olds the vector of segments directly instead of using another heap-allocated refcounted object to hold the segment vector. This refcounted object was used to facilitate sharing and copy on write. However, we can achieve the same thing by having the Path objects sharing the same PathStream and cloning the PathStream only on write.

[Chris Dumez]

Pull request: https://github.com/WebKit/WebKit/pull/18622

[EWS]

Committed 268923@main (4905aa0e3445): <https://commits.webkit.org/268923@main>

Reviewed commits have been landed. Closing PR #18622 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/116530088>

[Carlos Garcia Campos]

I'm seeing crashes now with MotionMark (just loading the page without starting any test) that seem related to this change:

#0  0x00007f4bffb79e18 in WebCore::PathStream::copy() const () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#1  0x00007f4bffb7bedc in WebCore::Path::ensurePlatformPathImpl() () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#2  0x00007f4bffb7c419 in WebCore::Path::platformPath() const () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#3  0x00007f4bffbbb575 in WebCore::Cairo::fillPath(WebCore::GraphicsContextCairo&, WebCore::Path const&, WebCore::Cairo::FillSource const&, WebCore::Cairo::ShadowState const&) ()
   from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#4  0x00007f4bfdceaa90 in Nicosia::PaintingContextCairo::ForPainting::replay(WTF::Vector<std::unique_ptr<Nicosia::PaintingOperation, std::default_delete<Nicosia::PaintingOperation> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#5  0x00007f4bfdcddcd5 in WTF::Detail::CallableWrapper<Nicosia::PaintingEngineThreaded::paint(WebCore::GraphicsLayer&, WTF::Ref<Nicosia::Buffer, WTF::RawPtrTraits<Nicosia::Buffer> >&&, WebCore::IntRect const&, WebCore::IntRect const&, WebCore::IntRect const&, float)::{lambda()#1}, void>::call() ()
   from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#6  0x00007f4bfdbdf40e in WTF::WorkerPool::Worker::work() () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#7  0x00007f4bfdb73053 in WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::{lambda()#1}, void>::call() ()
   from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#8  0x00007f4bfdba9ab6 in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#9  0x00007f4bfdc10df9 in WTF::wtfThreadEntryPoint(void*) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1
#10 0x00007f4bfb2a63ec in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:444
#11 0x00007f4bfb326a4c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #4)
> I'm seeing crashes now with MotionMark (just loading the page without
> starting any test) that seem related to this change:
> 
> #0  0x00007f4bffb79e18 in WebCore::PathStream::copy() const () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #1  0x00007f4bffb7bedc in WebCore::Path::ensurePlatformPathImpl() () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #2  0x00007f4bffb7c419 in WebCore::Path::platformPath() const () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #3  0x00007f4bffbbb575 in
> WebCore::Cairo::fillPath(WebCore::GraphicsContextCairo&, WebCore::Path
> const&, WebCore::Cairo::FillSource const&, WebCore::Cairo::ShadowState
> const&) ()
>    from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #4  0x00007f4bfdceaa90 in
> Nicosia::PaintingContextCairo::ForPainting::replay(WTF::Vector<std::
> unique_ptr<Nicosia::PaintingOperation,
> std::default_delete<Nicosia::PaintingOperation> >, 0ul,
> WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&) () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #5  0x00007f4bfdcddcd5 in
> WTF::Detail::CallableWrapper<Nicosia::PaintingEngineThreaded::paint(WebCore::
> GraphicsLayer&, WTF::Ref<Nicosia::Buffer, WTF::RawPtrTraits<Nicosia::Buffer>
> >&&, WebCore::IntRect const&, WebCore::IntRect const&, WebCore::IntRect
> const&, float)::{lambda()#1}, void>::call() ()
>    from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #6  0x00007f4bfdbdf40e in WTF::WorkerPool::Worker::work() () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #7  0x00007f4bfdb73053 in
> WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker
> const&)::{lambda()#1}, void>::call() ()
>    from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #8  0x00007f4bfdba9ab6 in
> WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #9  0x00007f4bfdc10df9 in WTF::wtfThreadEntryPoint(void*) () from
> /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/
> libWPEWebKit-2.0.so.1
> #10 0x00007f4bfb2a63ec in start_thread (arg=<optimized out>) at
> ./nptl/pthread_create.c:444
> #11 0x00007f4bfb326a4c in clone3 () at
> ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

I took a look but it is not immediately clear to me via code inspection what the issue is. Sadly, this is not something I can reproduce as we get no such crash with the Cocoa ports. I did my best to keep PathCairo working in my patch for glib-based ports but I guess I messed up something. If you could provide a bit more debug information, that would really be helpful.

[Carlos Garcia Campos]

It doesn't always happen here, so I think it might be related to threads. I haven't been able to reproduce it with threaded rendering disabled. I didn't expect as path could be used from multiple threads, though but I'm not familiar with that code.

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #6)
> It doesn't always happen here, so I think it might be related to threads. I
> haven't been able to reproduce it with threaded rendering disabled. I didn't
> expect as path could be used from multiple threads, though but I'm not
> familiar with that code.

Oh, maybe this:
```
class PathImpl : public RefCounted<PathImpl>
```

should subclass ThreadSafeRefCounted then?

If you can reproduce, even flakily, it'd be worth trying this change I think.

[Carlos Garcia Campos]

(In reply to Chris Dumez from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > It doesn't always happen here, so I think it might be related to threads. I
> > haven't been able to reproduce it with threaded rendering disabled. I didn't
> > expect as path could be used from multiple threads, though but I'm not
> > familiar with that code.
> 
> Oh, maybe this:
> ```
> class PathImpl : public RefCounted<PathImpl>
> ```
> 
> should subclass ThreadSafeRefCounted then?
> 
> If you can reproduce, even flakily, it'd be worth trying this change I think.

I haven't been able to reproduce it again with ThreadSafeRefCounted, so I'm confident that fixed it.

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Carlos Garcia Campos from comment #6)
> > > It doesn't always happen here, so I think it might be related to threads. I
> > > haven't been able to reproduce it with threaded rendering disabled. I didn't
> > > expect as path could be used from multiple threads, though but I'm not
> > > familiar with that code.
> > 
> > Oh, maybe this:
> > ```
> > class PathImpl : public RefCounted<PathImpl>
> > ```
> > 
> > should subclass ThreadSafeRefCounted then?
> > 
> > If you can reproduce, even flakily, it'd be worth trying this change I think.
> 
> I haven't been able to reproduce it again with ThreadSafeRefCounted, so I'm
> confident that fixed it.

Would mind doing the PR?

[Kimmo Kinnunen]

Re-opening for pull request https://github.com/apple/WebKit/pull/966

[EWS]

Committed 267815.630@safari-7617-branch (bc765d16baad): <https://commits.webkit.org/267815.630@safari-7617-branch>

Reviewed commits have been landed. Closing PR #966 and removing active labels.