Bug 164049 – [GTK] Dramatic increase on memory usage since 2.14.x

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Andres Gomez Garcia]

This discussion started in ephy's ML by Michael Gratton, but opening a bug for reference:
https://mail.gnome.org/archives/epiphany-list/2016-October/msg00016.html
 
Hey all,

I feel like Epiphany's/WebKitGTK's memory use has substantially 
increased with 3.22/2.14, under X11 on Ubuntu at least.

My current Epiphany session has ~70 tabs over two windows. If I quit 
and restart it, such that only two of those tabs have actually loaded 
any content, then the total memory consumption is ~7GB (as reported by 
gnome-system-monitor total free before and after launching Ephy). This 
seems to be a substantial increase over earlier versions.

Breaking that down, I am seeing the following approx Memory (from 
g-s-m):
 - Epiphany: 100M
 - Network + DB processes: 40M
 - Single web process for loaded tabs: 250M (+ 10M XServer memory)
 - Single web process for not-yet-loaded tabs: 55M (+ 10M XServer 
memory)

While that doesn't add up to ~7GB, adding up the RSS sizes as reported 
by `ps aux` does.

Switching to a single-shared-process gives 6.4G used after launch, with 
similar numbers for the aux processes, and the following for the single 
web process:

 - Shared web process: 2.1G (+ 550Mb XServer memory)

This is in comparison to the honourable competition, both of which use 
~1G after opening 70 blank tabs.

Is this to be expected? What's the best way to try to find out where 
all this memory is going?

//Mike

[Andres Gomez Garcia]

I can confirm that this is also my experience.

[Miguel Gomez]

As we enabled the Threaded Compositor by default, we are intensively using OpenGL for rendering and this has triggered the problem. We found that the extra memory consumption comes from the Intel Mesa driver, which is using the sofware rasterizer to perform OpenGL operations.

In order to fix this we need to change the way we create the OpenGL contexts: we need to request a concrete version where the Intel driver doesn't use the software rasterizer (3.2 core profile), and then we need to update the OpenGL operations to be coherent with that OpenGL version.

I'll attach a patch that changes the context creation, requesting a OpenGL 3.2 Core version, and fixes some gl operations when we are using that version. It also tweaks the ANGLE shader translator to output the appropriate code for the version (GLSL 1.50).

[Miguel Gomez]

Created attachment 295334 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 295334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295334&action=review

Thanks for working on this. I only have a few comments, I'll leave the actual review to Zan.

