|
|
Created:
3 years, 9 months ago by Saman Sami Modified:
3 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnce we move SurfaceId allocation from DelegatedFrameHost to
RendererCompositorFrameSink, DelegatedFrameHost will evict the renderer's
surfaces without the renderer knowing when we are under memory pressure.
This means that the renderer might continue sending frames using the same
local surface id that is destroyed, and we need to make sure we can handle
this situation properly.
1) If the renderer's surface is in the garbage collector's queue, we just
take it out of there and use it again.
2) If the renderer's surface is actually garbage collected, we create the
surface again.
I have added unit tests and also tested this CL together with
https://codereview.chromium.org/2731793002/. Without this CL, if the max
number of saved frames is 1 and we switch between two tabs very fast, a
DCHECK fails in SurfaceManager saying that the surface we are trying to
create already exists, but using this CL this issue doesn't happen.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2736053004
Cr-Commit-Position: refs/heads/master@{#455928}
Committed: https://chromium.googlesource.com/chromium/src/+/347e05c9fcda35ab2fd31cd7e7b9d89bc10aed26
Patch Set 1 #Patch Set 2 : Move surface creation to SurfaceManager #Patch Set 3 : Undo changes in surface #
Total comments: 10
Patch Set 4 : Addressed comments #Patch Set 5 : Split tests #
Total comments: 6
Patch Set 6 : Changed signature #
Total comments: 4
Patch Set 7 : Make deregister private #
Messages
Total messages: 46 (31 generated)
Description was changed from ========== SurfaceIds must be reusable as soon as a surface is destroyed ========== to ========== SurfaceIds must be reusable as soon as a surface is destroyed CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@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...
Description was changed from ========== SurfaceIds must be reusable as soon as a surface is destroyed CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== SurfaceIds must be reusable as soon as a surface is destroyed CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/08 17:24:33, Saman Sami wrote: > PTAL I will also add unit tests later. Just wanted to know how you feel about this approach for now.
not lgtm I'm not a fan of this approach. I think we should reuse the same SurfaceId because the new CompositorFrame might have unresolved dependencies.
not lgtm I'm not a fan of this approach. I think we should reuse the same SurfaceId because the new CompositorFrame might have unresolved dependencies.
On 2017/03/08 18:20:23, Fady Samuel wrote: > not lgtm > > I'm not a fan of this approach. I think we should reuse the same SurfaceId > because the new CompositorFrame might have unresolved dependencies. Oops, I meant to say same Surface, as opposed to same SurfaceId. Sorry for the confusion.
The CQ bit was checked by samans@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...
The CQ bit was checked by samans@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...
PTAL
Looking pretty good. I added some comments. A couple of unit tests would be useful too. https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:100: SurfaceId surface_id, const SurfaceId& https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:101: base::WeakPtr<SurfaceFactory> surface_factory) { Does thsi need to be a weakptr? https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:104: if (!surface_id.is_valid() || !surface_factory) DCHECK instead? It seems like a bug otherwise. https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:118: if (!surface_iter->second->destroyed()) This is a bug right? DCHECK instead? https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:124: for (auto it = surfaces_to_destroy_.begin(); it != surfaces_to_destroy_.end(); Use std::find? http://en.cppreference.com/w/cpp/algorithm/find
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 samans@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by samans@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...
PTAL https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:100: SurfaceId surface_id, On 2017/03/09 16:30:08, Fady Samuel wrote: > const SurfaceId& Done. https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:101: base::WeakPtr<SurfaceFactory> surface_factory) { On 2017/03/09 16:30:08, Fady Samuel wrote: > Does thsi need to be a weakptr? Surface keeps a weak ptr to its factory. https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:104: if (!surface_id.is_valid() || !surface_factory) On 2017/03/09 16:30:08, Fady Samuel wrote: > DCHECK instead? It seems like a bug otherwise. Done. https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:118: if (!surface_iter->second->destroyed()) On 2017/03/09 16:30:08, Fady Samuel wrote: > This is a bug right? DCHECK instead? Done. https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_man... cc/surfaces/surface_manager.cc:124: for (auto it = surfaces_to_destroy_.begin(); it != surfaces_to_destroy_.end(); On 2017/03/09 16:30:08, Fady Samuel wrote: > Use std::find? http://en.cppreference.com/w/cpp/algorithm/find Done.
Description was changed from ========== SurfaceIds must be reusable as soon as a surface is destroyed CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Once we move SurfaceId allocation from DelegatedFrameHost to RendererCompositorFrameSink, DelegatedFrameHost will evict the renderer's surfaces without the renderer knowing when we are under memory pressure. This means that the renderer might continue sending frames using the same local surface id that is destroyed, and we need to make sure we can handle this situation properly. 1) If the renderer's surface is in the garbage collector's queue, we just take it out of there and use it again. 2) If the renderer's surface is actually garbage collected, we create the surface again. I have added unit tests and also tested this CL together with https://codereview.chromium.org/2731793002/. Without this CL, if the max number of saved frames is 1 and we switch between two tabs very fast, a DCHECK fails in SurfaceManager saying that the surface we are trying to create already exists, but using this CL this issue doesn't happen. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Mostly looks good. I've proposed a safer signature for SurfaceManager::CreateSurface. Thanks! The tests are awesome! https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:100: SurfaceId surface_id, looks like you missed making this const SurfaceId& https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:70: std::unique_ptr<Surface> CreateSurface( Here's a safer signature: std::unique_ptr<Surface> CreateSurface(base::WeakPtr<SurfaceFactory>, const LocalSurfaceId& local_surface_id); Then use the frame_sink_id(), and local_surface_id to allocate the surface. That way you cannot allocate a SurfaceId that doesn't belong to the factory you're passing in? WDYT? https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:71: SurfaceId surface_id, looks like you missed making this const SurfaceId&
The CQ bit was checked by samans@chromium.org to run a CQ dry run
samans@chromium.org changed reviewers: + jbauman@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL jbauman@: This CL lets the renderer keep submitting frames even if the browser process removes the renderer's surface under memory pressure. It'll help with https://codereview.chromium.org/2731793002/. PTAL. https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.cc:100: SurfaceId surface_id, On 2017/03/09 21:32:16, Fady Samuel wrote: > looks like you missed making this const SurfaceId& Sorry one of my commits got deleted and I had to redo all the fixes. Forgot about this one. https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:70: std::unique_ptr<Surface> CreateSurface( On 2017/03/09 21:32:16, Fady Samuel wrote: > Here's a safer signature: > > std::unique_ptr<Surface> CreateSurface(base::WeakPtr<SurfaceFactory>, const > LocalSurfaceId& local_surface_id); > > Then use the frame_sink_id(), and local_surface_id to allocate the surface. That > way you cannot allocate a SurfaceId that doesn't belong to the factory you're > passing in? > > WDYT? I like it. https://codereview.chromium.org/2736053004/diff/100001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:71: SurfaceId surface_id, On 2017/03/09 21:32:16, Fady Samuel wrote: > looks like you missed making this const SurfaceId& Done.
lgtm https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:38: class Surface; add "class SurfaceFactory;" here (or add it as an include above if that doesn't work). https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:68: void DeregisterSurface(const SurfaceId& surface_id); Could you make this private while you're changing this around?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Thanks for the lgtm! https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_ma... File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:38: class Surface; On 2017/03/09 22:21:37, jbauman wrote: > add "class SurfaceFactory;" here (or add it as an include above if that doesn't > work). Done. https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_ma... cc/surfaces/surface_manager.h:68: void DeregisterSurface(const SurfaceId& surface_id); On 2017/03/09 22:21:37, jbauman wrote: > Could you make this private while you're changing this around? Done.
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 samans@google.com
The CQ bit was checked by samans@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2736053004/#ps140001 (title: "Make deregister private")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489103653189110, "parent_rev": "362f805fe22c5c92e5300526bc05c30aed5edb58", "commit_rev": "347e05c9fcda35ab2fd31cd7e7b9d89bc10aed26"}
Message was sent while issue was closed.
Description was changed from ========== Once we move SurfaceId allocation from DelegatedFrameHost to RendererCompositorFrameSink, DelegatedFrameHost will evict the renderer's surfaces without the renderer knowing when we are under memory pressure. This means that the renderer might continue sending frames using the same local surface id that is destroyed, and we need to make sure we can handle this situation properly. 1) If the renderer's surface is in the garbage collector's queue, we just take it out of there and use it again. 2) If the renderer's surface is actually garbage collected, we create the surface again. I have added unit tests and also tested this CL together with https://codereview.chromium.org/2731793002/. Without this CL, if the max number of saved frames is 1 and we switch between two tabs very fast, a DCHECK fails in SurfaceManager saying that the surface we are trying to create already exists, but using this CL this issue doesn't happen. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Once we move SurfaceId allocation from DelegatedFrameHost to RendererCompositorFrameSink, DelegatedFrameHost will evict the renderer's surfaces without the renderer knowing when we are under memory pressure. This means that the renderer might continue sending frames using the same local surface id that is destroyed, and we need to make sure we can handle this situation properly. 1) If the renderer's surface is in the garbage collector's queue, we just take it out of there and use it again. 2) If the renderer's surface is actually garbage collected, we create the surface again. I have added unit tests and also tested this CL together with https://codereview.chromium.org/2731793002/. Without this CL, if the max number of saved frames is 1 and we switch between two tabs very fast, a DCHECK fails in SurfaceManager saying that the surface we are trying to create already exists, but using this CL this issue doesn't happen. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2736053004 Cr-Commit-Position: refs/heads/master@{#455928} Committed: https://chromium.googlesource.com/chromium/src/+/347e05c9fcda35ab2fd31cd7e7b9... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/347e05c9fcda35ab2fd31cd7e7b9... |