Bug 36883 – Generic parallel painting in WebKit with Qt backend

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Zoltan Herczeg]

Created attachment 52157 [details]
An initial patch

Cpus supporting SMP are getting widespread even on embedded domains. Although WebKit is not designed SMP in mind, we could play around with different approaches.

This particular approach assign a rendering thread for each frame (using the tile storage as back buffer). The thread (will) get the primitive painting commands during the painting phase (like: paint a text using the given golor and coordinates), and perform the painting on a buffer. This buffer eventually send back to the main thread, where its content is copied to tile buffers.

The work is far from over, but I think the basic concept can already been seen. I would like to hear your opinion about the concept: like or dislike it. I am not sure this approach will be fruitful, maybe it is just too much effort for too little gain (if there will be a gain at all), and I need to know whether it is worth to follow this path or I should try to find another one.

The work can be followed here:
http://www.gitorious.org/+sedkit-developers/webkit/sedkit/commits/smp

[Zoltan Herczeg]

Created attachment 52413 [details]
Second patch

Now we can render to non-transparent (like www.google.hu) pages using the thread. The miracle is done using the QPicture thing, which can record the QPainter commands and replay them on the thread. Hopefully the recording is a cheap operation (except for some difficult resources like Images).

Fortunately, QPicture does not restore the painting state, so:

begin()
   translate(x, y)
   fillRect(0, 0, w, h)
end()

can be split to two QPictures:

begin()
   translate(x, y)
end()
begin()
   fillRect(0, 0, w, h)
end()

Or the second step can be executed "manually" (not using QPicture).

Therefore we could employ special handling for exceptions (like images), and use QPictures for all other cases.

Any thoughts would be appreciated!

[MORITA Hajime]

It looks cool!

I think adding thread object to the Frame class violates abstraction. 
It would be better if we could hide it behind GraphicsContext or BackingStore.

[Zoltan Herczeg]

Thank you for your feedbacks again!

Currently only simple pages can be painted with the help of QPicture, which is capable of recording QPainter events. Since the stream can be stored on the disk, this class contains all the necessary information for the replay. This is probably unnecessary for WebKit, since it uses only a subset of these commands, and keep everything in memory, so no need to store all the attributes in the stream, sometimes a reference to a WebKit object would be enough.

How the implementation works now:
 - a thread is created for each frame (the owner could be changed to a backing store). Qt implementation requires a back buffer, since the widget can be painted only during painting events. No idea about other ports. However, to avoid screen flickers, a back buffer would be useful on all implementations.
 - the communication happens through message queues. Since all messages are equally sized, we don't need to free them all the time, we keep maximum of n for faster message allocation. (n is a compile time constant)
 - since many allocators employs arenas for fast allocation, every resource is freed on the thread where they were allocated.

Since some other other ports are interested, I try to make the implementation less platform specific. To do this, the RenderContext messages will be transmitted instead of QPainters. I will write a simple wrapper for each of them. The "real" RenderContext will be created on the thread. Since the RenderContext has no virtual methods, we probably need to put an "if" inside the implementation:

RetVal RenderContext::anyEvent(args)
{
#if ENABLE(PAINTTHREAD)
    if (hasPaintThread()) // A pain thread :)
        paintThread()->anyEvent(args);
#endif
    // The original code executes inside the thread:
    ...
}

This is not set in stone, if you have a better idea, just tell me.

[Zoltan Herczeg]

Created attachment 52634 [details]
Third patch

This is the proposed layout of the platform independent version. Currently only the fillRect and drawText are overwritten, but it is working under Qt.

There is only one hack so far:
FontFallbackList.h:

    const SimpleFontData* primarySimpleFontData(const Font* f)
    {.
#if ENABLE(PAINTTHREAD)
        ASSERT(isMainThread() || m_cachedPrimarySimpleFontData);
#else
        ASSERT(isMainThread());
#endif
        ...
    }

What is the purpose of the following member in FontQt.cpp:

