100358 2012-10-25 02:24:43 -0700 [V8] Add histograms to measure V8 work done during window close and navigation 2012-10-25 15:15:16 -0700 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 100390 1 ulan ulan abarth arv haraken ojan oldest_to_newest 750582 0 ulan 2012-10-25 02:24:43 -0700 We need to track V8DOMWindowShell::clearForNavigation time to be able to quickly detect such performance issues as http://code.google.com/p/chromium/issues/detail?id=155270 750897 1 ojan 2012-10-25 09:09:32 -0700 Looking at the callers, I wonder if we actually want to instrument ScriptController::clearForNavigation. That seems to cover more of the code that we call pre navigating. I don't know this code at all though. 750905 2 ojan 2012-10-25 09:15:13 -0700 Digging a bit further, it looks like V8DOMWindowShell::clearForNavigation is only ever called by ScriptController::clearForNavigation, which is only ever called by ScriptController::clearWindowShell. Can we kill ScriptController::clearForNavigation? 750912 3 ojan 2012-10-25 09:19:14 -0700 Oh, I see. There's also a clearForClose. I suppose it's better self-documenting code? 750980 4 170685 ojan 2012-10-25 10:32:13 -0700 Created attachment 170685 Patch 751004 5 170685 abarth 2012-10-25 10:52:56 -0700 Comment on attachment 170685 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:119 > + WebKit::Platform::current()->histogramCustomCounts("ScriptController::~ScriptController", (currentTime() - start) * 1000, 0, 10000, 50); We have helper functions in HistogramSupport.h so that other ports can add their histogram backends if they desire. 751006 6 170685 abarth 2012-10-25 10:54:29 -0700 Comment on attachment 170685 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:162 > + WebKit::Platform::current()->histogramCustomCounts("ScriptController::reset", (currentTime() - start) * 1000, 0, 10000, 50); Is reset called from places other than clearForClose and clearWindowShell? It's a bit strange that you'll have several of these timers running at once. For example, in ~ScriptController, it looks like you'll have three running. 751018 7 170685 ojan 2012-10-25 10:59:45 -0700 Comment on attachment 170685 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review >> Source/WebCore/bindings/v8/ScriptController.cpp:119 >> + WebKit::Platform::current()->histogramCustomCounts("ScriptController::~ScriptController", (currentTime() - start) * 1000, 0, 10000, 50); > > We have helper functions in HistogramSupport.h so that other ports can add their histogram backends if they desire. Oh right. I'm a bit braindead this morning clearly. >> Source/WebCore/bindings/v8/ScriptController.cpp:162 >> + WebKit::Platform::current()->histogramCustomCounts("ScriptController::reset", (currentTime() - start) * 1000, 0, 10000, 50); > > Is reset called from places other than clearForClose and clearWindowShell? It's a bit strange that you'll have several of these timers running at once. For example, in ~ScriptController, it looks like you'll have three running. It's true. I could drop the reset one for now as it is a little redundant. Should I drop ~ScriptController too? The only extra thing that really measures is windowShell()->destroyGlobal(). 751029 8 ojan 2012-10-25 11:04:45 -0700 (In reply to comment #7) > (From update of attachment 170685 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review > > >> Source/WebCore/bindings/v8/ScriptController.cpp:162 > >> + WebKit::Platform::current()->histogramCustomCounts("ScriptController::reset", (currentTime() - start) * 1000, 0, 10000, 50); > > > > Is reset called from places other than clearForClose and clearWindowShell? It's a bit strange that you'll have several of these timers running at once. For example, in ~ScriptController, it looks like you'll have three running. > > It's true. I could drop the reset one for now as it is a little redundant. Should I drop ~ScriptController too? The only extra thing that really measures is windowShell()->destroyGlobal(). Now that I look, destroyGlobal literally just clears m_global. I'll remove this histogram too. 751122 9 170713 ojan 2012-10-25 12:36:08 -0700 Created attachment 170713 Patch 751125 10 170713 abarth 2012-10-25 12:37:36 -0700 Comment on attachment 170713 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170713&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:162 > + HistogramSupport::histogramCustomCounts("ScriptController::clearForClose", (currentTime() - start) * 1000, 0, 10000, 50); We usually use names more like WebCore.ScriptController.ClearForClose for histograms. 751133 11 ojan 2012-10-25 12:58:10 -0700 Committed r132517: <http://trac.webkit.org/changeset/132517> 170685 2012-10-25 10:32:13 -0700 2012-10-25 12:36:06 -0700 Patch bug-100358-20121025173047.patch text/plain 3328 ojan U3VidmVyc2lvbiBSZXZpc2lvbjogMTMyNDkwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMGQwOTFiY2NlMzc5NDQ0 NWI0MDNmYTg4MDNkZGYwYmUwOGNmZTA2Mi4uNWI1YWJjMWExMWU0ZmZiN2IxYjExNzEzMGYzMWFj MmYzMGIyYjhhMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACiAyMDEyLTEwLTI1ICBPamFu IFZhZmFpICA8b2phbkBjaHJvbWl1bS5vcmc+CiAKKyAgICAgICAgW1Y4XSBBZGQgaGlzdG9ncmFt cyB0byBtZWFzdXJlIFY4IHdvcmsgZG9uZSBkdXJpbmcgd2luZG93IGNsb3NlIGFuZCBuYXZpZ2F0 aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDAz NTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGJp bmRpbmdzL3Y4L1NjcmlwdENvbnRyb2xsZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0 Q29udHJvbGxlcjo6flNjcmlwdENvbnRyb2xsZXIpOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0 Q29udHJvbGxlcjo6cmVzZXQpOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0Q29udHJvbGxlcjo6 Y2xlYXJGb3JDbG9zZSk6CisgICAgICAgIChXZWJDb3JlOjpTY3JpcHRDb250cm9sbGVyOjpjbGVh cldpbmRvd1NoZWxsKToKKworMjAxMi0xMC0yNSAgT2phbiBWYWZhaSAgPG9qYW5AY2hyb21pdW0u b3JnPgorCiAgICAgICAgIEdldCByaWQgb2YgU2NyaXB0Q29udHJvbGxlcjo6Y2xlYXJGb3JOYXZp Z2F0aW9uCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDAzOTAKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvU2NyaXB0Q29u dHJvbGxlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9TY3JpcHRDb250cm9sbGVy LmNwcAppbmRleCBhZTJlMjVlYjM2MWE3N2VkNDcxNTJjM2E2ODQ0ZjcxN2QxYjY1ZTZlLi41NTZk MGJjYTRiNDM1Njg4OTk1YjU1N2I2OWE1NDQ3YjY3ODI4NzA1IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9iaW5kaW5ncy92OC9TY3JpcHRDb250cm9sbGVyLmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS9iaW5kaW5ncy92OC9TY3JpcHRDb250cm9sbGVyLmNwcApAQCAtNjUsNiArNjUsOCBAQAog I2luY2x1ZGUgIlY4TlBPYmplY3QuaCIKICNpbmNsdWRlICJWOFJlY3Vyc2lvblNjb3BlLmgiCiAj aW5jbHVkZSAiV2lkZ2V0LmgiCisjaW5jbHVkZSA8cHVibGljL1BsYXRmb3JtLmg+CisjaW5jbHVk ZSA8d3RmL0N1cnJlbnRUaW1lLmg+CiAjaW5jbHVkZSA8d3RmL1N0ZExpYkV4dHJhcy5oPgogI2lu Y2x1ZGUgPHd0Zi9TdHJpbmdFeHRyYXMuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5nLmg+ CkBAIC0xMDksOCArMTExLDEyIEBAIFNjcmlwdENvbnRyb2xsZXI6OlNjcmlwdENvbnRyb2xsZXIo RnJhbWUqIGZyYW1lKQogCiBTY3JpcHRDb250cm9sbGVyOjp+U2NyaXB0Q29udHJvbGxlcigpCiB7 CisgICAgZG91YmxlIHN0YXJ0ID0gY3VycmVudFRpbWUoKTsKKwogICAgIHdpbmRvd1NoZWxsKCkt PmRlc3Ryb3lHbG9iYWwoKTsKICAgICBjbGVhckZvckNsb3NlKCk7CisKKyAgICBXZWJLaXQ6OlBs YXRmb3JtOjpjdXJyZW50KCktPmhpc3RvZ3JhbUN1c3RvbUNvdW50cygiU2NyaXB0Q29udHJvbGxl cjo6flNjcmlwdENvbnRyb2xsZXIiLCAoY3VycmVudFRpbWUoKSAtIHN0YXJ0KSAqIDEwMDAsIDAs IDEwMDAwLCA1MCk7CiB9CiAKIHZvaWQgU2NyaXB0Q29udHJvbGxlcjo6Y2xlYXJTY3JpcHRPYmpl Y3RzKCkKQEAgLTE0NCwxOCArMTUwLDI0IEBAIHZvaWQgU2NyaXB0Q29udHJvbGxlcjo6Y2xlYXJT Y3JpcHRPYmplY3RzKCkKIAogdm9pZCBTY3JpcHRDb250cm9sbGVyOjpyZXNldCgpCiB7CisgICAg ZG91YmxlIHN0YXJ0ID0gY3VycmVudFRpbWUoKTsKKwogICAgIGZvciAoSXNvbGF0ZWRXb3JsZE1h cDo6aXRlcmF0b3IgaXRlciA9IG1faXNvbGF0ZWRXb3JsZHMuYmVnaW4oKTsKICAgICAgICAgIGl0 ZXIgIT0gbV9pc29sYXRlZFdvcmxkcy5lbmQoKTsgKytpdGVyKSB7CiAgICAgICAgIGl0ZXItPnZh bHVlLT5kZXN0cm95SXNvbGF0ZWRTaGVsbCgpOwogICAgIH0KICAgICBtX2lzb2xhdGVkV29ybGRz LmNsZWFyKCk7CiAgICAgVjhHQ0NvbnRyb2xsZXI6OmhpbnRGb3JDb2xsZWN0R2FyYmFnZSgpOwor CisgICAgV2ViS2l0OjpQbGF0Zm9ybTo6Y3VycmVudCgpLT5oaXN0b2dyYW1DdXN0b21Db3VudHMo IlNjcmlwdENvbnRyb2xsZXI6OnJlc2V0IiwgKGN1cnJlbnRUaW1lKCkgLSBzdGFydCkgKiAxMDAw LCAwLCAxMDAwMCwgNTApOwogfQogCiB2b2lkIFNjcmlwdENvbnRyb2xsZXI6OmNsZWFyRm9yQ2xv c2UoKQogeworICAgIGRvdWJsZSBzdGFydCA9IGN1cnJlbnRUaW1lKCk7CiAgICAgcmVzZXQoKTsK ICAgICB3aW5kb3dTaGVsbCgpLT5jbGVhckZvckNsb3NlKCk7CisgICAgV2ViS2l0OjpQbGF0Zm9y bTo6Y3VycmVudCgpLT5oaXN0b2dyYW1DdXN0b21Db3VudHMoIlNjcmlwdENvbnRyb2xsZXI6OmNs ZWFyRm9yQ2xvc2UiLCAoc3RhcnQgLSBjdXJyZW50VGltZSgpKSAqIDEwMDAsIDAsIDEwMDAwLCA1 MCk7CiB9CiAKIHZvaWQgU2NyaXB0Q29udHJvbGxlcjo6dXBkYXRlU2VjdXJpdHlPcmlnaW4oKQpA QCAtNjI3LDEwICs2MzksMTIgQEAgTlBPYmplY3QqIFNjcmlwdENvbnRyb2xsZXI6OmNyZWF0ZVNj cmlwdE9iamVjdEZvclBsdWdpbkVsZW1lbnQoSFRNTFBsdWdJbkVsZW1lbnQKIAogdm9pZCBTY3Jp cHRDb250cm9sbGVyOjpjbGVhcldpbmRvd1NoZWxsKERPTVdpbmRvdyosIGJvb2wpCiB7CisgICAg ZG91YmxlIHN0YXJ0ID0gY3VycmVudFRpbWUoKTsKICAgICByZXNldCgpOwogICAgIC8vIFY4IGJp bmRpbmcgZXhwZWN0cyBTY3JpcHRDb250cm9sbGVyOjpjbGVhcldpbmRvd1NoZWxsIG9ubHkgYmUg Y2FsbGVkCiAgICAgLy8gd2hlbiBhIGZyYW1lIGlzIGxvYWRpbmcgYSBuZXcgcGFnZS4gVGhpcyBj cmVhdGVzIGEgbmV3IGNvbnRleHQgZm9yIHRoZSBuZXcgcGFnZS4KICAgICB3aW5kb3dTaGVsbCgp LT5jbGVhckZvck5hdmlnYXRpb24oKTsKKyAgICBXZWJLaXQ6OlBsYXRmb3JtOjpjdXJyZW50KCkt Pmhpc3RvZ3JhbUN1c3RvbUNvdW50cygiU2NyaXB0Q29udHJvbGxlcjo6Y2xlYXJXaW5kb3dTaGVs bCIsIChjdXJyZW50VGltZSgpIC0gc3RhcnQpICogMTAwMCwgMCwgMTAwMDAsIDUwKTsKIH0KIAog I2lmIEVOQUJMRShJTlNQRUNUT1IpCg== 170713 2012-10-25 12:36:08 -0700 2012-10-25 15:15:16 -0700 Patch bug-100358-20121025123441.patch text/plain 2462 ojan U3VidmVyc2lvbiBSZXZpc2lvbjogMTMyNTEzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMmU3MjMwNjM0YmQyMmMz MDcyNDQ0YzVhZTU4MzM5ZTBmZmZmMDQ4YS4uNWZlYTkwZmJlOGU3ZGM3NjY2YWZiNTRmZTI1ZmY4 ZDY4NTFlYzY2NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEyLTEwLTI1ICBPamFu IFZhZmFpICA8b2phbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW1Y4XSBBZGQgaGlzdG9ncmFt cyB0byBtZWFzdXJlIFY4IHdvcmsgZG9uZSBkdXJpbmcgd2luZG93IGNsb3NlIGFuZCBuYXZpZ2F0 aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDAz NTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGJp bmRpbmdzL3Y4L1NjcmlwdENvbnRyb2xsZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0 Q29udHJvbGxlcjo6Y2xlYXJGb3JDbG9zZSk6CisgICAgICAgIChXZWJDb3JlOjpTY3JpcHRDb250 cm9sbGVyOjpjbGVhcldpbmRvd1NoZWxsKToKKwogMjAxMi0xMC0yNSAgRG9taW5payBSw7Z0dHNj aGVzICA8ZG9taW5pay5yb3R0c2NoZXNAaW50ZWwuY29tPgogCiAgICAgICAgIENvbmRpdGlvbmFs aXplIFhIUiB0aW1lb3V0IHN1cHBvcnQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRp bmdzL3Y4L1NjcmlwdENvbnRyb2xsZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvdjgv U2NyaXB0Q29udHJvbGxlci5jcHAKaW5kZXggYWUyZTI1ZWIzNjFhNzdlZDQ3MTUyYzNhNjg0NGY3 MTdkMWI2NWU2ZS4uMWI2ZmVmMzdlZDMzMWIxZDM1YjZmOGJlMGI5OWFhZjQ2NzhlMWRlYSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvU2NyaXB0Q29udHJvbGxlci5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvU2NyaXB0Q29udHJvbGxlci5jcHAKQEAg LTQwLDYgKzQwLDcgQEAKICNpbmNsdWRlICJFdmVudE5hbWVzLmgiCiAjaW5jbHVkZSAiRnJhbWUu aCIKICNpbmNsdWRlICJGcmFtZUxvYWRlckNsaWVudC5oIgorI2luY2x1ZGUgIkhpc3RvZ3JhbVN1 cHBvcnQuaCIKICNpbmNsdWRlICJJbnNwZWN0b3JJbnN0cnVtZW50YXRpb24uaCIKICNpbmNsdWRl ICJOUE9iamVjdFdyYXBwZXIuaCIKICNpbmNsdWRlICJOUFY4T2JqZWN0LmgiCkBAIC02NSw2ICs2 Niw3IEBACiAjaW5jbHVkZSAiVjhOUE9iamVjdC5oIgogI2luY2x1ZGUgIlY4UmVjdXJzaW9uU2Nv cGUuaCIKICNpbmNsdWRlICJXaWRnZXQuaCIKKyNpbmNsdWRlIDx3dGYvQ3VycmVudFRpbWUuaD4K ICNpbmNsdWRlIDx3dGYvU3RkTGliRXh0cmFzLmg+CiAjaW5jbHVkZSA8d3RmL1N0cmluZ0V4dHJh cy5oPgogI2luY2x1ZGUgPHd0Zi90ZXh0L0NTdHJpbmcuaD4KQEAgLTE1NCw4ICsxNTYsMTAgQEAg dm9pZCBTY3JpcHRDb250cm9sbGVyOjpyZXNldCgpCiAKIHZvaWQgU2NyaXB0Q29udHJvbGxlcjo6 Y2xlYXJGb3JDbG9zZSgpCiB7CisgICAgZG91YmxlIHN0YXJ0ID0gY3VycmVudFRpbWUoKTsKICAg ICByZXNldCgpOwogICAgIHdpbmRvd1NoZWxsKCktPmNsZWFyRm9yQ2xvc2UoKTsKKyAgICBIaXN0 b2dyYW1TdXBwb3J0OjpoaXN0b2dyYW1DdXN0b21Db3VudHMoIlNjcmlwdENvbnRyb2xsZXI6OmNs ZWFyRm9yQ2xvc2UiLCAoY3VycmVudFRpbWUoKSAtIHN0YXJ0KSAqIDEwMDAsIDAsIDEwMDAwLCA1 MCk7CiB9CiAKIHZvaWQgU2NyaXB0Q29udHJvbGxlcjo6dXBkYXRlU2VjdXJpdHlPcmlnaW4oKQpA QCAtNjI3LDEwICs2MzEsMTIgQEAgTlBPYmplY3QqIFNjcmlwdENvbnRyb2xsZXI6OmNyZWF0ZVNj cmlwdE9iamVjdEZvclBsdWdpbkVsZW1lbnQoSFRNTFBsdWdJbkVsZW1lbnQKIAogdm9pZCBTY3Jp cHRDb250cm9sbGVyOjpjbGVhcldpbmRvd1NoZWxsKERPTVdpbmRvdyosIGJvb2wpCiB7CisgICAg ZG91YmxlIHN0YXJ0ID0gY3VycmVudFRpbWUoKTsKICAgICByZXNldCgpOwogICAgIC8vIFY4IGJp bmRpbmcgZXhwZWN0cyBTY3JpcHRDb250cm9sbGVyOjpjbGVhcldpbmRvd1NoZWxsIG9ubHkgYmUg Y2FsbGVkCiAgICAgLy8gd2hlbiBhIGZyYW1lIGlzIGxvYWRpbmcgYSBuZXcgcGFnZS4gVGhpcyBj cmVhdGVzIGEgbmV3IGNvbnRleHQgZm9yIHRoZSBuZXcgcGFnZS4KICAgICB3aW5kb3dTaGVsbCgp LT5jbGVhckZvck5hdmlnYXRpb24oKTsKKyAgICBIaXN0b2dyYW1TdXBwb3J0OjpoaXN0b2dyYW1D dXN0b21Db3VudHMoIlNjcmlwdENvbnRyb2xsZXI6OmNsZWFyV2luZG93U2hlbGwiLCAoY3VycmVu dFRpbWUoKSAtIHN0YXJ0KSAqIDEwMDAsIDAsIDEwMDAwLCA1MCk7CiB9CiAKICNpZiBFTkFCTEUo SU5TUEVDVE9SKQo=