117637 2013-06-14 05:43:21 -0700 Improve the number of style resolutions for NodeRenderingContext 2013-07-02 23:24:39 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED DUPLICATE 118300 P2 Normal --- 74144 1 mmaerean mmaerean commit-queue esprehn+autocc simon.fraser stavila WebkitBugTracker oldest_to_newest 900520 0 mmaerean 2013-06-14 05:43:21 -0700 Answer Simon Fraser's concerns raised on https://bugs.webkit.org/show_bug.cgi?id=74144 that there could be less calls to styleForRenderer() in NodeRenderingContext. 900998 1 204813 mmaerean 2013-06-17 05:51:00 -0700 Created attachment 204813 patch This patch reduces the number of style resolutions (an expensive operation) by checking for moveToFlowThreadIfNeeded in 2 steps: * 1st step: we determine if the node is the child of a region. * 2nd step: after calling shouldCreateRenderer, we call moveToFlowThreadIfNeeded for all other nodes and, by that time, the style has already been computed. The number of style resolutions are as follows: WHEN OPENING STYLE RESOLUTIONS WITH THE _OLD_ CODE STYLE RESOLUTIONS WITH THE _PATCH_ http://www.whitehouse.gov/ 4027 3257 http://en.wikipedia.org/wiki/Main_Page 7513 5638 The results of running PerformanceTests/Parser/ are as follows: TEST UNIT NEW CODE DELTA OLD CODE /Parser/textarea-parsing:Runs runs/s 77.32 5.72% BETTER 73.14 /Parser/tiny-innerHTML:Runs runs/s 3.65 4.81% BETTER 3.48 /Parser/html-parser:Time ms 2423.15 0.49% BETTER 2435.15 /Parser/css-parser-yui:Runs runs/s 280.09 0.44% BETTER 278.87 /Parser/xml-parser:Runs runs/s 6.08 4.85% WORSE 6.39 /Parser/innerHTML-setter:Runs runs/s 185.71 4.61% WORSE 194.69 901053 2 204813 simon.fraser 2013-06-17 08:51:35 -0700 Comment on attachment 204813 patch View in context: https://bugs.webkit.org/attachment.cgi?id=204813&action=review I find it very hard to follow this code, because both Element and NodeRenderContext have moveToFlowThread* functions. I think some renaming first would improve understanding of the code. > Source/WebCore/dom/NodeRenderingContext.cpp:250 > + bool haveTestedForMoveToFlowThread = false; > + for (const Element* parentElement = m_node->parentElement(); parentElement; parentElement = parentElement->parentElement()) { You're adding a parent-chain walk here, which I assume will kick in for every renderer creation, which is bad too. 905567 3 stavila 2013-07-02 13:43:28 -0700 Considering https://bugs.webkit.org/show_bug.cgi?id=118300, is this issue still valid? 905704 4 mmaerean 2013-07-02 23:24:39 -0700 *** This bug has been marked as a duplicate of bug 118300 *** 204813 2013-06-17 05:51:00 -0700 2013-06-17 08:51:35 -0700 patch patch117637StyleNodeRenderingContext.diff text/plain 3961 mmaerean ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzMDEwMjhmLi42MTMzZjZjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzQg QEAKKzIwMTMtMDYtMTcgIE1paGFpIE1hZXJlYW4gIDxtbWFlcmVhbkBhZG9iZS5jb20+CisKKyAg ICAgICAgSW1wcm92ZSB0aGUgbnVtYmVyIG9mIHN0eWxlIHJlc29sdXRpb25zIGZvciBOb2RlUmVu ZGVyaW5nQ29udGV4dAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTE3NjM3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgVGhpcyBwYXRjaCByZWR1Y2VzIHRoZSBudW1iZXIgb2Ygc3R5bGUgcmVzb2x1dGlvbnMg KGFuIGV4cGVuc2l2ZSBvcGVyYXRpb24pIGJ5IGNoZWNraW5nIGZvcgorICAgICAgICBtb3ZlVG9G bG93VGhyZWFkSWZOZWVkZWQgaW4gMiBzdGVwczoKKyAgICAgICAgKiAxc3Qgc3RlcDogd2UgZGV0 ZXJtaW5lIGlmIHRoZSBub2RlIGlzIHRoZSBjaGlsZCBvZiBhIHJlZ2lvbi4KKyAgICAgICAgKiAy bmQgc3RlcDogYWZ0ZXIgY2FsbGluZyBzaG91bGRDcmVhdGVSZW5kZXJlciwgd2UgY2FsbCBtb3Zl VG9GbG93VGhyZWFkSWZOZWVkZWQgZm9yIGFsbCBvdGhlciBub2RlcyBhbmQsIGJ5CisgICAgICAg IHRoYXQgdGltZSwgdGhlIHN0eWxlIGhhcyBhbHJlYWR5IGJlZW4gY29tcHV0ZWQuCisKKyAgICAg ICAgVGhlIG51bWJlciBvZiBzdHlsZSByZXNvbHV0aW9ucyBhcmUgYXMgZm9sbG93czoKKyAgICAg ICAgV0hFTiBPUEVOSU5HICAgICAgICAgICAgICAgICAgICAgICAgICAgIFNUWUxFIFJFU09MVVRJ T05TIFdJVEggVEhFIF9PTERfIENPREUgICBTVFlMRSBSRVNPTFVUSU9OUyBXSVRIIFRIRSBfUEFU Q0hfCisgICAgICAgIGh0dHA6Ly93d3cud2hpdGVob3VzZS5nb3YvICAgICAgICAgICAgICA0MDI3 ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgMzI1NworICAgICAgICBodHRwOi8v ZW4ud2lraXBlZGlhLm9yZy93aWtpL01haW5fUGFnZSAgNzUxMyAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIDU2MzgKKworICAgICAgICBUaGUgcmVzdWx0cyBvZiBydW5uaW5nIFBl cmZvcm1hbmNlVGVzdHMvUGFyc2VyLyBhcmUgYXMgZm9sbG93czoKKyAgICAgICAgVEVTVCAgICAg ICAgICAgICAgICAgICAgICAgICAgICBVTklUICAgIE5FVyBDT0RFICAgIERFTFRBICAgICAgICAg ICBPTEQgQ09ERQorICAgICAgICAvUGFyc2VyL3RleHRhcmVhLXBhcnNpbmc6UnVucyAgIHJ1bnMv cyAgNzcuMzIgICAgICAgNS43MiUgQkVUVEVSICAgIDczLjE0CisgICAgICAgIC9QYXJzZXIvdGlu eS1pbm5lckhUTUw6UnVucyAgICAgcnVucy9zICAzLjY1ICAgICAgICA0LjgxJSBCRVRURVIgICAg My40OAorICAgICAgICAvUGFyc2VyL2h0bWwtcGFyc2VyOlRpbWUgICAgICAgIG1zICAgICAgMjQy My4xNSAgICAgMC40OSUgQkVUVEVSICAgIDI0MzUuMTUKKyAgICAgICAgL1BhcnNlci9jc3MtcGFy c2VyLXl1aTpSdW5zICAgICBydW5zL3MgIDI4MC4wOSAgICAgIDAuNDQlIEJFVFRFUiAgICAyNzgu ODcKKyAgICAgICAgL1BhcnNlci94bWwtcGFyc2VyOlJ1bnMgICAgICAgICBydW5zL3MgIDYuMDgg ICAgICAgIDQuODUlIFdPUlNFICAgICA2LjM5CisgICAgICAgIC9QYXJzZXIvaW5uZXJIVE1MLXNl dHRlcjpSdW5zICAgcnVucy9zICAxODUuNzEgICAgICA0LjYxJSBXT1JTRSAgICAgMTk0LjY5CisK KyAgICAgICAgKiBkb20vTm9kZVJlbmRlcmluZ0NvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6Tm9kZVJlbmRlcmluZ0NvbnRleHQ6OnBhcmVudFJlbmRlcmVyKToKKyAgICAgICAgKFdlYkNv cmU6Ok5vZGVSZW5kZXJpbmdDb250ZXh0OjpjcmVhdGVSZW5kZXJlckZvckVsZW1lbnRJZk5lZWRl ZCk6CisKIDIwMTMtMDYtMTQgIEFsbGFuIFNhbmRmZWxkIEplbnNlbiAgPGFsbGFuLmplbnNlbkBk aWdpYS5jb20+CiAKICAgICAgICAgW1F0XSBNYXAgcmV3aW5kIGFuZCBmYXN0IGZvcndhcmQga2V5 cyB0byBtYXRjaCBHb29nbGVUVgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZG9tL05vZGVS ZW5kZXJpbmdDb250ZXh0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Ob2RlUmVuZGVyaW5nQ29u dGV4dC5jcHAKaW5kZXggNTg0OTQyOC4uYTAwOGRjNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvZG9tL05vZGVSZW5kZXJpbmdDb250ZXh0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20v Tm9kZVJlbmRlcmluZ0NvbnRleHQuY3BwCkBAIC0xNzcsMTEgKzE3Nyw2IEBAIFJlbmRlck9iamVj dCogTm9kZVJlbmRlcmluZ0NvbnRleHQ6OnBhcmVudFJlbmRlcmVyKCkKICAgICBpZiAobV9wYXJl bnRGbG93UmVuZGVyZXIpCiAgICAgICAgIHJldHVybiBtX3BhcmVudEZsb3dSZW5kZXJlcjsKIAot ICAgIGlmIChtX25vZGUtPmlzRWxlbWVudE5vZGUoKSAmJiB0b0VsZW1lbnQobV9ub2RlKS0+bW92 ZVRvRmxvd1RocmVhZElzTmVlZGVkKG1fc3R5bGUpKSB7Ci0gICAgICAgIG1vdmVUb0Zsb3dUaHJl YWQoKTsKLSAgICAgICAgcmV0dXJuIG1fcGFyZW50Rmxvd1JlbmRlcmVyOwotICAgIH0KLQogICAg IHJldHVybiBtX3JlbmRlcmluZ1BhcmVudCA/IG1fcmVuZGVyaW5nUGFyZW50LT5yZW5kZXJlcigp IDogMDsKIH0KIApAQCAtMjQ5LDEzICsyNDQsMjggQEAgdm9pZCBOb2RlUmVuZGVyaW5nQ29udGV4 dDo6Y3JlYXRlUmVuZGVyZXJGb3JFbGVtZW50SWZOZWVkZWQoKQogCiAgICAgRWxlbWVudCogZWxl bWVudCA9IHRvRWxlbWVudChtX25vZGUpOwogCisgICAgLy8gTG9vayBmb3Igbm9kZXMgaW5zaWRl IGEgcmVnaW9uIHRoYXQgYXJlIGZsb3dlZCBpbnRvIGEgMm5kIHJlZ2lvbi4KKyAgICAvLyBUaGlz IGF2b2lkcyBkb2luZyBhIHN0eWxlIHJlc29sdXRpb24gZm9yIGFsbCBub2Rlcy4KKyAgICBib29s IGhhdmVUZXN0ZWRGb3JNb3ZlVG9GbG93VGhyZWFkID0gZmFsc2U7CisgICAgZm9yIChjb25zdCBF bGVtZW50KiBwYXJlbnRFbGVtZW50ID0gbV9ub2RlLT5wYXJlbnRFbGVtZW50KCk7IHBhcmVudEVs ZW1lbnQ7IHBhcmVudEVsZW1lbnQgPSBwYXJlbnRFbGVtZW50LT5wYXJlbnRFbGVtZW50KCkpIHsK KyAgICAgICAgaWYgKGNvbnN0IFJlbmRlck9iamVjdCogcmVuZGVyZXJQYXJlbnRFbGVtZW50ID0g cGFyZW50RWxlbWVudC0+cmVuZGVyZXIoKSkgeworICAgICAgICAgICAgaWYgKCFyZW5kZXJlclBh cmVudEVsZW1lbnQtPmNhbkhhdmVDaGlsZHJlbigpICYmIHJlbmRlcmVyUGFyZW50RWxlbWVudC0+ Y2FuRE9NQ2hpbGRyZW5IYXZlUmVuZGVyUGFyZW50KCkpIHsKKyAgICAgICAgICAgICAgICBtb3Zl VG9GbG93VGhyZWFkSWZOZWVkZWQoKTsKKyAgICAgICAgICAgICAgICBoYXZlVGVzdGVkRm9yTW92 ZVRvRmxvd1RocmVhZCA9IHRydWU7CisgICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAgICAg ICB9CisgICAgICAgIH0KKyAgICB9CisKICAgICBpZiAoIXNob3VsZENyZWF0ZVJlbmRlcmVyKCkp CiAgICAgICAgIHJldHVybjsKICAgICBpZiAoIW1fc3R5bGUpCiAgICAgICAgIG1fc3R5bGUgPSBl bGVtZW50LT5zdHlsZUZvclJlbmRlcmVyKCk7CiAgICAgQVNTRVJUKG1fc3R5bGUpOwogCi0gICAg bW92ZVRvRmxvd1RocmVhZElmTmVlZGVkKCk7CisgICAgLy8gVGhlIHVzdWFsIGNhc2Ugd2hlbiBl bGVtZW50cyBjb2xsZWN0ZWQgaW4gYSBmbG93IGFyZSBub3QgY2hpbGRyZW4gb2YgYSByZWdpb24u CisgICAgaWYgKCFoYXZlVGVzdGVkRm9yTW92ZVRvRmxvd1RocmVhZCkKKyAgICAgICAgbW92ZVRv Rmxvd1RocmVhZElmTmVlZGVkKCk7CiAKICAgICBpZiAoIWVsZW1lbnQtPnJlbmRlcmVySXNOZWVk ZWQoKnRoaXMpKQogICAgICAgICByZXR1cm47Cg==