QFont Font::font() const
{
   QFont f = primaryFont()->getQtFont();
   ...
}

[Zoltan Herczeg]

I would like to give you a little summarization where I am now, and I also have some questions. Now, a simple page (like www.google.com) can be rendered without a render context (QPainter) in the main thread (the pointer is set to 0). Unfortunately the border is missing around the input text box and the radio button images as well. How these graphics controls are rendered? Are they special image resources or they rendered directly by some toolkit?

Since QPixmaps are not thread safe, I had to switch QImages, but too many changes were required in the trunk, unfortunately. I don't really like the code now, but I have no better idea.

On the good side, I can see smoother animations and faster scrolling in some cases, but the other side is true: a more responsive main thread can issue more frames to paint when you pull down the scrollbar. Many of these internal frames are not displayed at all, but the last frame have to wait until all internal frames were rendered. In other words, several performance optimizations can be employed later (like splitting the wiewport to multiple painting threads, clever viewport change hnadling, etc), but now I focus on the GraphicsContext side.

Most of the commands are fairly simple, one can easily convert them to messages. However, the story is different with resources, especially many of them are not ref-counted at all.

These are:
Image - Images are ref-counted, I can simply send them to the thread, and deref on the main-thread after they were rendered
Generator - used by FillRect. I decided to keep this rare function in the main thread.
ImageBuffer - used by clipToImageBuffer, not implemented in Qt. Probably I will not support it as well.
TextRun - Not ref-counted. The necessary part is duplicated for the thread.
Font - Not ref-counted. Although it seems Qt does not free them at all
Path - Not ref-counted, and probably freed early. This is a tough one. Shall I duplicate it or make it refcounted?

Another tough problem is the transformations, since getCTM() and origin() should return with something useful. My idea is creating a QStack<QTransform> array, and execute the transformations there (on the main thread). Is it possible to repeat the transformations as the QPainter would do them?

GraphicsContext::drawFocusRing(const Vector<Path>& paths, ... 
   // FIXME: implement
   why not use NotImplemented() here as well?

drawPath(), fillPath(), strokePath() - looks like platform dependent (two platforms use them). Should be moved out of the common part of GraphicsContext.

[Zoltan Herczeg]

Created attachment 53162 [details]
fourth patch

Phew, this patch has been grown to quite big! (120k now) Now it covers most of the GraphicsContext, and works well with the majority of the pages, even with svg drawings. I put an r? to get some feedback from the reviewers, not because it is a finished patch.

My current blocker is the RenderTheme: it turned out not only the GraphicsContext draws to the screen. Unfortunately the RenderTheme object is far more platform dependent than GraphicsContext. QPicture might be a useful workaround here, but it will be Qt specific. The RenderFrame base class is heavily dependent from RenderObject, RenderStyle and Element pointers, and would not worth to clone them for a thread. I don't see any platform independent solution here :( I would appreciate any suggestion!

[WebKit Review Bot]

Attachment 53162 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/ImageBufferData.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:58:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/graphics/PaintThreadMessage.h:371:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebhistory.cpp"
WebCore/platform/graphics/qt/GraphicsContextQt.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/GraphicsContext.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:69:  Declaration has space between type name and * in GraphicsContext *context  [whitespace/declaration] [3]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:71:  Declaration has space between type name and * in QPainter *painter  [whitespace/declaration] [3]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:79:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:118:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:190:  Should have a space between // and comment  [whitespace/comments] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp"
WebCore/platform/graphics/Image.h:64:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThread.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 15 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Herczeg]

Created attachment 53242 [details]
fifth patch

Ok, now the patch working nicely now. Many web pages can be rendered now, and Qt doing antialias on QImages, so the pages are nice. There are still missing GraphicsContext features though, I will finish them soon.

Unfortunately the RenderThemeQt puts QPixmaps to the QPicture, and it cannot be replayed on the thread. Do you have any idea to avoid these QPixmaps?

