RESOLVED FIXED 107625
Reduce the number of calls to Functional and number of IPC messages by sending the created/deleted layers in a vector.
https://bugs.webkit.org/show_bug.cgi?id=107625
Summary Reduce the number of calls to Functional and number of IPC messages by sendin...
Seulgi Kim
Reported 2013-01-22 21:16:55 PST
Currently, LayerTreeRenderer in UI Process creates a layer when CreateCompositingLayer message is received. When a lot of CreateCompositingLayer messages are sent in the same cycle, some layers are not created in the UIProcess, and causes crash when trying to use the omitted layers.
Attachments
Patch (9.79 KB, patch)
2013-01-22 21:26 PST, Seulgi Kim
no flags
Patch (9.97 KB, patch)
2013-01-23 00:57 PST, Seulgi Kim
no flags
Patch (9.97 KB, patch)
2013-01-23 04:01 PST, Seulgi Kim
no flags
Patch (10.07 KB, patch)
2013-01-23 04:18 PST, Seulgi Kim
no flags
Patch (10.00 KB, patch)
2013-01-23 17:32 PST, Seulgi Kim
no flags
Seulgi Kim
Comment 1 2013-01-22 21:18:05 PST
I tested http://black.company100.net/test/TC/leaves1000. And stack trace is below. ASSERTION FAILED: m_layers.contains(id) /home/sgkim126/Project/WebKit/Qt/Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h(123) : WebCore::GraphicsLayer* WebKit::LayerTreeRenderer::layerByID(WebKit::CoordinatedLayerID) 1 0x7fb4605b3142 /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit17LayerTreeRenderer9layerByIDEj+0x54) [0x7fb4605b3142] 2 0x7fb4605b4d69 /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit17LayerTreeRenderer16setLayerChildrenEjRKN3WTF6VectorIjLm0EEE+0x67) [0x7fb4605b4d69] 3 0x7fb4605b2763 /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN3WTF15FunctionWrapperIMN6WebKit17LayerTreeRendererEFvjRKNS_6VectorIjLm0EEEEEclEPS2_jS6_+0x69) [0x7fb4605b2763] 4 0x7fb4605b1ad1 /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN3WTF17BoundFunctionImplINS_15FunctionWrapperIMN6WebKit17LayerTreeRendererEFvjRKNS_6VectorIjLm0EEEEEEFvPS3_jS5_EEclEv+0x73) [0x7fb4605b1ad1] 5 0x7fb4605b7cc0 /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZNK3WTF8FunctionIFvvEEclEv+0x72) [0x7fb4605b7cc0] 6 0x7fb4605b7280 /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit17LayerTreeRenderer17syncRemoteContentEv+0x9e) [0x7fb4605b7280] 7 0x7fb4605b3e5b /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit17LayerTreeRenderer23paintToCurrentGLContextERKN7WebCore20TransformationMatrixEfRKNS1_9FloatRectEj+0x109) [0x7fb4605b3e5b] 8 0x7fb4607eaacd /home/sgkim126/Project/WebKit/Qt/WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit14ContentsSGNode6renderERKN13QSGRenderNode11RenderStateE+0x2a1) [0x7fb4607eaacd] 9 0x7fb45f941b48 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(_ZN18QSGDefaultRenderer11renderNodesEPKP7QSGNodei+0x208) [0x7fb45f941b48] 10 0x7fb45f942653 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(_ZN18QSGDefaultRenderer6renderEv+0x353) [0x7fb45f942653] 11 0x7fb45f948529 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(_ZN11QSGRenderer11renderSceneERK11QSGBindable+0x69) [0x7fb45f948529] 12 0x7fb45f948657 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(_ZN11QSGRenderer11renderSceneEv+0x17) [0x7fb45f948657] 13 0x7fb45f952724 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(_ZN10QSGContext15renderNextFrameEP11QSGRendererj+0x14) [0x7fb45f952724] 14 0x7fb45f9822ee /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(_ZN19QQuickWindowPrivate16renderSceneGraphERK5QSize+0x1be) [0x7fb45f9822ee] 15 0x7fb45fa6b397 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Quick.so.5(+0x230397) [0x7fb45fa6b397] 16 0x7fb45e854886 /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5(+0x8c886) [0x7fb45e854886] 17 0x7fb456240734 /usr/lib/nvidia-current-updates/libGL.so.1(+0xaa734) [0x7fb456240734]
Seulgi Kim
Comment 2 2013-01-22 21:26:38 PST
Rafael Brandao
Comment 3 2013-01-22 22:02:16 PST
Comment on attachment 184131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184131&action=review > Source/WebKit2/ChangeLog:11 > + are not created in the UIProcess, and causes crash when trying to use Hm, do you have any idea why they are not created on UIProcess? All messages are delivered? If so, then why they are not created? If not, shouldn't we try to fix that first? I'm not against merging all layers in one message, but it looks like we're going to hide out the real problem... what if we have many layers and they're all sent in the same cycle. I think we can meet this buggy state in many ways.
Seulgi Kim
Comment 4 2013-01-22 22:50:33 PST
(In reply to comment #3) > (From update of attachment 184131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184131&action=review > > > Source/WebKit2/ChangeLog:11 > > + are not created in the UIProcess, and causes crash when trying to use > > Hm, do you have any idea why they are not created on UIProcess? All messages are delivered? If so, then why they are not created? Some messages are not delivered. I checked sent messages and received messages, and confirmed some messages are not received. I saw this behavior in efl and qt so I think CoreIPC in unix has some problems. > If not, shouldn't we try to fix that first? I'm not against merging all layers in one message, but it looks like we're going to hide out the real problem... what if we have many layers and they're all sent in the same cycle. I think we can meet this buggy state in many ways. I agree that we have to fix the CoreIPC bug. But this patch is still valuable since it makes CoordinatedLayerTreeHost uses IPC efficiently. I'll file another bug for the CoreIPC bug.
Noam Rosenthal
Comment 5 2013-01-22 22:57:53 PST
Comment on attachment 184131 [details] Patch I would rather see a real fix than a workaround. I'm not convinced that this optimization is significant.
Seulgi Kim
Comment 6 2013-01-22 23:07:16 PST
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 184131 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=184131&action=review > > > > > Source/WebKit2/ChangeLog:11 > > > + are not created in the UIProcess, and causes crash when trying to use > > > > Hm, do you have any idea why they are not created on UIProcess? All messages are delivered? If so, then why they are not created? > > Some messages are not delivered. I checked sent messages and received messages, and confirmed some messages are not received. I saw this behavior in efl and qt so I think CoreIPC in unix has some problems. > > > If not, shouldn't we try to fix that first? I'm not against merging all layers in one message, but it looks like we're going to hide out the real problem... what if we have many layers and they're all sent in the same cycle. I think we can meet this buggy state in many ways. > > I agree that we have to fix the CoreIPC bug. But this patch is still valuable since it makes CoordinatedLayerTreeHost uses IPC efficiently. > > I'll file another bug for the CoreIPC bug. I filed the bug at https://bugs.webkit.org/show_bug.cgi?id=107633
Seulgi Kim
Comment 7 2013-01-22 23:20:10 PST
(In reply to comment #5) > (From update of attachment 184131 [details]) > I would rather see a real fix than a workaround. I'm not convinced that this optimization is significant. Yes, I got it. If you think this optimization is not significant, I'll close this bug.
Seulgi Kim
Comment 8 2013-01-23 00:23:42 PST
On second thoughts, this patch has enough values. Less messages improve performance not only just for IPC time, but also CoreIPC creates Functional before sending a message. bool Connection::sendMessage(MessageID messageID, PassOwnPtr<MessageEncoder> encoder, unsigned messageSendFlags) { / ...... / m_connectionQueue.dispatch(WTF::bind(&Connection::sendOutgoingMessages, this)); / ...... / } And CoordinatedLayerTreeHostProxy also creates Functional to create or delete layers. If CoordinatedLayerTreeHost send n messages in a cycle, CoreIPC and CoordinatedLayerTreeHostProxy creates 2 * n functional. And this patch reduced 2 * n Functionals to 2 Functional. IMHO it will not be an insignificant performance improve. And also it doesn't harms to readability. Could you think again, noam?
Noam Rosenthal
Comment 9 2013-01-23 00:26:08 PST
Sure, but don't write in the Changelog that it fixes the bug, because that's misleading. All it does is postpone the bug to a later time :)
Seulgi Kim
Comment 10 2013-01-23 00:57:37 PST
Noam Rosenthal
Comment 11 2013-01-23 01:52:32 PST
Comment on attachment 184176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184176&action=review This looks OK to me; But you need a WebKit2 owner to review. > Source/WebKit2/ChangeLog:21 > + Currently, the number of messages sent by CoordinatedLayerTreeHost is > + equal to the number of layers created/deleted even though they requested > + in the same cycle. > + It's not good since CoreIPC creates functional before sending messages, > + and CoordinatedLayerTreeHostProxy creates functional before > + create/delete layers. > + > + This patch makes CoordinatedLayerTreeHost send just one > + CreateCompositingLayers message and CoordinatedLayerTreeHostProxy create > + just one functional in a cycle. The same work has been done with > + DeleteCompositingLayers message. > + > + This patch will use IPC more efficiently and create less Functional. Please reword to: Reduce the number of calls to Functional and number of IPC messages by sending the created/deleted layers in a vector.
Seulgi Kim
Comment 12 2013-01-23 03:38:00 PST
(In reply to comment #11) Thank you.
Seulgi Kim
Comment 13 2013-01-23 04:01:52 PST
Seulgi Kim
Comment 14 2013-01-23 04:18:07 PST
Kenneth Rohde Christiansen
Comment 15 2013-01-23 14:52:53 PST
Needs WK2 owner review, but the patch is informally r+'ed by Noam.
Benjamin Poulain
Comment 16 2013-01-23 17:16:48 PST
Comment on attachment 184199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184199&action=review RS=me for WK2 if No'am says the change is correct. > Source/WebKit2/ChangeLog:6 > + Patch by Seulgi Kim <seulgikim@company100.net> on 2013-01-22 How did you end up with this in a ChangeLog? :)
Seulgi Kim
Comment 17 2013-01-23 17:32:14 PST
Seulgi Kim
Comment 18 2013-01-23 17:33:21 PST
(In reply to comment #16) Sorry, my mistake. Could you review it again?
WebKit Review Bot
Comment 19 2013-01-23 18:59:15 PST
Comment on attachment 184363 [details] Patch Clearing flags on attachment: 184363 Committed r140634: <http://trac.webkit.org/changeset/140634>
WebKit Review Bot
Comment 20 2013-01-23 18:59:20 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.