Bug 111094 – [Qt] QWebSettings::clearMemoryCaches should clear JS garbage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Arunprasad]

Created attachment 190759 [details]
Initial patch

In certain situations clearing the WebKit's MemoryCache using QWebSettings::clearMemoryCaches doesn't releases all CachedResource to system. It is due to holding of CachedResource by dead but yet to Garbage Collectable objects.

This is required for memory constrained environments(CE devices like set-top box) where deletion of QWebView object + QWebSettings::clearMemoryCaches() should release all the CachedResource like QPixmap.

[Arunprasad]

Sorry, patch has a flaw.

-WebCore::gcController().garbageCollectSoon();
+WebCore::gcController().garbageCollectNow();

I will recreate soon with proper ChangeLog.

[Arunprasad Rajkumar]

Created attachment 190772 [details]
Patch

[Simon Hausmann]

Comment on attachment 190772 [details]
Patch

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

It sounds to me that this is a reasonable thing to do. Allan, what do you think?

(r- because of #ifdefs that aren't needed)

> Source/WebKit/qt/Api/qwebsettings.cpp:49
> +#if USE(JSC)
> +#include "GCController.h"
> +#else    
> +#include "V8GCController.h"
> +#endif

We don't support v8 as underlying JS engine at this point, so I suggest to remove the USE(JSC) and just use it directly.

[Arunprasad]

Thanks for reviewing it. I thought QtWebKit supporting v8 too.

/Tools/Scripts/build-webkit --help
..
..
--v8                              Use V8 as JavaScript engine (Qt only)
..
..


I will correct the change & upload it soon.

[Arunprasad Rajkumar]

Created attachment 190961 [details]
Patch

[Arunprasad]

Modified to call only JSC's GC

[WebKit Review Bot]

Attachment 190961 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebsettings.cpp', u'Source/WebKit/qt/ChangeLog']" exit_code: 1
Source/WebKit/qt/Api/qwebsettings.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 190961 [details]
Patch

Attachment 190961 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16826474

New failing tests:
editing/selection/selection-invalid-offset.html

[Arunprasad Rajkumar]

Created attachment 190967 [details]
Patch

[Arunprasad]

>https://bugs.webkit.org/show_bug.cgi?id=111094#c7

Does it mandates included header file should be alphabetically sorted order?

I think it is a nice way of enforcing the self contained header :)

[Arunprasad]

