| Summary: | Reduce the number of calls to Functional and number of IPC messages by sending the created/deleted layers in a vector. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Seulgi Kim <dev> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | benjamin, kenneth, noam, simon.fraser, webkit.review.bot, zeno | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Seulgi Kim
2013-01-22 21:16:55 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] Created attachment 184131 [details]
Patch
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. (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. 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.
(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 (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. 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?
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 :) Created attachment 184176 [details]
Patch
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. (In reply to comment #11) Thank you. Created attachment 184197 [details]
Patch
Created attachment 184199 [details]
Patch
Needs WK2 owner review, but the patch is informally r+'ed by Noam. 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? :) Created attachment 184363 [details]
Patch
(In reply to comment #16) Sorry, my mistake. Could you review it again? Comment on attachment 184363 [details] Patch Clearing flags on attachment: 184363 Committed r140634: <http://trac.webkit.org/changeset/140634> All reviewed patches have been landed. Closing bug. |