Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090903002:
aw: Share cc::Display instance (Closed)
Descriptionaw: Share cc::Display instance
When there are multiple webview instances, share the same
cc::Display instance, and other related objects.
Main goal is to save memory. Sharing the same GLRenderer
instance should allow sharing shaders. And sharing the same
command buffer instance saves on things like transfer
memory.
To avoid sync point churn when interleaving draws of
different HardwareRenderers, use CompositorFrameMetadata
referenced_surfaces always keep references to all child
surfaces from the root surface.
BUG=611623
Committed: https://crrev.com/8b58fe54a8524d58d91e6e3fa78b1c3a07671e5f
Cr-Commit-Position: refs/heads/master@{#402860}
Patch Set 1 #Patch Set 2 : works #
Total comments: 1
Patch Set 3 : clean up #Patch Set 4 : referenced_surfaces #Patch Set 5 : raw pointers #
Total comments: 9
Patch Set 6 : hush review #Patch Set 7 : rebase #
Messages
Total messages: 32 (10 generated)
boliu@chromium.org changed reviewers: + dcheng@chromium.org
hey Daniel, this CL is not ready for full review yet. but someone nominated you as someone who cares about object lifetime patterns. so any opinions on this pattern? https://codereview.chromium.org/2090903002/diff/20001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2090903002/diff/20001/android_webview/browser... android_webview/browser/surfaces_instance.cc:35: scoped_refptr<SurfacesInstance> SurfacesInstance::GetOrCreateInstance() { So the problem is that I want a singleton that's destroyed when it's not being used, because it's using a lot of memory. Everything is on one thread, so no thread safety issues here. And the solution I came up with: refcounted singleton, that has a weakptr to detect if been cleaned up already. Thoughts? Is this evil? Is there already an pattern for this?
Description was changed from ========== aw: Share cc::Display instance BUG=611623 ========== to ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. BUG=611623 ==========
boliu@chromium.org changed reviewers: + jbauman@chromium.org
+jbauman to review surfaces usage So imagine there is a single SurfacesInstance object shared by many HardwareRenderer objects. I still use roughly the same set up as before: root and child surfaces, root frame contains the transform and references child. Now there is a single root surface, and many children, but at any point, there is only one child that's drawing. And root and child are in different namespace/SurfaceFactory now.
On 2016/06/24 16:44:10, boliu wrote: > +jbauman to review surfaces usage > > So imagine there is a single SurfacesInstance object shared by many > HardwareRenderer objects. > > I still use roughly the same set up as before: root and child surfaces, root > frame contains the transform and references child. Now there is a single root > surface, and many children, but at any point, there is only one child that's > drawing. And root and child are in different namespace/SurfaceFactory now. Generally this works, but one issue is that the parent compositor will return all the child resources for one webview when it draws the next, and so it'll have to create a sync token and so on and generate lists of returned resources and so on. Currently it keeps them for the next frame, and only returns them if they're not used. That doesn't use a huge amount of CPU, but it might be noticeable. If it does hurt performance, we'll have to figure out a way to prevent SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the common case, and only run it when a webview is destroyed.
On 2016/06/24 22:47:55, jbauman wrote: > On 2016/06/24 16:44:10, boliu wrote: > > +jbauman to review surfaces usage > > > > So imagine there is a single SurfacesInstance object shared by many > > HardwareRenderer objects. > > > > I still use roughly the same set up as before: root and child surfaces, root > > frame contains the transform and references child. Now there is a single root > > surface, and many children, but at any point, there is only one child that's > > drawing. And root and child are in different namespace/SurfaceFactory now. > > Generally this works, but one issue is that the parent compositor will return > all the child resources for one webview when it draws the next, Ahh, that's actually not ok for webvew. Each child frame in webview can be drawn multiple times. Interesting though this didn't show up as a problem when I manually tested in gmail.. Any suggestions on keeping the child frame and also keep sharing everything? > and so it'll > have to create a sync token and so on and generate lists of returned resources > and so on. Currently it keeps them for the next frame, and only returns them if > they're not used. > > That doesn't use a huge amount of CPU, but it might be noticeable. If it does > hurt performance, we'll have to figure out a way to prevent > SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the common > case, and only run it when a webview is destroyed.
On 2016/06/24 23:00:48, boliu wrote: > On 2016/06/24 22:47:55, jbauman wrote: > > On 2016/06/24 16:44:10, boliu wrote: > > > +jbauman to review surfaces usage > > > > > > So imagine there is a single SurfacesInstance object shared by many > > > HardwareRenderer objects. > > > > > > I still use roughly the same set up as before: root and child surfaces, root > > > frame contains the transform and references child. Now there is a single > root > > > surface, and many children, but at any point, there is only one child that's > > > drawing. And root and child are in different namespace/SurfaceFactory now. > > > > Generally this works, but one issue is that the parent compositor will return > > all the child resources for one webview when it draws the next, > > Ahh, that's actually not ok for webvew. Each child frame in webview can be drawn > multiple times. Interesting though this didn't show up as a problem when I > manually tested in gmail.. Just checked a trace with gmail, side swipes always produces new child frames, so each child frame is only drawn once, so not affected by this issue. > > Any suggestions on keeping the child frame and also keep sharing everything? I see SurfaceFactory::RefResources/UnrefResources. Can I use that to keep resources in the display even if it's not used by the current draw? > > > and so it'll > > have to create a sync token and so on and generate lists of returned resources > > and so on. Currently it keeps them for the next frame, and only returns them > if > > they're not used. > > > > That doesn't use a huge amount of CPU, but it might be noticeable. If it does > > hurt performance, we'll have to figure out a way to prevent > > SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the common > > case, and only run it when a webview is destroyed.
On 2016/06/24 23:11:27, boliu wrote: > On 2016/06/24 23:00:48, boliu wrote: > > On 2016/06/24 22:47:55, jbauman wrote: > > > On 2016/06/24 16:44:10, boliu wrote: > > > > +jbauman to review surfaces usage > > > > > > > > So imagine there is a single SurfacesInstance object shared by many > > > > HardwareRenderer objects. > > > > > > > > I still use roughly the same set up as before: root and child surfaces, > root > > > > frame contains the transform and references child. Now there is a single > > root > > > > surface, and many children, but at any point, there is only one child > that's > > > > drawing. And root and child are in different namespace/SurfaceFactory now. > > > > > > Generally this works, but one issue is that the parent compositor will > return > > > all the child resources for one webview when it draws the next, > > > > Ahh, that's actually not ok for webvew. Each child frame in webview can be > drawn > > multiple times. Interesting though this didn't show up as a problem when I > > manually tested in gmail.. > > Just checked a trace with gmail, side swipes always produces new child frames, > so each child frame is only drawn once, so not affected by this issue. > > > > > Any suggestions on keeping the child frame and also keep sharing everything? > > I see SurfaceFactory::RefResources/UnrefResources. Can I use that to keep > resources in the display even if it's not used by the current draw? So, to be clear, this is just a performance issue. Correctness-wise it should be fine. One thing to try is to keep all child surface ids in CompositorFrameMetadata::referenced_surfaces of the root frame (except when a child is destroyed, in which case you clear it for one frame). That would require the SurfaceAggregator to do the tree prewalk for every child frame, but that's pretty cheap. > > > > > > and so it'll > > > have to create a sync token and so on and generate lists of returned > resources > > > and so on. Currently it keeps them for the next frame, and only returns them > > if > > > they're not used. > > > > > > That doesn't use a huge amount of CPU, but it might be noticeable. If it > does > > > hurt performance, we'll have to figure out a way to prevent > > > SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the common > > > case, and only run it when a webview is destroyed.
On 2016/06/24 23:18:30, jbauman wrote: > On 2016/06/24 23:11:27, boliu wrote: > > On 2016/06/24 23:00:48, boliu wrote: > > > On 2016/06/24 22:47:55, jbauman wrote: > > > > On 2016/06/24 16:44:10, boliu wrote: > > > > > +jbauman to review surfaces usage > > > > > > > > > > So imagine there is a single SurfacesInstance object shared by many > > > > > HardwareRenderer objects. > > > > > > > > > > I still use roughly the same set up as before: root and child surfaces, > > root > > > > > frame contains the transform and references child. Now there is a single > > > root > > > > > surface, and many children, but at any point, there is only one child > > that's > > > > > drawing. And root and child are in different namespace/SurfaceFactory > now. > > > > > > > > Generally this works, but one issue is that the parent compositor will > > return > > > > all the child resources for one webview when it draws the next, > > > > > > Ahh, that's actually not ok for webvew. Each child frame in webview can be > > drawn > > > multiple times. Interesting though this didn't show up as a problem when I > > > manually tested in gmail.. > > > > Just checked a trace with gmail, side swipes always produces new child frames, > > so each child frame is only drawn once, so not affected by this issue. > > > > > > > > Any suggestions on keeping the child frame and also keep sharing everything? > > > > I see SurfaceFactory::RefResources/UnrefResources. Can I use that to keep > > resources in the display even if it's not used by the current draw? > > So, to be clear, this is just a performance issue. Correctness-wise it should be > fine. iiuc, it is a correctness issue in webview. Not every HardwareRenderer::DrawGL will come with a new delegated frame from the renderer compositor. And when there is no new delegated frame, HardwareRenderer::DrawGL is expected to redraw the previous one. So if two HardwareRenderers interleave their draws, it's possible case above can end up with no frame/resources? Or did I misunderstand something here? > > One thing to try is to keep all child surface ids in > CompositorFrameMetadata::referenced_surfaces of the root frame (except when a > child is destroyed, in which case you clear it for one frame). > > That would require the SurfaceAggregator to do the tree prewalk for every child > frame, but that's pretty cheap. Cool, I'll try that, and add a test for this specific case. > > > > > > > > > > and so it'll > > > > have to create a sync token and so on and generate lists of returned > > resources > > > > and so on. Currently it keeps them for the next frame, and only returns > them > > > if > > > > they're not used. > > > > > > > > That doesn't use a huge amount of CPU, but it might be noticeable. If it > > does > > > > hurt performance, we'll have to figure out a way to prevent > > > > SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the > common > > > > case, and only run it when a webview is destroyed.
On 2016/06/24 23:30:13, boliu wrote: > On 2016/06/24 23:18:30, jbauman wrote: > > On 2016/06/24 23:11:27, boliu wrote: > > > On 2016/06/24 23:00:48, boliu wrote: > > > > On 2016/06/24 22:47:55, jbauman wrote: > > > > > On 2016/06/24 16:44:10, boliu wrote: > > > > > > +jbauman to review surfaces usage > > > > > > > > > > > > So imagine there is a single SurfacesInstance object shared by many > > > > > > HardwareRenderer objects. > > > > > > > > > > > > I still use roughly the same set up as before: root and child > surfaces, > > > root > > > > > > frame contains the transform and references child. Now there is a > single > > > > root > > > > > > surface, and many children, but at any point, there is only one child > > > that's > > > > > > drawing. And root and child are in different namespace/SurfaceFactory > > now. > > > > > > > > > > Generally this works, but one issue is that the parent compositor will > > > return > > > > > all the child resources for one webview when it draws the next, > > > > > > > > Ahh, that's actually not ok for webvew. Each child frame in webview can be > > > drawn > > > > multiple times. Interesting though this didn't show up as a problem when I > > > > manually tested in gmail.. > > > > > > Just checked a trace with gmail, side swipes always produces new child > frames, > > > so each child frame is only drawn once, so not affected by this issue. > > > > > > > > > > > Any suggestions on keeping the child frame and also keep sharing > everything? > > > > > > I see SurfaceFactory::RefResources/UnrefResources. Can I use that to keep > > > resources in the display even if it's not used by the current draw? > > > > So, to be clear, this is just a performance issue. Correctness-wise it should > be > > fine. > > iiuc, it is a correctness issue in webview. Not every HardwareRenderer::DrawGL > will come with a new delegated frame from the renderer compositor. And when > there is no new delegated frame, HardwareRenderer::DrawGL is expected to redraw > the previous one. > > So if two HardwareRenderers interleave their draws, it's possible case above can > end up with no frame/resources? > > Or did I misunderstand something here? What'll happen is the SurfaceAggregator will tell the ResourceProvider to release all the child resources. It'll create a synctoken and return them all to the SurfaceFactory. The Surface still has a frame that references them, so it'll hold on to the resources and not return them to the child compositor. The next frame the SurfaceAggregator will add them back to the ResourceProvider and queue a wait for the synctoken it just returned. So the SurfaceFactory prevents the resources from being completely returned, but some unnecessary work will be done. > > > > > One thing to try is to keep all child surface ids in > > CompositorFrameMetadata::referenced_surfaces of the root frame (except when a > > child is destroyed, in which case you clear it for one frame). > > > > That would require the SurfaceAggregator to do the tree prewalk for every > child > > frame, but that's pretty cheap. > > Cool, I'll try that, and add a test for this specific case. > > > > > > > > > > > > > > > and so it'll > > > > > have to create a sync token and so on and generate lists of returned > > > resources > > > > > and so on. Currently it keeps them for the next frame, and only returns > > them > > > > if > > > > > they're not used. > > > > > > > > > > That doesn't use a huge amount of CPU, but it might be noticeable. If it > > > does > > > > > hurt performance, we'll have to figure out a way to prevent > > > > > SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the > > common > > > > > case, and only run it when a webview is destroyed.
On 2016/06/24 23:38:13, jbauman wrote: > On 2016/06/24 23:30:13, boliu wrote: > > On 2016/06/24 23:18:30, jbauman wrote: > > > On 2016/06/24 23:11:27, boliu wrote: > > > > On 2016/06/24 23:00:48, boliu wrote: > > > > > On 2016/06/24 22:47:55, jbauman wrote: > > > > > > On 2016/06/24 16:44:10, boliu wrote: > > > > > > > +jbauman to review surfaces usage > > > > > > > > > > > > > > So imagine there is a single SurfacesInstance object shared by many > > > > > > > HardwareRenderer objects. > > > > > > > > > > > > > > I still use roughly the same set up as before: root and child > > surfaces, > > > > root > > > > > > > frame contains the transform and references child. Now there is a > > single > > > > > root > > > > > > > surface, and many children, but at any point, there is only one > child > > > > that's > > > > > > > drawing. And root and child are in different > namespace/SurfaceFactory > > > now. > > > > > > > > > > > > Generally this works, but one issue is that the parent compositor will > > > > return > > > > > > all the child resources for one webview when it draws the next, > > > > > > > > > > Ahh, that's actually not ok for webvew. Each child frame in webview can > be > > > > drawn > > > > > multiple times. Interesting though this didn't show up as a problem when > I > > > > > manually tested in gmail.. > > > > > > > > Just checked a trace with gmail, side swipes always produces new child > > frames, > > > > so each child frame is only drawn once, so not affected by this issue. > > > > > > > > > > > > > > Any suggestions on keeping the child frame and also keep sharing > > everything? > > > > > > > > I see SurfaceFactory::RefResources/UnrefResources. Can I use that to keep > > > > resources in the display even if it's not used by the current draw? > > > > > > So, to be clear, this is just a performance issue. Correctness-wise it > should > > be > > > fine. > > > > iiuc, it is a correctness issue in webview. Not every HardwareRenderer::DrawGL > > will come with a new delegated frame from the renderer compositor. And when > > there is no new delegated frame, HardwareRenderer::DrawGL is expected to > redraw > > the previous one. > > > > So if two HardwareRenderers interleave their draws, it's possible case above > can > > end up with no frame/resources? > > > > Or did I misunderstand something here? > > What'll happen is the SurfaceAggregator will tell the ResourceProvider to > release all the child resources. It'll create a synctoken and return them all to > the SurfaceFactory. The Surface still has a frame that references them, so it'll > hold on to the resources and not return them to the child compositor. The next > frame the SurfaceAggregator will add them back to the ResourceProvider and queue > a wait for the synctoken it just returned. So the SurfaceFactory prevents the > resources from being completely returned, but some unnecessary work will be > done. Ohh I see. Thanks for the explanation. I think I'll do the referenced_surfaces change as you suggest. > > > > > > > > > One thing to try is to keep all child surface ids in > > > CompositorFrameMetadata::referenced_surfaces of the root frame (except when > a > > > child is destroyed, in which case you clear it for one frame). > > > > > > That would require the SurfaceAggregator to do the tree prewalk for every > > child > > > frame, but that's pretty cheap. > > > > Cool, I'll try that, and add a test for this specific case. > > > > > > > > > > > > > > > > > > > > and so it'll > > > > > > have to create a sync token and so on and generate lists of returned > > > > resources > > > > > > and so on. Currently it keeps them for the next frame, and only > returns > > > them > > > > > if > > > > > > they're not used. > > > > > > > > > > > > That doesn't use a huge amount of CPU, but it might be noticeable. If > it > > > > does > > > > > > hurt performance, we'll have to figure out a way to prevent > > > > > > SurfaceAggregator::ProcessAddedAndRemovedSurfaces from running in the > > > common > > > > > > case, and only run it when a webview is destroyed.
Description was changed from ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. BUG=611623 ========== to ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. To avoid sync point churn when interleaving draws of different HardwareRenderers, use CompositorFrameMetadata referenced_surfaces always keep references to all child surfaces from the root surface. BUG=611623 ==========
added referenced_surfaces change, and checked nothing crashes, but still need to verify if there is any effect on the number of sync points
If it's all on the same thread, do we need to use base::LazyInstance? Maybe we could just use a raw pointer and have it set / unset in the ctor / dtor? I'm having trouble deciding if this is actually a better idea than using WeakPtr, but LazyInstance does kind of imply a thread-safety requirement.
On 2016/06/27 19:18:46, dcheng wrote: > If it's all on the same thread, do we need to use base::LazyInstance? Maybe we > could just use a raw pointer and have it set / unset in the ctor / dtor? I'm > having trouble deciding if this is actually a better idea than using WeakPtr, > but LazyInstance does kind of imply a thread-safety requirement. Switched raw pointers. I wanted to DCHECK there is only one instance, and raw pointer seems more natural than weakptr.
boliu@chromium.org changed reviewers: + hush@chromium.org
jbauman: could you take another look as well, I made sure root referenced_surfaces always contains the complete list of child ids. +hush for spreading knowledge on webview team and general review
On 2016/06/27 21:15:04, boliu wrote: > jbauman: could you take another look as well, I made sure root > referenced_surfaces always contains the complete list of child ids. > > +hush for spreading knowledge on webview team and general review lgtm, this should work.
https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:45: g_surfaces_instance = this; shouldn't this line be at the last line of constructor? because "this" is not fully fledged yet. Callers can start using a half done object. https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:156: // Reset the root surface frame so child surface lifetime remains independent. what does this mean? I don't see this thing in the old code. How does this empty root frame affect the life time of the child surfaces? https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:161: DCHECK(std::find(child_ids_.begin(), child_ids_.end(), child_id) == i'm just curious how this is different from child_is_.find(child_id) == child_ids.end(). No difference? https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.h:73: scoped_refptr<AwGLSurface> gl_surface_; why is this scoped_refptr while others are unique_ptrs? It is not shared by anybody else and it's completely own by this class. Can this be unique_ptr as well?
https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:45: g_surfaces_instance = this; On 2016/06/29 00:03:12, hush wrote: > shouldn't this line be at the last line of constructor? because "this" is not > fully fledged yet. Callers can start using a half done object. Done fwiw this is all single threaded, so not really that big of an issue. https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:156: // Reset the root surface frame so child surface lifetime remains independent. On 2016/06/29 00:03:11, hush wrote: > what does this mean? I don't see this thing in the old code. > How does this empty root frame affect the life time of the child surfaces? Removed. It's "nice property" that root doesn't reference the specific child that was drawn last. But then root references all children now, so really doesn't matter. https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:161: DCHECK(std::find(child_ids_.begin(), child_ids_.end(), child_id) == On 2016/06/29 00:03:12, hush wrote: > i'm just curious how this is different from child_is_.find(child_id) == > child_ids.end(). No difference? vector doesn't have a find method :p https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.h:73: scoped_refptr<AwGLSurface> gl_surface_; On 2016/06/29 00:03:12, hush wrote: > why is this scoped_refptr while others are unique_ptrs? It is not shared by > anybody else and it's completely own by this class. > Can this be unique_ptr as well? GLSurface is refcounted. Can't mix refcounted and unique_ptr. It is also held by the InProcessCommandBuffer for example, so it's not like this is the only reference to it.
lgtm
The CQ bit was checked by boliu@chromium.org
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/2090903002/#ps120001 (title: "rebase")
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.
Description was changed from ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. To avoid sync point churn when interleaving draws of different HardwareRenderers, use CompositorFrameMetadata referenced_surfaces always keep references to all child surfaces from the root surface. BUG=611623 ========== to ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. To avoid sync point churn when interleaving draws of different HardwareRenderers, use CompositorFrameMetadata referenced_surfaces always keep references to all child surfaces from the root surface. BUG=611623 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. To avoid sync point churn when interleaving draws of different HardwareRenderers, use CompositorFrameMetadata referenced_surfaces always keep references to all child surfaces from the root surface. BUG=611623 ========== to ========== aw: Share cc::Display instance When there are multiple webview instances, share the same cc::Display instance, and other related objects. Main goal is to save memory. Sharing the same GLRenderer instance should allow sharing shaders. And sharing the same command buffer instance saves on things like transfer memory. To avoid sync point churn when interleaving draws of different HardwareRenderers, use CompositorFrameMetadata referenced_surfaces always keep references to all child surfaces from the root surface. BUG=611623 Committed: https://crrev.com/8b58fe54a8524d58d91e6e3fa78b1c3a07671e5f Cr-Commit-Position: refs/heads/master@{#402860} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8b58fe54a8524d58d91e6e3fa78b1c3a07671e5f Cr-Commit-Position: refs/heads/master@{#402860}
Message was sent while issue was closed.
torne@chromium.org changed reviewers: + torne@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2090903002/diff/80001/android_webview/browser... android_webview/browser/surfaces_instance.cc:45: g_surfaces_instance = this; On 2016/06/29 01:01:03, boliu wrote: > On 2016/06/29 00:03:12, hush wrote: > > shouldn't this line be at the last line of constructor? because "this" is not > > fully fledged yet. Callers can start using a half done object. > > Done > > fwiw this is all single threaded, so not really that big of an issue. It's not sufficient to just put this as the last line if you were really concerned about thread safety. Since this code is single threaded it doesn't matter, but if this was actually being used from multiple threads, it would be required to have a real memory barrier. The order of memory accesses in source code is not meaningful. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