(In reply to comment #3)
> We don't support v8 as underlying JS engine at this point, so I suggest to remove the USE(JSC) and just use it directly.

Corrected :). Please review it.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 190772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190772&action=review
> 
> It sounds to me that this is a reasonable thing to do. Allan, what do you think?
> 
Yeah, looks good. I would use gcController().discardAllCompiledCode(). It is more powerfull if you really want to free all cached code (also frees cached JIT code blocks).

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (In reply to comment #3)
> > (From update of attachment 190772 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190772&action=review
> > 
> > It sounds to me that this is a reasonable thing to do. Allan, what do you think?
> > 
> Yeah, looks good. I would use gcController().discardAllCompiledCode(). It is more powerfull if you really want to free all cached code (also frees cached JIT code blocks).

Actually I would suggest we took a look at and cleaned the same caches as MemoryPressureMac::releaseMemory() does. I am surprised we use such an agressive call to FontCache for instance.

[Arunprasad]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #3)
> > > (From update of attachment 190772 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=190772&action=review
> > > 
> > > It sounds to me that this is a reasonable thing to do. Allan, what do you think?
> > > 
> > Yeah, looks good. I would use gcController().discardAllCompiledCode(). It is more powerfull if you really want to free all cached code (also frees cached JIT code blocks).
> 
> Actually I would suggest we took a look at and cleaned the same caches as MemoryPressureMac::releaseMemory() does. I am surprised we use such an agressive call to FontCache for instance.

I just go through the MemoryPressureHandler::releaseMemory @ MemoryPressureHandlerMac.mm. Looks nice and does more than expected :)

Can we introduce one more static member function QWebSettings::releaseMemory(bool critical)? Which does exactly like mac counterpart.?

(or) Just add it inside QWebSettings::clearMemoryCache()?

[Allan Sandfeld Jensen]

(In reply to comment #14)
> I just go through the MemoryPressureHandler::releaseMemory @ MemoryPressureHandlerMac.mm. Looks nice and does more than expected :)
> 
> Can we introduce one more static member function QWebSettings::releaseMemory(bool critical)? Which does exactly like mac counterpart.?
> 
> (or) Just add it inside QWebSettings::clearMemoryCache()?

I would just add the other caches inside clearMemoryCache(), it is already documented as clearing as much cache as possible.

If anything we need a less aggressive method for manual garbage collection. Similar to how you can call collectGarbage() on QScriptEngine or QJSEngine, but that is beyond this bug.

[Arunprasad]

(In reply to comment #12)
> (In reply to comment #3)
> > (From update of attachment 190772 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190772&action=review
> > 
> > It sounds to me that this is a reasonable thing to do. Allan, what do you think?
> > 
> Yeah, looks good. I would use gcController().discardAllCompiledCode(). It is more powerfull if you reaally want to free all cached code (also frees cached JIT code blocks).

Is it really needed? Because Script is also a part of CacheResource right? Compiled code will be equivalent to DecodedData?

    if (!WebCore::memoryCache()->disabled()) {
        WebCore::memoryCache()->setDisabled(true);
        WebCore::memoryCache()->setDisabled(false);
    }

I guess. Above should purge the compiled code as well.

Sorry in case I'm wrong :)


Anyhow I will add all those things from MemoryPressureHandlerMac.mm

[Arunprasad Rajkumar]

Created attachment 191508 [details]
Patch

[Allan Sandfeld Jensen]

(In reply to comment #16)
> Is it really needed? Because Script is also a part of CacheResource right? Compiled code will be equivalent to DecodedData?

The script source is a CacheResource, but the compiled JIT code is not. It has its own separate cache called ExecutableAllocator.

[Arunprasad]

(In reply to comment #18)
> (In reply to comment #16)
> > Is it really needed? Because Script is also a part of CacheResource right? Compiled code will be equivalent to DecodedData?
> 
> The script source is a CacheResource, but the compiled JIT code is not. It has its own separate cache called ExecutableAllocator.

Understood. Thanks Allen :)

[Arunprasad]

(In reply to comment #17)
> Created an attachment (id=191508) [details]
> Patch

Please review the patch. It is updated as like MemoryPressureHandler::releaseMemory @ MemoryPressureHandlerMac.mm. Thanks :)

[Allan Sandfeld Jensen]

Comment on attachment 191508 [details]
Patch

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

> Source/WebKit/qt/Api/qwebsettings.cpp:853
> +    // Garbage Collect to release the references of CachedResource from dead objects
> +    WebCore::gcController().garbageCollectNow();

You don't need to call garbageCollect when you call discardAllCompiledCode. It does that as well.

> Source/WebKit/qt/Api/qwebsettings.cpp:868
> +    // FastMalloc has lock-free thread specific caches that can only be cleared from the thread itself.
> +    WebCore::StorageThread::releaseFastMallocFreeMemoryInAllThreads();
> +#if ENABLE(WORKERS)
> +    WebCore::WorkerThread::releaseFastMallocFreeMemoryInAllThreads();
> +#endif
> +#if ENABLE(THREADED_SCROLLING)
> +    WebCore::ScrollingThread::dispatch(bind(WTF::releaseFastMallocFreeMemory));
> +#endif
> +    WTF::releaseFastMallocFreeMemory();    

I would skip these for now. They are not really caches, and we also don't support scrolling threads.

[Arunprasad]

(In reply to comment #21)
> (From update of attachment 191508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191508&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:853
> > +    // Garbage Collect to release the references of CachedResource from dead objects
> > +    WebCore::gcController().garbageCollectNow();
> 
> You don't need to call garbageCollect when you call discardAllCompiledCode. It does that as well.

From Heap.cpp
void Heap::deleteAllCompiledCode()
{
    // If JavaScript is running, it's not safe to delete code, since we'll end
    // up deleting code that is live on the stack.
    if (m_globalData->dynamicGlobalObject)
        return;

    for (ExecutableBase* current = m_compiledCode.head(); current; current = current->next()) {
        if (!current->isFunctionExecutable())
            continue;
        static_cast<FunctionExecutable*>(current)->clearCodeIfNotCompiling();
    }

    m_dfgCodeBlocks.clearMarks();
    m_dfgCodeBlocks.deleteUnmarkedJettisonedCodeBlocks();
}


I think it is not calling GarbageCollector. Sorry incase i'm wrong. Can you please show me where it is calling the GC?

> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:868
> > +    // FastMalloc has lock-free thread specific caches that can only be cleared from the thread itself.
> > +    WebCore::StorageThread::releaseFastMallocFreeMemoryInAllThreads();
> > +#if ENABLE(WORKERS)
> > +    WebCore::WorkerThread::releaseFastMallocFreeMemoryInAllThreads();
> > +#endif
> > +#if ENABLE(THREADED_SCROLLING)
> > +    WebCore::ScrollingThread::dispatch(bind(WTF::releaseFastMallocFreeMemory));
> > +#endif
> > +    WTF::releaseFastMallocFreeMemory();    
> 
> I would skip these for now. They are not really caches, and we also don't support scrolling threads.

 +    WebCore::StorageThread::releaseFastMallocFreeMemoryInAllThreads();
 +#if ENABLE(WORKERS)
 +    WebCore::WorkerThread::releaseFastMallocFreeMemoryInAllThreads();
 +#endif
 +    WTF::releaseFastMallocFreeMemory();

Anyhow releasing Workers and FastMalloc stuffs needed know?

[Arunprasad Rajkumar]

Created attachment 191764 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 191764 [details]
Patch

r=me

[Jocelyn Turcotte]

Comment on attachment 191764 [details]
Patch

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

> Source/WebKit/qt/Api/qwebsettings.cpp:821
> +    as page, object and font cache. It calls the JavaScript Garbage Collector to
> +    release the references of CachedResource from dead objects and frees up memory to System
> +    as much as possible.

Just a detail before this lands, this comment is going to be very cryptic for most users of QtWebKit.
"CachedResource from dead objects" is bound to the implementation and can be confusing.

I think that the garbage collector point is valuable though.
Could you update it with terms that mortals can understand?

[Arunprasad Rajkumar]

Created attachment 191995 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 191996 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 191996 [details]
Patch

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

> Source/WebKit/qt/Api/qwebsettings.cpp:817
> -    Frees up as much memory as possible by cleaning all memory caches such
> +    Frees up as much memory as possible by calling JavaScript Garbager Collector, cleaning all memory caches such

Proper english please, the documentation is generated from this.
"Frees up as much memory as possible by calling the JavaScript garbage collector and cleaning..."

[Arunprasad]

(In reply to comment #28)
> (From update of attachment 191996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191996&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:817
> > -    Frees up as much memory as possible by cleaning all memory caches such
> > +    Frees up as much memory as possible by calling JavaScript Garbager Collector, cleaning all memory caches such
> 
> Proper english please, the documentation is generated from this.
> "Frees up as much memory as possible by calling the JavaScript garbage collector and cleaning..."

Sorry for the typo :(

[Arunprasad Rajkumar]

Created attachment 192008 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 192008 [details]
Patch

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

> Source/WebKit/qt/Api/qwebsettings.cpp:818
> +    Frees up as much memory as possible by calling the JavaScript garbage collector and cleaning all memory caches such
>      as page, object and font cache.

The first line should have been cut but this is a minor issue. I should have told you earlier.
Thanks for fixing it, r=me

[WebKit Review Bot]

Comment on attachment 192008 [details]
Patch

Clearing flags on attachment: 192008

Committed r145085: <http://trac.webkit.org/changeset/145085>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arunprasad]

(In reply to comment #31)
> (From update of attachment 192008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192008&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:818
> > +    Frees up as much memory as possible by calling the JavaScript garbage collector and cleaning all memory caches such
> >      as page, object and font cache.
> 
> The first line should have been cut but this is a minor issue. I should have told you earlier.
> Thanks for fixing it, r=me

Is it too long? I will avoid these kind of mistakes in future. Anyway thanks :)