New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 751080 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 732572



Sign in to add a comment

Move frame eviction code to components/viz/client

Project Member Reported by fsam...@chromium.org, Aug 1 2017

Issue description

As part of enabling frame eviction in Viz, I think the frame eviction code in components/viz/service/frame_sinks better belongs in components/viz/client.
 

Comment 1 by samans@chromium.org, Sep 22 2017

Cc: fsam...@chromium.org
Seems like frame eviction code depends on ServerSharedBitmapManager which cannot be accessed in the clients. If we have too many SharedBitmaps, we start evicting CompositorFrames hoping to reduce the number of open file descriptors 
(See https://cs.chromium.org/chromium/src/components/viz/service/frame_sinks/frame_eviction_manager.cc?rcl=2cea61215d2cee33b4335c36f10723e80444bc7a&l=136). However, I think this code is obsolete and should be removed. This code was written when we used to keep the file descriptors of SharedBitmaps open, but now we immediately close them once they're mapped, so nothing is actually gained by evicting CompositorFrames.

Before:
https://cs.chromium.org/chromium/src/content/common/host_shared_bitmap_manager.cc?rcl=6896d7f921d0f0bf1abfd65b25fbe97cb9c7f1c8&l=72

Now:
https://cs.chromium.org/chromium/src/components/viz/service/display_embedder/server_shared_bitmap_manager.cc?rcl=fe71bbc1fb996e9132e702be8cda4f79b055c5d5&l=171

Relevant CL: https://codereview.chromium.org/762273002/
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2472fcee0f30ffeb1f51a71782a1212caaa4749

commit d2472fcee0f30ffeb1f51a71782a1212caaa4749
Author: Saman Sami <samans@chromium.org>
Date: Mon Sep 25 20:10:41 2017

Don't evict CompositorFrames based on the number of SharedBitmaps

We used to keep the file descriptors of SharedBitmaps open and therefore
evicting frames would help avoid reaching the limit of max number of
open file descriptors. However, now we just close the file descriptor
after it is mapped to virtual memory and evicting frames isn't necessary
anymore.

Bug:  766470 , 751080 
Change-Id: I9c0f54218a0efae8f7a918d808fbda9e0a9556e0
Reviewed-on: https://chromium-review.googlesource.com/682103
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504145}
[modify] https://crrev.com/d2472fcee0f30ffeb1f51a71782a1212caaa4749/components/viz/service/frame_sinks/frame_eviction_manager.cc
[modify] https://crrev.com/d2472fcee0f30ffeb1f51a71782a1212caaa4749/components/viz/service/frame_sinks/frame_eviction_manager.h
[modify] https://crrev.com/d2472fcee0f30ffeb1f51a71782a1212caaa4749/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9e1785096b05d7cf18e154f7edb5c4659f0d515

commit d9e1785096b05d7cf18e154f7edb5c4659f0d515
Author: Saman Sami <samans@chromium.org>
Date: Thu Sep 28 04:54:00 2017

Move frame eviction code to components/viz/client

Move frame eviction code to components/viz/client to make it accessible
in the clients and prevent further introduction of dependency on service
code.

Bug:  751080 , 766470 
Change-Id: I40acd6ddae91380e30d9c8f6b6a3a3308a022fa1
Reviewed-on: https://chromium-review.googlesource.com/682995
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504905}
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/BUILD.gn
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/client_layer_tree_frame_sink.h
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/client_shared_bitmap_manager.h
[rename] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/frame_eviction_manager.cc
[rename] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/frame_eviction_manager.h
[rename] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/frame_evictor.cc
[rename] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/frame_evictor.h
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/hit_test_data_provider.h
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/local_surface_id_provider.h
[add] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/client/viz_client_export.h
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/components/viz/service/BUILD.gn
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/content/browser/BUILD.gn
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/content/browser/renderer_host/delegated_frame_host.h
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/d9e1785096b05d7cf18e154f7edb5c4659f0d515/content/test/BUILD.gn

Comment 4 by samans@chromium.org, Sep 28 2017

Status: Fixed (was: Assigned)

Sign in to add a comment