164517 2016-11-08 09:06:51 -0800 [GTK] Allow to use WebMemorySampler feature. 2017-08-30 06:03:22 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=91214 https://bugs.webkit.org/show_bug.cgi?id=176099 P2 Normal --- 1 clopez clopez achristensen berto bugs-noreply cgarcia commit-queue gustavo mcatanzaro mrobinson sam oldest_to_newest 1249051 0 clopez 2016-11-08 09:06:51 -0800 The WebMemorySampler records memory usage of WebProcess and UI Process. It also includes stats about JIT, JS heap, fastmalloc stats and application memory info from /proc/process_id/statm. This feature was added on r127195 <https://trac.webkit.org/r127195> for the EFL port. I would like to have this also on the GTK+ port, as I think its useful when debugging memory issues. 1249057 1 294163 clopez 2016-11-08 09:14:50 -0800 Created attachment 294163 Patch 1249060 2 commit-queue 2016-11-08 09:17:46 -0800 Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API 1249061 3 294163 mcatanzaro 2016-11-08 09:21:16 -0800 Comment on attachment 294163 Patch Could this feature be used to create an about://memory page for Epiphany, or is it only a debug tool? 1249065 4 clopez 2016-11-08 09:26:05 -0800 (In reply to comment #3) > Comment on attachment 294163 [details] > Patch > > Could this feature be used to create an about://memory page for Epiphany, or > is it only a debug tool? Right now, is only used as a debug tool. It prints the stats of each process on a separate file at /tmp. I watch it with a simple "tail -f /tmp/file" But I guess it can be used as basis for something like that. In an about://memory page you have to take the info from all the webkit related process and summarize or present that information in a more user friendly way. 1249251 5 294163 cgarcia 2016-11-09 00:34:23 -0800 Comment on attachment 294163 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294163&action=review Please don't land this, see my comments. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:284 > + static bool initializeMemorySampler = false; This should be initializedMemorySampler or it should be true initially. But anyway, if the memory sampler can be started/stopped per context, why are we enabling it based on a static variable? This means that it will be enable only for the first context created. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:285 > + static const char environmentVariable[] = "SAMPLE_MEMORY"; I don't think we need this static variable just to use the literal that is only used here. And the value should be something like WEBKIT_SAMPLE_MEMORY or something like that. I'm not sure an env var is the best way to expose this feature though, because it's also possible to stop the memory sampler, but we can't do that with env vars. I think this feature should be in the inspector, so that you can start/stop the memory sampler at any time. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:288 > + WKContextStartMemorySampler(toAPI(webkitWebContextGetProcessPool(webContext)), adoptWK(WKDoubleCreate(0.0)).get()); Do not use the C API, we already have priv->processPool here, you can do priv->processPool->startMemorySampler(0); and avoid all the C API mess here. If we keep the env vars model, we could pass in the variable the interval, that way it will sample the memory every second and stop after that interval, passing 0 would be infinite like this patch does. 1249561 6 clopez 2016-11-09 16:15:31 -0800 (In reply to comment #5) > Comment on attachment 294163 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294163&action=review > > Please don't land this, see my comments. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:284 > > + static bool initializeMemorySampler = false; > > This should be initializedMemorySampler or it should be true initially. But > anyway, if the memory sampler can be started/stopped per context, why are we > enabling it based on a static variable? This means that it will be enable > only for the first context created. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:285 > > + static const char environmentVariable[] = "SAMPLE_MEMORY"; > > I don't think we need this static variable just to use the literal that is > only used here. And the value should be something like WEBKIT_SAMPLE_MEMORY > or something like that. I'm not sure an env var is the best way to expose > this feature though, because it's also possible to stop the memory sampler, > but we can't do that with env vars. I think this feature should be in the > inspector, so that you can start/stop the memory sampler at any time. > I don't see how this feature would be useful or desired on the inspector. This is a very low-level debug feature for webkit developers, its not really intended for users or web developers as it is. The stats are printed on a file on the /tmp directory, and the most sane way to check them is with a terminal and a tail -f. So it just feels like an undesired overhead to have to deal with the webinspector to activate this. Probably when you want to use this you are on a remote session via SSH and you don't have an easy way to send input events to the browser. And activating the remote inspector just for this would be extra overhead. So I find way easier and more practical an environment variable. Also, I don't find useful the possibility of starting/stopping on demand the sampler, or running it for a determined interval of time. What is the purpose of doing that? The only useful usage I find for this feature is the one I wanted to implement. Just start the browser with a magic environment variable, and get all the memory stats dumped to a temporally file on /tmp each second until you close the browser. Watch that file with a tail -f. And that's it. What I would find useful is an about:memory page that allows the user to capture and show the memory stats in an user-friendly way. But that its a different history. First, I would like to have this small patch to dump the stats to /tmp to help myself (and I guess any other webkitgtk+ dev) when debugging a memory issue on webkit. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:288 > > + WKContextStartMemorySampler(toAPI(webkitWebContextGetProcessPool(webContext)), adoptWK(WKDoubleCreate(0.0)).get()); > > Do not use the C API, we already have priv->processPool here, you can do > priv->processPool->startMemorySampler(0); and avoid all the C API mess here. > If we keep the env vars model, we could pass in the variable the interval, > that way it will sample the memory every second and stop after that > interval, passing 0 would be infinite like this patch does. As explained before, passing 0 its done on purpose. What is the use case of only running the sampler for X seconds? 1249567 7 294304 clopez 2016-11-09 16:26:27 -0800 Created attachment 294304 Patch 1249709 8 cgarcia 2016-11-10 00:24:40 -0800 (In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 294163 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=294163&action=review > > > > Please don't land this, see my comments. > > > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:284 > > > + static bool initializeMemorySampler = false; > > > > This should be initializedMemorySampler or it should be true initially. But > > anyway, if the memory sampler can be started/stopped per context, why are we > > enabling it based on a static variable? This means that it will be enable > > only for the first context created. > > > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:285 > > > + static const char environmentVariable[] = "SAMPLE_MEMORY"; > > > > I don't think we need this static variable just to use the literal that is > > only used here. And the value should be something like WEBKIT_SAMPLE_MEMORY > > or something like that. I'm not sure an env var is the best way to expose > > this feature though, because it's also possible to stop the memory sampler, > > but we can't do that with env vars. I think this feature should be in the > > inspector, so that you can start/stop the memory sampler at any time. > > > > I don't see how this feature would be useful or desired on the inspector. I agree, the inspector already has a memory profiling tool to check the web sites memory. > This is a very low-level debug feature for webkit developers, its not really > intended for users or web developers as it is. > > The stats are printed on a file on the /tmp directory, and the most sane way > to check them is with a terminal and a tail -f. So it just feels like an > undesired overhead to have to deal with the webinspector to activate this. > Probably when you want to use this you are on a remote session via SSH and > you don't have an easy way to send input events to the browser. And > activating the remote inspector just for this would be extra overhead. Fair enough. > So I find way easier and more practical an environment variable. > > Also, I don't find useful the possibility of starting/stopping on demand the > sampler, or running it for a determined interval of time. What is the > purpose of doing that? You wouldn't need to stop your browser and start it again, and them stop again. I think Safari has a start/stop memory sampler option in the debug menu. Amnd for the interval, imagine you want to measure the memory used by ephy at start up, you run it for 30 seconds, for example, and then you don't want to continue measuring, or having to close ephy again to continue using it. I tend to think that when there's an option to pass an interval, and an option to stop the memory sampler they were added for a reason. > The only useful usage I find for this feature is the one I wanted to > implement. > > Just start the browser with a magic environment variable, and get all the > memory stats dumped to a temporally file on /tmp each second until you close > the browser. Watch that file with a tail -f. And that's it. Yes, that's a simple use case. > What I would find useful is an about:memory page that allows the user to > capture and show the memory stats in an user-friendly way. But that its a > different history. Yes, that's different thing. > First, I would like to have this small patch to dump the stats to /tmp to > help myself (and I guess any other webkitgtk+ dev) when debugging a memory > issue on webkit. > > > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:288 > > > + WKContextStartMemorySampler(toAPI(webkitWebContextGetProcessPool(webContext)), adoptWK(WKDoubleCreate(0.0)).get()); > > > > Do not use the C API, we already have priv->processPool here, you can do > > priv->processPool->startMemorySampler(0); and avoid all the C API mess here. > > If we keep the env vars model, we could pass in the variable the interval, > > that way it will sample the memory every second and stop after that > > interval, passing 0 would be infinite like this patch does. > > As explained before, passing 0 its done on purpose. > What is the use case of only running the sampler for X seconds? I explained it above. In any case none of these things were the reason for the r-, my main concern was the fact that we were enabling it only for the first web context created. 1249710 9 294304 cgarcia 2016-11-10 00:25:17 -0800 Comment on attachment 294304 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294304&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:286 > +#if ENABLE(MEMORY_SAMPLER) > + if (getenv("WEBKIT_SAMPLE_MEMORY")) > + priv->processPool->startMemorySampler(0); > +#endif A lot simpler, thanks! 1249717 10 294304 commit-queue 2016-11-10 00:49:17 -0800 Comment on attachment 294304 Patch Clearing flags on attachment: 294304 Committed r208529: <http://trac.webkit.org/changeset/208529> 1249718 11 commit-queue 2016-11-10 00:49:22 -0800 All reviewed patches have been landed. Closing bug. 1249749 12 clopez 2016-11-10 03:28:24 -0800 (In reply to comment #8) > > So I find way easier and more practical an environment variable. > > > > Also, I don't find useful the possibility of starting/stopping on demand the > > sampler, or running it for a determined interval of time. What is the > > purpose of doing that? > > You wouldn't need to stop your browser and start it again, and them stop > again. I think Safari has a start/stop memory sampler option in the debug > menu. Amnd for the interval, imagine you want to measure the memory used by > ephy at start up, you run it for 30 seconds, for example, and then you don't > want to continue measuring, or having to close ephy again to continue using > it. I tend to think that when there's an option to pass an interval, and an > option to stop the memory sampler they were added for a reason. > > > The only useful usage I find for this feature is the one I wanted to Yes. The feature can be stopped and started on demand, and also can be set to run for each X seconds. Probably safari has such debug menu that makes using this feature on demand useful. In the webkitgtk+ case, I don't see how we could use the memory sampler in that way from a webkitgtk+ application without adding extra API to allow starting/stopping on demand. And I don't think this feature is interesting enough for webkitgtk+ applications to make it part of our API. 294163 2016-11-08 09:14:50 -0800 2016-11-09 16:24:53 -0800 Patch bug-164517-20161108181040.patch text/plain 2073 clopez U3VidmVyc2lvbiBSZXZpc2lvbjogMjA4NDIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNDYyZDk3OGE3YWNiZjRl NDAzMzhjZjRiMWJhOTVkMzlmYzdlMWU3Yi4uOTgzNGVmMDQyNjIxMjRlZWI1MWEwYWUxYTQwYTQ3 MGYwNzk3ZTIzNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE2LTExLTA4ICBDYXJs b3MgQWxiZXJ0byBMb3BleiBQZXJleiAgPGNsb3BlekBpZ2FsaWEuY29tPgorCisgICAgICAgIFtH VEtdIEFsbG93IHRvIHVzZSBXZWJNZW1vcnlTYW1wbGVyIGZlYXR1cmUuCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjQ1MTcKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBFbmFibGUgdGhlIFdlYk1lbW9yeVNh bXBsZXIgd2l0aCB0aGUgZW52aXJvbm1lbnQgdmFyaWFibGUKKyAgICAgICAgU0FNUExFX01FTU9S WSAoc2FtZSBuYW1lIHRoYW4gdGhlIG9uZSB1c2VkIGJ5IHRoZSBFRkwgcG9ydCkuCisKKyAgICAg ICAgV2hlbiBlbmFibGVkLCBpdCB3aWxsIGR1bXAgdGhlIHN0YXRzIHRvIC90bXAgZWFjaCBzZWNv bmQuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJDb250ZXh0LmNwcDoK KyAgICAgICAgKHdlYmtpdFdlYkNvbnRleHRDb25zdHJ1Y3RlZCk6CisKIDIwMTYtMTEtMDUgIENh cmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtTT1VQ XSBMYXlvdXQgdGVzdCBodHRwL3Rlc3RzL21pc2MvYXV0aGVudGljYXRpb24tcmVkaXJlY3QtMy9h dXRoZW50aWNhdGlvbi1zZW50LXRvLXJlZGlyZWN0LXNhbWUtb3JpZ2luLXdpdGgtbG9jYXRpb24t Y3JlZGVudGlhbHMuaHRtbCBmYWlscwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL0FQSS9ndGsvV2ViS2l0V2ViQ29udGV4dC5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nl c3MvQVBJL2d0ay9XZWJLaXRXZWJDb250ZXh0LmNwcAppbmRleCBmMDc3YjBjZDhiNjk2NDRkYzkz MzVlMDllOWJiNWRmOGRhYWU0ZWRmLi5jMWY2OTFjM2U2Y2U3MTFlOGJiY2M2MGY2ZGM3OTY0NjVk MmE2M2Q0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJL aXRXZWJDb250ZXh0LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9X ZWJLaXRXZWJDb250ZXh0LmNwcApAQCAtMjgwLDYgKzI4MCwxNiBAQCBzdGF0aWMgdm9pZCB3ZWJr aXRXZWJDb250ZXh0Q29uc3RydWN0ZWQoR09iamVjdCogb2JqZWN0KQogICAgIHByaXYtPnRsc0Vy cm9yc1BvbGljeSA9IFdFQktJVF9UTFNfRVJST1JTX1BPTElDWV9GQUlMOwogICAgIHByaXYtPnBy b2Nlc3NQb29sLT5zZXRJZ25vcmVUTFNFcnJvcnMoZmFsc2UpOwogCisjaWYgRU5BQkxFKE1FTU9S WV9TQU1QTEVSKQorICAgIHN0YXRpYyBib29sIGluaXRpYWxpemVNZW1vcnlTYW1wbGVyID0gZmFs c2U7CisgICAgc3RhdGljIGNvbnN0IGNoYXIgZW52aXJvbm1lbnRWYXJpYWJsZVtdID0gIlNBTVBM RV9NRU1PUlkiOworCisgICAgaWYgKCFpbml0aWFsaXplTWVtb3J5U2FtcGxlciAmJiBnZXRlbnYo ZW52aXJvbm1lbnRWYXJpYWJsZSkpIHsKKyAgICAgICAgV0tDb250ZXh0U3RhcnRNZW1vcnlTYW1w bGVyKHRvQVBJKHdlYmtpdFdlYkNvbnRleHRHZXRQcm9jZXNzUG9vbCh3ZWJDb250ZXh0KSksIGFk b3B0V0soV0tEb3VibGVDcmVhdGUoMC4wKSkuZ2V0KCkpOworICAgICAgICBpbml0aWFsaXplTWVt b3J5U2FtcGxlciA9IHRydWU7CisgICAgfQorI2VuZGlmCisKICAgICBhdHRhY2hJbmplY3RlZEJ1 bmRsZUNsaWVudFRvQ29udGV4dCh3ZWJDb250ZXh0KTsKICAgICBhdHRhY2hEb3dubG9hZENsaWVu dFRvQ29udGV4dCh3ZWJDb250ZXh0KTsKICAgICBhdHRhY2hSZXF1ZXN0TWFuYWdlckNsaWVudFRv Q29udGV4dCh3ZWJDb250ZXh0KTsK 294304 2016-11-09 16:26:27 -0800 2016-11-10 00:49:17 -0800 Patch bug-164517-20161110012215.patch text/plain 1694 clopez U3VidmVyc2lvbiBSZXZpc2lvbjogMjA4NTA3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMzU5YTc2MDk5NzQ1ZTcy MWE1Y2UyMzI5NjVhYTYxYTU4MWU0NjY0Mi4uYzE2ZTQwNWI4OTcyZDU1ZGVkNmIxNTI2NDdlMTA4 YjU0OTAwM2Q3NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE2LTExLTEwICBDYXJs b3MgQWxiZXJ0byBMb3BleiBQZXJleiAgPGNsb3BlekBpZ2FsaWEuY29tPgorCisgICAgICAgIFtH VEtdIEFsbG93IHRvIHVzZSBXZWJNZW1vcnlTYW1wbGVyIGZlYXR1cmUuCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjQ1MTcKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBFbmFibGUgdGhlIFdlYk1lbW9yeVNh bXBsZXIgd2l0aCB0aGUgZW52aXJvbm1lbnQgdmFyaWFibGUgV0VCS0lUX1NBTVBMRV9NRU1PUlku CisKKyAgICAgICAgV2hlbiBlbmFibGVkLCBpdCB3aWxsIGR1bXAgdGhlIHN0YXRzIHRvIC90bXAg ZWFjaCBzZWNvbmQuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJDb250 ZXh0LmNwcDoKKyAgICAgICAgKHdlYmtpdFdlYkNvbnRleHRDb25zdHJ1Y3RlZCk6CisKIDIwMTYt MTEtMDkgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAg SW1wbGVtZW50IHZpc3VhbC12aWV3cG9ydCBiYXNlZCBwb3NpdGlvbjpmaXhlZCBoYW5kbGluZyBm b3IgTWFjIGFzeW5jIHNjcm9sbGluZwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL0FQSS9ndGsvV2ViS2l0V2ViQ29udGV4dC5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nl c3MvQVBJL2d0ay9XZWJLaXRXZWJDb250ZXh0LmNwcAppbmRleCBmMDc3YjBjZDhiNjk2NDRkYzkz MzVlMDllOWJiNWRmOGRhYWU0ZWRmLi44MmJiNmNmZDZhNWQ4ZDFhMTE4OTRiYTBhMWE4MTRhYzU3 ZjIxZGI2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJL aXRXZWJDb250ZXh0LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9X ZWJLaXRXZWJDb250ZXh0LmNwcApAQCAtMjgwLDYgKzI4MCwxMSBAQCBzdGF0aWMgdm9pZCB3ZWJr aXRXZWJDb250ZXh0Q29uc3RydWN0ZWQoR09iamVjdCogb2JqZWN0KQogICAgIHByaXYtPnRsc0Vy cm9yc1BvbGljeSA9IFdFQktJVF9UTFNfRVJST1JTX1BPTElDWV9GQUlMOwogICAgIHByaXYtPnBy b2Nlc3NQb29sLT5zZXRJZ25vcmVUTFNFcnJvcnMoZmFsc2UpOwogCisjaWYgRU5BQkxFKE1FTU9S WV9TQU1QTEVSKQorICAgIGlmIChnZXRlbnYoIldFQktJVF9TQU1QTEVfTUVNT1JZIikpCisgICAg ICAgIHByaXYtPnByb2Nlc3NQb29sLT5zdGFydE1lbW9yeVNhbXBsZXIoMCk7CisjZW5kaWYKKwog ICAgIGF0dGFjaEluamVjdGVkQnVuZGxlQ2xpZW50VG9Db250ZXh0KHdlYkNvbnRleHQpOwogICAg IGF0dGFjaERvd25sb2FkQ2xpZW50VG9Db250ZXh0KHdlYkNvbnRleHQpOwogICAgIGF0dGFjaFJl cXVlc3RNYW5hZ2VyQ2xpZW50VG9Db250ZXh0KHdlYkNvbnRleHQpOwo=