| Summary: | [Qt] QWebSettings::clearMemoryCaches should clear JS garbage | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Arunprasad <ararunprasad> | ||||||||||||||||||||
| Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Major | CC: | allan.jensen, ararunprasad, arurajku, bedupont, buildbot, hausmann, rniwa, webkit.review.bot | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||
| Bug Blocks: | 111605 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
Sorry, patch has a flaw. -WebCore::gcController().garbageCollectSoon(); +WebCore::gcController().garbageCollectNow(); I will recreate soon with proper ChangeLog. Created attachment 190772 [details]
Patch
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. 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. Created attachment 190961 [details]
Patch
Modified to call only JSC's GC 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 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 Created attachment 190967 [details]
Patch
>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 :)
(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. (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). (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. (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()? (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. (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 Created attachment 191508 [details]
Patch
(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. (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 :) (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 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. (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? Created attachment 191764 [details]
Patch
Comment on attachment 191764 [details]
Patch
r=me
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? Created attachment 191995 [details]
Patch
Created attachment 191996 [details]
Patch
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..." (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 :( Created attachment 192008 [details]
Patch
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 on attachment 192008 [details] Patch Clearing flags on attachment: 192008 Committed r145085: <http://trac.webkit.org/changeset/145085> All reviewed patches have been landed. Closing bug. (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 :) |
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.