101619 2012-11-08 09:51:04 -0800 Skip frame owner disconnect when there's no frames 2012-11-15 01:15:00 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 102350 1 esprehn esprehn abarth ap eric haraken ojan webkit.review.bot oldest_to_newest 761892 0 esprehn 2012-11-08 09:51:04 -0800 Skip frame owner disconnect when there's no frames 761894 1 173059 esprehn 2012-11-08 09:56:58 -0800 Created attachment 173059 Patch 761896 2 173059 ojan 2012-11-08 09:59:01 -0800 Comment on attachment 173059 Patch Looks great! I'm curious to see if we'll see benefit on any of the perf tests. 761899 3 esprehn 2012-11-08 10:01:04 -0800 (In reply to comment #2) > (From update of attachment 173059 [details]) > Looks great! I'm curious to see if we'll see benefit on any of the perf tests. Yeah. Complicated pages seem to always have frames on them though, even the Review Patch page has an iframe for the comment form! 761955 4 173059 abarth 2012-11-08 10:41:25 -0800 Comment on attachment 173059 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review > Source/WebCore/dom/ContainerNodeAlgorithms.h:280 > + // If we know there's no frames to disconnect then don't bother traversing > + // the tree looking for them. > + Page* page = root->document()->page(); > + if (page && !page->subframeCount()) > + return; Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? 761959 5 173059 ojan 2012-11-08 10:44:36 -0800 Comment on attachment 173059 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review >> Source/WebCore/dom/ContainerNodeAlgorithms.h:280 >> + return; > > Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? Page is what has the data at the moment. Long-term plan is to add a count of frames in the subtree to NodeRareData and only do this traversal if the subtree has any frames. 761967 6 esprehn 2012-11-08 10:49:14 -0800 (In reply to comment #4) > (From update of attachment 173059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review > > > Source/WebCore/dom/ContainerNodeAlgorithms.h:280 > > + // If we know there's no frames to disconnect then don't bother traversing > > + // the tree looking for them. > > + Page* page = root->document()->page(); > > + if (page && !page->subframeCount()) > > + return; > > Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? Yeah it is, but we don't track that currently. This is the first step in a longer road to optimization. 761970 7 esprehn 2012-11-08 10:50:47 -0800 (In reply to comment #6) > (In reply to comment #4) > ... > > Why do we need to do this on a per-page basis? Isn't it enough to know that the current document has no subframes? > > Yeah it is, but we don't track that currently. This is the first step in a longer road to optimization. Actually, maybe I can just use document()->frame()->tree()->firstChild() to detect if there's subframes? 761976 8 173073 esprehn 2012-11-08 10:59:34 -0800 Created attachment 173073 Patch Use the frame tree instead. 761989 9 173073 abarth 2012-11-08 11:12:02 -0800 Comment on attachment 173073 Patch LGTM 761996 10 ojan 2012-11-08 11:21:09 -0800 Clever! I like it. 762069 11 ap 2012-11-08 12:37:36 -0800 How will this affect frames that are in shadow trees? 762070 12 173073 webkit.review.bot 2012-11-08 12:37:53 -0800 Comment on attachment 173073 Patch Clearing flags on attachment: 173073 Committed r133933: <http://trac.webkit.org/changeset/133933> 762071 13 webkit.review.bot 2012-11-08 12:37:57 -0800 All reviewed patches have been landed. Closing bug. 762073 14 abarth 2012-11-08 12:40:02 -0800 > How will this affect frames that are in shadow trees? Are those not added to the FrameTree? 762078 15 abarth 2012-11-08 12:42:51 -0800 It looks like they are added to the FrameTree, but they're skipped over in the "scoped" versions of these functions by testing Frame::inScope 762079 16 abarth 2012-11-08 12:43:56 -0800 @esprehn: Can you add a test for this case? 762115 17 esprehn 2012-11-08 13:28:40 -0800 (In reply to comment #16) > @esprehn: Can you add a test for this case? Sure. 763217 18 ojan 2012-11-09 11:59:31 -0800 This is a 9%-14% improvement on some of the dromaeo dom benchmarks! http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_appendChild&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_appendChild&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_insertBefore&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_insertBefore&trace=score&history=50 173059 2012-11-08 09:56:58 -0800 2012-11-08 10:59:15 -0800 Patch bug-101619-20121108095507.patch text/plain 1980 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzNzcwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMTc3NTg0NGIwYjY5MDMz M2M0ZDBkYTY0NGY2ZmIyM2Y2NmVhYmM1Mi4uMWE4MWZjZTE0MzY0MDNmNWYxYThjZTJjNmUwMmM2 YzIyZDhkNWFhNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTExLTA4ICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFNraXAgZnJhbWUg b3duZXIgZGlzY29ubmVjdCB3aGVuIHRoZXJlJ3Mgbm8gZnJhbWVzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDE2MTkKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBFdmVuIHdoZW4gdGhlcmUncyBubyBzdWJm cmFtZXMgaW4gdGhlIGRvY3VtZW50IHdlIHRyYXZlcnNlIGRvd24gZXZlcnkKKyAgICAgICAgc3Vi dHJlZSBvbiBOb2RlIHJlbW92YWwgbG9va2luZyBmb3IgZnJhbWVzIHRvIGRpc2Nvbm5lY3QuIFRo aXMgcGF0Y2gKKyAgICAgICAgY2hlY2tzIFBhZ2U6OnN1YmZyYW1lQ291bnQoKSB0byBza2lwIHRo aXMgdHJhdmVyc2FsLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cywgdGhpcyBqdXN0IHNob3J0IGNp cmN1aXRzIGNvZGUgZm9yIHNwZWVkLgorCisgICAgICAgICogZG9tL0NvbnRhaW5lck5vZGVBbGdv cml0aG1zLmg6CisgICAgICAgIChXZWJDb3JlOjpDaGlsZEZyYW1lRGlzY29ubmVjdG9yOjpDaGls ZEZyYW1lRGlzY29ubmVjdG9yKToKKwogMjAxMi0xMS0wNyAgU2VyZ2lvIFZpbGxhciBTZW5pbiAg PHN2aWxsYXJAaWdhbGlhLmNvbT4KIAogICAgICAgICBbUXRdIEltcGxlbWVudCBkZWxldGVDb29r aWUoKSBmb3IgcGVyc2lzdGVudCBzdG9yYWdlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9k b20vQ29udGFpbmVyTm9kZUFsZ29yaXRobXMuaCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Db250YWlu ZXJOb2RlQWxnb3JpdGhtcy5oCmluZGV4IGM0ZDBiZTRkZmI1ZmU5ZjExZjA3NmIyMjEzNWZkYjNi YzIwMjg1OWYuLmJiYmEzMjE2OGJiZWQ4NTNjYzYzYjFmMzliYjUxNWVlMjYxNGRhMTkgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2RvbS9Db250YWluZXJOb2RlQWxnb3JpdGhtcy5oCisrKyBi L1NvdXJjZS9XZWJDb3JlL2RvbS9Db250YWluZXJOb2RlQWxnb3JpdGhtcy5oCkBAIC0yNSw2ICsy NSw3IEBACiAjaW5jbHVkZSAiRG9jdW1lbnQuaCIKICNpbmNsdWRlICJIVE1MRnJhbWVPd25lckVs ZW1lbnQuaCIKICNpbmNsdWRlICJJbnNwZWN0b3JJbnN0cnVtZW50YXRpb24uaCIKKyNpbmNsdWRl ICJQYWdlLmgiCiAjaW5jbHVkZSA8d3RmL0Fzc2VydGlvbnMuaD4KIAogbmFtZXNwYWNlIFdlYkNv cmUgewpAQCAtMjcyLDYgKzI3MywxMSBAQCBwdWJsaWM6CiAgICAgZXhwbGljaXQgQ2hpbGRGcmFt ZURpc2Nvbm5lY3RvcihOb2RlKiByb290LCBTaG91bGRJbmNsdWRlUm9vdCBzaG91bGRJbmNsdWRl Um9vdCA9IEluY2x1ZGVSb290KQogICAgICAgICA6IG1fcm9vdChyb290KQogICAgIHsKKyAgICAg ICAgLy8gSWYgd2Uga25vdyB0aGVyZSdzIG5vIGZyYW1lcyB0byBkaXNjb25uZWN0IHRoZW4gZG9u J3QgYm90aGVyIHRyYXZlcnNpbmcKKyAgICAgICAgLy8gdGhlIHRyZWUgbG9va2luZyBmb3IgdGhl bS4KKyAgICAgICAgUGFnZSogcGFnZSA9IHJvb3QtPmRvY3VtZW50KCktPnBhZ2UoKTsKKyAgICAg ICAgaWYgKHBhZ2UgJiYgIXBhZ2UtPnN1YmZyYW1lQ291bnQoKSkKKyAgICAgICAgICAgIHJldHVy bjsKICAgICAgICAgY29sbGVjdERlc2NlbmRhbnQobV9yb290LCBzaG91bGRJbmNsdWRlUm9vdCk7 CiAgICAgICAgIHJvb3ROb2RlcygpLmFkZChtX3Jvb3QpOwogICAgIH0K 173073 2012-11-08 10:59:34 -0800 2012-11-08 12:37:53 -0800 Patch bug-101619-20121108105737.patch text/plain 2054 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzNzcwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMTc3NTg0NGIwYjY5MDMz M2M0ZDBkYTY0NGY2ZmIyM2Y2NmVhYmM1Mi4uZTk5OWMzMTBjODJjZWFmMTdiZmM1MDExYTQwNTFh OGIwYmY1OGQ0NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTExLTA4ICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFNraXAgZnJhbWUg b3duZXIgZGlzY29ubmVjdCB3aGVuIHRoZXJlJ3Mgbm8gZnJhbWVzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDE2MTkKKworICAgICAgICBSZXZpZXdl ZCBieSBPamFuIFZhZmFpLgorCisgICAgICAgIEV2ZW4gd2hlbiB0aGVyZSdzIG5vIHN1YmZyYW1l cyBpbiB0aGUgZG9jdW1lbnQgd2UgdHJhdmVyc2UgZG93biBldmVyeQorICAgICAgICBzdWJ0cmVl IG9uIE5vZGUgcmVtb3ZhbCBsb29raW5nIGZvciBmcmFtZXMgdG8gZGlzY29ubmVjdC4gVGhpcyBw YXRjaAorICAgICAgICBjaGVja3MgZG9jdW1lbnQoKS0+ZnJhbWUoKS0+dHJlZSgpLT5maXJzdENo aWxkKCkgdG8gc2tpcCB0aGlzIHRyYXZlcnNhbAorICAgICAgICBpZiB0aGVyZSdzIG5vIHN1YmZy YW1lcy4KKworICAgICAgICBObyBuZXcgdGVzdHMsIHRoaXMganVzdCBzaG9ydCBjaXJjdWl0cyBj b2RlIGZvciBzcGVlZC4KKworICAgICAgICAqIGRvbS9Db250YWluZXJOb2RlQWxnb3JpdGhtcy5o OgorICAgICAgICAoV2ViQ29yZTo6Q2hpbGRGcmFtZURpc2Nvbm5lY3Rvcjo6Q2hpbGRGcmFtZURp c2Nvbm5lY3Rvcik6CisKIDIwMTItMTEtMDcgIFNlcmdpbyBWaWxsYXIgU2VuaW4gIDxzdmlsbGFy QGlnYWxpYS5jb20+CiAKICAgICAgICAgW1F0XSBJbXBsZW1lbnQgZGVsZXRlQ29va2llKCkgZm9y IHBlcnNpc3RlbnQgc3RvcmFnZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZG9tL0NvbnRh aW5lck5vZGVBbGdvcml0aG1zLmggYi9Tb3VyY2UvV2ViQ29yZS9kb20vQ29udGFpbmVyTm9kZUFs Z29yaXRobXMuaAppbmRleCBjNGQwYmU0ZGZiNWZlOWYxMWYwNzZiMjIxMzVmZGIzYmMyMDI4NTlm Li45ODQwODg5N2IxYjA0MWE4ZmJjYTQ3ZjJlOGQ5YzM1YzVjMzU2MjkzIDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9kb20vQ29udGFpbmVyTm9kZUFsZ29yaXRobXMuaAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9kb20vQ29udGFpbmVyTm9kZUFsZ29yaXRobXMuaApAQCAtMjMsNiArMjMsNyBAQAog I2RlZmluZSBDb250YWluZXJOb2RlQWxnb3JpdGhtc19oCiAKICNpbmNsdWRlICJEb2N1bWVudC5o IgorI2luY2x1ZGUgIkZyYW1lLmgiCiAjaW5jbHVkZSAiSFRNTEZyYW1lT3duZXJFbGVtZW50Lmgi CiAjaW5jbHVkZSAiSW5zcGVjdG9ySW5zdHJ1bWVudGF0aW9uLmgiCiAjaW5jbHVkZSA8d3RmL0Fz c2VydGlvbnMuaD4KQEAgLTI3Miw2ICsyNzMsMTEgQEAgcHVibGljOgogICAgIGV4cGxpY2l0IENo aWxkRnJhbWVEaXNjb25uZWN0b3IoTm9kZSogcm9vdCwgU2hvdWxkSW5jbHVkZVJvb3Qgc2hvdWxk SW5jbHVkZVJvb3QgPSBJbmNsdWRlUm9vdCkKICAgICAgICAgOiBtX3Jvb3Qocm9vdCkKICAgICB7 CisgICAgICAgIC8vIElmIHdlIGtub3cgdGhlcmUncyBubyBmcmFtZXMgdG8gZGlzY29ubmVjdCB0 aGVuIGRvbid0IGJvdGhlciB0cmF2ZXJzaW5nCisgICAgICAgIC8vIHRoZSB0cmVlIGxvb2tpbmcg Zm9yIHRoZW0uCisgICAgICAgIEZyYW1lKiBmcmFtZSA9IHJvb3QtPmRvY3VtZW50KCktPmZyYW1l KCk7CisgICAgICAgIGlmIChmcmFtZSAmJiAhZnJhbWUtPnRyZWUoKS0+Zmlyc3RDaGlsZCgpKQor ICAgICAgICAgICAgcmV0dXJuOwogICAgICAgICBjb2xsZWN0RGVzY2VuZGFudChtX3Jvb3QsIHNo b3VsZEluY2x1ZGVSb290KTsKICAgICAgICAgcm9vdE5vZGVzKCkuYWRkKG1fcm9vdCk7CiAgICAg fQo=