Bug 30210 – [Cache] Add embedded profile to prune decoded resources more early

• Status: NEW
• Component: WebCore Misc.

[Holger Freyther]

Add a high watermark and if m_liveSize is bigger is bigger than this watermark ignore the calculated elapsedTime. This will make WebCore::Cache throw away decoded images even from within the paint routine which is helpful on sites with many images.

This change does not make sure that m_liveSize will always stay below the watermark and is really only limited to decoded images. So if 3GB of CSS is downloaded m_liveSize will still grow beyond the high watermark.

[Holger Freyther]

Created attachment 40869 [details]
Add proof of concept for the approach

I'm mostly seeking for feedback on if this is a good way to limit the memory usage. In the worse case the same image will be decoded more than once during painting. I would solve this in the ImageDecoder to asynchronously decode images.

The proposed patch adds a new EMBEDDED_PROFILE that will in general prefer memory usage over speed. The proposed code will not add any branch or assignment for the normal build.

[Adam Barth]

Comment on attachment 40869 [details]
Add proof of concept for the approach

This patch does not have a ChangeLog.  If you'd like feedback on your patch, I recommend asking on IRC or emailing the relevant folks directly.  Having this patch in the review queue is just making it hard to review complete patches.

[Holger Freyther]

Created attachment 41475 [details]
Add proof of concept for the approach.

Same change with a ChangeLog entry.

[Adam Barth]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebCore/loader/Cache.h
WebCore/loader/Cache.cpp:279:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/loader/Cache.cpp:280:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Done processing WebCore/loader/Cache.cpp
Total errors found: 2

[Adam Barth]

Comment on attachment 41475 [details]
Add proof of concept for the approach.

Please fix the style issues.  Also, please use a const bool instead of EMBEDDED_PROFILE_CHECK.  This code

+ if (EMBEDDED_PROFILE_CHECK elapsedTime < cMinDelayBeforeLiveDecodedPrune)

looks super wonky.  The compiler will optimize const bools properly.

[Geoffrey Garen]

If you want to add a feature with an #ifdef, all data members and functions should be surrounded by that #ifdef.

I don't think EMBEDDED_PROFILE is a good name for this #ifdef, since some embedded profiles do not want to use it.

Also, couldn't you write this whole patch in one line but just setting cMinDelayBeforeLiveDecodedPrune to 0?

[Holger Freyther]

(In reply to comment #5)
> (From update of attachment 41475 [details])
> Please fix the style issues.  Also, please use a const bool instead of
> EMBEDDED_PROFILE_CHECK.  This code
> 
> + if (EMBEDDED_PROFILE_CHECK elapsedTime < cMinDelayBeforeLiveDecodedPrune)
> 
> looks super wonky.  The compiler will optimize const bools properly.

The downside is that we normally don't use "const" for local variables.

[Holger Freyther]

(In reply to comment #6)
> If you want to add a feature with an #ifdef, all data members and functions
> should be surrounded by that #ifdef.
> 
> I don't think EMBEDDED_PROFILE is a good name for this #ifdef, since some
> embedded profiles do not want to use it.
> 
> Also, couldn't you write this whole patch in one line but just setting
> cMinDelayBeforeLiveDecodedPrune to 0?

Good hint, let me research that.