Bug 100341

Our team is currently implementing threaded model of Coordinated Graphics in GTK+ port. We have updated the design document in the link below: https://docs.google.com/document/pub?id=1UoI1zk-6nTUFtz8i4evURM8aQIjkDRC8boO1zPdMMBg Our prototype for this implementation is shared in the GitHub. https://github.com/ryumiel/webkit-experimental The prototype is still in development, and only implemented up to step 1 in our design document. The purpose of sharing is to report our progress and make reviewers easier to understand the overview picture. Any comments/concerns are appreciated.

> Any comments/concerns are appreciated. OK, some comments/concerns :) First of all you guys are doing an astounding job with this, and I'm happy with any progress re. coordinated graphics, so don't get my architectural ideas as criticism... 1. GTK would run into the same problem we ran with with Qt when running this on embedded systems. With the "threading on the web process" approach, you still have to pass via a compositing step (e.g. XComposite on GLX) for every frame in a CSS animation. For desktop this is fine, for some platforms you'd find that the performance is unacceptable and the animations lag behind. We went through the trouble of writing IPC coordinated graphics because of this. 2. The optimizations we have in place today for fixed positioned elements and later for overflow-scroll would not work with the threaded model. 3. I think WebKit2 is the wrong place for this. If you really want to go down this route, I'd prefer it if we made TExtureMapper into more of a transaction/actor model, and make it threadable (it's already pretty thread safe), rather than trying to put threading code in WebKit2. It feels a little bit hacky to put something like LayerTreeCoordinatorClientThreaded.h inside WebProcess/WebPage/CoordinatedGraphics; Feels like all of this belongs in WebCore/.../texmap, and then we can make the IPC version work as a client that is implemented inside WebKit2. Also I don't see why this should not work on WebKit1. It feels like some of the architectural decisions are based on convenience of using existing code, but don't take the whole picture unto account :)

(In reply to comment #2) > OK, some comments/concerns :) > First of all you guys are doing an astounding job with this, and I'm happy with any progress re. coordinated graphics, so don't get my architectural ideas as criticism... Thank for your valuable feedback. Your concern is very reasonable. I’ll explain why we refactored Coordinated Graphics instead of GrahpicsLayerTextureMapper. > 1. GTK would run into the same problem we ran with with Qt when running this on embedded systems. With the "threading on the web process" approach, you still have to pass via a compositing step (e.g. XComposite on GLX) for every frame in a CSS animation. For desktop this is fine, for some platforms you'd find that the performance is unacceptable and the animations lag behind. We went through the trouble of writing IPC coordinated graphics because of this. We separated your first concern into two questions: 1. Does the "threading on the web process" approach cause an additional compositing step? 2. Is threaded coordinated graphics unacceptable because of unpredictable performance of sharing a surface between processes (e.g. XComposite)? The answer to the first question is “no” in GTK+. GTK+ main loop in UI Process needs to composite the result of TexMap and other UI toolkits, as QSGNode did. So we always need an additional compositing step for every frame, regardless of using IPC coordinated graphics or Threaded coordinated graphics. I’ll describe our opinion for the second question. If GTK+ uses IPC coordinated graphics, we would use some cairo way to share a texture between threads so that the main thread composites the result that painting thread painted. If GTK+ uses Threaded coordinated graphics, we need platform specific ways, something like XComposite, to share a surface between processes. I agree on your opinion that the performance of XComposite is unpredictable on some platforms. However, each platform has a cross process surface sharing mechanism for windows compositor and, IMHO, Threaded coordinated graphics can have similar performance to IPC coordinated graphics if we choose a proper surface sharing mechanism on the platform (e.g. EGLImage). How about your opinion, mrobinson and noam? p.s. You said the performance was unacceptable on embedded platforms. Could you explain in more detail? For example, which device or platform did you try on? Did you use XComposite or EGLImage? > 2. The optimizations we have in place today for fixed positioned elements and later for overflow-scroll would not work with the threaded model. I cannot fully understand how threaded coordinated graphics cannot satisfy some requirements that IPC coordinated graphics satisfies. Both implementations are very similar except for the message passing mechanism. Could you explain in more detail? > 3. I think WebKit2 is the wrong place for this. If you really want to go down this route, I'd prefer it if we made TExtureMapper into more of a transaction/actor model, and make it threadable (it's already pretty thread safe), rather than trying to put threading code in WebKit2. > > It feels a little bit hacky to put something like LayerTreeCoordinatorClientThreaded.h inside WebProcess/WebPage/CoordinatedGraphics; Feels like all of this belongs in WebCore/.../texmap, and then we can make the IPC version work as a client that is implemented inside WebKit2. Also I don't see why this should not work on WebKit1. > It feels like some of the architectural decisions are based on convenience of using existing code, but don't take the whole picture unto account :) Making a threaded version of GraphicsLayerTextureMapper is also a good idea. I agree that TextureMapperLayer is already pretty thread safe. However, if we focus on changing GraphicsLayerTextureMapper, we need to make new classes like GraphicsLayerThreadedTextureMapper, because we have to change GraphicsLayerTextureMapper a lot, and other clients (e.g. coordinated graphics) would not want GraphicsLayerTextureMapper to becomes complex. We thought GraphicsLayerThreadedTextureMapper will be very similar to CoordinatedGraphicsLayer after we add all features for threading. For example, if we use TiledBackingStore in GraphicsLayerThreadedTextureMapper to share backing store, GraphicsLayerThreadedTextureMapper will be similar to CoordinatedGraphicsLayer. Threaded model also need the class that plays a similar role to LayerTreeCoordinator, as chromium compositor has CCLayerTreeHost. So, we thought reusing is better than reinventing wheel. I absolutely agree that this can also work on WebKit1. Our next step is to make WebKit1 use Threaded coordinated graphics. We will need to move shareable code from WebKit2/coordinatedgraphics to platform/graphics/texmap/coordinatedgraphics to reuse them in WebKit1. Moreover, I think reusing coordinated graphics is more convenient for maintaining. For example, if we have another threaded model implementation, alex who made coordinated graphics cache custom filter programs would have to change the threaded model implementation too. I want to listen to your opinion. :)

For starters, we are going to implement threaded coordinated graphics in Qt WebKit1. Then, we will apply it to Gtk WebKit1 & 2.

(In reply to comment #4) > For starters, we are going to implement threaded coordinated graphics in Qt WebKit1. Then, we will apply it to Gtk WebKit1 & 2. I'm not sure if Qt actually needs threaded coordinated graphics. I need some clarifications if GTK is actually going to use the threaded coordinated graphics system before we continue with the patch-by-patch refactors.

(In reply to comment #5) > (In reply to comment #4) > > For starters, we are going to implement threaded coordinated graphics in Qt WebKit1. Then, we will apply it to Gtk WebKit1 & 2. > > I'm not sure if Qt actually needs threaded coordinated graphics. > I need some clarifications if GTK is actually going to use the threaded coordinated graphics system before we continue with the patch-by-patch refactors. Nowaday, I feel similar to you. I think refactorings until now have their own value. But we need to clarify goal.

(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > For starters, we are going to implement threaded coordinated graphics in Qt WebKit1. Then, we will apply it to Gtk WebKit1 & 2. > > > > I'm not sure if Qt actually needs threaded coordinated graphics. > > I need some clarifications if GTK is actually going to use the threaded coordinated graphics system before we continue with the patch-by-patch refactors. > > Nowaday, I feel similar to you. > I think refactorings until now have their own value. But we need to clarify goal. We already discussed about this issue in IRC, bugzilla, and mailing-lists. And we agreed to make threaded compositor in GTK, the goal of this project. I think refactoring is a by-product of this.

(In reply to comment #7) > We already discussed about this issue in IRC, bugzilla, and mailing-lists. > And we agreed to make threaded compositor in GTK, the goal of this project. > > I think refactoring is a by-product of this. I think a way to move forward here is to do all this work in an open branch (perhaps on github or in the WebKit repository itself). Then reviewers can work through the patches in the branch, understand them as a whole, and look at the performance changes. I would also really appreciate it if this work wasn't duplicated between two teams as it is now. Is there any way that both teams can work together to create one branch?

(In reply to comment #8) > I think a way to move forward here is to do all this work in an open branch (perhaps on github or in the WebKit repository itself). Then reviewers can work through the patches in the branch, understand them as a whole, and look at the performance changes. I would also really appreciate it if this work wasn't duplicated between two teams as it is now. Is there any way that both teams can work together to create one branch? Our plan was to finish refactoring WebKit Qt1 (https://bugs.webkit.org/show_bug.cgi?id=102994) in patch-by-patch review process, and then, work on a new branch for WebKit GTK+. IMHO, I think this is the right approach because current Qt refactoring patches has its own value, improving code maintainability. Moreover, this work is already in progress, and should be finished up before applying Threaded Coordinated Graphics on the GTK+ port. For WebKit GTK+, we will make a seperate branch so that the reviewers can understand them as a whole, and look at the performance changes. We will upload our work sometime before the end of this month (hopefully).

Created attachment 182799 [details] Proof-of-concept prototype. Not for commit. Hi, I have been working on a proof-of-concept prototype that implements a thread for compositing layers in the TextureMapperLayer. I am not fully aware of the final implementation proposed in this meta bug, but I believe the prototype follows roughly the same path. https://github.com/dnomi/webkit/tree/threaded-coordinated-gfx The idea is to just run TextureMapperLayer::paint() on a new thread, and to check the performance improvement that the threaded coordinated graphics will potentially give us on the WebKit2 GTK+ port. So the initial results are encouraging, suggesting an improvement of about 20%~30% in the frame rate when displaying some pages with WebGL on a Linux PC (e.g. http://code.google.com/p/webglsamples). I have also uploaded the patch here for reference and review. Comments are welcome.

Attachment 182799 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1 Source/WebCore/platform/graphics/texmap/TextureMapperLayerThread.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/platform/graphics/texmap/TextureMapperLayerThread.cpp:29: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/platform/graphics/texmap/TextureMapperLayerThread.cpp:69: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/graphics/texmap/TextureMapperLayerTask.cpp:26: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/WebCore/platform/graphics/texmap/TextureMapperLayerTask.cpp:83: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Source/WebCore/platform/graphics/texmap/TextureMapperLayerThread.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/platform/graphics/texmap/TextureMapperLayerTask.h:35: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/platform/graphics/cairo/GLContext.h:52: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 8 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 182799 [details] Proof-of-concept prototype. Not for commit. Attachment 182799 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15868950

Comment on attachment 182799 [details] Proof-of-concept prototype. Not for commit. Attachment 182799 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15902038

Comment on attachment 182799 [details] Proof-of-concept prototype. Not for commit. Attachment 182799 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15874962

Comment on attachment 182799 [details] Proof-of-concept prototype. Not for commit. Attachment 182799 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15905044

(In reply to comment #10) > Created an attachment (id=182799) [details] > Proof-of-concept prototype. Not for commit. > > Hi, I have been working on a proof-of-concept prototype that implements a thread for compositing layers in the TextureMapperLayer. I am not fully aware of the final implementation proposed in this meta bug, but I believe the prototype follows roughly the same path. > > https://github.com/dnomi/webkit/tree/threaded-coordinated-gfx > > The idea is to just run TextureMapperLayer::paint() on a new thread, and to check the performance improvement that the threaded coordinated graphics will potentially give us on the WebKit2 GTK+ port. > > So the initial results are encouraging, suggesting an improvement of about 20%~30% in the frame rate when displaying some pages with WebGL on a Linux PC (e.g. http://code.google.com/p/webglsamples). > > I have also uploaded the patch here for reference and review. Comments are welcome. Hi, Denis. Thanks for sharing your prototype with us! Our design of Threaded Coordinated Graphics is quite different from your prototype. As you can see in the design document, our plan is to refactor and reuse current Coordinated Graphics System by implementing communication mechanism between threads, instead of IPC. That's why we call this "Threaded Coordinated Graphics". It's still great to see a performance improvement just by running TextureMapperLayer::paint() on a new thread. Thank you for your interest in this project.

We are still heavily refactoring Coordinated Graphics to implement Threaded Coordinated Graphics, as we have mentioned several times. However, since refactoring Coordinated Graphics takes quite long, we have implemented a proof-of-concept prototype for WebKit2 GTK+. As we have expected, we saw a great performance improvement in PC. We have uploaded our prototype in github (https://github.com/DorothyBrowser/webkit/tree/threaded-compositor). The prototype implementation is based on the original WebKit GTK+ design document (https://docs.google.com/document/pub?id=1UoI1zk-6nTUFtz8i4evURM8aQIjkDRC8boO1zPdMMBg). The following chart shows the performance improvement compared to current WebKit2 GTK+. https://docs.google.com/document/pub?id=1UoI1zk-6nTUFtz8i4evURM8aQIjkDRC8boO1zPdMMBg#h.g7chzqe91w31 ASAP we finish refactoring Coordinated Graphics, we will reimplement our revised design adapted to the latest WebKit. Stay tuned!

We have presented our current project, "Threaded Coordinated Graphics", in LCA Browser Miniconf! http://mcc.id.au/BrowserMiniconf/LCA2013/Schedule#hwang We will upload a video of the presentation in a couple of weeks.

Created attachment 205840 [details] Patch

I guess this is obsolete now, closing.