Bug 111094 - [Qt] QWebSettings::clearMemoryCaches should clear JS garbage
Summary: [Qt] QWebSettings::clearMemoryCaches should clear JS garbage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 111605
  Show dependency treegraph
 
Reported: 2013-02-28 10:59 PST by Arunprasad
Modified: 2013-03-07 09:31 PST (History)
8 users (show)

See Also:


Attachments
Initial patch (798 bytes, patch)
2013-02-28 10:59 PST, Arunprasad
no flags Details | Formatted Diff | Diff
Patch (1.53 KB, patch)
2013-02-28 11:49 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2013-03-01 06:54 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (1.72 KB, patch)
2013-03-01 07:42 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (3.11 KB, patch)
2013-03-05 09:43 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (2.04 KB, patch)
2013-03-06 09:19 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (1.80 KB, patch)
2013-03-07 07:35 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (1.80 KB, patch)
2013-03-07 07:38 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2013-03-07 08:42 PST, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arunprasad 2013-02-28 10:59:09 PST
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.
Comment 1 Arunprasad 2013-02-28 11:07:12 PST
Sorry, patch has a flaw.

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

I will recreate soon with proper ChangeLog.
Comment 2 Arunprasad Rajkumar 2013-02-28 11:49:51 PST
Created attachment 190772 [details]
Patch
Comment 3 Simon Hausmann 2013-03-01 00:36:46 PST
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.
Comment 4 Arunprasad 2013-03-01 00:59:30 PST
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.
Comment 5 Arunprasad Rajkumar 2013-03-01 06:54:21 PST
Created attachment 190961 [details]
Patch
Comment 6 Arunprasad 2013-03-01 06:55:51 PST
Modified to call only JSC's GC
Comment 7 WebKit Review Bot 2013-03-01 06:59:42 PST
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.
Comment 8 Build Bot 2013-03-01 07:32:01 PST
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
Comment 9 Arunprasad Rajkumar 2013-03-01 07:42:12 PST
Created attachment 190967 [details]
Patch
Comment 10 Arunprasad 2013-03-01 07:43:13 PST
>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 :)
Comment 11 Arunprasad 2013-03-04 01:17:43 PST
(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.
Comment 12 Allan Sandfeld Jensen 2013-03-05 06:31:18 PST
(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).
Comment 13 Allan Sandfeld Jensen 2013-03-05 06:39:22 PST
(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.
Comment 14 Arunprasad 2013-03-05 08:23:07 PST
(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()?
Comment 15 Allan Sandfeld Jensen 2013-03-05 09:13:05 PST
(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.
Comment 16 Arunprasad 2013-03-05 09:29:40 PST
(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
Comment 17 Arunprasad Rajkumar 2013-03-05 09:43:37 PST
Created attachment 191508 [details]
Patch
Comment 18 Allan Sandfeld Jensen 2013-03-05 09:57:08 PST
(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.
Comment 19 Arunprasad 2013-03-05 10:03:00 PST
(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 :)
Comment 20 Arunprasad 2013-03-05 11:24:54 PST
(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 :)
Comment 21 Allan Sandfeld Jensen 2013-03-06 06:42:58 PST
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.
Comment 22 Arunprasad 2013-03-06 06:57:13 PST
(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?
Comment 23 Arunprasad Rajkumar 2013-03-06 09:19:12 PST
Created attachment 191764 [details]
Patch
Comment 24 Allan Sandfeld Jensen 2013-03-07 02:49:35 PST
Comment on attachment 191764 [details]
Patch

r=me
Comment 25 Jocelyn Turcotte 2013-03-07 02:57:10 PST
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?
Comment 26 Arunprasad Rajkumar 2013-03-07 07:35:52 PST
Created attachment 191995 [details]
Patch
Comment 27 Arunprasad Rajkumar 2013-03-07 07:38:55 PST
Created attachment 191996 [details]
Patch
Comment 28 Jocelyn Turcotte 2013-03-07 08:28:12 PST
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..."
Comment 29 Arunprasad 2013-03-07 08:39:31 PST
(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 :(
Comment 30 Arunprasad Rajkumar 2013-03-07 08:42:12 PST
Created attachment 192008 [details]
Patch
Comment 31 Jocelyn Turcotte 2013-03-07 08:55:36 PST
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
Comment 32 WebKit Review Bot 2013-03-07 09:14:01 PST
Comment on attachment 192008 [details]
Patch

Clearing flags on attachment: 192008

Committed r145085: <http://trac.webkit.org/changeset/145085>
Comment 33 WebKit Review Bot 2013-03-07 09:14:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Arunprasad 2013-03-07 09:31:43 PST
(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 :)