Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2455663003:
Add cc::Surface ref counting. (Closed)
DescriptionAdd cc::Surface references.
Start framework for tracking reference in cc::SurfaceManager. This will
replace the SurfaceSequence dependency tracking.
The general concept is parent surface references a child surface it is
embedding. When the parent surface no longer needs the child surface, it
removes the reference. We can GC all surfaces with 0 references.
1. Add helper methods to add or remove a reference from a SurfaceId to
another SurfaceId. Add maps to track references in both directions.
2. Add check in SurfaceManager::GarbageCollectSurfaces() if references
are zero. If both references and sequences are zero we can garbage
collect the surface. This will allow references and sequences to both
work until sequences can be replaced.
3. Add unit tests that check references works correctly.
The surface reference code is unused outside tests at this point. It's
designed so that dependency tracking and references can be used together
while code is converted.
The next CL will start replacing surface dependencies with surface
references in mus-ws.
BUG=659227
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/0de3732f5b55c4e7ccbaec6487acfe0742a33e2b
Cr-Commit-Position: refs/heads/master@{#429594}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Address CL comments. #
Total comments: 1
Patch Set 3 : Start adding Surfaces for all tests. #Patch Set 4 : Delete remove all refs code, do cleanup and fix tests. #Patch Set 5 : More cleanup. #
Total comments: 8
Patch Set 6 : Fixes for fsamuel comments. #
Total comments: 13
Patch Set 7 : Address comments. #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== Add cc::Surface ref counting. Start framework for reference counting in cc::SurfaceManager. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add helper method to destroy all references that involve a particular FrameSinkId when a connection is lost. 3. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 4. Add basic tests that reference counting works correctly. BUG=659227 ========== to ========== Add cc::Surface ref counting. Start framework for reference counting in cc::SurfaceManager. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add helper method to destroy all references that involve a particular FrameSinkId when a connection is lost. 3. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 4. Add basic tests that reference counting works correctly. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
I really like this. It's going in a good direction! Thanks for sending me an early review! https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:96: // Should we allow multiple references? Maybe? IDK. To reduce complexity here and IPCs I think we should not allow multiple references, now that I think about it. That info can be managed in the client I think? Although that adds complexity in the client...I don't have a strong opinion either way. let's keep it simple here and add complexity if necessary I think. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:107: void SurfaceManager::RemoveSurfaceIdReference(const SurfaceId& parent_id, We should garbage collect if a surface ID no longer has references. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:115: DCHECK_EQ(parent_to_child_refs_.count(parent_id), 1u); This actually doesn't work very well for restartability. If a reference doesn't exist, then I think we should just ignore it instead of DCHECK'ing. The client may end up returning a surface ID that the gpu doesn't know about, and it's easier to ignore it here than to worry about solving races in the client, in my opinion. Other cc OWNERS may feel differently, I don't know. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:130: SurfaceIdSet child_child_refs = parent_to_child_refs_[child_id]; // Copy Add a comment here that you need to do this because you're modifying the set? https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:134: DCHECK_EQ(CountSurfaceReferees(child_id), 0); We should start a garbage collect if a surface ID no longer has references. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:145: for (auto& map_entry : parent_to_child_refs_) { Could you add a description of what we discussed here as to why a FrameSink may have multiple surface IDs in flight at once? A parent may refer to an old surface ID which retains all sub references. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:145: for (auto& map_entry : parent_to_child_refs_) { nit: const auto& https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:153: frame_sink_surfaces.insert(surface_id); I'm confused about when we'd hit this case? https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:156: // Clear all references involving FrameSink surfaces. nit: This comment is confusing. // Clear all surfaces belonging to FrameSink |frame_sink_id|. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:158: // Remove any references from parents to this surface, since this surface It seems like all the code in this block could move to a helper method to easy readability: RemoveAllSurfaceIdReferences(surface_id); https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:179: int SurfaceManager::CountSurfaceReferences(const SurfaceId& surface_id) const { nit: GetSurfaceReferenceCount? Make it uint32_t? https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:186: int SurfaceManager::CountSurfaceReferees(const SurfaceId& surface_id) const { GetReferencedSurfaceCountBySurface? https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.h:87: int CountSurfaceReferences(const SurfaceId& surface_id) const; nit: GetSurfaceReferenceCount? https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_ref_unittest.cc:41: std::unique_ptr<SurfaceManager> manager_; No need to place it in a unique_ptr I think. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_ref_unittest.cc:48: EXPECT_EQ(manager().CountSurfaceReferences(kId1), 0); Hmm, what do we do about top-level surfaces? I guess we don't need refs on them? https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_ref_unittest.cc:129: TEST_F(SurfaceManagerRefTest, RemoveAllRecusiveMultipleSurfaces) { An interesting test or two or three to add would mix SurfaceSequences with Surface references showing that we don't garbage collect incorrectly.
I've addressed some of the comments with the latest patch. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:96: // Should we allow multiple references? Maybe? IDK. On 2016/10/27 04:14:48, Fady Samuel wrote: > To reduce complexity here and IPCs I think we should not allow multiple > references, now that I think about it. That info can be managed in the client I > think? Although that adds complexity in the client...I don't have a strong > opinion either way. let's keep it simple here and add complexity if necessary I > think. Changed per our offline discussion. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:115: DCHECK_EQ(parent_to_child_refs_.count(parent_id), 1u); On 2016/10/27 04:14:48, Fady Samuel wrote: > This actually doesn't work very well for restartability. If a reference doesn't > exist, then I think we should just ignore it instead of DCHECK'ing. The client > may end up returning a surface ID that the gpu doesn't know about, and it's > easier to ignore it here than to worry about solving races in the client, in my > opinion. Other cc OWNERS may feel differently, I don't know. I'm checking the reference exists instead of DCHECK'ing. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:130: SurfaceIdSet child_child_refs = parent_to_child_refs_[child_id]; // Copy On 2016/10/27 04:14:48, Fady Samuel wrote: > Add a comment here that you need to do this because you're modifying the set? Changed, PTAL. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:134: DCHECK_EQ(CountSurfaceReferees(child_id), 0); On 2016/10/27 04:14:48, Fady Samuel wrote: > We should start a garbage collect if a surface ID no longer has references. Done. I've added an optional parameter to avoid GC'ing more than once per call top level call to RemoveSurfaceIdReference(). https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:145: for (auto& map_entry : parent_to_child_refs_) { On 2016/10/27 04:14:48, Fady Samuel wrote: > nit: const auto& Done. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:153: frame_sink_surfaces.insert(surface_id); On 2016/10/27 04:14:48, Fady Samuel wrote: > I'm confused about when we'd hit this case? This is for leaf surfaces. So if we have A -> B -> C there will be the following entries: parent_to_child_refs_[A] = {B} parent_to_child_refs_[B] = {C} child_to_parent_refs_[B] = {A} child_to_parent_refs_[C] = {B} Since C doesn't reference anything it's not in parent_to_child_refs_ but we still have a reference from B -> C we need to remove. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:156: // Clear all references involving FrameSink surfaces. On 2016/10/27 04:14:48, Fady Samuel wrote: > nit: This comment is confusing. // Clear all surfaces belonging to FrameSink > |frame_sink_id|. Done. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:158: // Remove any references from parents to this surface, since this surface On 2016/10/27 04:14:48, Fady Samuel wrote: > It seems like all the code in this block could move to a helper method to easy > readability: > > RemoveAllSurfaceIdReferences(surface_id); Done. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:179: int SurfaceManager::CountSurfaceReferences(const SurfaceId& surface_id) const { On 2016/10/27 04:14:48, Fady Samuel wrote: > nit: GetSurfaceReferenceCount? Make it uint32_t? Changed name. If I'm going to use an unsigned type I'll use size_t. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.cc:186: int SurfaceManager::CountSurfaceReferees(const SurfaceId& surface_id) const { On 2016/10/27 04:14:48, Fady Samuel wrote: > GetReferencedSurfaceCountBySurface? Changed name. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager.h:87: int CountSurfaceReferences(const SurfaceId& surface_id) const; On 2016/10/27 04:14:49, Fady Samuel wrote: > nit: GetSurfaceReferenceCount? Done. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_ref_unittest.cc:41: std::unique_ptr<SurfaceManager> manager_; On 2016/10/27 04:14:49, Fady Samuel wrote: > No need to place it in a unique_ptr I think. I'd have to add a reset method to SurfaceManager otherwise to clear internal state. It isn't expensive to build, so my personal preference is to wipe the object and start with a new one. https://codereview.chromium.org/2455663003/diff/1/cc/surfaces/surface_manager... cc/surfaces/surface_manager_ref_unittest.cc:48: EXPECT_EQ(manager().CountSurfaceReferences(kId1), 0); On 2016/10/27 04:14:49, Fady Samuel wrote: > Hmm, what do we do about top-level surfaces? I guess we don't need refs on them? So my WIP patch beyond this one has a constant "SurfaceId kRootSuraceId" that SurfaceManager knows about. kRootSuraceId is the only top level surface and SurfaceManager skips checking references for it. The display root surfaces have a ref kRootSuraceId -> display_root_surface_id. So when we resize a display we just have to clear that one reference and all the old surfaces get GCd'.
Description was changed from ========== Add cc::Surface ref counting. Start framework for reference counting in cc::SurfaceManager. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add helper method to destroy all references that involve a particular FrameSinkId when a connection is lost. 3. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 4. Add basic tests that reference counting works correctly. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add cc::Surface ref counting. Start framework for reference counting in cc::SurfaceManager. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 3. Add unit tests that check references works correctly. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Update to a landable version. I'll send it out for OWNERS tomorrow sometime.
This is amazing so far Kyle! Thank you! https://codereview.chromium.org/2455663003/diff/20001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/20001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:151: if (should_run_gc) nit: add a comment as to when you'd run gc? https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:78: for (uint32_t value : *sequence) { nit: drop braces. https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:92: size_t GetSurfaceRefereeCount(const SurfaceId& surface_id) const; I'm not a fan of the name, maybe GetReferencedSurfacesCount? This is just a suggestion. Feel free to ignore. https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:230: // dependency from |kFrameSink2| nit: period at the end of the sentence. https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:291: } This is a very comprehensive set of tests! Thanks! What about a test that has a cycle? This shouldn't happen on valid input, but it's possible we end up with invalid input (compromised renderer) and we don't want to break the display compositor.
https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:78: for (uint32_t value : *sequence) { On 2016/11/02 03:10:36, Fady Samuel wrote: > nit: drop braces. Done. https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager.h:92: size_t GetSurfaceRefereeCount(const SurfaceId& surface_id) const; On 2016/11/02 03:10:36, Fady Samuel wrote: > I'm not a fan of the name, maybe GetReferencedSurfacesCount? This is just a > suggestion. Feel free to ignore. Done. https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... File cc/surfaces/surface_manager_ref_unittest.cc (right): https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:230: // dependency from |kFrameSink2| On 2016/11/02 03:10:36, Fady Samuel wrote: > nit: period at the end of the sentence. Done. https://codereview.chromium.org/2455663003/diff/80001/cc/surfaces/surface_man... cc/surfaces/surface_manager_ref_unittest.cc:291: } On 2016/11/02 03:10:36, Fady Samuel wrote: > This is a very comprehensive set of tests! Thanks! What about a test that has a > cycle? This shouldn't happen on valid input, but it's possible we end up with > invalid input (compromised renderer) and we don't want to break the display > compositor. Huh. Interesting. Fixing that would require the GC to be changed a bit, which I'm not sure is possible with the mix of sequences/references, but would be the end goal I think it makes the most sense to do a mark and sweep starting from kRootSurfaceId, anything that isn't reachable from there is destroyed. A bad renderer could make cycles all day with surfaces it knows about but once it's killed there is no way to reach the cycles and all the surfaces will be destroyed. So it's no different than a bad renderer refusing to give up references. Since we aren't using references for all (as of this CL any) surfaces that doesn't work yet...
Description was changed from ========== Add cc::Surface ref counting. Start framework for reference counting in cc::SurfaceManager. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 3. Add unit tests that check references works correctly. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add cc::Surface references. Start framework for tracking reference in cc::SurfaceManager. This will replace the SurfaceSequence dependency tracking. The general concept is parent surface references a child surface it is embedding. When the parent surface no longer needs the child surface, it removes the reference. We can GC all surfaces with 0 references. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 3. Add unit tests that check references works correctly. The surface reference code is unused outside tests at this point. It's designed so that dependency tracking and references can be used together while code is converted. The next CL will start replacing surface dependencies with surface references in mus-ws. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
kylechar@chromium.org changed reviewers: + enne@chromium.org
+enne for OWNERS
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_id.h File cc/surfaces/surface_id.h (right): https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_id... cc/surfaces/surface_id.h:26: constexpr SurfaceId(const SurfaceId& other) Can this also be = default? https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:100: if (parent_id == child_id) { Should this be a DCHECK/CHECK isntead? DCHECK(parent_id != child_id) << "Cannot add self reference for " << parent_id.ToString(); (same for the other two if checks) https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:119: bool should_run_gc) { Since we don't like default values, and since this function is recursive, you can structure it as: RemoveSurfaceReference(parent, child) { DCHECK(thread_checker_.CalledOnValidThread()); RemoveSurfaceReferenceRecursive(parent, child); GarbageCollectSurfaces(); } where RemoveSurfaceReferenceRecursive is a private function that has the rest of this function. https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:124: if (parent_to_child_refs_.count(parent_id) == 0 || DCHECK/CHECK https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:178: std::unordered_set<SurfaceId, SurfaceIdHash> live_surfaces_set; I know this was here before, but why both a vector and a set? Can there be duplicate SurfaceIds in the vector? https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:217: std::vector<std::unique_ptr<Surface>> to_destroy; Also, it was here before, but what's the reasoning for saving the deletion until after the loop? Can the surface be accessed after it's moved to the to_destroy vector? https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:84: bool should_run_gc = true); No default values, please
kylechar@chromium.org changed reviewers: - enne@chromium.org
+vmpstr for OWNERS https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_id.h File cc/surfaces/surface_id.h (right): https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_id... cc/surfaces/surface_id.h:26: constexpr SurfaceId(const SurfaceId& other) On 2016/11/02 20:28:12, vmpstr wrote: > Can this also be = default? Done. https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:100: if (parent_id == child_id) { On 2016/11/02 20:28:12, vmpstr wrote: > Should this be a DCHECK/CHECK isntead? > > DCHECK(parent_id != child_id) << "Cannot add self reference for " << > parent_id.ToString(); > > (same for the other two if checks) I think it would have to be a CHECK if anything, since we don't a misbehaving client to insert garbage references. Although I'm not sure we'd want a misbehaving client to crash the GPU process so easily? I wrote it with DCHECKs originally but after talking with fsamuel we thought it might be a problem. https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:119: bool should_run_gc) { On 2016/11/02 20:28:13, vmpstr wrote: > Since we don't like default values, and since this function is recursive, you > can structure it as: > > RemoveSurfaceReference(parent, child) { > DCHECK(thread_checker_.CalledOnValidThread()); > RemoveSurfaceReferenceRecursive(parent, child); > GarbageCollectSurfaces(); > } > > where RemoveSurfaceReferenceRecursive is a private function that has the rest of > this function. Done. https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:178: std::unordered_set<SurfaceId, SurfaceIdHash> live_surfaces_set; On 2016/11/02 20:28:12, vmpstr wrote: > I know this was here before, but why both a vector and a set? Can there be > duplicate SurfaceIds in the vector? Yep, duplicate surface ids. The same surface can be embedded more than once (eg. I think overview mode in CrOS). The set checks for duplicates, the vector can be looped over while we're adding new elements to the back. https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:217: std::vector<std::unique_ptr<Surface>> to_destroy; On 2016/11/02 20:28:12, vmpstr wrote: > Also, it was here before, but what's the reasoning for saving the deletion until > after the loop? Can the surface be accessed after it's moved to the to_destroy > vector? https://chromium.googlesource.com/chromium/src/+/1845ade677d2250edc922243244f... https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2455663003/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:84: bool should_run_gc = true); On 2016/11/02 20:28:13, vmpstr wrote: > No default values, please Done.
lgtm
The CQ bit was checked by kylechar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kylechar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add cc::Surface references. Start framework for tracking reference in cc::SurfaceManager. This will replace the SurfaceSequence dependency tracking. The general concept is parent surface references a child surface it is embedding. When the parent surface no longer needs the child surface, it removes the reference. We can GC all surfaces with 0 references. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 3. Add unit tests that check references works correctly. The surface reference code is unused outside tests at this point. It's designed so that dependency tracking and references can be used together while code is converted. The next CL will start replacing surface dependencies with surface references in mus-ws. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add cc::Surface references. Start framework for tracking reference in cc::SurfaceManager. This will replace the SurfaceSequence dependency tracking. The general concept is parent surface references a child surface it is embedding. When the parent surface no longer needs the child surface, it removes the reference. We can GC all surfaces with 0 references. 1. Add helper methods to add or remove a reference from a SurfaceId to another SurfaceId. Add maps to track references in both directions. 2. Add check in SurfaceManager::GarbageCollectSurfaces() if references are zero. If both references and sequences are zero we can garbage collect the surface. This will allow references and sequences to both work until sequences can be replaced. 3. Add unit tests that check references works correctly. The surface reference code is unused outside tests at this point. It's designed so that dependency tracking and references can be used together while code is converted. The next CL will start replacing surface dependencies with surface references in mus-ws. BUG=659227 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/0de3732f5b55c4e7ccbaec6487acfe0742a33e2b Cr-Commit-Position: refs/heads/master@{#429594} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0de3732f5b55c4e7ccbaec6487acfe0742a33e2b Cr-Commit-Position: refs/heads/master@{#429594}
Message was sent while issue was closed.
Belated lgtm. This is great, thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