> Source/WebCore/platform/graphics/GLContext.cpp:170
> +    if (!m_version) {
> +        GC3Dint mayor = 0;
> +        GC3Dint minor = 0;
> +        ::glGetIntegerv(GL_MAJOR_VERSION, &mayor);
> +        ::glGetIntegerv(GL_MINOR_VERSION, &minor);
> +        m_version = mayor * 100 + minor * 10;

mayor -> major

I think we could initialize this always in the constructor since this will probably always be called, and then we can make this a simpler const getter.

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:63
> +#if !PLATFORM(GTK)
> +    return false;
> +#endif

I don't think this should be GTK specific. This code is only used by GTK+ in any case, so we don't need any platform ifdef.

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:87
> +    int nConfigs = 0;

Use numConfigs or configsCount

[Miguel Gomez]

Created attachment 295335 [details]
Patch

[Miguel Gomez]

> > Source/WebCore/platform/graphics/GLContext.cpp:170
> > +    if (!m_version) {
> > +        GC3Dint mayor = 0;
> > +        GC3Dint minor = 0;
> > +        ::glGetIntegerv(GL_MAJOR_VERSION, &mayor);
> > +        ::glGetIntegerv(GL_MINOR_VERSION, &minor);
> > +        m_version = mayor * 100 + minor * 10;
> 
> mayor -> major
> 
> I think we could initialize this always in the constructor since this will
> probably always be called, and then we can make this a simpler const getter.

I haven't changed the initialization in the new patch. The gl version cannot be initialized in the constructor because we need a current gl context set in order to call glGetIntegerv(), and the context is not current in the constructor yet.

[Zan Dobersek]

Comment on attachment 295335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295335&action=review

> Source/WebCore/platform/graphics/GLContext.h:50
> +    uint version();

unsigned should be used, not uint.

> Source/WebCore/platform/graphics/GLContext.h:81
> +    uint m_version { 0 };

Same here.

[Miguel Gomez]

(In reply to comment #7)
> Comment on attachment 295335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295335&action=review
> 
> > Source/WebCore/platform/graphics/GLContext.h:50
> > +    uint version();
> 
> unsigned should be used, not uint.
> 
> > Source/WebCore/platform/graphics/GLContext.h:81
> > +    uint m_version { 0 };
> 
> Same here.

Ok, I'll fix those, rebase the patch and commit it.

[Miguel Gomez]

Created attachment 295484 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295484 [details]
Patch

Clearing flags on attachment: 295484

Committed r208997: <http://trac.webkit.org/changeset/208997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #10)
> Comment on attachment 295484 [details]
> Patch
> 
> Clearing flags on attachment: 295484
> 
> Committed r208997: <http://trac.webkit.org/changeset/208997>

It broke the WinCairo build:
..\..\Source\WebCore\platform\graphics\GLContext.cpp(168): error C2039: 'glGetIntegerv': is not a member of '`global namespace''
..\..\Source\WebCore\platform\graphics\GLContext.cpp(168): error C2065: 'GL_MAJOR_VERSION': undeclared identifier
..\..\Source\WebCore\platform\graphics\GLContext.cpp(168): error C3861: 'glGetIntegerv': identifier not found
..\..\Source\WebCore\platform\graphics\GLContext.cpp(169): error C2039: 'glGetIntegerv': is not a member of '`global namespace''
..\..\Source\WebCore\platform\graphics\GLContext.cpp(169): error C2065: 'GL_MINOR_VERSION': undeclared identifier
..\..\Source\WebCore\platform\graphics\GLContext.cpp(169): error C3861: 'glGetIntegerv': identifier not found

cc-ing folks, who can be interested in fixing the WinCairo build.

[Csaba Osztrogonác]

(In reply to comment #10)
> Comment on attachment 295484 [details]
> Patch
> 
> Clearing flags on attachment: 295484
> 
> Committed r208997: <http://trac.webkit.org/changeset/208997>

And it broke performance testing, see https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Perf%29/builds/7073 for details.

[Miguel Gomez]

If the WinCairo folks are interested in the added feature they can adapt the code. If not, I can add some platforms guards to avoid the build failure. Just tell me what you prefer.

Regarding the perf bot, I suspect it has the GLX_ARB_create_context extension, but it doesn't support OpenGL 3.2. From what I read, in this case glXCreateContextAttribsARB, besides returning a null context, can throw that GLXBadFBConfig error. That's not being checked in the patch.

[Miguel Gomez]

For the record, the perf bot failure is being handled in bug 165200.

Any comment about the WinCairo build issue? Should I just add the needed platform guards?

[Michael Catanzaro]

Yes, please add platform guards. It will probably be a while before anyone working on WinCairo tries to fix it otherwise.

[Csaba Osztrogonác]

Just out of curiosity, does WinCairo port have at least one full-time maintainer? 

There is a 3 day long build break and nobody has time/interest to comment it.
Why should the community maintain this port build if nobody cares about it?
(Who knows if it works at all? WinCairo port didn't have public tester bot ever.)

[Michael Catanzaro]

(In reply to comment #17)
> Just out of curiosity, does WinCairo port have at least one full-time
> maintainer? 

Of course not.

> There is a 3 day long build break and nobody has time/interest to comment it.
> Why should the community maintain this port build if nobody cares about it?
> (Who knows if it works at all? WinCairo port didn't have public tester bot
> ever.)

I really really do not want to suggest removing the WinCairo port, as it is the only way for non-Apple companies to use WebKit on Windows, but as none of the companies that depend on it seem to be interested in maintaining it... and especially since it is the only port without an EWS bot, so there's no way to know if we're breaking it or not... this should probably be discussed in webkit-dev@.

[Alex Christensen]

https://bugs.webkit.org/show_bug.cgi?id=165283

[Alex Christensen]

Ossy, some of us have been known to go on vacation for more than three days at a time.

[Andres Gomez Garcia]

Please, take a look to attachment 296649 [details]

[Michael Catanzaro]

(In reply to comment #17)
> Just out of curiosity, does WinCairo port have at least one full-time
> maintainer? 
> 
> There is a 3 day long build break and nobody has time/interest to comment it.
> Why should the community maintain this port build if nobody cares about it?
> (Who knows if it works at all? WinCairo port didn't have public tester bot
> ever.)

It also broke the GTK GLES build, nobody noticed for a month, see bug #166455. Guess we shouldn't be so hard on WinCairo. ;)

Carlos Lopez, do we have capacity to set up a GLES buildbot to check for this? Would anybody even notice if such a bot was broken, since it wouldn't be on EWS?

[Carlos Alberto Lopez Perez]

(In reply to comment #22)
> It also broke the GTK GLES build, nobody noticed for a month, see bug
> #166455. Guess we shouldn't be so hard on WinCairo. ;)
> 
> Carlos Lopez, do we have capacity to set up a GLES buildbot to check for
> this? 

I would like to set a bot that tries builds disabling/enabling options to see if some build configuration is broken.

Apart from that, setting an ARM buildbot with GLES can be a good idea.

> Would anybody even notice if such a bot was broken, since it wouldn't be on EWS?

They would notice after the fact, because the buildbot only tries already committed revisions.