Bug 164517 – [GTK] Allow to use WebMemorySampler feature.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

Created attachment 294163 [details]
Patch

[WebKit Commit Bot]

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

[Michael Catanzaro]

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?

[Carlos Alberto Lopez Perez]

(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.

[Carlos Garcia Campos]

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.

> 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.

[Carlos Alberto Lopez Perez]

(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?

[Carlos Alberto Lopez Perez]

Created attachment 294304 [details]
Patch

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Comment on attachment 294304 [details]
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!

[WebKit Commit Bot]

Comment on attachment 294304 [details]
Patch

Clearing flags on attachment: 294304

Committed r208529: <http://trac.webkit.org/changeset/208529>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(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.