111627 2013-03-06 14:41:21 -0800 recalcStyle() with a Detach styleDiff calculates style twice 2014-04-10 22:10:21 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 113123 115042 1 eric esprehn allan.jensen buildbot cmarcelo darin dglazkov esprehn+autocc esprehn hyatt koivisto macpherson menard ojan.autocc rniwa simon.fraser webkit.review.bot oldest_to_newest 849313 0 eric 2013-03-06 14:41:21 -0800 recalcStyle() with a Detach styleDiff calculates style twice As noted in: https://bugs.webkit.org/show_bug.cgi?id=111494#c16 This is extra-bad, because I believe using lazyAttach will always go down this code path. Fixing this could be as much as an 8% win on html5-full-render, if it cuts our style-resolves in half. :) 849465 1 191867 esprehn 2013-03-06 16:51:37 -0800 Created attachment 191867 Patch 849467 2 eric 2013-03-06 16:53:52 -0800 I'm not sure this is the solution we want to go with long-term. :) But it would appear to solve the bug. Do you have any perf data to show this is a win? 849479 3 rniwa 2013-03-06 16:58:06 -0800 There got to be a better way of doing this? 849487 4 esprehn 2013-03-06 16:59:26 -0800 (In reply to comment #2) > I'm not sure this is the solution we want to go with long-term. :) But it would appear to solve the bug. > > Do you have any perf data to show this is a win? I'm having issues getting the perf tests to not hang after running them, but I did a few runs and it didn't seem like it was a win at all on html5-full-render.html 849494 5 esprehn 2013-03-06 17:01:38 -0800 (In reply to comment #3) > There got to be a better way of doing this? It requires considerable amounts of plumbing since we'd need to pass the style from recalcStyle => reattach => attach => createRendererIfNeeded => NodeRenderingContext::createRendererForElementIfNeeded() which finally calls styleForRenderer(). I guess we could use the same class and use the style in createRendererForElementIfNeeded instead though which means StyleResolver doesn't need this knowledge, and it would work for PseudoElements too. 849495 6 191867 koivisto 2013-03-06 17:02:12 -0800 Comment on attachment 191867 Patch That's just too ugly. I have a patch somewhere that fixes this properly, by passing the style to attach(). 849503 7 191867 esprehn 2013-03-06 17:10:17 -0800 Comment on attachment 191867 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191867&action=review > Source/WebCore/css/StyleResolver.cpp:1388 > + return style; If we did this approach, this code should be inside NodeRenderingContext::createRendererForElementIfNeeded 849532 8 esprehn 2013-03-06 17:36:08 -0800 Ran perf tests for html5-full-render: With this caching: 3202.66 ± 0.23% 3205.94 ± 0.17% Without this caching: 3198.89 ± 0.24% 3202.49 ± 0.12% So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? 849545 9 191867 webkit.review.bot 2013-03-06 17:49:36 -0800 Comment on attachment 191867 Patch Attachment 191867 did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17062114 New failing tests: fast/css/pseudo-empty-dynamic-empty.html fast/events/drag-display-none-element.html fast/selectors/empty-element-made-non-empty.html 849550 10 191867 buildbot 2013-03-06 17:56:05 -0800 Comment on attachment 191867 Patch Attachment 191867 did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17062117 New failing tests: fast/css/pseudo-empty-dynamic-empty.html http/tests/misc/acid3.html fast/selectors/empty-element-made-non-empty.html 849994 11 allan.jensen 2013-03-07 05:34:57 -0800 (In reply to comment #8) > Ran perf tests for html5-full-render: > > With this caching: > 3202.66 ± 0.23% > 3205.94 ± 0.17% > > Without this caching: > 3198.89 ± 0.24% > 3202.49 ± 0.12% > > So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? It only caches in the sense that it will reuse the calculated style of a sibling if the elements are similar enough. To get a performance improvement you probably need to trigger a style that is unique. 850305 12 koivisto 2013-03-07 11:14:23 -0800 > So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? StyleResolver has multiple levels of caches. Also the case just doesn't generally get hit that much. When reattach() gets called the whole subtree under the element will get attached outside recalcStyle without hitting the double calculation case. I have yet to see a real case where this is practical problem. It would still be nice to fix it as it is silly. 850405 13 eric 2013-03-07 13:01:06 -0800 (In reply to comment #12) > > So, surprisingly, it doesn't seem to really matter. I guess there's caching stuff down inside StyleResolver already? > > Also the case just doesn't generally get hit that much. When reattach() gets called the whole subtree under the element will get attached outside recalcStyle without hitting the double calculation case. Ah! this is the key bit I was missing. My understanding is lazyAttach() puts us into this state all the time. But it doesn't end up showing up because we only double-style-calc for the root most element of a subtree. 850779 14 191867 buildbot 2013-03-07 19:34:00 -0800 Comment on attachment 191867 Patch Attachment 191867 did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17081252 New failing tests: fast/css/pseudo-empty-dynamic-empty.html fast/events/drag-display-none-element.html http/tests/misc/acid3.html fast/selectors/empty-element-made-non-empty.html 861148 15 esprehn 2013-03-21 23:38:41 -0700 (In reply to comment #14) > (From update of attachment 191867 [details]) > Attachment 191867 [details] did not pass mac-ews (mac): > Output: http://webkit-commit-queue.appspot.com/results/17081252 > > New failing tests: > fast/css/pseudo-empty-dynamic-empty.html > fast/events/drag-display-none-element.html > http/tests/misc/acid3.html > fast/selectors/empty-element-made-non-empty.html These failures have me pretty confused. Apparently when calling styleForRenderer() inside Element::recalcStyle we get a different style than we would when later calling it inside NodeRenderingContext. Specifically: style1 = styleForRenderer(); detach(); style2 = styleForRenderer(); These two styles don't have the same properties which breaks the idea of passing the style down through the reattach() call. @antti Do you know what's going on here? 861210 16 esprehn 2013-03-22 01:16:38 -0700 (In reply to comment #15) > (In reply to comment #14) > ... > > These failures have me pretty confused. Apparently when calling styleForRenderer() inside Element::recalcStyle we get a different style than we would when later calling it inside NodeRenderingContext. > > Specifically: > > style1 = styleForRenderer(); > detach(); > style2 = styleForRenderer(); > > These two styles don't have the same properties which breaks the idea of passing the style down through the reattach() call. > > @antti Do you know what's going on here? Hmm, I'm wrong, there's something else weird going on here I'm missing. :/ 861730 17 esprehn 2013-03-22 11:55:02 -0700 (In reply to comment #16) > ... > > Hmm, I'm wrong, there's something else weird going on here I'm missing. :/ Figured this out. We used to store the affectedBy bits in the RenderStyle, but now we store them in the ElementRareData and reset them in detach() which means calling styleForRenderer in recalcStyle sets a bunch of bits, but then detach() inside reattach() clears the affectedBy bits so we get a RenderStyle but none of the correct affectedBy bits. So we need to tell detach() not to clear the bits so we get the right values since we're not going to call styleForRenderer() again to set them. 861783 18 eric 2013-03-22 13:00:43 -0700 These are some of the side effects of style resolve which we'll need to make explicit. Dimitri and I were just musing about this yesterday. 861978 19 esprehn 2013-03-22 16:52:10 -0700 (In reply to comment #18) > These are some of the side effects of style resolve which we'll need to make explicit. Dimitri and I were just musing about this yesterday. Yeah. So not calling resetComputedStyle() inside detach() (from reattach) fixes some of the tests, but I'm still stuck for LayoutTests/fast/events/drag-display-none-element.html This test fails because the paragraph remains hidden. I tried not calling resetDynamicRestyleObservations but that doesn't seem to help. I need to figure out what other shared state change happened inside style recalc. 862042 20 esprehn 2013-03-22 17:58:25 -0700 This would be resolved by passing the context in Bug 113123 862046 21 esprehn 2013-03-22 18:01:23 -0700 (In reply to comment #19) > (In reply to comment #18) > > These are some of the side effects of style resolve which we'll need to make explicit. Dimitri and I were just musing about this yesterday. > > Yeah. So not calling resetComputedStyle() inside detach() (from reattach) fixes some of the tests, but I'm still stuck for LayoutTests/fast/events/drag-display-none-element.html > > This test fails because the paragraph remains hidden. I tried not calling resetDynamicRestyleObservations but that doesn't seem to help. > > I need to figure out what other shared state change happened inside style recalc. I figured this out btw, it's because SelectorChecker looks at the renderer for isDragging to set some state which is different after detach() since we lose the renderer. We should probably break that dependency. 879887 22 koivisto 2013-04-23 09:24:20 -0700 http://trac.webkit.org/changeset/148970 fixed the most common instance of this problem. 191867 2013-03-06 16:51:37 -0800 2013-03-07 19:34:00 -0800 Patch bug-111627-20130306164742.patch text/plain 4516 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ1MDAwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWY0ZDIxNGI3ZTcyNmNl OTg2YWU2NWRkMDExYzNjNjE3OGZjN2ZjYy4uNWRiYWUyYTdkYjhmMjk1MzhmOWU1YzgxMWJjMzUy ZjJkNGRjMmZjNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDEzLTAzLTA2ICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIHJlY2FsY1N0eWxl KCkgd2l0aCBhIERldGFjaCBzdHlsZURpZmYgY2FsY3VsYXRlcyBzdHlsZSB0d2ljZQorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTExNjI3CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ2FjaGUgdGhlIGNvbXB1 dGVkIHN0eWxlIGJlZm9yZSBjYWxsaW5nIHJlYXR0YWNoKCkgaW4gRWxlbWVudDo6cmVjYWxjU3R5 bGUuCisgICAgICAgIFRoaXMgbWVhbnMgd2UgZG9uJ3QgbmVlZCB0byBkbyBzdHlsZSByZXNvbHV0 aW9uIHR3aWNlIGZvciBldmVyeSBsYXp5CisgICAgICAgIGF0dGFjaGVkIGVsZW1lbnQgZHVyaW5n IHBhZ2UgbG9hZC4KKworICAgICAgICBObyBuZXcgdGVzdHMsIG5vIGNoYW5nZSBpbiBiZWhhdmlv ci4KKworICAgICAgICAqIGNzcy9TdHlsZVJlc29sdmVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OlN0eWxlUmVzb2x2ZXI6OnN0eWxlRm9yRWxlbWVudCk6CisgICAgICAgICogY3NzL1N0eWxlUmVz b2x2ZXIuaDoKKyAgICAgICAgKENhY2hlZFN0eWxlRm9yUmVhdHRhY2gpOgorICAgICAgICAoV2Vi Q29yZTo6Q2FjaGVkU3R5bGVGb3JSZWF0dGFjaDo6Q2FjaGVkU3R5bGVGb3JSZWF0dGFjaCk6Cisg ICAgICAgIChXZWJDb3JlOjpDYWNoZWRTdHlsZUZvclJlYXR0YWNoOjp+Q2FjaGVkU3R5bGVGb3JS ZWF0dGFjaCk6CisgICAgICAgIChXZWJDb3JlOjpDYWNoZWRTdHlsZUZvclJlYXR0YWNoOjpzdHls ZUZvckVsZW1lbnQpOgorICAgICAgICAqIGRvbS9FbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkVsZW1lbnQ6OnJlY2FsY1N0eWxlKToKKwogMjAxMy0wMy0wNiAgTGV2aSBXZWludHJhdWIg IDxsZXZpd0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgSW5zZXJ0VW5vcmRlcmVkTGlzdCBjYW4g bGVhZCB0byBsb3N0IGNvbnRlbnQgYW5kIGFzc2VydGlvbnMgaW4gbW92ZVBhcmFncmFwaHMKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZVJlc29sdmVyLmNwcCBiL1NvdXJjZS9X ZWJDb3JlL2Nzcy9TdHlsZVJlc29sdmVyLmNwcAppbmRleCBlZWFlMWI2YmFhNmRiOWUxMmYxNjli ZTRlZTk2NjRhYzdiNDRlZmI0Li4xNTNjOWQ5MDkzMDUxODQ0M2Q5YmRiYmI5NGUzMGNjNjliNWRj NGNkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuY3BwCkBAIC0xOTUsNiArMTk1LDkg QEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiB1c2luZyBuYW1lc3BhY2UgSFRNTE5hbWVzOwogCitF bGVtZW50KiBDYWNoZWRTdHlsZUZvclJlYXR0YWNoOjpzX2VsZW1lbnQgPSAwOworUmVuZGVyU3R5 bGUqIENhY2hlZFN0eWxlRm9yUmVhdHRhY2g6OnNfc3R5bGUgPSAwOworCiAjZGVmaW5lIEhBTkRM RV9JTkhFUklUKHByb3AsIFByb3ApIFwKIGlmIChpc0luaGVyaXQpIHsgXAogICAgIG1fc3RhdGUu c3R5bGUoKS0+c2V0IyNQcm9wKG1fc3RhdGUucGFyZW50U3R5bGUoKS0+cHJvcCgpKTsgXApAQCAt MTM4MSw2ICsxMzg0LDkgQEAgc3RhdGljIGlubGluZSBib29sIGlzQXRTaGFkb3dCb3VuZGFyeShj b25zdCBFbGVtZW50KiBlbGVtZW50KQogUGFzc1JlZlB0cjxSZW5kZXJTdHlsZT4gU3R5bGVSZXNv bHZlcjo6c3R5bGVGb3JFbGVtZW50KEVsZW1lbnQqIGVsZW1lbnQsIFJlbmRlclN0eWxlKiBkZWZh dWx0UGFyZW50LAogICAgIFN0eWxlU2hhcmluZ0JlaGF2aW9yIHNoYXJpbmdCZWhhdmlvciwgUnVs ZU1hdGNoaW5nQmVoYXZpb3IgbWF0Y2hpbmdCZWhhdmlvciwgUmVuZGVyUmVnaW9uKiByZWdpb25G b3JTdHlsaW5nKQogeworICAgIGlmIChSZW5kZXJTdHlsZSogc3R5bGUgPSBDYWNoZWRTdHlsZUZv clJlYXR0YWNoOjpzdHlsZUZvckVsZW1lbnQoZWxlbWVudCkpCisgICAgICAgIHJldHVybiBzdHls ZTsKKwogICAgIC8vIE9uY2UgYW4gZWxlbWVudCBoYXMgYSByZW5kZXJlciwgd2UgZG9uJ3QgdHJ5 IHRvIGRlc3Ryb3kgaXQsIHNpbmNlIG90aGVyd2lzZSB0aGUgcmVuZGVyZXIKICAgICAvLyB3aWxs IHZhbmlzaCBpZiBhIHN0eWxlIHJlY2FsYyBoYXBwZW5zIGR1cmluZyBsb2FkaW5nLgogICAgIGlm IChzaGFyaW5nQmVoYXZpb3IgPT0gQWxsb3dTdHlsZVNoYXJpbmcgJiYgIWVsZW1lbnQtPmRvY3Vt ZW50KCktPmhhdmVTdHlsZXNoZWV0c0xvYWRlZCgpICYmICFlbGVtZW50LT5yZW5kZXJlcigpKSB7 CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5oIGIvU291cmNl L1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuaAppbmRleCA0MmM5NWM5MWZkODRlM2YyYTNiYWU2 NjExMTUxMjQ2MmU4ZjAyMzk3Li4xNGRhMDhlN2M0NzBjZjk0ODY1ZmI3NjMwYjcwYzNiNWU3MWQx ZTMyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5oCisrKyBi L1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZVJlc29sdmVyLmgKQEAgLTE2NCw2ICsxNjQsMzAgQEAg cHVibGljOgogICAgIGNvbnN0IFNlbGVjdG9yQ2hlY2tlcjo6QmVoYXZpb3JBdEJvdW5kYXJ5IGJl aGF2aW9yQXRCb3VuZGFyeTsKIH07CiAKK2NsYXNzIENhY2hlZFN0eWxlRm9yUmVhdHRhY2ggewor cHVibGljOgorICAgIENhY2hlZFN0eWxlRm9yUmVhdHRhY2goRWxlbWVudCogZWxlbWVudCwgUmVu ZGVyU3R5bGUqIHN0eWxlKQorICAgIHsKKyAgICAgICAgc19lbGVtZW50ID0gZWxlbWVudDsKKyAg ICAgICAgc19zdHlsZSA9IHN0eWxlOworICAgIH0KKworICAgIH5DYWNoZWRTdHlsZUZvclJlYXR0 YWNoKCkKKyAgICB7CisgICAgICAgIHNfZWxlbWVudCA9IDA7CisgICAgICAgIHNfc3R5bGUgPSAw OworICAgIH0KKworICAgIHN0YXRpYyBSZW5kZXJTdHlsZSogc3R5bGVGb3JFbGVtZW50KGNvbnN0 IEVsZW1lbnQqIGVsZW1lbnQpCisgICAgeworICAgICAgICByZXR1cm4gZWxlbWVudCA9PSBzX2Vs ZW1lbnQgPyBzX3N0eWxlIDogMDsKKyAgICB9CisKK3ByaXZhdGU6CisgICAgc3RhdGljIEVsZW1l bnQqIHNfZWxlbWVudDsKKyAgICBzdGF0aWMgUmVuZGVyU3R5bGUqIHNfc3R5bGU7Cit9OworCiAv LyBUaGlzIGNsYXNzIHNlbGVjdHMgYSBSZW5kZXJTdHlsZSBmb3IgYSBnaXZlbiBlbGVtZW50IGJh c2VkIG9uIGEgY29sbGVjdGlvbiBvZiBzdHlsZXNoZWV0cy4KIGNsYXNzIFN0eWxlUmVzb2x2ZXIg ewogICAgIFdURl9NQUtFX05PTkNPUFlBQkxFKFN0eWxlUmVzb2x2ZXIpOyBXVEZfTUFLRV9GQVNU X0FMTE9DQVRFRDsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcCBi L1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcAppbmRleCA5NTMxODU1ZWVlMGZjNWNlODJk MmNmNWEzMzU4NmIzNzE5ZTE3MzlhLi4yYzhiMzlmNThmMDAzNjRmNjFkYzVjM2RmZDJjZDcxNzI5 YWRkYjc1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9kb20vRWxlbWVudC5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCkBAIC0xMzc5LDcgKzEzNzksOCBAQCB2b2lk IEVsZW1lbnQ6OnJlY2FsY1N0eWxlKFN0eWxlQ2hhbmdlIGNoYW5nZSkKICAgICAgICAgUmVmUHRy PFJlbmRlclN0eWxlPiBuZXdTdHlsZSA9IHN0eWxlRm9yUmVuZGVyZXIoKTsKICAgICAgICAgU3R5 bGVDaGFuZ2UgY2ggPSBOb2RlOjpkaWZmKGN1cnJlbnRTdHlsZS5nZXQoKSwgbmV3U3R5bGUuZ2V0 KCksIGRvY3VtZW50KCkpOwogICAgICAgICBpZiAoY2ggPT0gRGV0YWNoIHx8ICFjdXJyZW50U3R5 bGUpIHsKLSAgICAgICAgICAgIC8vIEZJWE1FOiBUaGUgc3R5bGUgZ2V0cyBjb21wdXRlZCB0d2lj ZSBieSBjYWxsaW5nIGF0dGFjaC4gV2UgY291bGQgZG8gYmV0dGVyIGlmIHdlIHBhc3NlZCB0aGUg c3R5bGUgYWxvbmcuCisgICAgICAgICAgICAvLyBGSVhNRTogVGhpcyBkb2Vzbid0IGNhY2hlIGZv ciBwc2V1ZG8gZWxlbWVudHMuCisgICAgICAgICAgICBDYWNoZWRTdHlsZUZvclJlYXR0YWNoIGNh Y2hlZFN0eWxlKHRoaXMsIG5ld1N0eWxlLmdldCgpKTsKICAgICAgICAgICAgIHJlYXR0YWNoKCk7 CiAgICAgICAgICAgICAvLyBhdHRhY2ggcmVjYWxjdWxhdGVzIHRoZSBzdHlsZSBmb3IgYWxsIGNo aWxkcmVuLiBObyBuZWVkIHRvIGRvIGl0IHR3aWNlLgogICAgICAgICAgICAgY2xlYXJOZWVkc1N0 eWxlUmVjYWxjKCk7Cg==