Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1082393003:
TrimMemory strategy while offscreenPreRaster is on. (Closed)
|
Created:
5 years, 8 months ago by hush (inactive) Modified:
5 years, 8 months ago Reviewers:
boliu CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/a/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrimMemory strategy while offscreenPreRaster is on.
BUG=460638
Committed: https://crrev.com/6ef62050066dea9d188549b12df605c3a4f18a6b
Cr-Commit-Position: refs/heads/master@{#325394}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : comments #Patch Set 4 : Make sure the memory policy is 0 when hardware is disabled. #Patch Set 5 : #Patch Set 6 : tiny bug #
Total comments: 13
Patch Set 7 : comments #
Messages
Total messages: 20 (3 generated)
hush@chromium.org changed reviewers: + boliu@chromium.org
PTAL. I am looking for ways to trigger onTrimMemory to test this..
https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:341: compositor_->SetMemoryPolicy(CalculateDesiredMemoryPolicy()); This ignores hardware_enabled_ https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/shared_renderer_state.cc:305: browser_view_renderer_->ReleaseHardware(); Then this is also skipping ReleaseHardware in detach, which is not right. Probably need to expose a separate TrimMemory call, then do some refactoring in this file
PTAL PS3 https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:341: compositor_->SetMemoryPolicy(CalculateDesiredMemoryPolicy()); On 2015/04/15 00:31:01, boliu wrote: > This ignores hardware_enabled_ What do you mean? Do I need to do anything here? Do you mean only calling this method when hardware is enabled? https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/shared_renderer_state.cc:305: browser_view_renderer_->ReleaseHardware(); On 2015/04/15 00:31:01, boliu wrote: > Then this is also skipping ReleaseHardware in detach, which is not right. > > Probably need to expose a separate TrimMemory call, then do some refactoring in > this file Yeah. That is a problem. And it actually caused some crashes in my test. Sorry about that! I just refactored this file.
https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:341: compositor_->SetMemoryPolicy(CalculateDesiredMemoryPolicy()); On 2015/04/15 21:35:48, hush wrote: > On 2015/04/15 00:31:01, boliu wrote: > > This ignores hardware_enabled_ > > What do you mean? Do I need to do anything here? Do you mean only calling this > method when hardware is enabled? policy should be 0 if hardware_enabled_ is false, this will set it to > 0 if called when webview is not hardware_enabled
https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:341: compositor_->SetMemoryPolicy(CalculateDesiredMemoryPolicy()); On 2015/04/15 22:27:45, boliu wrote: > On 2015/04/15 21:35:48, hush wrote: > > On 2015/04/15 00:31:01, boliu wrote: > > > This ignores hardware_enabled_ > > > > What do you mean? Do I need to do anything here? Do you mean only calling this > > method when hardware is enabled? > > policy should be 0 if hardware_enabled_ is false, this will set it to > 0 if > called when webview is not hardware_enabled Maybe CalcuateDesiredMemoryPolicy should take into account hardware_enabled_, and directly call SetMemoryPolicy. Then it can be renamed to something like "UpdateMemoryPolicy"
https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:341: compositor_->SetMemoryPolicy(CalculateDesiredMemoryPolicy()); On 2015/04/15 22:57:04, boliu wrote: > On 2015/04/15 22:27:45, boliu wrote: > > On 2015/04/15 21:35:48, hush wrote: > > > On 2015/04/15 00:31:01, boliu wrote: > > > > This ignores hardware_enabled_ > > > > > > What do you mean? Do I need to do anything here? Do you mean only calling > this > > > method when hardware is enabled? > > > > policy should be 0 if hardware_enabled_ is false, this will set it to > 0 if > > called when webview is not hardware_enabled > > Maybe CalcuateDesiredMemoryPolicy should take into account hardware_enabled_, > and directly call SetMemoryPolicy. Then it can be renamed to something like > "UpdateMemoryPolicy" I agree doing so keeps the logic sane. But setting the memory policy to non-zero when hardware is not enabled will not cause any bugs, right? (as long as you have the right memory policy when hardware is enabled.)
On Apr 15, 2015 6:13 PM, <hush@chromium.org> wrote: > > > https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... > File android_webview/browser/browser_view_renderer.cc (right): > > https://codereview.chromium.org/1082393003/diff/20001/android_webview/browser... > android_webview/browser/browser_view_renderer.cc:341: > compositor_->SetMemoryPolicy(CalculateDesiredMemoryPolicy()); > On 2015/04/15 22:57:04, boliu wrote: >> >> On 2015/04/15 22:27:45, boliu wrote: >> > On 2015/04/15 21:35:48, hush wrote: >> > > On 2015/04/15 00:31:01, boliu wrote: >> > > > This ignores hardware_enabled_ >> > > >> > > What do you mean? Do I need to do anything here? Do you mean only > > calling >> >> this >> > > method when hardware is enabled? >> > >> > policy should be 0 if hardware_enabled_ is false, this will set it > > to > 0 if >> >> > called when webview is not hardware_enabled > > >> Maybe CalcuateDesiredMemoryPolicy should take into account > > hardware_enabled_, >> >> and directly call SetMemoryPolicy. Then it can be renamed to something > > like >> >> "UpdateMemoryPolicy" > > > I agree doing so keeps the logic sane. But setting the memory policy to > non-zero when hardware is not enabled will not cause any bugs, right? That right there is the bug. The budget must be zero if hardware is not enabled. > (as long as you have the right memory policy when hardware is enabled.) > > https://codereview.chromium.org/1082393003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL! Make sure the memory policy is 0 when hardware is disabled.
On 2015/04/16 01:22:59, hush wrote: > PTAL! > Make sure the memory policy is 0 when hardware is disabled. hold on.. there are bugs..
Okay.. PTAL PS6
https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:153: } Can we do if (>= MODERATE) { if (offscreen_pre_raster) Delete else Release } You'll need to check offscreen_pre_raster before SetMemoryPolicy, but I think it's overall easier to understand https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.h:69: bool offscreen_pre_raster() const { return offscreen_pre_raster_; } not needed anymore https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:296: bool keep_child_compositor_tiles) { release_hardware_draw? This class doesn't know anything about tiles https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:302: if (hardware_initialized) { This check doesn't catch all cases anymore, since it's possible to have hardware_enabled() and yet not have a hardware renderer (ie get trimmed twice) But can't think of anything better either, so maybe just add a comment about that https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:326: if (hardware_initialized && !keep_child_compositor_tiles) { Can't skip this (in theory), some other thread can still get hold of the key from RendererManager, and schedule something synchronous, which needs to be cleared, ie don't change this line. https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/shared_renderer_state.h (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.h:52: void ReleaseCompositorResourcesIfNeededOnUI(bool keep_child_compositor_tiles); private
https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:153: } On 2015/04/16 01:58:39, boliu wrote: > Can we do > > if (>= MODERATE) { > if (offscreen_pre_raster) > Delete > else > Release > } > > You'll need to check offscreen_pre_raster before SetMemoryPolicy, but I think > it's overall easier to understand Yes. Done https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:296: bool keep_child_compositor_tiles) { On 2015/04/16 01:58:40, boliu wrote: > release_hardware_draw? > > This class doesn't know anything about tiles Done. https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:302: if (hardware_initialized) { On 2015/04/16 01:58:40, boliu wrote: > This check doesn't catch all cases anymore, since it's possible to have > hardware_enabled() and yet not have a hardware renderer (ie get trimmed twice) > > But can't think of anything better either, so maybe just add a comment about > that Are you only worried about the 2nd onTrimMemory being wasteful? If WebView gets trimmed twice, we just call hardware_renderer_->reset() twice in a row. It is wasteful, but still not a bug. Or we can introduce another boolean state for cases where the hardware renderer is gone but the UI compositor is still valid. But this would make the code more complicated. https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:326: if (hardware_initialized && !keep_child_compositor_tiles) { On 2015/04/16 01:58:40, boliu wrote: > Can't skip this (in theory), some other thread can still get hold of the key > from RendererManager, and schedule something synchronous, which needs to be > cleared, ie don't change this line. Removed the additional condition. What's the situation for this to happen? You mean something in GPU thread could get hold of the key during "PerformIdleWork" after hardware renderer destruction and schedule something synchronous? https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/shared_renderer_state.h (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.h:52: void ReleaseCompositorResourcesIfNeededOnUI(bool keep_child_compositor_tiles); On 2015/04/16 01:58:40, boliu wrote: > private Done.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.h:69: bool offscreen_pre_raster() const { return offscreen_pre_raster_; } On 2015/04/16 01:58:39, boliu wrote: > not needed anymore Done.
lgtm https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/1082393003/diff/100001/android_webview/browse... android_webview/browser/shared_renderer_state.cc:302: if (hardware_initialized) { On 2015/04/16 04:06:25, hush wrote: > On 2015/04/16 01:58:40, boliu wrote: > > This check doesn't catch all cases anymore, since it's possible to have > > hardware_enabled() and yet not have a hardware renderer (ie get trimmed twice) > > > > But can't think of anything better either, so maybe just add a comment about > > that > Are you only worried about the 2nd onTrimMemory being wasteful? > If WebView gets trimmed twice, we just call hardware_renderer_->reset() twice in > a row. It is wasteful, but still not a bug. The wasteful part is 2 state restores, and blocking the UI thread. > Or we can introduce another boolean > state for cases where the hardware renderer is gone but the UI compositor is > still valid. But this would make the code more complicated. Yeah not worth the complexity. That's why I said just add a comment.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1082393003/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6ef62050066dea9d188549b12df605c3a4f18a6b Cr-Commit-Position: refs/heads/master@{#325394} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
