Summary: | [GTK] Dramatic increase on memory usage since 2.14.x | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andres Gomez Garcia <agomez> | ||||||||
Component: | WebKitGTK | Assignee: | Miguel Gomez <magomez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, agomez, aperez, bfulgham, bugs-noreply, clopez, commit-queue, dino, kondapallykalyan, magomez, mcatanzaro, mike, noam, ossy, pvollan | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Andres Gomez Garcia
2016-10-27 01:48:47 PDT
I can confirm that this is also my experience. 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). Created attachment 295334 [details]
Patch
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 Created attachment 295335 [details]
Patch
> > 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.
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. (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. Created attachment 295484 [details]
Patch
Comment on attachment 295484 [details] Patch Clearing flags on attachment: 295484 Committed r208997: <http://trac.webkit.org/changeset/208997> All reviewed patches have been landed. Closing bug. (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. (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. 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. 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? Yes, please add platform guards. It will probably be a while before anyone working on WinCairo tries to fix it otherwise. 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.) (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@. Ossy, some of us have been known to go on vacation for more than three days at a time. Please, take a look to attachment 296649 [details]
(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? (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. |