Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1016733002:
cc: Free gpu resources when we are invisible or TM is oom. (Closed)
|
Created:
5 years, 9 months ago by sohanjg Modified:
5 years, 2 months ago CC:
bsalomon_chromium, cc-bugs_chromium.org, chromium-reviews, Jvsquare, joshua.litt Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Free Gpu resources when we are invisible or TileManager is OOM.
This patch free's the gpu resources held up by GrContext, when LTHI
becomes invisible or our hard memory limit has reduced to 0 (OOM).
BUG=367196
Patch Set 1 #
Total comments: 6
Patch Set 2 : review comments addressed. #
Total comments: 8
Patch Set 3 : restore gr cache limit #
Total comments: 1
Patch Set 4 : review comments addressed. #
Total comments: 10
Patch Set 5 : review comments addressed. #
Total comments: 4
Patch Set 6 : use freegpuresource instead of cache limit maintenance. #
Messages
Total messages: 67 (5 generated)
sohan.jyoti@samsung.com changed reviewers: + danakj@chromium.org, reveman@chromium.org, vmiura@chromium.org
Please take a look, thanks. TileManager doesnt seem to handle any OOM tests explicitly now, should we add something there ? https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:643: rasterizer_->ClearCache(); Will this be the only place to clear cache when OOM ?
On 2015/03/17 11:20:18, sohanjg wrote: > Please take a look, thanks. > > TileManager doesnt seem to handle any OOM tests explicitly now, should we add > something there ? > > https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.cc > File cc/resources/tile_manager.cc (right): > > https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.c... > cc/resources/tile_manager.cc:643: rasterizer_->ClearCache(); > Will this be the only place to clear cache when OOM ? Just a side note, i saw the raster perf visibly a little better when i was just experimenting to clear-up the cache every now and then. Are we planning to optimize the cache usage in any way now (as we would still raster w/o cache), for non-static pages? Currently, max texture = 2048 and tex bytes = 100663296.
vmpstr@, danakj@ could you provide some guidance on where to handle a memory policy change? I think we'd want to reduce Ganesh's memory limits to 0 when TileManager has 0 memory, and also restore them when TileManager was != 0. https://codereview.chromium.org/1016733002/diff/1/cc/resources/gpu_rasterizer.cc File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/1/cc/resources/gpu_rasterizer... cc/resources/gpu_rasterizer.cc:195: GrContext* gr_context = GetContextProvider(false)->GrContext(); For threaded GPU raster we need to take a lock on the worker context. https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/gpu_t... ContextProvider* context_provider = rasterizer_->resource_provider() ->output_surface() ->worker_context_provider(); // The context lock must be held while accessing the context on a // worker thread. base::AutoLock context_lock(*context_provider->GetLock()); // Allow context to bind to current thread. context_provider->DetachFromThread(); // Rasterize source into resource. reduce cache limit here // Barrier to sync worker context output to cc context. context_provider->ContextGL()->OrderingBarrierCHROMIUM(); // Allow context to bind to other threads. context_provider->DetachFromThread(); https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:643: rasterizer_->ClearCache(); On 2015/03/17 11:20:18, sohanjg wrote: > Will this be the only place to clear cache when OOM ? I don't think this is the OOM condition where we should clear the cache. What I think we want is that we clear the cache when TileManager's hard memory policy changes to 0 bytes.
danakj@chromium.org changed reviewers: + enne@chromium.org
Yeah, LayerTreeHostImpl::UpdateTileManagerMemoryPolicy/EnforceMamangedMemoryPolicy seems like a better spot for this sort of thing. Maybe we also want to clear the cache when the tab goes invisible?
Thanks for the review, please take a look. https://codereview.chromium.org/1016733002/diff/1/cc/resources/gpu_rasterizer.cc File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/1/cc/resources/gpu_rasterizer... cc/resources/gpu_rasterizer.cc:195: GrContext* gr_context = GetContextProvider(false)->GrContext(); On 2015/03/19 21:20:35, vmiura wrote: > For threaded GPU raster we need to take a lock on the worker context. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/gpu_t... > > ContextProvider* context_provider = rasterizer_->resource_provider() > ->output_surface() > ->worker_context_provider(); > > // The context lock must be held while accessing the context on a > // worker thread. > base::AutoLock context_lock(*context_provider->GetLock()); > > // Allow context to bind to current thread. > context_provider->DetachFromThread(); > > // Rasterize source into resource. > reduce cache limit here > > // Barrier to sync worker context output to cc context. > context_provider->ContextGL()->OrderingBarrierCHROMIUM(); > > // Allow context to bind to other threads. > context_provider->DetachFromThread(); Done. https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:643: rasterizer_->ClearCache(); On 2015/03/19 21:20:35, vmiura wrote: > On 2015/03/17 11:20:18, sohanjg wrote: > > Will this be the only place to clear cache when OOM ? > > I don't think this is the OOM condition where we should clear the cache. > > What I think we want is that we clear the cache when TileManager's hard memory > policy changes to 0 bytes. Acknowledged.
https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:643: rasterizer_->ClearCache(); On 2015/03/17 11:20:18, sohanjg wrote: > Will this be the only place to clear cache when OOM ? I'm not sure this is actually what we want to do. What I think we want is that we clear the Ganesh cache when TileManager hard memory limit is reduced to 0. vmpstr@ could you provide some guidance? https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:208: gr_context->setResourceCacheLimits(0, 0); We'll want to not have limits stuck at 0 forever. Maybe restore the prior limits returned by getResourceCacheLimits after? https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:211: context_provider->ContextGL()->OrderingBarrierCHROMIUM(); I think it's not necessary to use an ordering barrier here.
On 2015/03/23 20:34:40, vmiura wrote: > https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.cc > File cc/resources/tile_manager.cc (right): > > https://codereview.chromium.org/1016733002/diff/1/cc/resources/tile_manager.c... > cc/resources/tile_manager.cc:643: rasterizer_->ClearCache(); > On 2015/03/17 11:20:18, sohanjg wrote: > > Will this be the only place to clear cache when OOM ? > > I'm not sure this is actually what we want to do. > > What I think we want is that we clear the Ganesh cache when TileManager hard > memory limit is reduced to 0. > > vmpstr@ could you provide some guidance? Please ignore this stale comment, thanks.
https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:208: gr_context->setResourceCacheLimits(0, 0); On 2015/03/23 20:34:40, vmiura wrote: > We'll want to not have limits stuck at 0 forever. > > Maybe restore the prior limits returned by getResourceCacheLimits after? Acknowledged. Also, should we maintain the limits as gpu_rasterizer member? and restore them when hard_memory_limit_in_bytes is no longer zero in LTHI global_tile_state_? https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:211: context_provider->ContextGL()->OrderingBarrierCHROMIUM(); On 2015/03/23 20:34:40, vmiura wrote: > I think it's not necessary to use an ordering barrier here. Acknowledged. And i wonder, ClearCache() will be invoked from LTHI::UpdateTileManagerMemoryPolicy(), will it really be in worker thread context unlike RasterBufferImpl::Playback() ?
https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:208: gr_context->setResourceCacheLimits(0, 0); On 2015/03/24 05:42:09, sohanjg wrote: > On 2015/03/23 20:34:40, vmiura wrote: > > We'll want to not have limits stuck at 0 forever. > > > > Maybe restore the prior limits returned by getResourceCacheLimits after? > > Acknowledged. > Also, should we maintain the limits as gpu_rasterizer member? > and restore them when hard_memory_limit_in_bytes is no longer zero in LTHI > global_tile_state_? Sounds like a good solution. https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:211: context_provider->ContextGL()->OrderingBarrierCHROMIUM(); On 2015/03/24 05:42:09, sohanjg wrote: > On 2015/03/23 20:34:40, vmiura wrote: > > I think it's not necessary to use an ordering barrier here. > > Acknowledged. > > And i wonder, ClearCache() will be invoked from > LTHI::UpdateTileManagerMemoryPolicy(), will it really be in worker thread > context unlike RasterBufferImpl::Playback() ? Ah, we have to be careful here. Threaded-GPU mode uses 'worker_context_provider', but sync-GPU uses 'context_provider'. The latter will be removed, but for now we shouldn't break it. For rasterization, that's just picked here: https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou... In case it's threaded-GPU, both the worker thread and this function on the CC thread will contend for 'worker_context_provider', hence needing the lock on context_provider->GetLock(), context_provider->DetachFromThread(), etc. For 'context_provider' case there is no contention.
Can you please take a look. thanks. https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:208: gr_context->setResourceCacheLimits(0, 0); On 2015/03/24 06:32:52, vmiura wrote: > On 2015/03/24 05:42:09, sohanjg wrote: > > On 2015/03/23 20:34:40, vmiura wrote: > > > We'll want to not have limits stuck at 0 forever. > > > > > > Maybe restore the prior limits returned by getResourceCacheLimits after? > > > > Acknowledged. > > Also, should we maintain the limits as gpu_rasterizer member? > > and restore them when hard_memory_limit_in_bytes is no longer zero in LTHI > > global_tile_state_? > > Sounds like a good solution. Done. https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:211: context_provider->ContextGL()->OrderingBarrierCHROMIUM(); On 2015/03/24 06:32:52, vmiura wrote: > On 2015/03/24 05:42:09, sohanjg wrote: > > On 2015/03/23 20:34:40, vmiura wrote: > > > I think it's not necessary to use an ordering barrier here. > > > > Acknowledged. > > > > And i wonder, ClearCache() will be invoked from > > LTHI::UpdateTileManagerMemoryPolicy(), will it really be in worker thread > > context unlike RasterBufferImpl::Playback() ? > > Ah, we have to be careful here. > > Threaded-GPU mode uses 'worker_context_provider', but sync-GPU uses > 'context_provider'. The latter will be removed, but for now we shouldn't break > it. > > For rasterization, that's just picked here: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou... > > In case it's threaded-GPU, both the worker thread and this function on the CC > thread will contend for 'worker_context_provider', hence needing the lock on > context_provider->GetLock(), context_provider->DetachFromThread(), etc. > > For 'context_provider' case there is no contention. Acknowledged.
On 2015/03/24 13:03:36, sohanjg wrote: > Can you please take a look. thanks. > > https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... > File cc/resources/gpu_rasterizer.cc (right): > > https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... > cc/resources/gpu_rasterizer.cc:208: gr_context->setResourceCacheLimits(0, 0); > On 2015/03/24 06:32:52, vmiura wrote: > > On 2015/03/24 05:42:09, sohanjg wrote: > > > On 2015/03/23 20:34:40, vmiura wrote: > > > > We'll want to not have limits stuck at 0 forever. > > > > > > > > Maybe restore the prior limits returned by getResourceCacheLimits after? > > > > > > Acknowledged. > > > Also, should we maintain the limits as gpu_rasterizer member? > > > and restore them when hard_memory_limit_in_bytes is no longer zero in LTHI > > > global_tile_state_? > > > > Sounds like a good solution. > > Done. > > https://codereview.chromium.org/1016733002/diff/20001/cc/resources/gpu_raster... > cc/resources/gpu_rasterizer.cc:211: > context_provider->ContextGL()->OrderingBarrierCHROMIUM(); > On 2015/03/24 06:32:52, vmiura wrote: > > On 2015/03/24 05:42:09, sohanjg wrote: > > > On 2015/03/23 20:34:40, vmiura wrote: > > > > I think it's not necessary to use an ordering barrier here. > > > > > > Acknowledged. > > > > > > And i wonder, ClearCache() will be invoked from > > > LTHI::UpdateTileManagerMemoryPolicy(), will it really be in worker thread > > > context unlike RasterBufferImpl::Playback() ? > > > > Ah, we have to be careful here. > > > > Threaded-GPU mode uses 'worker_context_provider', but sync-GPU uses > > 'context_provider'. The latter will be removed, but for now we shouldn't > break > > it. > > > > For rasterization, that's just picked here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou... > > > > In case it's threaded-GPU, both the worker thread and this function on the CC > > thread will contend for 'worker_context_provider', hence needing the lock on > > context_provider->GetLock(), context_provider->DetachFromThread(), etc. > > > > For 'context_provider' case there is no contention. > > Acknowledged. ping! :)
https://codereview.chromium.org/1016733002/diff/40001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/40001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:211: gr_context->getResourceCacheLimits(&maxTextures_, &maxTextureBytes_); Who sets these limits in the first place? It's weird that you have to stash the limits while you reset it.
On 2015/03/31 18:43:06, enne wrote: > https://codereview.chromium.org/1016733002/diff/40001/cc/resources/gpu_raster... > File cc/resources/gpu_rasterizer.cc (right): > > https://codereview.chromium.org/1016733002/diff/40001/cc/resources/gpu_raster... > cc/resources/gpu_rasterizer.cc:211: > gr_context->getResourceCacheLimits(&maxTextures_, &maxTextureBytes_); > Who sets these limits in the first place? It's weird that you have to stash the > limits while you reset it. Hmm..GrContextForWebGraphicsContext3D in its ctor sets the limits. When we access contextprovider grcontext for the first time the cache limits are set. Should we plan to save the old limits differently (outside gpu rasterizer?)
vmiura@chromium.org changed reviewers: + bsalomon@chromium.org
+ bsalomon@ Brian, should we default these values in CC instead of using GrContext defaults? At some point, we might want to start adjusting these by device.
On 2015/04/01 18:27:47, vmiura wrote: > + bsalomon@ > > Brian, should we default these values in CC instead of using GrContext defaults? > At some point, we might want to start adjusting these by device. Yes, I think so. Our defaults are meant to be taken with a grain of salt.
On 2015/04/01 21:08:29, bsalomon wrote: > On 2015/04/01 18:27:47, vmiura wrote: > > + bsalomon@ > > > > Brian, should we default these values in CC instead of using GrContext > defaults? > > At some point, we might want to start adjusting these by device. > > Yes, I think so. Our defaults are meant to be taken with a grain of salt. Sounds good. Sohan, could you initialize the limits in GpuRasterizer constructor, matching current GrContext defaults? Also I would factor the logic for updating the limits into a SetGrCacheLimits() method, and use between ClearCache() & RestoreCacheLimits(). I also wonder if ClearCache/RestoreCacheLimits naming could be more descriptive, like SetZeroMemoryLimit/SetDefaultMemoryLimit?
thanks for the review, can you please take a look.
Thanks for the update. A few comments. Enne/Dana, what do you think we should be doing for memory limits on GLRenderer's GrContext? Is it a concern? I kind of expect that no textures would be cached after a filter runs. https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:64: ContextProvider* worker_context_provider = As this is in three places, please refactor the code for setting cache limits to a single method. https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.h (right): https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.h:73: int maxTextures_; nit: maxTextures_ -> max_textures_. maxTextureBytes_ -> max_texture_bytes_. https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.h:75: bool cache_cleared_; Instead of cache_cleared_, invert to "bool default_memory_limit_set_"? https://codereview.chromium.org/1016733002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1016733002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1179: if (use_gpu_rasterization_ && nit: I think this would be easier to understand that it's either Zero or Default -- if (use_gpu_rasterization_ && rasterizer_) { if (!visible_ || global_tile_state_.hard_memory_limit_in_bytes == 0) rasterizer_->SetZeroMemoryLimit(); else rasterizer_->SetDefaultMemoryLimit(); } https://codereview.chromium.org/1016733002/diff/60001/webkit/common/gpu/grcon... File webkit/common/gpu/grcontext_for_webgraphicscontext3d.cc (left): https://codereview.chromium.org/1016733002/diff/60001/webkit/common/gpu/grcon... webkit/common/gpu/grcontext_for_webgraphicscontext3d.cc:71: kMaxGaneshResourceCacheBytes); I'm not sure it's OK to remove this initialization. GrContext can be used outside of GpuRasterizer, e.g. in GlRenderer.
piman@ mentioned we may want to call GLES2Implementation::SetSurfaceVisible(false/true) as well, to free transfer buffers used by GLES2Implementation. Would an API like this be better than SetZeroMemoryLimit/SetDefaultMemoryLimit? void Rasterizer::SetMemoryLimits(bool visible, size_t hard_memory_bytes);
On 2015/04/02 at 19:12:34, vmiura wrote: > Enne/Dana, what do you think we should be doing for memory limits on GLRenderer's GrContext? Is it a concern? I kind of expect that no textures would be cached after a filter runs. Yeah, there are temporary textures, but it doesn't keep them around. So, I don't think there's any work to do there.
Sorry for being late on this, was OOO. Please take a look, thanks. https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:64: ContextProvider* worker_context_provider = On 2015/04/02 19:12:34, vmiura wrote: > As this is in three places, please refactor the code for setting cache limits to > a single method. Done. https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.h (right): https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.h:73: int maxTextures_; On 2015/04/02 19:12:34, vmiura wrote: > nit: maxTextures_ -> max_textures_. maxTextureBytes_ -> max_texture_bytes_. Done. https://codereview.chromium.org/1016733002/diff/60001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.h:75: bool cache_cleared_; On 2015/04/02 19:12:34, vmiura wrote: > Instead of cache_cleared_, invert to "bool default_memory_limit_set_"? Done. https://codereview.chromium.org/1016733002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1016733002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1179: if (use_gpu_rasterization_ && On 2015/04/02 19:12:34, vmiura wrote: > nit: I think this would be easier to understand that it's either Zero or Default > -- > > if (use_gpu_rasterization_ && rasterizer_) { > if (!visible_ || global_tile_state_.hard_memory_limit_in_bytes == 0) > rasterizer_->SetZeroMemoryLimit(); > else > rasterizer_->SetDefaultMemoryLimit(); > } Done. https://codereview.chromium.org/1016733002/diff/60001/webkit/common/gpu/grcon... File webkit/common/gpu/grcontext_for_webgraphicscontext3d.cc (left): https://codereview.chromium.org/1016733002/diff/60001/webkit/common/gpu/grcon... webkit/common/gpu/grcontext_for_webgraphicscontext3d.cc:71: kMaxGaneshResourceCacheBytes); On 2015/04/02 19:12:34, vmiura wrote: > I'm not sure it's OK to remove this initialization. > > GrContext can be used outside of GpuRasterizer, e.g. in GlRenderer. Acknowledged. Added it back.
Sorry for being late on this, was OOO. Please take a look, thanks.
https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:30: const int kMaxGaneshResourceCacheCount = 2048; Maybe you could make these LayerTreeSettings, and then LayerTreeHost could call SetMemoryLimits(cache_count, cache_bytes), taking into account visible=0 or hard memory limit=0 itself?
+hendrikw@ Heads up that crrev.com/1063493002 moved the Renderer instance from LTHI to under GpuTileTaskWorkerPool.
Ahh! this makes things interesting :) what should be the plan, have TileTaskWorkerPool expose a Rasterizer to LTHI to set the memory limits ? But then again, GpuRasterizer doesn't inehrit from Rasterizer anymore :/, (bring it back?) Or its logical to setmemorylimit for TileTaskWorkerPool, in LTHI, and internally make GpuTileTaskWorkerPool use this to set limits to GpuRasterizer/GrContext ? https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:30: const int kMaxGaneshResourceCacheCount = 2048; On 2015/04/08 17:19:34, enne wrote: > Maybe you could make these LayerTreeSettings, and then LayerTreeHost could call > SetMemoryLimits(cache_count, cache_bytes), taking into account visible=0 or hard > memory limit=0 itself? OK, but the memory limit we are maintaining in the Impl side, do we have the info in LTH to setthememorylimit there? And we would need an additional commit to push the cache limits to the rasterizer, will that be helpful ? Or its still worth doing ?
hendrikw@chromium.org changed reviewers: + hendrikw@chromium.org
One last thing. I guess this is being done to save memory on mobile. Would it have a performance hit on desktop? Like when we switch through tabs quickly? https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... File cc/resources/gpu_rasterizer.cc (right): https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:30: const int kMaxGaneshResourceCacheCount = 2048; On 2015/04/08 17:19:34, enne wrote: > Maybe you could make these LayerTreeSettings, and then LayerTreeHost could call > SetMemoryLimits(cache_count, cache_bytes), taking into account visible=0 or hard > memory limit=0 itself? I agree with enne (I think they meant LayerTreeHostImpl). We've removed the rasterizer abstraction, so this approach won't work anyway. I don't like that we're duplicating these values in a third location. third_party\skia\src\gpu\GrResourceCache.cpp(59) webkit\common\gpu\grcontext_for_webgraphicscontext3d.cc(65) https://codereview.chromium.org/1016733002/diff/80001/cc/resources/gpu_raster... cc/resources/gpu_rasterizer.cc:219: void GpuRasterizer::SetMemoryLimits(bool visible, size_t memory_limit_bytes) { Wouldn't it make sense to call freeGpuResources() instead of setting the limit to 0 and back to max? Then we wouldn't need to know what the defaults are, and I think it would have the same effect (the tab is out of view, it shouldn't create anymore resources, right?) freeGpuResources will also clear more resource than setResourceCacheLimits, but maybe this is a bad thing. bsalomon@, freeGpuResources when we hide the tab, good? bad? really bad?
> Ahh! this makes things interesting :) > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer to LTHI to > set the memory limits ? Hmm, I discussed a bit offline with Hendrik. The GrContexts are owned by OutputSurface which LTHI has direct access to it. Would OutputSurface be a good place for SetMemoryLimits; or perhaps ResourceProvider?
On 2015/04/09 23:10:42, vmiura wrote: > > Ahh! this makes things interesting :) > > > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer to LTHI > to > > set the memory limits ? > > Hmm, I discussed a bit offline with Hendrik. > > The GrContexts are owned by OutputSurface which LTHI has direct access to it. > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > ResourceProvider? ok. So should we maintain the cache limits in resourceprovider too ? Doesnt it make more sense to keep it in gpu rasterizer (if we dont plan to remove it in future that is). Also, reg hendrik@'s comment about maintaining the limits in multiple places, what should be the plan ? And about freegpuresourec vs clear cache, is cache clearing good enough for now ? thanks!:)
On 2015/04/13 17:01:08, sohanjg wrote: > On 2015/04/09 23:10:42, vmiura wrote: > > > Ahh! this makes things interesting :) > > > > > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer to LTHI > > to > > > set the memory limits ? > > > > Hmm, I discussed a bit offline with Hendrik. > > > > The GrContexts are owned by OutputSurface which LTHI has direct access to it. > > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > > ResourceProvider? > > ok. So should we maintain the cache limits in resourceprovider too ? > Doesnt it make more sense to keep it in gpu rasterizer (if we dont plan to > remove it in future that is). > Also, reg hendrik@'s comment about maintaining the limits in multiple places, > what should be the plan ? > And about freegpuresourec vs clear cache, is cache clearing good enough for now > ? > thanks!:) Hi, bsalomon@ said that the free should be sufficient assuming the hidden tabs aren't rendering, and I checked with brianderson@ (and enne@) who said that the scheduler won't commit on tabs that aren't visible. It sounds like freeGpuResources() would work here.
On 2015/04/13 17:19:42, hendrikw wrote: > On 2015/04/13 17:01:08, sohanjg wrote: > > On 2015/04/09 23:10:42, vmiura wrote: > > > > Ahh! this makes things interesting :) > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer to > LTHI > > > to > > > > set the memory limits ? > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct access to > it. > > > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > > > ResourceProvider? > > > > ok. So should we maintain the cache limits in resourceprovider too ? > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont plan to > > remove it in future that is). > > Also, reg hendrik@'s comment about maintaining the limits in multiple places, > > what should be the plan ? > > And about freegpuresourec vs clear cache, is cache clearing good enough for > now > > ? > > thanks!:) > > Hi, bsalomon@ said that the free should be sufficient assuming the hidden tabs > aren't rendering, and I checked with brianderson@ (and enne@) who said that the > scheduler won't commit on tabs that aren't visible. It sounds like > freeGpuResources() would work here. OK, thanks. How about when tilemanager memory hard limit is 0, we can still be visible then isnt it? would freeing gpu resources in that case be all right ?
On 2015/04/14 04:49:45, sohanjg wrote: > On 2015/04/13 17:19:42, hendrikw wrote: > > On 2015/04/13 17:01:08, sohanjg wrote: > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > Ahh! this makes things interesting :) > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer to > > LTHI > > > > to > > > > > set the memory limits ? > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct access to > > it. > > > > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > > > > ResourceProvider? > > > > > > ok. So should we maintain the cache limits in resourceprovider too ? > > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont plan to > > > remove it in future that is). > > > Also, reg hendrik@'s comment about maintaining the limits in multiple > places, > > > what should be the plan ? > > > And about freegpuresourec vs clear cache, is cache clearing good enough for > > now > > > ? > > > thanks!:) > > > > Hi, bsalomon@ said that the free should be sufficient assuming the hidden tabs > > aren't rendering, and I checked with brianderson@ (and enne@) who said that > the > > scheduler won't commit on tabs that aren't visible. It sounds like > > freeGpuResources() would work here. > > OK, thanks. > How about when tilemanager memory hard limit is 0, we can still be visible then > isnt it? would freeing gpu resources in that case be all right ? I don't know about that case in particular, but the way to think about it is that freeGpuResources() is a one time purge that doesn't change the budget. So if you keep using the GrContext the amount of allocated GPU memory will creep back up.
On 2015/04/14 17:33:57, bsalomon wrote: > On 2015/04/14 04:49:45, sohanjg wrote: > > On 2015/04/13 17:19:42, hendrikw wrote: > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer > to > > > LTHI > > > > > to > > > > > > set the memory limits ? > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct access > to > > > it. > > > > > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > > > > > ResourceProvider? > > > > > > > > ok. So should we maintain the cache limits in resourceprovider too ? > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont plan to > > > > remove it in future that is). > > > > Also, reg hendrik@'s comment about maintaining the limits in multiple > > places, > > > > what should be the plan ? > > > > And about freegpuresourec vs clear cache, is cache clearing good enough > for > > > now > > > > ? > > > > thanks!:) > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming the hidden > tabs > > > aren't rendering, and I checked with brianderson@ (and enne@) who said that > > the > > > scheduler won't commit on tabs that aren't visible. It sounds like > > > freeGpuResources() would work here. > > > > OK, thanks. > > How about when tilemanager memory hard limit is 0, we can still be visible > then > > isnt it? would freeing gpu resources in that case be all right ? > > I don't know about that case in particular, but the way to think about it is > that freeGpuResources() is a one time purge that doesn't change the budget. So > if you keep using the GrContext the amount of allocated GPU memory will creep > back up. Thanks for the details. I have updated the patch, please take a look. thanks :)
Patchset #6 (id:100001) has been deleted
On 2015/04/15 11:04:23, sohanjg wrote: > On 2015/04/14 17:33:57, bsalomon wrote: > > On 2015/04/14 04:49:45, sohanjg wrote: > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a Rasterizer > > to > > > > LTHI > > > > > > to > > > > > > > set the memory limits ? > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct access > > to > > > > it. > > > > > > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > > > > > > ResourceProvider? > > > > > > > > > > ok. So should we maintain the cache limits in resourceprovider too ? > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont plan > to > > > > > remove it in future that is). > > > > > Also, reg hendrik@'s comment about maintaining the limits in multiple > > > places, > > > > > what should be the plan ? > > > > > And about freegpuresourec vs clear cache, is cache clearing good enough > > for > > > > now > > > > > ? > > > > > thanks!:) > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming the hidden > > tabs > > > > aren't rendering, and I checked with brianderson@ (and enne@) who said > that > > > the > > > > scheduler won't commit on tabs that aren't visible. It sounds like > > > > freeGpuResources() would work here. > > > > > > OK, thanks. > > > How about when tilemanager memory hard limit is 0, we can still be visible > > then > > > isnt it? would freeing gpu resources in that case be all right ? > > > > I don't know about that case in particular, but the way to think about it is > > that freeGpuResources() is a one time purge that doesn't change the budget. So > > if you keep using the GrContext the amount of allocated GPU memory will creep > > back up. > > Thanks for the details. > I have updated the patch, please take a look. thanks :) Ping! :)
On 2015/04/20 16:38:14, sohanjg wrote: > On 2015/04/15 11:04:23, sohanjg wrote: > > On 2015/04/14 17:33:57, bsalomon wrote: > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a > Rasterizer > > > to > > > > > LTHI > > > > > > > to > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct > access > > > to > > > > > it. > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; or perhaps > > > > > > > ResourceProvider? > > > > > > > > > > > > ok. So should we maintain the cache limits in resourceprovider too ? > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont > plan > > to > > > > > > remove it in future that is). > > > > > > Also, reg hendrik@'s comment about maintaining the limits in multiple > > > > places, > > > > > > what should be the plan ? > > > > > > And about freegpuresourec vs clear cache, is cache clearing good > enough > > > for > > > > > now > > > > > > ? > > > > > > thanks!:) > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming the > hidden > > > tabs > > > > > aren't rendering, and I checked with brianderson@ (and enne@) who said > > that > > > > the > > > > > scheduler won't commit on tabs that aren't visible. It sounds like > > > > > freeGpuResources() would work here. > > > > > > > > OK, thanks. > > > > How about when tilemanager memory hard limit is 0, we can still be visible > > > then > > > > isnt it? would freeing gpu resources in that case be all right ? > > > > > > I don't know about that case in particular, but the way to think about it is > > > that freeGpuResources() is a one time purge that doesn't change the budget. > So > > > if you keep using the GrContext the amount of allocated GPU memory will > creep > > > back up. > > > > Thanks for the details. > > I have updated the patch, please take a look. thanks :) > > Ping! :) To me, this looks better. It seems like it should be tested as well. I think you can use getResourceCacheUsage() to determine if the we successfully clear the memory after making the tab not visible (I assume there's a test for that somewhere) I'd still like to know if and how this impacts desktop when switching tabs.
On 2015/04/20 17:58:19, hendrikw wrote: > On 2015/04/20 16:38:14, sohanjg wrote: > > On 2015/04/15 11:04:23, sohanjg wrote: > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a > > Rasterizer > > > > to > > > > > > LTHI > > > > > > > > to > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct > > access > > > > to > > > > > > it. > > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; or > perhaps > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > ok. So should we maintain the cache limits in resourceprovider too ? > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont > > plan > > > to > > > > > > > remove it in future that is). > > > > > > > Also, reg hendrik@'s comment about maintaining the limits in > multiple > > > > > places, > > > > > > > what should be the plan ? > > > > > > > And about freegpuresourec vs clear cache, is cache clearing good > > enough > > > > for > > > > > > now > > > > > > > ? > > > > > > > thanks!:) > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming the > > hidden > > > > tabs > > > > > > aren't rendering, and I checked with brianderson@ (and enne@) who said > > > that > > > > > the > > > > > > scheduler won't commit on tabs that aren't visible. It sounds like > > > > > > freeGpuResources() would work here. > > > > > > > > > > OK, thanks. > > > > > How about when tilemanager memory hard limit is 0, we can still be > visible > > > > then > > > > > isnt it? would freeing gpu resources in that case be all right ? > > > > > > > > I don't know about that case in particular, but the way to think about it > is > > > > that freeGpuResources() is a one time purge that doesn't change the > budget. > > So > > > > if you keep using the GrContext the amount of allocated GPU memory will > > creep > > > > back up. > > > > > > Thanks for the details. > > > I have updated the patch, please take a look. thanks :) > > > > Ping! :) > > To me, this looks better. It seems like it should be tested as well. I think > you can use getResourceCacheUsage() to determine if the we successfully clear > the memory after making the tab not visible (I assume there's a test for that > somewhere) > > I'd still like to know if and how this impacts desktop when switching tabs. Is there a way to enable gpu-raster on desktop linux builds ? Regarding using getResourceCacheUsage in tests, it would need a grcontext, right ? Wont we use null grcontexts in Tests ? thanks.
On 2015/04/23 15:05:23, sohanjg wrote: > On 2015/04/20 17:58:19, hendrikw wrote: > > On 2015/04/20 16:38:14, sohanjg wrote: > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a > > > Rasterizer > > > > > to > > > > > > > LTHI > > > > > > > > > to > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has direct > > > access > > > > > to > > > > > > > it. > > > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; or > > perhaps > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in resourceprovider too > ? > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we dont > > > plan > > > > to > > > > > > > > remove it in future that is). > > > > > > > > Also, reg hendrik@'s comment about maintaining the limits in > > multiple > > > > > > places, > > > > > > > > what should be the plan ? > > > > > > > > And about freegpuresourec vs clear cache, is cache clearing good > > > enough > > > > > for > > > > > > > now > > > > > > > > ? > > > > > > > > thanks!:) > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming the > > > hidden > > > > > tabs > > > > > > > aren't rendering, and I checked with brianderson@ (and enne@) who > said > > > > that > > > > > > the > > > > > > > scheduler won't commit on tabs that aren't visible. It sounds like > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > OK, thanks. > > > > > > How about when tilemanager memory hard limit is 0, we can still be > > visible > > > > > then > > > > > > isnt it? would freeing gpu resources in that case be all right ? > > > > > > > > > > I don't know about that case in particular, but the way to think about > it > > is > > > > > that freeGpuResources() is a one time purge that doesn't change the > > budget. > > > So > > > > > if you keep using the GrContext the amount of allocated GPU memory will > > > creep > > > > > back up. > > > > > > > > Thanks for the details. > > > > I have updated the patch, please take a look. thanks :) > > > > > > Ping! :) > > > > To me, this looks better. It seems like it should be tested as well. I > think > > you can use getResourceCacheUsage() to determine if the we successfully clear > > the memory after making the tab not visible (I assume there's a test for that > > somewhere) > > > > I'd still like to know if and how this impacts desktop when switching tabs. > > Is there a way to enable gpu-raster on desktop linux builds ? --force-gpu-rasterization > Regarding using getResourceCacheUsage in tests, it would need a grcontext, right > ? > Wont we use null grcontexts in Tests ? I think I fixed the last null grcontext a while ago, are you running into one? > thanks.
On 2015/04/23 15:43:59, hendrikw wrote: > On 2015/04/23 15:05:23, sohanjg wrote: > > On 2015/04/20 17:58:19, hendrikw wrote: > > > On 2015/04/20 16:38:14, sohanjg wrote: > > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a > > > > Rasterizer > > > > > > to > > > > > > > > LTHI > > > > > > > > > > to > > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has > direct > > > > access > > > > > > to > > > > > > > > it. > > > > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; or > > > perhaps > > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in resourceprovider > too > > ? > > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we > dont > > > > plan > > > > > to > > > > > > > > > remove it in future that is). > > > > > > > > > Also, reg hendrik@'s comment about maintaining the limits in > > > multiple > > > > > > > places, > > > > > > > > > what should be the plan ? > > > > > > > > > And about freegpuresourec vs clear cache, is cache clearing good > > > > enough > > > > > > for > > > > > > > > now > > > > > > > > > ? > > > > > > > > > thanks!:) > > > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming the > > > > hidden > > > > > > tabs > > > > > > > > aren't rendering, and I checked with brianderson@ (and enne@) who > > said > > > > > that > > > > > > > the > > > > > > > > scheduler won't commit on tabs that aren't visible. It sounds > like > > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > > > OK, thanks. > > > > > > > How about when tilemanager memory hard limit is 0, we can still be > > > visible > > > > > > then > > > > > > > isnt it? would freeing gpu resources in that case be all right ? > > > > > > > > > > > > I don't know about that case in particular, but the way to think about > > it > > > is > > > > > > that freeGpuResources() is a one time purge that doesn't change the > > > budget. > > > > So > > > > > > if you keep using the GrContext the amount of allocated GPU memory > will > > > > creep > > > > > > back up. > > > > > > > > > > Thanks for the details. > > > > > I have updated the patch, please take a look. thanks :) > > > > > > > > Ping! :) > > > > > > To me, this looks better. It seems like it should be tested as well. I > > think > > > you can use getResourceCacheUsage() to determine if the we successfully > clear > > > the memory after making the tab not visible (I assume there's a test for > that > > > somewhere) > > > > > > I'd still like to know if and how this impacts desktop when switching tabs. > > > > Is there a way to enable gpu-raster on desktop linux builds ? > > --force-gpu-rasterization > > > Regarding using getResourceCacheUsage in tests, it would need a grcontext, > right > > ? > > Wont we use null grcontexts in Tests ? > > I think I fixed the last null grcontext a while ago, are you running into one? > > > thanks. We currently use a real GrContext with a SkNullGLContext. https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_conte...
On 2015/04/23 17:01:37, vmiura wrote: > On 2015/04/23 15:43:59, hendrikw wrote: > > On 2015/04/23 15:05:23, sohanjg wrote: > > > On 2015/04/20 17:58:19, hendrikw wrote: > > > > On 2015/04/20 16:38:14, sohanjg wrote: > > > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose a > > > > > Rasterizer > > > > > > > to > > > > > > > > > LTHI > > > > > > > > > > > to > > > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has > > direct > > > > > access > > > > > > > to > > > > > > > > > it. > > > > > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; or > > > > perhaps > > > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in resourceprovider > > too > > > ? > > > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if we > > dont > > > > > plan > > > > > > to > > > > > > > > > > remove it in future that is). > > > > > > > > > > Also, reg hendrik@'s comment about maintaining the limits in > > > > multiple > > > > > > > > places, > > > > > > > > > > what should be the plan ? > > > > > > > > > > And about freegpuresourec vs clear cache, is cache clearing > good > > > > > enough > > > > > > > for > > > > > > > > > now > > > > > > > > > > ? > > > > > > > > > > thanks!:) > > > > > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming > the > > > > > hidden > > > > > > > tabs > > > > > > > > > aren't rendering, and I checked with brianderson@ (and enne@) > who > > > said > > > > > > that > > > > > > > > the > > > > > > > > > scheduler won't commit on tabs that aren't visible. It sounds > > like > > > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > > > > > OK, thanks. > > > > > > > > How about when tilemanager memory hard limit is 0, we can still be > > > > visible > > > > > > > then > > > > > > > > isnt it? would freeing gpu resources in that case be all right ? > > > > > > > > > > > > > > I don't know about that case in particular, but the way to think > about > > > it > > > > is > > > > > > > that freeGpuResources() is a one time purge that doesn't change the > > > > budget. > > > > > So > > > > > > > if you keep using the GrContext the amount of allocated GPU memory > > will > > > > > creep > > > > > > > back up. > > > > > > > > > > > > Thanks for the details. > > > > > > I have updated the patch, please take a look. thanks :) > > > > > > > > > > Ping! :) > > > > > > > > To me, this looks better. It seems like it should be tested as well. I > > > think > > > > you can use getResourceCacheUsage() to determine if the we successfully > > clear > > > > the memory after making the tab not visible (I assume there's a test for > > that > > > > somewhere) > > > > > > > > I'd still like to know if and how this impacts desktop when switching > tabs. > > > > > > Is there a way to enable gpu-raster on desktop linux builds ? > > > > --force-gpu-rasterization > > > > > Regarding using getResourceCacheUsage in tests, it would need a grcontext, > > right > > > ? > > > Wont we use null grcontexts in Tests ? > > > > I think I fixed the last null grcontext a while ago, are you running into one? > > > > > thanks. > > We currently use a real GrContext with a SkNullGLContext. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_conte... Yeah, the testcontextprovider needed to be bound to the thread, i think i missed that. hence the crashes. I tried the desktop tab switch and unit test behavior in LayerTreeHostImplTest.MemoryPolicy with getResourceCacheUsage. Desktop Tab Switch - Has lot of artefacts when we switch between tabs with forced gpu raster (there are text corruption in gpu-raster w/o the patch too, but with patch it is worse). getResourceCacheUsage - This doesnt clear the cache, we still maintain same resource count and bytes, even though we invoke purgeAllUnlocked :( Should we get back to the cache limit maintenance path ? :/
On 2015/04/24 10:19:20, sohanjg wrote: > On 2015/04/23 17:01:37, vmiura wrote: > > On 2015/04/23 15:43:59, hendrikw wrote: > > > On 2015/04/23 15:05:23, sohanjg wrote: > > > > On 2015/04/20 17:58:19, hendrikw wrote: > > > > > On 2015/04/20 16:38:14, sohanjg wrote: > > > > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool expose > a > > > > > > Rasterizer > > > > > > > > to > > > > > > > > > > LTHI > > > > > > > > > > > > to > > > > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has > > > direct > > > > > > access > > > > > > > > to > > > > > > > > > > it. > > > > > > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; > or > > > > > perhaps > > > > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in > resourceprovider > > > too > > > > ? > > > > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if > we > > > dont > > > > > > plan > > > > > > > to > > > > > > > > > > > remove it in future that is). > > > > > > > > > > > Also, reg hendrik@'s comment about maintaining the limits in > > > > > multiple > > > > > > > > > places, > > > > > > > > > > > what should be the plan ? > > > > > > > > > > > And about freegpuresourec vs clear cache, is cache clearing > > good > > > > > > enough > > > > > > > > for > > > > > > > > > > now > > > > > > > > > > > ? > > > > > > > > > > > thanks!:) > > > > > > > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient assuming > > the > > > > > > hidden > > > > > > > > tabs > > > > > > > > > > aren't rendering, and I checked with brianderson@ (and enne@) > > who > > > > said > > > > > > > that > > > > > > > > > the > > > > > > > > > > scheduler won't commit on tabs that aren't visible. It sounds > > > like > > > > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > > > > > > > OK, thanks. > > > > > > > > > How about when tilemanager memory hard limit is 0, we can still > be > > > > > visible > > > > > > > > then > > > > > > > > > isnt it? would freeing gpu resources in that case be all right ? > > > > > > > > > > > > > > > > I don't know about that case in particular, but the way to think > > about > > > > it > > > > > is > > > > > > > > that freeGpuResources() is a one time purge that doesn't change > the > > > > > budget. > > > > > > So > > > > > > > > if you keep using the GrContext the amount of allocated GPU memory > > > will > > > > > > creep > > > > > > > > back up. > > > > > > > > > > > > > > Thanks for the details. > > > > > > > I have updated the patch, please take a look. thanks :) > > > > > > > > > > > > Ping! :) > > > > > > > > > > To me, this looks better. It seems like it should be tested as well. > I > > > > think > > > > > you can use getResourceCacheUsage() to determine if the we successfully > > > clear > > > > > the memory after making the tab not visible (I assume there's a test for > > > that > > > > > somewhere) > > > > > > > > > > I'd still like to know if and how this impacts desktop when switching > > tabs. > > > > > > > > Is there a way to enable gpu-raster on desktop linux builds ? > > > > > > --force-gpu-rasterization > > > > > > > Regarding using getResourceCacheUsage in tests, it would need a grcontext, > > > right > > > > ? > > > > Wont we use null grcontexts in Tests ? > > > > > > I think I fixed the last null grcontext a while ago, are you running into > one? > > > > > > > thanks. > > > > We currently use a real GrContext with a SkNullGLContext. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_conte... > > Yeah, the testcontextprovider needed to be bound to the thread, i think i missed > that. hence the crashes. > > I tried the desktop tab switch and unit test behavior in > LayerTreeHostImplTest.MemoryPolicy with getResourceCacheUsage. > Desktop Tab Switch - Has lot of artefacts when we switch between tabs with > forced gpu raster (there are text corruption in gpu-raster w/o the patch too, > but with patch it is worse). > getResourceCacheUsage - This doesnt clear the cache, we still maintain same > resource count and bytes, even though we invoke purgeAllUnlocked :( > > Should we get back to the cache limit maintenance path ? :/ In what situation are we seeing artifacts prior to your patch? Can you provide repro steps? Is it logged?
On 2015/04/24 16:22:21, hendrikw wrote: > On 2015/04/24 10:19:20, sohanjg wrote: > > On 2015/04/23 17:01:37, vmiura wrote: > > > On 2015/04/23 15:43:59, hendrikw wrote: > > > > On 2015/04/23 15:05:23, sohanjg wrote: > > > > > On 2015/04/20 17:58:19, hendrikw wrote: > > > > > > On 2015/04/20 16:38:14, sohanjg wrote: > > > > > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool > expose > > a > > > > > > > Rasterizer > > > > > > > > > to > > > > > > > > > > > LTHI > > > > > > > > > > > > > to > > > > > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI has > > > > direct > > > > > > > access > > > > > > > > > to > > > > > > > > > > > it. > > > > > > > > > > > > > Would OutputSurface be a good place for SetMemoryLimits; > > or > > > > > > perhaps > > > > > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in > > resourceprovider > > > > too > > > > > ? > > > > > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer (if > > we > > > > dont > > > > > > > plan > > > > > > > > to > > > > > > > > > > > > remove it in future that is). > > > > > > > > > > > > Also, reg hendrik@'s comment about maintaining the limits > in > > > > > > multiple > > > > > > > > > > places, > > > > > > > > > > > > what should be the plan ? > > > > > > > > > > > > And about freegpuresourec vs clear cache, is cache > clearing > > > good > > > > > > > enough > > > > > > > > > for > > > > > > > > > > > now > > > > > > > > > > > > ? > > > > > > > > > > > > thanks!:) > > > > > > > > > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient > assuming > > > the > > > > > > > hidden > > > > > > > > > tabs > > > > > > > > > > > aren't rendering, and I checked with brianderson@ (and > enne@) > > > who > > > > > said > > > > > > > > that > > > > > > > > > > the > > > > > > > > > > > scheduler won't commit on tabs that aren't visible. It > sounds > > > > like > > > > > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > > > > > > > > > OK, thanks. > > > > > > > > > > How about when tilemanager memory hard limit is 0, we can > still > > be > > > > > > visible > > > > > > > > > then > > > > > > > > > > isnt it? would freeing gpu resources in that case be all right > ? > > > > > > > > > > > > > > > > > > I don't know about that case in particular, but the way to think > > > about > > > > > it > > > > > > is > > > > > > > > > that freeGpuResources() is a one time purge that doesn't change > > the > > > > > > budget. > > > > > > > So > > > > > > > > > if you keep using the GrContext the amount of allocated GPU > memory > > > > will > > > > > > > creep > > > > > > > > > back up. > > > > > > > > > > > > > > > > Thanks for the details. > > > > > > > > I have updated the patch, please take a look. thanks :) > > > > > > > > > > > > > > Ping! :) > > > > > > > > > > > > To me, this looks better. It seems like it should be tested as well. > > > I > > > > > think > > > > > > you can use getResourceCacheUsage() to determine if the we > successfully > > > > clear > > > > > > the memory after making the tab not visible (I assume there's a test > for > > > > that > > > > > > somewhere) > > > > > > > > > > > > I'd still like to know if and how this impacts desktop when switching > > > tabs. > > > > > > > > > > Is there a way to enable gpu-raster on desktop linux builds ? > > > > > > > > --force-gpu-rasterization > > > > > > > > > Regarding using getResourceCacheUsage in tests, it would need a > grcontext, > > > > right > > > > > ? > > > > > Wont we use null grcontexts in Tests ? > > > > > > > > I think I fixed the last null grcontext a while ago, are you running into > > one? > > > > > > > > > thanks. > > > > > > We currently use a real GrContext with a SkNullGLContext. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_conte... > > > > Yeah, the testcontextprovider needed to be bound to the thread, i think i > missed > > that. hence the crashes. > > > > I tried the desktop tab switch and unit test behavior in > > LayerTreeHostImplTest.MemoryPolicy with getResourceCacheUsage. > > Desktop Tab Switch - Has lot of artefacts when we switch between tabs with > > forced gpu raster (there are text corruption in gpu-raster w/o the patch too, > > but with patch it is worse). > > getResourceCacheUsage - This doesnt clear the cache, we still maintain same > > resource count and bytes, even though we invoke purgeAllUnlocked :( > > > > Should we get back to the cache limit maintenance path ? :/ > > In what situation are we seeing artifacts prior to your patch? Can you provide > repro steps? Is it logged? Logged https://code.google.com/p/chromium/issues/detail?id=481480 with the details. thanks.
On 2015/04/27 09:07:37, sohanjg wrote: > On 2015/04/24 16:22:21, hendrikw wrote: > > On 2015/04/24 10:19:20, sohanjg wrote: > > > On 2015/04/23 17:01:37, vmiura wrote: > > > > On 2015/04/23 15:43:59, hendrikw wrote: > > > > > On 2015/04/23 15:05:23, sohanjg wrote: > > > > > > On 2015/04/20 17:58:19, hendrikw wrote: > > > > > > > On 2015/04/20 16:38:14, sohanjg wrote: > > > > > > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > > > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool > > expose > > > a > > > > > > > > Rasterizer > > > > > > > > > > to > > > > > > > > > > > > LTHI > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI > has > > > > > direct > > > > > > > > access > > > > > > > > > > to > > > > > > > > > > > > it. > > > > > > > > > > > > > > Would OutputSurface be a good place for > SetMemoryLimits; > > > or > > > > > > > perhaps > > > > > > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in > > > resourceprovider > > > > > too > > > > > > ? > > > > > > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer > (if > > > we > > > > > dont > > > > > > > > plan > > > > > > > > > to > > > > > > > > > > > > > remove it in future that is). > > > > > > > > > > > > > Also, reg hendrik@'s comment about maintaining the > limits > > in > > > > > > > multiple > > > > > > > > > > > places, > > > > > > > > > > > > > what should be the plan ? > > > > > > > > > > > > > And about freegpuresourec vs clear cache, is cache > > clearing > > > > good > > > > > > > > enough > > > > > > > > > > for > > > > > > > > > > > > now > > > > > > > > > > > > > ? > > > > > > > > > > > > > thanks!:) > > > > > > > > > > > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient > > assuming > > > > the > > > > > > > > hidden > > > > > > > > > > tabs > > > > > > > > > > > > aren't rendering, and I checked with brianderson@ (and > > enne@) > > > > who > > > > > > said > > > > > > > > > that > > > > > > > > > > > the > > > > > > > > > > > > scheduler won't commit on tabs that aren't visible. It > > sounds > > > > > like > > > > > > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > > > > > > > > > > > OK, thanks. > > > > > > > > > > > How about when tilemanager memory hard limit is 0, we can > > still > > > be > > > > > > > visible > > > > > > > > > > then > > > > > > > > > > > isnt it? would freeing gpu resources in that case be all > right > > ? > > > > > > > > > > > > > > > > > > > > I don't know about that case in particular, but the way to > think > > > > about > > > > > > it > > > > > > > is > > > > > > > > > > that freeGpuResources() is a one time purge that doesn't > change > > > the > > > > > > > budget. > > > > > > > > So > > > > > > > > > > if you keep using the GrContext the amount of allocated GPU > > memory > > > > > will > > > > > > > > creep > > > > > > > > > > back up. > > > > > > > > > > > > > > > > > > Thanks for the details. > > > > > > > > > I have updated the patch, please take a look. thanks :) > > > > > > > > > > > > > > > > Ping! :) > > > > > > > > > > > > > > To me, this looks better. It seems like it should be tested as > well. > > > > > I > > > > > > think > > > > > > > you can use getResourceCacheUsage() to determine if the we > > successfully > > > > > clear > > > > > > > the memory after making the tab not visible (I assume there's a test > > for > > > > > that > > > > > > > somewhere) > > > > > > > > > > > > > > I'd still like to know if and how this impacts desktop when > switching > > > > tabs. > > > > > > > > > > > > Is there a way to enable gpu-raster on desktop linux builds ? > > > > > > > > > > --force-gpu-rasterization > > > > > > > > > > > Regarding using getResourceCacheUsage in tests, it would need a > > grcontext, > > > > > right > > > > > > ? > > > > > > Wont we use null grcontexts in Tests ? > > > > > > > > > > I think I fixed the last null grcontext a while ago, are you running > into > > > one? > > > > > > > > > > > thanks. > > > > > > > > We currently use a real GrContext with a SkNullGLContext. > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_conte... > > > > > > Yeah, the testcontextprovider needed to be bound to the thread, i think i > > missed > > > that. hence the crashes. > > > > > > I tried the desktop tab switch and unit test behavior in > > > LayerTreeHostImplTest.MemoryPolicy with getResourceCacheUsage. > > > Desktop Tab Switch - Has lot of artefacts when we switch between tabs with > > > forced gpu raster (there are text corruption in gpu-raster w/o the patch > too, > > > but with patch it is worse). > > > getResourceCacheUsage - This doesnt clear the cache, we still maintain same > > > resource count and bytes, even though we invoke purgeAllUnlocked :( > > > > > > Should we get back to the cache limit maintenance path ? :/ > > > > In what situation are we seeing artifacts prior to your patch? Can you > provide > > repro steps? Is it logged? > > Logged https://code.google.com/p/chromium/issues/detail?id=481480 with the > details. > thanks. It seems with some old Mesa driver (2.1 Mesa 10.1.0) in desktop, base issue is not reproducible. But, with the patch on tab switch, we see blank screen/artefacts etc. Should we go ahead with this patch ?
On 2015/04/28 05:17:10, sohanjg wrote: > On 2015/04/27 09:07:37, sohanjg wrote: > > On 2015/04/24 16:22:21, hendrikw wrote: > > > On 2015/04/24 10:19:20, sohanjg wrote: > > > > On 2015/04/23 17:01:37, vmiura wrote: > > > > > On 2015/04/23 15:43:59, hendrikw wrote: > > > > > > On 2015/04/23 15:05:23, sohanjg wrote: > > > > > > > On 2015/04/20 17:58:19, hendrikw wrote: > > > > > > > > On 2015/04/20 16:38:14, sohanjg wrote: > > > > > > > > > On 2015/04/15 11:04:23, sohanjg wrote: > > > > > > > > > > On 2015/04/14 17:33:57, bsalomon wrote: > > > > > > > > > > > On 2015/04/14 04:49:45, sohanjg wrote: > > > > > > > > > > > > On 2015/04/13 17:19:42, hendrikw wrote: > > > > > > > > > > > > > On 2015/04/13 17:01:08, sohanjg wrote: > > > > > > > > > > > > > > On 2015/04/09 23:10:42, vmiura wrote: > > > > > > > > > > > > > > > > Ahh! this makes things interesting :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > what should be the plan, have TileTaskWorkerPool > > > expose > > > > a > > > > > > > > > Rasterizer > > > > > > > > > > > to > > > > > > > > > > > > > LTHI > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > set the memory limits ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hmm, I discussed a bit offline with Hendrik. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The GrContexts are owned by OutputSurface which LTHI > > has > > > > > > direct > > > > > > > > > access > > > > > > > > > > > to > > > > > > > > > > > > > it. > > > > > > > > > > > > > > > Would OutputSurface be a good place for > > SetMemoryLimits; > > > > or > > > > > > > > perhaps > > > > > > > > > > > > > > > ResourceProvider? > > > > > > > > > > > > > > > > > > > > > > > > > > > > ok. So should we maintain the cache limits in > > > > resourceprovider > > > > > > too > > > > > > > ? > > > > > > > > > > > > > > Doesnt it make more sense to keep it in gpu rasterizer > > (if > > > > we > > > > > > dont > > > > > > > > > plan > > > > > > > > > > to > > > > > > > > > > > > > > remove it in future that is). > > > > > > > > > > > > > > Also, reg hendrik@'s comment about maintaining the > > limits > > > in > > > > > > > > multiple > > > > > > > > > > > > places, > > > > > > > > > > > > > > what should be the plan ? > > > > > > > > > > > > > > And about freegpuresourec vs clear cache, is cache > > > clearing > > > > > good > > > > > > > > > enough > > > > > > > > > > > for > > > > > > > > > > > > > now > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > thanks!:) > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, bsalomon@ said that the free should be sufficient > > > assuming > > > > > the > > > > > > > > > hidden > > > > > > > > > > > tabs > > > > > > > > > > > > > aren't rendering, and I checked with brianderson@ (and > > > enne@) > > > > > who > > > > > > > said > > > > > > > > > > that > > > > > > > > > > > > the > > > > > > > > > > > > > scheduler won't commit on tabs that aren't visible. It > > > sounds > > > > > > like > > > > > > > > > > > > > freeGpuResources() would work here. > > > > > > > > > > > > > > > > > > > > > > > > OK, thanks. > > > > > > > > > > > > How about when tilemanager memory hard limit is 0, we can > > > still > > > > be > > > > > > > > visible > > > > > > > > > > > then > > > > > > > > > > > > isnt it? would freeing gpu resources in that case be all > > right > > > ? > > > > > > > > > > > > > > > > > > > > > > I don't know about that case in particular, but the way to > > think > > > > > about > > > > > > > it > > > > > > > > is > > > > > > > > > > > that freeGpuResources() is a one time purge that doesn't > > change > > > > the > > > > > > > > budget. > > > > > > > > > So > > > > > > > > > > > if you keep using the GrContext the amount of allocated GPU > > > memory > > > > > > will > > > > > > > > > creep > > > > > > > > > > > back up. > > > > > > > > > > > > > > > > > > > > Thanks for the details. > > > > > > > > > > I have updated the patch, please take a look. thanks :) > > > > > > > > > > > > > > > > > > Ping! :) > > > > > > > > > > > > > > > > To me, this looks better. It seems like it should be tested as > > well. > > > > > > > I > > > > > > > think > > > > > > > > you can use getResourceCacheUsage() to determine if the we > > > successfully > > > > > > clear > > > > > > > > the memory after making the tab not visible (I assume there's a > test > > > for > > > > > > that > > > > > > > > somewhere) > > > > > > > > > > > > > > > > I'd still like to know if and how this impacts desktop when > > switching > > > > > tabs. > > > > > > > > > > > > > > Is there a way to enable gpu-raster on desktop linux builds ? > > > > > > > > > > > > --force-gpu-rasterization > > > > > > > > > > > > > Regarding using getResourceCacheUsage in tests, it would need a > > > grcontext, > > > > > > right > > > > > > > ? > > > > > > > Wont we use null grcontexts in Tests ? > > > > > > > > > > > > I think I fixed the last null grcontext a while ago, are you running > > into > > > > one? > > > > > > > > > > > > > thanks. > > > > > > > > > > We currently use a real GrContext with a SkNullGLContext. > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/test_conte... > > > > > > > > Yeah, the testcontextprovider needed to be bound to the thread, i think i > > > missed > > > > that. hence the crashes. > > > > > > > > I tried the desktop tab switch and unit test behavior in > > > > LayerTreeHostImplTest.MemoryPolicy with getResourceCacheUsage. > > > > Desktop Tab Switch - Has lot of artefacts when we switch between tabs with > > > > forced gpu raster (there are text corruption in gpu-raster w/o the patch > > too, > > > > but with patch it is worse). > > > > getResourceCacheUsage - This doesnt clear the cache, we still maintain > same > > > > resource count and bytes, even though we invoke purgeAllUnlocked :( > > > > > > > > Should we get back to the cache limit maintenance path ? :/ > > > > > > In what situation are we seeing artifacts prior to your patch? Can you > > provide > > > repro steps? Is it logged? > > > > Logged https://code.google.com/p/chromium/issues/detail?id=481480 with the > > details. > > thanks. > > It seems with some old Mesa driver (2.1 Mesa 10.1.0) in desktop, base issue is > not reproducible. > But, with the patch on tab switch, we see blank screen/artefacts etc. > > Should we go ahead with this patch ? Wouldnt this code take care of clearing cache when we become invisible already ? https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/delegati...
On 2015/04/28 06:39:43, sohanjg wrote: > Wouldnt this code take care of clearing cache when we become invisible already ? > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/delegati... No, that code will only release the resource provider's memory, Ganesh's cache will remain unchanged. To test this out for yourself, you can run a trace with skia.gpu.cache on, navigate to a page that uses a lot of Ganesh's cache (eg. wwww.ugamsolutions.com, scroll to bottom), then switch to another tab, and back. The trace will show that skia's gpu cache remain full. I suspect all of the artifacts you're running into are related to your drivers, we'd need to test this patch out on another machine to verify that it works correctly. With this cl, I'm still concerned with what the impact will be on desktop when quickly switching through tabs. We need to some way to make sure we're not regressing performance. vmiura@, I re-read the bug that's attached to this cl, I think that this patch may not address it. The bug states "When tile manager goes out of memory", which is not the same as "when the tab is not visible or the memory limit is set to zero", but maybe I'm misunderstanding it, do you know the story behind it?
On 2015/04/30 16:15:30, hendrikw wrote: > On 2015/04/28 06:39:43, sohanjg wrote: > > Wouldnt this code take care of clearing cache when we become invisible already > ? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/delegati... > > No, that code will only release the resource provider's memory, Ganesh's cache > will remain unchanged. > > To test this out for yourself, you can run a trace with skia.gpu.cache on, > navigate to a page that uses a lot of Ganesh's cache (eg. > http://wwww.ugamsolutions.com, scroll to bottom), then switch to another tab, and back. > The trace will show that skia's gpu cache remain full. > > I suspect all of the artifacts you're running into are related to your drivers, > we'd need to test this patch out on another machine to verify that it works > correctly. > > With this cl, I'm still concerned with what the impact will be on desktop when > quickly switching through tabs. We need to some way to make sure we're not > regressing performance. > > vmiura@, I re-read the bug that's attached to this cl, I think that this patch > may not address it. The bug states "When tile manager goes out of memory", > which is not the same as "when the tab is not visible or the memory limit is set > to zero", but maybe I'm misunderstanding it, do you know the story behind it? I had checked with various drivers, for drivers which doesnt have a problem with gpu raster also has a problem with this patch, when we switch tab. Text turns completely blank on switching tabs. Regarding clearing cache when visibility of LTHI changes, this was proposed by enne@ at 1 of the patchset.
On 2015/04/30 17:02:34, sohanjg wrote: > On 2015/04/30 16:15:30, hendrikw wrote: > > On 2015/04/28 06:39:43, sohanjg wrote: > > > Wouldnt this code take care of clearing cache when we become invisible > already > > ? > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/delegati... > > > > No, that code will only release the resource provider's memory, Ganesh's cache > > will remain unchanged. > > > > To test this out for yourself, you can run a trace with skia.gpu.cache on, > > navigate to a page that uses a lot of Ganesh's cache (eg. > > http://wwww.ugamsolutions.com, scroll to bottom), then switch to another tab, > and back. > > The trace will show that skia's gpu cache remain full. > > > > I suspect all of the artifacts you're running into are related to your > drivers, > > we'd need to test this patch out on another machine to verify that it works > > correctly. > > > > With this cl, I'm still concerned with what the impact will be on desktop when > > quickly switching through tabs. We need to some way to make sure we're not > > regressing performance. > > > > vmiura@, I re-read the bug that's attached to this cl, I think that this patch > > may not address it. The bug states "When tile manager goes out of memory", > > which is not the same as "when the tab is not visible or the memory limit is > set > > to zero", but maybe I'm misunderstanding it, do you know the story behind it? > > I had checked with various drivers, for drivers which doesnt have a problem with > gpu raster also has a problem with this patch, when we switch tab. Text turns > completely blank on switching tabs. > > Regarding clearing cache when visibility of LTHI changes, this was proposed by > enne@ at 1 of the patchset. And TM's hard memory limit as 0 is what we are interpreting as TM is oom.
> > > vmiura@, I re-read the bug that's attached to this cl, I think that this > patch > > > may not address it. The bug states "When tile manager goes out of memory", > > > which is not the same as "when the tab is not visible or the memory limit is > > set > > > to zero", but maybe I'm misunderstanding it, do you know the story behind > it? I see, yes I think that is confusing but I think you're doing what I had in mind.
On 2015/05/06 21:38:55, vmiura wrote: > > > > vmiura@, I re-read the bug that's attached to this cl, I think that this > > patch > > > > may not address it. The bug states "When tile manager goes out of > memory", > > > > which is not the same as "when the tab is not visible or the memory limit > is > > > set > > > > to zero", but maybe I'm misunderstanding it, do you know the story behind > > it? > > I see, yes I think that is confusing but I think you're doing what I had in > mind. Is there something to be done here ?
On 2015/05/07 02:08:05, sohanjg wrote: > On 2015/05/06 21:38:55, vmiura wrote: > > > > > vmiura@, I re-read the bug that's attached to this cl, I think that this > > > patch > > > > > may not address it. The bug states "When tile manager goes out of > > memory", > > > > > which is not the same as "when the tab is not visible or the memory > limit > > is > > > > set > > > > > to zero", but maybe I'm misunderstanding it, do you know the story > behind > > > it? > > > > I see, yes I think that is confusing but I think you're doing what I had in > > mind. > > Is there something to be done here ? i am not sure, but should we get the cache limit patch back ? Or this is not imp for now ?
On 2015/05/11 13:50:19, sohanjg wrote: > On 2015/05/07 02:08:05, sohanjg wrote: > > On 2015/05/06 21:38:55, vmiura wrote: > > > > > > vmiura@, I re-read the bug that's attached to this cl, I think that > this > > > > patch > > > > > > may not address it. The bug states "When tile manager goes out of > > > memory", > > > > > > which is not the same as "when the tab is not visible or the memory > > limit > > > is > > > > > set > > > > > > to zero", but maybe I'm misunderstanding it, do you know the story > > behind > > > > it? > > > > > > I see, yes I think that is confusing but I think you're doing what I had in > > > mind. > > > > Is there something to be done here ? > > i am not sure, but should we get the cache limit patch back ? Or this is not imp > for now ? Sounds like we do want to try to get this in. I think the key remaining tests are: - Verifying that the current code doesn't cause regressions (we should check both performance and correctness of tab switching, including on desktop). - Releasing memory from GLES2DeviceContext (see the earlier note re piman's comment) - Adding some test coverage for this. I'm happy to assist in any way - I'll begin by looking into the GLES2DeviceContext related work and potentially add that as a follow-up patch. I can also look into some of the unit test work if that would be helpful. Thanks
On 2015/05/22 17:21:37, ericrk wrote: > On 2015/05/11 13:50:19, sohanjg wrote: > > On 2015/05/07 02:08:05, sohanjg wrote: > > > On 2015/05/06 21:38:55, vmiura wrote: > > > > > > > vmiura@, I re-read the bug that's attached to this cl, I think that > > this > > > > > patch > > > > > > > may not address it. The bug states "When tile manager goes out of > > > > memory", > > > > > > > which is not the same as "when the tab is not visible or the memory > > > limit > > > > is > > > > > > set > > > > > > > to zero", but maybe I'm misunderstanding it, do you know the story > > > behind > > > > > it? > > > > > > > > I see, yes I think that is confusing but I think you're doing what I had > in > > > > mind. > > > > > > Is there something to be done here ? > > > > i am not sure, but should we get the cache limit patch back ? Or this is not > imp > > for now ? > > Sounds like we do want to try to get this in. I think the key remaining tests > are: > - Verifying that the current code doesn't cause regressions (we should check > both performance and correctness of tab switching, including on desktop). > - Releasing memory from GLES2DeviceContext (see the earlier note re piman's > comment) > - Adding some test coverage for this. > > I'm happy to assist in any way - I'll begin by looking into the > GLES2DeviceContext related work and potentially add that as a follow-up patch. I > can also look into some of the unit test work if that would be helpful. > > Thanks I've found that on Windows Desktop I get some amount of visual corruption when switching tabs with this patch applied - without the patch everything seems fine (in my limited testing).... Investigating this now.
On 2015/05/22 18:14:27, ericrk wrote: > On 2015/05/22 17:21:37, ericrk wrote: > > On 2015/05/11 13:50:19, sohanjg wrote: > > > On 2015/05/07 02:08:05, sohanjg wrote: > > > > On 2015/05/06 21:38:55, vmiura wrote: > > > > > > > > vmiura@, I re-read the bug that's attached to this cl, I think > that > > > this > > > > > > patch > > > > > > > > may not address it. The bug states "When tile manager goes out of > > > > > memory", > > > > > > > > which is not the same as "when the tab is not visible or the > memory > > > > limit > > > > > is > > > > > > > set > > > > > > > > to zero", but maybe I'm misunderstanding it, do you know the story > > > > behind > > > > > > it? > > > > > > > > > > I see, yes I think that is confusing but I think you're doing what I had > > in > > > > > mind. > > > > > > > > Is there something to be done here ? > > > > > > i am not sure, but should we get the cache limit patch back ? Or this is not > > imp > > > for now ? > > > > Sounds like we do want to try to get this in. I think the key remaining tests > > are: > > - Verifying that the current code doesn't cause regressions (we should check > > both performance and correctness of tab switching, including on desktop). > > - Releasing memory from GLES2DeviceContext (see the earlier note re piman's > > comment) > > - Adding some test coverage for this. > > > > I'm happy to assist in any way - I'll begin by looking into the > > GLES2DeviceContext related work and potentially add that as a follow-up patch. > I > > can also look into some of the unit test work if that would be helpful. > > > > Thanks > > I've found that on Windows Desktop I get some amount of visual corruption when > switching tabs with this patch applied - without the patch everything seems fine > (in my limited testing).... Investigating this now. Narrowed things down slightly - the issue I'm seeing is specifically related to fBatchFontCache->freeAll(); in GrContext::freeGpuResources. If I comment out that line, things seem to work normally.... From looking at the corruption, it looks like the font cache is being used after being freed (text is rendered with a bunch of garbage) - looking into why the font cache doesn't seem to be being regenerated correctly.
On 2015/05/22 18:30:40, ericrk wrote: > On 2015/05/22 18:14:27, ericrk wrote: > > On 2015/05/22 17:21:37, ericrk wrote: > > > On 2015/05/11 13:50:19, sohanjg wrote: > > > > On 2015/05/07 02:08:05, sohanjg wrote: > > > > > On 2015/05/06 21:38:55, vmiura wrote: > > > > > > > > > vmiura@, I re-read the bug that's attached to this cl, I think > > that > > > > this > > > > > > > patch > > > > > > > > > may not address it. The bug states "When tile manager goes out > of > > > > > > memory", > > > > > > > > > which is not the same as "when the tab is not visible or the > > memory > > > > > limit > > > > > > is > > > > > > > > set > > > > > > > > > to zero", but maybe I'm misunderstanding it, do you know the > story > > > > > behind > > > > > > > it? > > > > > > > > > > > > I see, yes I think that is confusing but I think you're doing what I > had > > > in > > > > > > mind. > > > > > > > > > > Is there something to be done here ? > > > > > > > > i am not sure, but should we get the cache limit patch back ? Or this is > not > > > imp > > > > for now ? > > > > > > Sounds like we do want to try to get this in. I think the key remaining > tests > > > are: > > > - Verifying that the current code doesn't cause regressions (we should check > > > both performance and correctness of tab switching, including on desktop). > > > - Releasing memory from GLES2DeviceContext (see the earlier note re piman's > > > comment) > > > - Adding some test coverage for this. > > > > > > I'm happy to assist in any way - I'll begin by looking into the > > > GLES2DeviceContext related work and potentially add that as a follow-up > patch. > > I > > > can also look into some of the unit test work if that would be helpful. > > > > > > Thanks > > > > I've found that on Windows Desktop I get some amount of visual corruption when > > switching tabs with this patch applied - without the patch everything seems > fine > > (in my limited testing).... Investigating this now. > > Narrowed things down slightly - the issue I'm seeing is specifically related to > fBatchFontCache->freeAll(); in GrContext::freeGpuResources. If I comment out > that line, things seem to work normally.... > > From looking at the corruption, it looks like the font cache is being used after > being freed (text is rendered with a bunch of garbage) - looking into why the > font cache doesn't seem to be being regenerated correctly. Adding Jim and Joshua.
this CL: https://codereview.chromium.org/1156693004/ Should hopefully fix the batchfontcache issue. Let me know if that is not the case.
On 2015/05/22 21:04:42, joshualitt wrote: > this CL: > https://codereview.chromium.org/1156693004/ > > Should hopefully fix the batchfontcache issue. Let me know if that is not the > case. Thanks! Patching that change in manually doesn't seem to fix things (does it rely on other recent changes? doing a full sync now)... it still seems like the deleted glyph references in |fTextBlobCache| would be a problem? I think we may also need something like this (fixes the issue I'm seeing): https://codereview.chromium.org/1155923004
On 2015/05/22 22:35:46, ericrk wrote: > On 2015/05/22 21:04:42, joshualitt wrote: > > this CL: > > https://codereview.chromium.org/1156693004/ > > > > Should hopefully fix the batchfontcache issue. Let me know if that is not the > > case. > > Thanks! > > Patching that change in manually doesn't seem to fix things (does it rely on > other recent changes? doing a full sync now)... it still seems like the deleted > glyph references in |fTextBlobCache| would be a problem? I think we may also > need something like this (fixes the issue I'm seeing): > > https://codereview.chromium.org/1155923004 ericrk@, thanks for looking into this! i was essentially waiting for the reviewers for a direction.Because, clearing all of gpu resource on visibility change was having artifacts (on desktops), as you had seen too. Also, since the orignal bug was about clearing up the gr cache and not all of gpu resources, so i was thinking we would only do that (infact the older patchsets were about adjusting gr cache limits only :)). But anyways, good that this got moving!
On 2015/05/23 13:16:51, sohanjg wrote: > On 2015/05/22 22:35:46, ericrk wrote: > > On 2015/05/22 21:04:42, joshualitt wrote: > > > this CL: > > > https://codereview.chromium.org/1156693004/ > > > > > > Should hopefully fix the batchfontcache issue. Let me know if that is not > the > > > case. > > > > Thanks! > > > > Patching that change in manually doesn't seem to fix things (does it rely on > > other recent changes? doing a full sync now)... it still seems like the > deleted > > glyph references in |fTextBlobCache| would be a problem? I think we may also > > need something like this (fixes the issue I'm seeing): > > > > https://codereview.chromium.org/1155923004 > > ericrk@, thanks for looking into this! > > i was essentially waiting for the reviewers for a direction.Because, clearing > all of gpu resource on visibility change was having artifacts (on desktops), as > you had seen too. > > Also, since the orignal bug was about clearing up the gr cache and not all of > gpu resources, so i was thinking we would only do that (infact the older > patchsets were about adjusting gr cache limits only :)). > > But anyways, good that this got moving! @ericrk, please try: https://codereview.chromium.org/1160633002/
On 2015/05/26 14:40:59, joshualitt wrote: > On 2015/05/23 13:16:51, sohanjg wrote: > > On 2015/05/22 22:35:46, ericrk wrote: > > > On 2015/05/22 21:04:42, joshualitt wrote: > > > > this CL: > > > > https://codereview.chromium.org/1156693004/ > > > > > > > > Should hopefully fix the batchfontcache issue. Let me know if that is not > > the > > > > case. > > > > > > Thanks! > > > > > > Patching that change in manually doesn't seem to fix things (does it rely on > > > other recent changes? doing a full sync now)... it still seems like the > > deleted > > > glyph references in |fTextBlobCache| would be a problem? I think we may also > > > need something like this (fixes the issue I'm seeing): > > > > > > https://codereview.chromium.org/1155923004 > > > > ericrk@, thanks for looking into this! > > > > i was essentially waiting for the reviewers for a direction.Because, clearing > > all of gpu resource on visibility change was having artifacts (on desktops), > as > > you had seen too. > > > > Also, since the orignal bug was about clearing up the gr cache and not all of > > gpu resources, so i was thinking we would only do that (infact the older > > patchsets were about adjusting gr cache limits only :)). > > > > But anyways, good that this got moving! > > @ericrk, please try: > https://codereview.chromium.org/1160633002/ @joshualitt - thanks for your help here! This seems to have fixed the corruption I was seeing - will continue testing. @sohanjg - sounds good - the patch seems to be working for me at this point. Adding code to free GLES2DeviceContext's GPU resources doesn't seem to add new issues. I'm going to start working on some unit tests.
On 2015/05/26 18:11:30, ericrk wrote: > On 2015/05/26 14:40:59, joshualitt wrote: > > On 2015/05/23 13:16:51, sohanjg wrote: > > > On 2015/05/22 22:35:46, ericrk wrote: > > > > On 2015/05/22 21:04:42, joshualitt wrote: > > > > > this CL: > > > > > https://codereview.chromium.org/1156693004/ > > > > > > > > > > Should hopefully fix the batchfontcache issue. Let me know if that is > not > > > the > > > > > case. > > > > > > > > Thanks! > > > > > > > > Patching that change in manually doesn't seem to fix things (does it rely > on > > > > other recent changes? doing a full sync now)... it still seems like the > > > deleted > > > > glyph references in |fTextBlobCache| would be a problem? I think we may > also > > > > need something like this (fixes the issue I'm seeing): > > > > > > > > https://codereview.chromium.org/1155923004 > > > > > > ericrk@, thanks for looking into this! > > > > > > i was essentially waiting for the reviewers for a direction.Because, > clearing > > > all of gpu resource on visibility change was having artifacts (on desktops), > > as > > > you had seen too. > > > > > > Also, since the orignal bug was about clearing up the gr cache and not all > of > > > gpu resources, so i was thinking we would only do that (infact the older > > > patchsets were about adjusting gr cache limits only :)). > > > > > > But anyways, good that this got moving! > > > > @ericrk, please try: > > https://codereview.chromium.org/1160633002/ > > @joshualitt - thanks for your help here! This seems to have fixed the corruption > I was seeing - will continue testing. > > @sohanjg - sounds good - the patch seems to be working for me at this point. > Adding code to free GLES2DeviceContext's GPU resources doesn't seem to add new > issues. I'm going to start working on some unit tests. urfortunately, even with https://codereview.chromium.org/1160633002/, i am still seeing text corruption on my linux desktop on tab switch, when i force gpu-raster for all layers. my config, Optimus true GL_VENDOR Tungsten Graphics, Inc GL_RENDERER Mesa DRI Intel(R) Ivybridge Mobile GL_VERSION 3.0 Mesa 8.0.4 Let me know, if you need any further info. thanks. ericrk@, is there something else i should do for this patch, while you take care of the tests ?
On 2015/05/27 12:33:24, sohanjg wrote: > On 2015/05/26 18:11:30, ericrk wrote: > > On 2015/05/26 14:40:59, joshualitt wrote: > > > On 2015/05/23 13:16:51, sohanjg wrote: > > > > On 2015/05/22 22:35:46, ericrk wrote: > > > > > On 2015/05/22 21:04:42, joshualitt wrote: > > > > > > this CL: > > > > > > https://codereview.chromium.org/1156693004/ > > > > > > > > > > > > Should hopefully fix the batchfontcache issue. Let me know if that is > > not > > > > the > > > > > > case. > > > > > > > > > > Thanks! > > > > > > > > > > Patching that change in manually doesn't seem to fix things (does it > rely > > on > > > > > other recent changes? doing a full sync now)... it still seems like the > > > > deleted > > > > > glyph references in |fTextBlobCache| would be a problem? I think we may > > also > > > > > need something like this (fixes the issue I'm seeing): > > > > > > > > > > https://codereview.chromium.org/1155923004 > > > > > > > > ericrk@, thanks for looking into this! > > > > > > > > i was essentially waiting for the reviewers for a direction.Because, > > clearing > > > > all of gpu resource on visibility change was having artifacts (on > desktops), > > > as > > > > you had seen too. > > > > > > > > Also, since the orignal bug was about clearing up the gr cache and not all > > of > > > > gpu resources, so i was thinking we would only do that (infact the older > > > > patchsets were about adjusting gr cache limits only :)). > > > > > > > > But anyways, good that this got moving! > > > > > > @ericrk, please try: > > > https://codereview.chromium.org/1160633002/ > > > > @joshualitt - thanks for your help here! This seems to have fixed the > corruption > > I was seeing - will continue testing. > > > > @sohanjg - sounds good - the patch seems to be working for me at this point. > > Adding code to free GLES2DeviceContext's GPU resources doesn't seem to add new > > issues. I'm going to start working on some unit tests. > > urfortunately, even with https://codereview.chromium.org/1160633002/, i am > still seeing text corruption on my linux desktop on tab switch, when i force > gpu-raster for all layers. > my config, > Optimus true > GL_VENDOR Tungsten Graphics, Inc > GL_RENDERER Mesa DRI Intel(R) Ivybridge Mobile > GL_VERSION 3.0 Mesa 8.0.4 > > Let me know, if you need any further info. > thanks. > > ericrk@, is there something else i should do for this patch, while you take care > of the tests ? Did you apply both https://codereview.chromium.org/1160633002/ and https://codereview.chromium.org/1156693004/? I think both are needed. I'm still investigating performance implications of this patch... It looks like this will lead to regressions in tab switch performance on desktop, so a more complex policy might be needed (or we might want to turn this on on Android only for the moment). I'll respond in a few minutes with some results from my investigation.
On 2015/05/28 18:25:07, ericrk wrote: > On 2015/05/27 12:33:24, sohanjg wrote: > > On 2015/05/26 18:11:30, ericrk wrote: > > > On 2015/05/26 14:40:59, joshualitt wrote: > > > > On 2015/05/23 13:16:51, sohanjg wrote: > > > > > On 2015/05/22 22:35:46, ericrk wrote: > > > > > > On 2015/05/22 21:04:42, joshualitt wrote: > > > > > > > this CL: > > > > > > > https://codereview.chromium.org/1156693004/ > > > > > > > > > > > > > > Should hopefully fix the batchfontcache issue. Let me know if that > is > > > not > > > > > the > > > > > > > case. > > > > > > > > > > > > Thanks! > > > > > > > > > > > > Patching that change in manually doesn't seem to fix things (does it > > rely > > > on > > > > > > other recent changes? doing a full sync now)... it still seems like > the > > > > > deleted > > > > > > glyph references in |fTextBlobCache| would be a problem? I think we > may > > > also > > > > > > need something like this (fixes the issue I'm seeing): > > > > > > > > > > > > https://codereview.chromium.org/1155923004 > > > > > > > > > > ericrk@, thanks for looking into this! > > > > > > > > > > i was essentially waiting for the reviewers for a direction.Because, > > > clearing > > > > > all of gpu resource on visibility change was having artifacts (on > > desktops), > > > > as > > > > > you had seen too. > > > > > > > > > > Also, since the orignal bug was about clearing up the gr cache and not > all > > > of > > > > > gpu resources, so i was thinking we would only do that (infact the older > > > > > patchsets were about adjusting gr cache limits only :)). > > > > > > > > > > But anyways, good that this got moving! > > > > > > > > @ericrk, please try: > > > > https://codereview.chromium.org/1160633002/ > > > > > > @joshualitt - thanks for your help here! This seems to have fixed the > > corruption > > > I was seeing - will continue testing. > > > > > > @sohanjg - sounds good - the patch seems to be working for me at this point. > > > Adding code to free GLES2DeviceContext's GPU resources doesn't seem to add > new > > > issues. I'm going to start working on some unit tests. > > > > urfortunately, even with https://codereview.chromium.org/1160633002/, i am > > still seeing text corruption on my linux desktop on tab switch, when i force > > gpu-raster for all layers. > > my config, > > Optimus true > > GL_VENDOR Tungsten Graphics, Inc > > GL_RENDERER Mesa DRI Intel(R) Ivybridge Mobile > > GL_VERSION 3.0 Mesa 8.0.4 > > > > Let me know, if you need any further info. > > thanks. > > > > ericrk@, is there something else i should do for this patch, while you take > care > > of the tests ? > > Did you apply both https://codereview.chromium.org/1160633002/ and > https://codereview.chromium.org/1156693004/ I think both are needed. > > I'm still investigating performance implications of this patch... It looks like > this will lead to regressions in tab switch performance on desktop, so a more > complex policy might be needed (or we might want to turn this on on Android only > for the moment). > > I'll respond in a few minutes with some results from my investigation. Sorry i was OOO for the last few days. Indeed if we apply both https://codereview.chromium.org/1160633002/, and https://codereview.chromium.org/1156693004/, the tab switch corruption is not observed. And i saw your email about memory findings. They are really useful. 1 interesting thing was, Android tab switch didnt have this text corruption issue when i last tested, even without this 2 patches. And i saw you have resubmitted this patch for review ? Do you want to take it forward ? or its kool if i submit this and you work on the follow up u mentioned in your email ? Please let me know if i could be of help in anyway. thanks!
vmiura@, hendrikw@, whats more is reqd in this patch to land ? ericrk@ has started over the followups in separate CL. thanks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
