Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
226363004:
Global GPU memory manager for android webview (Closed)
|
Created:
6 years, 8 months ago by hush (inactive) Modified:
6 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGlobal GPU memory manager for android webview
BUG=354155
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268006
Patch Set 1 #
Total comments: 3
Patch Set 2 : Only manage tiles now #Patch Set 3 : Remove "HardwareRenderer::GetMemoryPolicy" declaration #
Total comments: 2
Patch Set 4 : Decide memory policy on UI thread in BVR #Patch Set 5 : Remove some debug msgs #Patch Set 6 : Remove unnecessary code #
Total comments: 91
Patch Set 7 : Rebase #Patch Set 8 : address Bo's comments #
Total comments: 17
Patch Set 9 : address comments. Add unit test. Rebase. #Patch Set 10 : Fix shared renderer state initialize order #Patch Set 11 : Make clang happy #Patch Set 12 : Use sequence checker in GlobalTileManager. #Patch Set 13 : Rewrote the API for GlobalTileManager and Client. #
Total comments: 1
Patch Set 14 : Use command line switch and move that from HR to BVR. #
Total comments: 18
Patch Set 15 : Address comments and added 2 more unit tests. #
Total comments: 2
Patch Set 16 : Type casts and private methods in BVR. #Messages
Total messages: 31 (0 generated)
Hi Bo! This is kind of finished except for these tasks: 1. Scale the gpu memory limit based on device type (like Clank did in ui/gl/gl_context_android.cc). I need to use that code and preferably not copying it over 2. Maybe come up with a better name for "GpuMemoryManager" since it is already taken by content::GpuMemoryManager 3. Write tests
https://codereview.chromium.org/226363004/diff/1/android_webview/browser/gpu_... File android_webview/browser/gpu_memory_manager.cc (right): https://codereview.chromium.org/226363004/diff/1/android_webview/browser/gpu_... android_webview/browser/gpu_memory_manager.cc:20: const size_t GpuMemoryManager::kGpuMemoryLimit = 256 * 1024 * 1024; how about assume the limiting resource is always fd, and don't worry about memory? https://codereview.chromium.org/226363004/diff/1/android_webview/browser/hard... File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/226363004/diff/1/android_webview/browser/hard... android_webview/browser/hardware_renderer.cc:189: current_policy, new_policy, manager_key_); Too aggressive and not stable? And I think it doesn't actually work since you don't force call ManageTiles. Stability is probably the most important issue. If we solve that, the other two shouldn't be problems anymore... What if we do something like this: First webview, use half of available Next one comes, use quarter etc... If first one ever draws again, it can adjust itself. If first one never draws again, then at some point, we start dropping its tiles. That's just my first idea that came to mind. https://codereview.chromium.org/226363004/diff/1/content/browser/android/in_p... File content/browser/android/in_process/synchronous_compositor_impl.h (right): https://codereview.chromium.org/226363004/diff/1/content/browser/android/in_p... content/browser/android/in_process/synchronous_compositor_impl.h:106: SynchronousCompositorMemoryPolicy memory_policy_; Why not keep this in HardwareRenderer? You shouldn't need any changes in content.
Hi Bo! As we discussed in person last time, I made the following changes: 1. Only manage tiles. And the global max is 450 tiles. That leaves the app 1024-450*2=124 FDs to do other things, in the case where the webviews use up 450 tiles. 2. Evict the tiles of other webviews when a webview is starved. A webview is starved when it gets less than kGrallocHungryLimit(100) tiles consecutively for kStarvedTimes(5). 3. Hardware renderer keeps track of how many tiles it uses and the global tile manager assumes each hardware renderer use all of tiles. 4. Evict the tiles of other webviews by setting their memory policy to 0, followed by a fake software draw. This is a hack but we need it because the evicted webview may not be drawing anymore, so setting its memory policy alone does not work. The compositor only manages its tiles after draw.
Didn't look in detail. A few large structural things: Which thread (UI or RT) should tile management happen? Eventually with ubercomp, it will all be on UI. Currently, I think we should move as much of it to UI as possible. So how about this: Keep the current policy in SharedRendererState. HardwareRenderer just grabs said policy and applies it before draw. All other logic happens in BVR on UI thread. Given that, can GlobalTileManager be merged into GLViewRendererManager?
https://codereview.chromium.org/226363004/diff/40001/content/browser/android/... File content/browser/android/in_process/synchronous_compositor_impl.cc (right): https://codereview.chromium.org/226363004/diff/40001/content/browser/android/... content/browser/android/in_process/synchronous_compositor_impl.cc:143: output_surface_->SetMemoryPolicy(policy); Do this separately to avoid having to have a content owner look at this.
On 2014/04/18 16:13:00, boliu wrote: > Didn't look in detail. A few large structural things: > > Which thread (UI or RT) should tile management happen? Eventually with ubercomp, > it will all be on UI. Currently, I think we should move as much of it to UI as > possible. So how about this: > > Keep the current policy in SharedRendererState. HardwareRenderer just grabs said > policy and applies it before draw. All other logic happens in BVR on UI thread. > > Given that, can GlobalTileManager be merged into GLViewRendererManager? Right now the tile manager is doing its work on render thread, because we set memory policy in drawGL functor, which is run on the render thread. I will merge GlobalTileManager into GlViewRendererManager considering the future with ubercomp.
On 2014/04/18 16:13:00, boliu wrote: > Keep the current policy in SharedRendererState. HardwareRenderer just grabs said > policy and applies it before draw. All other logic happens in BVR on UI thread. > > Given that, can GlobalTileManager be merged into GLViewRendererManager? Actually I am not very sure of the "contract" of SharedRendererState. Is it supposed to only store states shared between UI and Render thread? Also, I am trying to keep it simple and avoid turning SharedRendererState into a dumping ground. The same reasoning applies to GLViewRendererManager: conceptually, GLViewRendererManager and GlobalTileManager do 2 different things. GLViewRendererManager keeps track of the what webviews are drawn, and their order. GlobalTileManager allocates and enforces tile resources for the webviews. Although our current tile management policy implementation chooses to evict the tiles of the LRU, we are free to implement other policies that may not need the drawn order. Anyways, I think merging GlobalTileManager with GLViewRendererManager ties the evicting policy to LRU. And makes the GLViewRendererManager messier. Thoughts?
https://codereview.chromium.org/226363004/diff/40001/android_webview/browser/... File android_webview/browser/global_tile_manager.h (right): https://codereview.chromium.org/226363004/diff/40001/android_webview/browser/... android_webview/browser/global_tile_manager.h:23: class GlobalTileManager { *cough* unittests *cough* :)
On 2014/04/23 14:10:08, mkosiba wrote: > https://codereview.chromium.org/226363004/diff/40001/android_webview/browser/... > File android_webview/browser/global_tile_manager.h (right): > > https://codereview.chromium.org/226363004/diff/40001/android_webview/browser/... > android_webview/browser/global_tile_manager.h:23: class GlobalTileManager { > *cough* unittests *cough* :) Sure... I will do that
Hi Bo! From our talk on Friday, here is the current status of global tile manager: 1. memory policy is written on shared_renderer_state by BrowserViewRenderer on the UI thread, during onDraw before DrawGL happens on the render thread 2. the memory policy is read by hardware renderer when DrawGL runs on render thread. 3. during trim memory, hardware renderer will clear the tiles on render thread. After that, on the UI thread, browser view renderer will update the cached memory policy in shared_renderer_state and global tile manager 4. hardware renderer destruction: same as #3. Browser view renderer thinks its corresponding hardware renderer is destructed when onDetachedFromWindow is called. I will write some unite tests for global tile manager when we agree to go with this approach.
https://codereview.chromium.org/226363004/diff/100001/android_webview/android... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/android... android_webview/android_webview.gyp:156: 'browser/global_tile_manager.h', add tile_resource_consumer.h too https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:41: // Becomes starved if the resource request cannot be granted kStarvedTimes in a heh, hungry -> starved, actually makes sense https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:45: typedef content::SynchronousCompositorMemoryPolicy Policy; Never do this. This is just confusing. If you want to save typing "content::", you can use "using". https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:107: SetMemoryPolicy(zero_policy); This should already happen in OnDetached, not needed here https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:114: void BrowserViewRenderer::TrimMemory() { Is TileManager aware that we are dropping tiles on our own? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:119: ForceDropTiles(); Isn't ForceDropTiles enough? Why both? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:122: Policy BrowserViewRenderer::CalculateMemoryPolicy() { IdealPolicy? Or DesiredPolicy? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:136: void BrowserViewRenderer::SetMemoryPolicy(Policy& new_policy) { SetMemoryPolicy sounds like a simple setter, this is doing way more. How about RequestMemoryPolicy? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:144: num_hungry_++; this should only be modified in OnDraw. Too easy to add a call to this, and then suddenly your policy changed. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:203: GlobalTileManager::GetInstance()->DidDrawGL(tile_manager_key_); Hmm, why do You you need to condition this on did_draw_gl? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:205: SetMemoryPolicy(old_policy); Why set back to old policy? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:375: shared_renderer_state_->SetCompositorOnUiThread(NULL); DCHECK here memory policy is 0, (this should only happen after detach/destroy) https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (left): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:38: spurious delete https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:107: spurious new line https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:116: void SetMemoryPolicy(content::SynchronousCompositorMemoryPolicy& policy); Comment does not match function at all https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:142: size_t CalculateTileRequest() OVERRIDE; virtual, and move comments to the interface https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:223: // kGrallocHungryLimit. s/kGrallocHungryLimit/CalculateTileRequest/ https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:224: size_t num_hungry_; num_consecutive_hungry_frames_? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/global_tile_manager.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:12: base::LazyInstance<GlobalTileManager>::Leaky g_tile_manager = no indent for namespace https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:13: LAZY_INSTANCE_INITIALIZER; blank line below https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:26: void GlobalTileManager::DidDrawGL(Key key) { Call this DidDraw, or DidUse. And group the code together with PushBack. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:30: inactive_views_++; This is not what I described, you are only moving inactive_views_ to the back of the list, treating everything that was ever done a hardware OnDraw as active. We want views that have stopped OnDraw to eventually become inactive. What you want is *before* moving the current view to the front, check how many views are in front of it. Add 1 to include the view itself, that's the number of active views (yes, it will be different for each view) https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:48: for (it = mru_list_.rbegin(); it != mru_list_.rend(); it++) { Shouldn't you begin at something like inactive_views_? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:65: size_t new_num_of_tiles, bool is_starved, Key key) { ditto on multi-line argument style https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:74: size_t tiles = allocated_tiles_ - old_num_of_tiles + new_num_of_tiles; s/tiles/new_total_allocated_tiles/ https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:77: allocated_tiles_ = tiles; Because individual view policy is inside each view, this class should have DCHECKs that allocated_tiles_ remains the sum of everything. I guess add up everything in a for loop, and only do it if DCHECK_IS_ON is set. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:83: if (!is_starved) { The staved concept does not help with stability at all. If there are 10 views drawing, then each view can get at most 450 / 10 = 45 tiles, which is below the 100. After 5 cycles, the views are going to start killing each other. I think you should only drop inactive views. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/global_tile_manager.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:25: typedef std::list<TileResourceConsumer*> ListType; newline below https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:34: size_t RequestTiles(size_t old_num_of_tiles, size_t new_num_of_tiles, c++ has a different new line wrap style, either all arguments are one a single one or one argument on each line. Recommend using clang-format and not think about it: https://code.google.com/p/chromium/wiki/ClangFormat https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:43: void Remove(Key key); newline below https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:47: ~GlobalTileManager(); new line below https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:55: size_t allocated_tiles_; total_allocated_tiles_ https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:59: This class lives on UI, right? Add a SequenceChecker: base/sequence_checker.h https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:65: #endif comment on endif https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/hardware_renderer.cc (left): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/hardware_renderer.cc:187: if (memory_policy_ == new_policy) This check is still important, but I understand the source of truth has moved and modified somewhere else. How about add a "bool memory_policy_dirty" to shared state, BVR can set it in OnDraw if things changed, and HR only updates policy if it's true. It could also make TrimMemory work entirely within BVR. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/hardware_renderer.cc:36: spurious new line https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/hardware_renderer.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/hardware_renderer.h:29: virtual ~HardwareRenderer(); why virtual? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/shared_renderer_state.h (left): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/shared_renderer_state.h:16: spurious remove https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/tile_resource_consumer.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:11: class TileResourceConsumer { Call this GlobalTileManagerClient https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:13: virtual size_t CalculateTileRequest() = 0; const https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:15: virtual bool IsStarved() = 0; Not needed https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:16: virtual ~TileResourceConsumer() {} In protected section https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:21: #endif comment on endif (yes I know, stupid...) https://codereview.chromium.org/226363004/diff/100001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/native/... android_webview/native/aw_contents.cc:1063: browser_view_renderer_.TrimMemory(); This might be out of scope for this patch, but can you try this? Move everything in HardwareRenderer::TrimMemory directly into BVR, call it directly on UI thread, and test if it works (ie doesn't deadlock)? I think it shouldn't deadlock but you never know... Also another idea. What if TrimMemory goes to GlobalTileManager instead of BVR? Is GlobalTileManager able to make better decisions about what to do?
https://codereview.chromium.org/226363004/diff/100001/android_webview/android... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/android... android_webview/android_webview.gyp:156: 'browser/global_tile_manager.h', why did it compile correctly when I didn't add it? On 2014/04/28 22:22:15, boliu wrote: > add tile_resource_consumer.h too https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:45: typedef content::SynchronousCompositorMemoryPolicy Policy; On 2014/04/28 22:22:15, boliu wrote: > Never do this. This is just confusing. If you want to save typing "content::", > you can use "using". Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:107: SetMemoryPolicy(zero_policy); I'm just being safe here. Why is OnDetached guaranteed to be called before destruction? Is it a contract enforced by Android Framework? Anyways, putting a DCHECK in DidDestroyCompositor should do it. On 2014/04/28 22:22:15, boliu wrote: > This should already happen in OnDetached, not needed here https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:114: void BrowserViewRenderer::TrimMemory() { You mean global tile manager? Yes it is aware of it, because "setMemoryPolicy" will notify the global tile manager of this. On 2014/04/28 22:22:15, boliu wrote: > Is TileManager aware that we are dropping tiles on our own? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:119: ForceDropTiles(); Yes it is enough. Removed. On 2014/04/28 22:22:15, boliu wrote: > Isn't ForceDropTiles enough? Why both? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:122: Policy BrowserViewRenderer::CalculateMemoryPolicy() { I changed the function name to CalculateDesiredMemoryPolicy On 2014/04/28 22:22:15, boliu wrote: > IdealPolicy? Or DesiredPolicy? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:136: void BrowserViewRenderer::SetMemoryPolicy(Policy& new_policy) { Good point (although I put some comments there to explain what this function does) On 2014/04/28 22:22:15, boliu wrote: > SetMemoryPolicy sounds like a simple setter, this is doing way more. > > How about RequestMemoryPolicy? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:144: num_hungry_++; SetMemoryPolicy is called in 3 places: OnDraw, TrimMemory, and DetachedFromWindow The latter 2 cases will just set memory policy to 0 and reset num_hungry_ to 0. Only OnDraw will potentially request some more memory and make the BVR hungry. On 2014/04/28 22:22:15, boliu wrote: > this should only be modified in OnDraw. Too easy to add a call to this, and then > suddenly your policy changed. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:203: GlobalTileManager::GetInstance()->DidDrawGL(tile_manager_key_); I want to be accurate about the MRU inside GlobalTileManager. The most recently drawn view should be the head of MRU. DidDrawGL will put "this" BVR to the head of the MRU. And that should only happen if DrawGL actually succeeded. On 2014/04/28 22:22:15, boliu wrote: > Hmm, why do You you need to condition this on did_draw_gl? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:205: SetMemoryPolicy(old_policy); The memory policy is supposedly read by HardwareRenderer, if the HardwareRenderer indeed make a DrawGL. I don't want to change the memory policy if the HardwareRenderer did not actually DrawGL. On 2014/04/28 22:22:15, boliu wrote: > Why set back to old policy? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:375: shared_renderer_state_->SetCompositorOnUiThread(NULL); Yeah. That makes sense. On 2014/04/28 22:22:15, boliu wrote: > DCHECK here memory policy is 0, (this should only happen after detach/destroy) https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (left): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:38: On 2014/04/28 22:22:15, boliu wrote: > spurious delete Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:107: On 2014/04/28 22:22:15, boliu wrote: > spurious new line Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:116: void SetMemoryPolicy(content::SynchronousCompositorMemoryPolicy& policy); Changed comments On 2014/04/28 22:22:15, boliu wrote: > Comment does not match function at all https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:142: size_t CalculateTileRequest() OVERRIDE; On 2014/04/28 22:22:15, boliu wrote: > virtual, and move comments to the interface Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:223: // kGrallocHungryLimit. CalculateTileRequest returns kNumGrallocLimit (150). The BVR needs kNumGrallocLimit to draw idealy, but it does not get "hungry" until it gets less than kGrallocHungryLimit (100). I did this because I don't want to mark a BVR as hungry when it wants 150 tiles but get 140 (which is still pretty close to ideal). On 2014/04/28 22:22:15, boliu wrote: > s/kGrallocHungryLimit/CalculateTileRequest/ https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.h:224: size_t num_hungry_; On 2014/04/28 22:22:15, boliu wrote: > num_consecutive_hungry_frames_? Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/global_tile_manager.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:12: base::LazyInstance<GlobalTileManager>::Leaky g_tile_manager = On 2014/04/28 22:22:15, boliu wrote: > no indent for namespace Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:13: LAZY_INSTANCE_INITIALIZER; On 2014/04/28 22:22:15, boliu wrote: > blank line below Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:26: void GlobalTileManager::DidDrawGL(Key key) { On 2014/04/28 22:22:15, boliu wrote: > Call this DidDraw, or DidUse. And group the code together with PushBack. Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:30: inactive_views_++; This is essentially the same. Talked in person On 2014/04/28 22:22:15, boliu wrote: > This is not what I described, you are only moving inactive_views_ to the back of > the list, treating everything that was ever done a hardware OnDraw as active. We > want views that have stopped OnDraw to eventually become inactive. > > What you want is *before* moving the current view to the front, check how many > views are in front of it. Add 1 to include the view itself, that's the number of > active views (yes, it will be different for each view) https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:48: for (it = mru_list_.rbegin(); it != mru_list_.rend(); it++) { Yes. On 2014/04/28 22:22:15, boliu wrote: > Shouldn't you begin at something like inactive_views_? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:65: size_t new_num_of_tiles, bool is_starved, Key key) { On 2014/04/28 22:22:15, boliu wrote: > ditto on multi-line argument style Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:74: size_t tiles = allocated_tiles_ - old_num_of_tiles + new_num_of_tiles; On 2014/04/28 22:22:15, boliu wrote: > s/tiles/new_total_allocated_tiles/ Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:77: allocated_tiles_ = tiles; On 2014/04/28 22:22:15, boliu wrote: > Because individual view policy is inside each view, this class should have > DCHECKs that allocated_tiles_ remains the sum of everything. I guess add up > everything in a for loop, and only do it if DCHECK_IS_ON is set. Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.cc:83: if (!is_starved) { We talked about this in person... On 2014/04/28 22:22:15, boliu wrote: > The staved concept does not help with stability at all. If there are 10 views > drawing, then each view can get at most 450 / 10 = 45 tiles, which is below the > 100. After 5 cycles, the views are going to start killing each other. > > I think you should only drop inactive views. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/global_tile_manager.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:25: typedef std::list<TileResourceConsumer*> ListType; On 2014/04/28 22:22:15, boliu wrote: > newline below Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:34: size_t RequestTiles(size_t old_num_of_tiles, size_t new_num_of_tiles, Pretty cool. Just did git cl format. On 2014/04/28 22:22:15, boliu wrote: > c++ has a different new line wrap style, either all arguments are one a single > one or one argument on each line. Recommend using clang-format and not think > about it: https://code.google.com/p/chromium/wiki/ClangFormat https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:43: void Remove(Key key); On 2014/04/28 22:22:15, boliu wrote: > newline below Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:47: ~GlobalTileManager(); On 2014/04/28 22:22:15, boliu wrote: > new line below Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:55: size_t allocated_tiles_; On 2014/04/28 22:22:15, boliu wrote: > total_allocated_tiles_ Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:59: yes it does. But I don't understand "sequenceChecker". If I put CalledOnValidSequencedThread in the functions, it just ensures the functions are called on the same thread, right? Why not just BrowserThread::UI? On 2014/04/28 22:22:15, boliu wrote: > This class lives on UI, right? Add a SequenceChecker: base/sequence_checker.h https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:65: #endif On 2014/04/28 22:22:15, boliu wrote: > comment on endif Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/hardware_renderer.cc (left): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/hardware_renderer.cc:187: if (memory_policy_ == new_policy) I add the boolean in shared renderer state and hardware renderer will check if the policy is dirty before setting the policy to compositor. And after that, the dirty boolean will false. On 2014/04/28 22:22:15, boliu wrote: > This check is still important, but I understand the source of truth has moved > and modified somewhere else. How about add a "bool memory_policy_dirty" to > shared state, BVR can set it in OnDraw if things changed, and HR only updates > policy if it's true. > > It could also make TrimMemory work entirely within BVR. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/hardware_renderer.cc:36: On 2014/04/28 22:22:15, boliu wrote: > spurious new line Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/hardware_renderer.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/hardware_renderer.h:29: virtual ~HardwareRenderer(); Thanks for pointing this out! I will remove it. It is an unrelated change when I was doing some local hacks. On 2014/04/28 22:22:15, boliu wrote: > why virtual? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/shared_renderer_state.h (left): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/shared_renderer_state.h:16: On 2014/04/28 22:22:15, boliu wrote: > spurious remove Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/tile_resource_consumer.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:11: class TileResourceConsumer { On 2014/04/28 22:22:15, boliu wrote: > Call this GlobalTileManagerClient Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:13: virtual size_t CalculateTileRequest() = 0; On 2014/04/28 22:22:15, boliu wrote: > const Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:15: virtual bool IsStarved() = 0; On 2014/04/28 22:22:15, boliu wrote: > Not needed Actually, we need a function for the client to tell the manager that if the request is unmet, the manager needs to force drop tiles of other clients to *try to* satisfy the requesting client. (it is okay that the manager still fails to satisfy the request, after the manager evicts all other clients) This function does not need to have the notion of "starvation", but it needs to indicate that the requesting client wants the manager to evict. So I will rename this function to "NeedsEviction". How does that sound? https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:16: virtual ~TileResourceConsumer() {} On 2014/04/28 22:22:15, boliu wrote: > In protected section Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/tile_resource_consumer.h:21: #endif On 2014/04/28 22:22:15, boliu wrote: > comment on endif (yes I know, stupid...) Done. https://codereview.chromium.org/226363004/diff/100001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/native/... android_webview/native/aw_contents.cc:1063: browser_view_renderer_.TrimMemory(); I put trim memory into BVR. Haven't thought about putting it into GlobalTileManager yet. Right now, every webview will lose all tiles during trim memory. If we need to trim less memory and keep some tiles for some webviews, then we will need to do it GlobalTileManager. But I don't think this is necessary in near future. On 2014/04/28 22:22:15, boliu wrote: > This might be out of scope for this patch, but can you try this? > > Move everything in HardwareRenderer::TrimMemory directly into BVR, call it > directly on UI thread, and test if it works (ie doesn't deadlock)? I think it > shouldn't deadlock but you never know... > > Also another idea. What if TrimMemory goes to GlobalTileManager instead of BVR? > Is GlobalTileManager able to make better decisions about what to do?
https://codereview.chromium.org/226363004/diff/100001/android_webview/android... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/android... android_webview/android_webview.gyp:156: 'browser/global_tile_manager.h', On 2014/04/30 20:50:17, hush wrote: > why did it compile correctly when I didn't add it? > On 2014/04/28 22:22:15, boliu wrote: > > add tile_resource_consumer.h too > At least with gcc, header file dependency can be deduced at compile time. But I'm not sure if that's true across all toolchains that chromium supports, so better be safe. https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:107: SetMemoryPolicy(zero_policy); On 2014/04/30 20:50:17, hush wrote: > I'm just being safe here. > Why is OnDetached guaranteed to be called before destruction? Is it a contract > enforced by Android Framework? First, webview is never gc-ed before it is detached. So far looks like that assumption holds. We destroy native when we are gc-ed, so detach before destruction is guaranteed. The snag is that we also destroy native side when app calls Webview.destroy(), and that was a problem for us before. But if you look at the implementation of destroy, if destroy is called while the view is attached, then we don't actually destroy native, but wait until we are detached to do it. So yes, the detach before destroy must hold, otherwise we never get a chance to clean up gl resources > > Anyways, putting a DCHECK in DidDestroyCompositor should do it. > On 2014/04/28 22:22:15, boliu wrote: > > This should already happen in OnDetached, not needed here > https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:205: SetMemoryPolicy(old_policy); On 2014/04/30 20:50:17, hush wrote: > The memory policy is supposedly read by HardwareRenderer, if the > HardwareRenderer indeed make a DrawGL. I don't want to change the memory policy > if the HardwareRenderer did not actually DrawGL. > On 2014/04/28 22:22:15, boliu wrote: > > Why set back to old policy? > Let's assume that never happens, that if HardwareRenderer is up, then did_draw_gl_ must be true. What we are really missing is a commit, ie a function copies everything HR *if and only if* HR is about to draw the new frame. I think we'll get that eventually https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... File android_webview/browser/global_tile_manager.h (right): https://codereview.chromium.org/226363004/diff/100001/android_webview/browser... android_webview/browser/global_tile_manager.h:59: On 2014/04/30 20:50:17, hush wrote: > yes it does. But I don't understand "sequenceChecker". If I put > CalledOnValidSequencedThread in the functions, it just ensures the functions are > called on the same thread, right? Why not just BrowserThread::UI? > On 2014/04/28 22:22:15, boliu wrote: > > This class lives on UI, right? Add a SequenceChecker: base/sequence_checker.h > Yes. Let's try not depending on BrowserThreads in this class to make testing easier. https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:178: double area = (double)draw_gl_input_.global_visible_rect.width() * against style guide to use c-style casts, always use things like static_cast<> Don't read out of draw_gl_input, pass them in as parameters Viewport is not the right number here, what you want is the screen size. View size can be way bigger than screen (think gmail). Considering the current screen sizes of devices, and the fact that (max) tile size is 372x372, can we use a heuristic that does not rely on screen size? So like how we calculate memory as visible size * constant, can we do the same here? Of course experiment first. https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/global_tile_manager.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager.cc:39: for (Key it = inactive_views_; it != mru_list_.end(); it++) { On 2014/04/30 20:50:17, hush wrote: > Yes. > On 2014/04/28 22:22:15, boliu wrote: > > Shouldn't you begin at something like inactive_views_? > No...I was wrong. You should use rbegin, but *stop* when you get to inactive_views_. After you get rid of inactive_views, you can just stop on line when *it == *key. https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager.cc:56: bool is_starved, var name mismatch with header, so are you keeping this around to do some testing? because I'd rather you not commit it if it turns out to be not needed https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager.cc:104: inactive_views_++; Ehh, I thought we could do this without keeping inactive_views_, ie call RequestTiles *before* moving view to front? https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/global_tile_manager_client.h (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager_client.h:13: virtual size_t CalculateTileRequest() const = 0; new line after each method declaration https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager_client.h:20: virtual size_t GetTilesNum() const = 0; GetNumTiles? Also add comment saying it's only called for debugging purpose https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/shared_renderer_state.cc:23: ui_thread_weak_ptr_(weak_factory_on_ui_thread_.GetWeakPtr()) { initialize dirty var https://codereview.chromium.org/226363004/diff/140001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/native/... android_webview/native/aw_contents.cc:1004: base::Bind(&AwContents::TrimMemory, ui_thread_weak_ptr_, level, visible)); You don't need this trip to RT, change AwContents.java to not run executeHardwareAction, and directly call TrimMemory below and into BVR. It should all just work...
https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:178: double area = (double)draw_gl_input_.global_visible_rect.width() * CalculateTileRequest() is an inherited method. I will overload CalculateTileRequest so that it takes width and height. For now, I will just use a multiplier of 12. On a Nexus 5, a full screen view will use around 150. On a Nexus 7, a full screen will use around 80 tiles. Plumbing down the screen sizes from Java to native bvr takes additional work and may be done later. On 2014/05/01 00:37:40, boliu wrote: > against style guide to use c-style casts, always use things like static_cast<> > > Don't read out of draw_gl_input, pass them in as parameters > > Viewport is not the right number here, what you want is the screen size. View > size can be way bigger than screen (think gmail). > > Considering the current screen sizes of devices, and the fact that (max) tile > size is 372x372, can we use a heuristic that does not rely on screen size? So > like how we calculate memory as visible size * constant, can we do the same > here? Of course experiment first. https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/global_tile_manager.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager.cc:39: for (Key it = inactive_views_; it != mru_list_.end(); it++) { On 2014/05/01 00:37:40, boliu wrote: > On 2014/04/30 20:50:17, hush wrote: > > Yes. > > On 2014/04/28 22:22:15, boliu wrote: > > > Shouldn't you begin at something like inactive_views_? > > > > No...I was wrong. > > You should use rbegin, but *stop* when you get to inactive_views_. After you get > rid of inactive_views, you can just stop on line when *it == *key. I will backwards traverse from the end of the list until I reach "inactive_views". https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager.cc:56: bool is_starved, I will change the bool name to needs_eviction On 2014/05/01 00:37:40, boliu wrote: > var name mismatch with header, so are you keeping this around to do some > testing? because I'd rather you not commit it if it turns out to be not needed https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager.cc:104: inactive_views_++; Manager needs a pointer to inactive_views_ anyways when it calls Evict(), because it only evicts the inactive views and needs to traverse from the end of the list until inactive_views_ On 2014/05/01 00:37:40, boliu wrote: > Ehh, I thought we could do this without keeping inactive_views_, ie call > RequestTiles *before* moving view to front? https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/global_tile_manager_client.h (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager_client.h:13: virtual size_t CalculateTileRequest() const = 0; On 2014/05/01 00:37:40, boliu wrote: > new line after each method declaration Done. https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/global_tile_manager_client.h:20: virtual size_t GetTilesNum() const = 0; Changed the function name. Actually I use it more places than debugging. In GlobalTileManager::RequestTiles, we actually don't need to pass in "old_num_of_tiles", because we can get it from "key". In GlobalTileManager::Remove, I will get the number of tiles of the removed client and update the total_allocated_tiles_ (This change will appear in the next patch set) It also hides/encapsulates the fact that we not directly storing the number of tiles in the client, but in a "shared renderer state" On 2014/05/01 00:37:40, boliu wrote: > GetNumTiles? > > Also add comment saying it's only called for debugging purpose https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... File android_webview/browser/shared_renderer_state.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/browser... android_webview/browser/shared_renderer_state.cc:23: ui_thread_weak_ptr_(weak_factory_on_ui_thread_.GetWeakPtr()) { On 2014/05/01 00:37:40, boliu wrote: > initialize dirty var Done. https://codereview.chromium.org/226363004/diff/140001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/226363004/diff/140001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:235: // This is a no-op if not currently attached. this change will be gone after rebasing https://codereview.chromium.org/226363004/diff/140001/android_webview/native/... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/226363004/diff/140001/android_webview/native/... android_webview/native/aw_contents.cc:1004: base::Bind(&AwContents::TrimMemory, ui_thread_weak_ptr_, level, visible)); I will rebase onto your change here: https://codereview.chromium.org/255023004/ I removed TrimMemoryOnRenderThread On 2014/05/01 00:37:40, boliu wrote: > You don't need this trip to RT, change AwContents.java to not run > executeHardwareAction, and directly call TrimMemory below and into BVR. It > should all just work...
Hi Bo, We discussed this yesterday. PTAL the new patch.
https://codereview.chromium.org/226363004/diff/220001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/220001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:44: const size_t kTileArea = 372 * 372; I lied, it's kDefaultTileSize in HR, which is 384. Can you not duplicate this the constant?
On 2014/05/02 17:54:34, boliu wrote: > https://codereview.chromium.org/226363004/diff/220001/android_webview/browser... > File android_webview/browser/browser_view_renderer.cc (right): > > https://codereview.chromium.org/226363004/diff/220001/android_webview/browser... > android_webview/browser/browser_view_renderer.cc:44: const size_t kTileArea = > 372 * 372; > I lied, it's kDefaultTileSize in HR, which is 384. > > Can you not duplicate this the constant? I will use the command line switch then.
https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:191: size_t BrowserViewRenderer::CalculateTileRequest( Don't need this helper, this is only called from one place. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:195: static_cast<double>(global_visible_width * global_visible_height); Do we really need to cast it to double? We are rounding to a multiple of kTileAllocationStep already, so losing a bit of precision shouldn't matter. And the calculation below, you can do area * kTileMultiplier / g_tile_area to avoid division resulting in 0. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:206: num_tiles_ = num_tiles; Early out if these two are the same already. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:215: ForceFakeCompositeSW(); Set dirty to false after this. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:381: RequestMemoryPolicy(zero_policy, true); Hmm, right now we don't need effective_immediately in detach (hardware is already destroyed when it happens) In new ubercomp world, this method will tear down hardware here right after requesting memory policy, so no need to set effective_immdiately either. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/global_tile_manager.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager.cc:62: bool effective_immediately, You shouldn't need to pass this into Manager. Manager should know that the clients not calling RequestTiles are the ones that need to drop immediately. The calling client can decide what it wants to do immediately after RequestTiles. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/global_tile_manager_unittest.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager_unittest.cc:36: size_t num_tiles_; No underscore if aren't actually private. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager_unittest.cc:51: key[i] = manager()->PushBack(&clients[i]); PushBack and Remove should be handled by client constructor/destructor. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager_unittest.cc:129: } Requesting a few more tests. N active clients drawing in a cycle, each client should converge to total / n. N active clients and M inactive clients, make sure only tiles of inactive clients are dropped, and the active clients should converge to total / n.
PTAL. All 5 unit tests passed. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:191: size_t BrowserViewRenderer::CalculateTileRequest( On 2014/05/02 20:28:34, boliu wrote: > Don't need this helper, this is only called from one place. Done. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:195: static_cast<double>(global_visible_width * global_visible_height); (talked in person) I wanted to use double to prevent size_t overflow. But this is unlikely and we are using size_t to store the number of bytes anyway. > area * kTileMultiplier / g_tile_area to avoid division resulting in 0. I think with extremely small "area", the division could still be 0. On 2014/05/02 20:28:34, boliu wrote: > Do we really need to cast it to double? We are rounding to a multiple of > kTileAllocationStep already, so losing a bit of precision shouldn't matter. And > the calculation below, you can do > > area * kTileMultiplier / g_tile_area to avoid division resulting in 0. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:206: num_tiles_ = num_tiles; On 2014/05/02 20:28:34, boliu wrote: > Early out if these two are the same already. Done. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:215: ForceFakeCompositeSW(); On 2014/05/02 20:28:34, boliu wrote: > Set dirty to false after this. Done. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:381: RequestMemoryPolicy(zero_policy, true); On 2014/05/02 20:28:34, boliu wrote: > Hmm, right now we don't need effective_immediately in detach (hardware is > already destroyed when it happens) > > In new ubercomp world, this method will tear down hardware here right after > requesting memory policy, so no need to set effective_immdiately either. Done. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/global_tile_manager.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager.cc:62: bool effective_immediately, On 2014/05/02 20:28:34, boliu wrote: > You shouldn't need to pass this into Manager. Manager should know that the > clients not calling RequestTiles are the ones that need to drop immediately. > > The calling client can decide what it wants to do immediately after > RequestTiles. Done. https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/global_tile_manager_unittest.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager_unittest.cc:36: size_t num_tiles_; On 2014/05/02 20:28:34, boliu wrote: > No underscore if aren't actually private. Made these 2 private https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager_unittest.cc:51: key[i] = manager()->PushBack(&clients[i]); On 2014/05/02 20:28:34, boliu wrote: > PushBack and Remove should be handled by client constructor/destructor. Done.
https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... File android_webview/browser/global_tile_manager_unittest.cc (right): https://codereview.chromium.org/226363004/diff/240001/android_webview/browser... android_webview/browser/global_tile_manager_unittest.cc:129: } On 2014/05/02 20:28:34, boliu wrote: > Requesting a few more tests. > > N active clients drawing in a cycle, each client should converge to total / n. > > N active clients and M inactive clients, make sure only tiles of inactive > clients are dropped, and the active clients should converge to total / n. Done.
lgtm https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... android_webview/browser/browser_view_renderer.cc:175: std::max(width * height * kTileMultiplier / g_tile_area, (size_t)1); 1u (unsigned integer 1) Also never do c-style casts https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... android_webview/browser/browser_view_renderer.h:115: content::SynchronousCompositorMemoryPolicy policy); I think these two methods can become private now.
On 2014/05/02 22:39:17, boliu wrote: > lgtm > > https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... > File android_webview/browser/browser_view_renderer.cc (right): > > https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... > android_webview/browser/browser_view_renderer.cc:175: std::max(width * height * > kTileMultiplier / g_tile_area, (size_t)1); > 1u (unsigned integer 1) > > Also never do c-style casts > > https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... > File android_webview/browser/browser_view_renderer.h (right): > > https://codereview.chromium.org/226363004/diff/250001/android_webview/browser... > android_webview/browser/browser_view_renderer.h:115: > content::SynchronousCompositorMemoryPolicy policy); > I think these two methods can become private now. Done
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/226363004/270001
I will create a separate CL to fix the policy.bytes_limit allocation after this is committed. Right now there is an off by one error. We talked this in person.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium
On 2014/05/03 00:16:16, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_chromium_x64_rel on tryserver.chromium my changes are not related to windows at all. Retrying...
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/226363004/270001
Message was sent while issue was closed.
Change committed as 268006 |