We can also experiment with differnt setups, like put the WebKit into a thread, and do all drawings in the gui thread, or something different. Anyway, is it really imposible to use QPixmaps in a thread? Is there a workaround for them? Creating multiple QApplication-s or something?

[WebKit Review Bot]

Attachment 53242 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/ImageBufferData.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:58:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/graphics/PaintThreadMessage.h:371:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebhistory.cpp"
WebCore/platform/graphics/qt/GraphicsContextQt.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/GraphicsContext.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:69:  Declaration has space between type name and * in GraphicsContext *context  [whitespace/declaration] [3]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:71:  Declaration has space between type name and * in QPainter *painter  [whitespace/declaration] [3]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:79:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:118:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:190:  Should have a space between // and comment  [whitespace/comments] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp"
WebCore/platform/graphics/Image.h:64:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThread.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 15 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Herczeg]

Created attachment 53327 [details]
sixth patch

Refactoring the message sending stubs.

Unimplemented functions:

fillRoundedRect

addRoundedRectClip - implemented as clip (but perhaps worth to send it instead of a path)
clipOutRoundedRect - implemented as clipOut
drawLineForText - implemented as drawLine in Qt

drawLineForMisspellingOrBadGrammar - not implemented in Qt
drawFocusRingPath - not implemented in Qt
setURLForRect - not implemented in Qt

[WebKit Review Bot]

Attachment 53327 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/ImageBufferData.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadMessage.h:63:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/graphics/PaintThreadMessage.h:412:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebhistory.cpp"
WebCore/platform/graphics/qt/GraphicsContextQt.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/GraphicsContext.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:107:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/PaintThreadRunLoop.cpp:451:  Should have a space between // and comment  [whitespace/comments] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp"
WebCore/platform/graphics/Image.h:64:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThread.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 15 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Herczeg]

Let me summarize my thoughts on this work. You can see fast, anti-aliased images on multicore systems, but I feel neither Qt nor WebKit is ready to fully utilize smp graphics now. However, this will surely change in the (hopefully near) future, and it is worth to play around this feature. However, I feel this is not a one-man can do project, I need others experiences since I touch platform independent code. Do you know experts who are good at Qt or WebKit graphics? Currently keepeng the non-refcounted resources (I don't want to duplicate everything) and RenderTheme issues my biggest problems (pixmap draw by QStyle).

[Adam Treat]

Comment on attachment 53327 [details]
sixth patch

r- for styling and mac build issues.  in general, Zoltan, are you ready to offer this up for real review?  if not please do not mark it as such.

[Zoltan Herczeg]

As I mentioned I just need someone to help me decide some design issues. I thought an r? could raise visibilty.

[Zoltan Herczeg]

Created attachment 53800 [details]
patch for GraphicsContext to support parallel painting

Since this work is too big to go in one patch, I stripped down everything except the core parallel GraphicsContext. It works under Qt, although it is slow, since it copies all resources (including images), just to touch as little as possible in other parts of WebKit. RenderTheme support is removed as well, no ScrollBars or Buttons are painted now. The patch is still big enough, no worries :)

And ready to review.

[WebKit Review Bot]

Attachment 53800 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/PaintThreadMessage.h:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/graphics/qt/GraphicsContextQt.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/PaintThread.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

On a high-level there are two things that come to my mind:

1) Rendering into a QPicture is very cheap. Replaying the QPicture on the real paint engine is likely to consume most of the painting time, as only at that point expensive operations such as path drawing and glyph rasterization happen. Therefore these operations are not parallelized.

2) I see three main "CPU" consumers in WebKit: JavaScript execution, document layout and painting. This approach parallelizes only the rendering, and in the Qt case only a smaller portion of it.

I admit this is where I like the WebKit2 model of doing the entire heavy lifting in either a secondary thread or a process.

My guts feeling is that the threaded approach is more interesting for embedding applications and that it would be nice to scale up the process model to more than one process (one per tab, like in chrome?) based on the WebKit2 work.

[Zoltan Herczeg]

