Bug 85801 - [chromium] setContentsMemoryAllocationLimitBytes needs to setNeedsCommit.
Summary: [chromium] setContentsMemoryAllocationLimitBytes needs to setNeedsCommit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michal Mocny
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-07 06:59 PDT by Michal Mocny
Modified: 2012-05-07 10:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.17 KB, patch)
2012-05-07 07:01 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2012-05-07 09:15 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Mocny 2012-05-07 06:59:51 PDT
[chromium] setContentsMemoryAllocationLimitBytes needs to setNeedsCommit.
Comment 1 Michal Mocny 2012-05-07 07:01:35 PDT
Created attachment 140523 [details]
Patch
Comment 2 Michal Mocny 2012-05-07 07:11:59 PDT
Comment on attachment 140523 [details]
Patch

This patch fixes: http://code.google.com/p/chromium/issues/detail?id=126079

On pages that did not need to update/draw, we would push a single white frame while our memory allocation was 0, and not push another frame once that memory allocation changed.

As well as calling setNeedsCommit, this patch resets the memory allocation to its initial value on visibility if it was purged down to 0.  This way, we can push a frame before waiting for a round trip memory allocation adjustments message to arrive.  If the message arrives before the next frame, great.  If not, at least our allocation is some conservative non 0 value, and also matches our current initialized state behaviour.
Comment 3 Nat Duca 2012-05-07 09:05:57 PDT
Comment on attachment 140523 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:382
> +    // Reset memory allocation to initial value if we have purged down to 0.

Why? This feels wrong... you're basically goign to let any newly visible tab ignore the gpu memory manager's recommendation?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:427
> +    setNeedsCommit();

This part LGTM
Comment 4 Michal Mocny 2012-05-07 09:15:35 PDT
Created attachment 140535 [details]
Patch
Comment 5 Michal Mocny 2012-05-07 09:16:59 PDT
Comment on attachment 140523 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:382
>> +    // Reset memory allocation to initial value if we have purged down to 0.
> 
> Why? This feels wrong... you're basically goign to let any newly visible tab ignore the gpu memory manager's recommendation?

Discussed offline: it should be a separate bug to optimize returning from 0 memory allocation, as there are many possible solutions to this.
Removing from this patch for now.
Comment 6 Adrienne Walker 2012-05-07 09:47:00 PDT
Comment on attachment 140535 [details]
Patch

R=me.  I agree with the approach of fixing this first and optimizing it later.
Comment 7 WebKit Review Bot 2012-05-07 10:24:43 PDT
Comment on attachment 140535 [details]
Patch

Clearing flags on attachment: 140535

Committed r116316: <http://trac.webkit.org/changeset/116316>
Comment 8 WebKit Review Bot 2012-05-07 10:24:50 PDT
All reviewed patches have been landed.  Closing bug.