Bug 30210 - [Cache] Add embedded profile to prune decoded resources more early
Summary: [Cache] Add embedded profile to prune decoded resources more early
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-08 07:36 PDT by Holger Freyther
Modified: 2010-10-18 11:57 PDT (History)
6 users (show)

See Also:


Attachments
Add proof of concept for the approach (4.13 KB, patch)
2009-10-08 07:39 PDT, Holger Freyther
abarth: review-
Details | Formatted Diff | Diff
Add proof of concept for the approach. (5.85 KB, patch)
2009-10-19 19:47 PDT, Holger Freyther
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2009-10-08 07:36:43 PDT
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.
Comment 1 Holger Freyther 2009-10-08 07:39:29 PDT
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.
Comment 2 Adam Barth 2009-10-18 22:53:22 PDT
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.
Comment 3 Holger Freyther 2009-10-19 19:47:50 PDT
Created attachment 41475 [details]
Add proof of concept for the approach.

Same change with a ChangeLog entry.
Comment 4 Adam Barth 2009-11-30 12:19:20 PST
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
Comment 5 Adam Barth 2009-12-04 13:02:08 PST
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.
Comment 6 Geoffrey Garen 2009-12-04 13:43:49 PST
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?
Comment 7 Holger Freyther 2009-12-04 21:52:19 PST
(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.
Comment 8 Holger Freyther 2009-12-04 21:55:34 PST
(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.