1) I thought so. As QPicture is Qt specific, I asked the community whether they would prefer a less platform dependent solution, and some said yes. Since GraphicsContext and QPainter basically have the same purpose, I can record the GraphiscContext commands as well.

2) I know the first title was misleading. The focus of the project is speeding up the painting. Anti-aliasing is also applied to the images, which I don't get when drawing on the screen.

Still, I think this approach is a benefit regardless of the process model, would make both WebKit and WebKit2 model faster. Although the QPixmap drawing restrictions are still a blocker, but I hope Qt will eventually make it thread safe.

[Simon Hausmann]

(In reply to comment #18)
> 1) I thought so. As QPicture is Qt specific, I asked the community whether they
> would prefer a less platform dependent solution, and some said yes. Since
> GraphicsContext and QPainter basically have the same purpose, I can record the
> GraphiscContext commands as well.
> 
> 2) I know the first title was misleading. The focus of the project is speeding
> up the painting. Anti-aliasing is also applied to the images, which I don't get
> when drawing on the screen.
> 
> Still, I think this approach is a benefit regardless of the process model,
> would make both WebKit and WebKit2 model faster. Although the QPixmap drawing
> restrictions are still a blocker, but I hope Qt will eventually make it thread
> safe.

My point remains even with a platform-independent implementation of QPicture, doesn't it? Just recording the paint commands should be considerably less work than _actually_ rasterizing text, tesselating polygons and uploading textures.

What's the gain that remains when _that_ work is still done in the gui thread, blockingly?

Less threading related, but I wonder how well things work with "recording" WebKit's painting into GL display lists :)

[Zoltan Herczeg]

> My point remains even with a platform-independent implementation of QPicture,
> doesn't it? Just recording the paint commands should be considerably less work
> than _actually_ rasterizing text, tesselating polygons and uploading textures.

I am not sure I understand your point. The WebKit (main) thread does not rasterize anything in my patch. Just send the GraphicsContext commands to a paintihng thread. Such commands are "draw a rectangle" "draw a text", "clip to a path". It is nothing to with WebGl, textures are not handled. This approach advantage over QPicture is that I don't need to stream all data to a buffer, a reference counted pointer is enough to share a resource between threads. (Although now I simplifed the patch to copy most of the data, since it requires less changes in WebKit.)

> Less threading related, but I wonder how well things work with "recording"
> WebKit's painting into GL display lists :)

Do you want to drop the current 2D GraphicsContext and move to a fully OpenGL based rendering? OpenGL has a similar approach, true, and maybe a reasonable approach, although the clipping capabilities of OpenGL as far more limited than QPainter and such contexts.

[Zoltan Herczeg]

Now, that WebKit 2 Qt work started, there is a new, interesting opportunity for parallel painting. Its primary purpose is redirecting the paint events to some other thread. The WebKit 2 has a thread model, which is not yet supported by the Qt implementation because of QPixmap. Using parallel painting, we could redirect the painting events to the main thread, where we could use these QPixmaps without restrictions. What is your opinion?

[Adam Barth]

Comment on attachment 53800 [details]
patch for GraphicsContext to support parallel painting

Please do not nomiate patches for review that are not meant to be reviewed for landing.  This patch appears to be experimental rather than meant to for landing.  If you'd like feedback on your design, feel free to upload patches and CC the interested parties.

[Zoltan Herczeg]

(In reply to comment #22)
> (From update of attachment 53800 [details])
> Please do not nomiate patches for review that are not meant to be reviewed for landing.  This patch appears to be experimental rather than meant to for landing.  If you'd like feedback on your design, feel free to upload patches and CC the interested parties.

The previous patches has experimental parts, but the last patch has a clear, "touch what it is absolutely necessary" design (smallest step), which focusing on the non-RenderTheme painting redirection. Thus, it is absolutely ready for review.

[Zoltan Herczeg]

Created attachment 59631 [details]
Updated patch

[Benjamin Poulain]

Closing this bug, the work on threaded rendering is now part of WebKit 2.