WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(9.97 KB, patch)
2013-01-23 00:57 PST
,
Seulgi Kim
no flags
Details
Formatted Diff
Diff
Patch
(9.97 KB, patch)
2013-01-23 04:01 PST
,
Seulgi Kim
no flags
Details
Formatted Diff
Diff
Patch
(10.07 KB, patch)
2013-01-23 04:18 PST
,
Seulgi Kim
no flags
Details
Formatted Diff
Diff
Patch
(10.00 KB, patch)
2013-01-23 17:32 PST
,
Seulgi Kim
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 184131
[details]
Patch
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
Created
attachment 184176
[details]
Patch
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
Created
attachment 184197
[details]
Patch
Seulgi Kim
Comment 14
2013-01-23 04:18:07 PST
Created
attachment 184199
[details]
Patch
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
Created
attachment 184363
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug