[chromium] Add resource transfer functions to CCResourceProvider

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

[chromium] Add resource transfer functions to CCResourceProvider

[Antoine Labour]

Created attachment 157296 [details]
Patch

[Antoine Labour]

This will need to be reconciled with https://bugs.webkit.org/show_bug.cgi?id=93442 obviously.

[Alexandre Elias]

The public calls can be added to the base class as pure virtual methods.  Hopefully with a few tweaks, this API can also be made to work with software compositing.  In that case, we will be pushing a software pixel blob to a method in Nat's new WebCompositorOutputSurface.  Perhaps we can make WebCompositorOutputSurface also generate a "mailbox" string identifier.

[Alexandre Elias]

On second thought, this isn't needed at all for software compositing, since we can just treat an "upload()" as a transfer to the root compositor there.

Hmm, since I gather this is just for accelerated painting, it seems like it would make a more straightforward data pipeline to send up the Ganesh commands to create or modify the texture to the root compositor as a form of "upload".  Is there something preventing that?

[Antoine Labour]

This is not just accelerated painting, it's for all resource types (webgl, canvas, pepper, video, etc.) on top of tiles which may or may not be accelerated.
Besides, AFAIK Ganesh doesn't serialize across processes, it keeps some data by reference.

Finally, even if upload() will results as a transfer to the root compositor (which I think at least in the short term would allow the renderer to overload the browser), you still need to synchronize wrt drawing by the parent compositor, i.e. you can't send the IPC at upload() time, you need to keep it around until the sendFrameToParent, at which point you need to gather the uploads for all the resources you want to send. This may be what comes out of prepareSendToParent (which would add a list of resource/shm handle/copy rects).

[Alexandre Elias]

Sounds reasonable, I agree this model is more robust against client misbehavior and we may as well use it in all cases.  Apology for the late design question, I went back to read your resource management doc.

[Alexandre Elias]

Comment on attachment 157296 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157296&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:337
> +        deleteResource(*it);

This will call deleteTexture on the context3d.  Is that intended?

[Antoine Labour]

Comment on attachment 157296 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157296&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:337
>> +        deleteResource(*it);
> 
> This will call deleteTexture on the context3d.  Is that intended?

It is intended, corresponding to the createTexture() done in receiveFromChild. The idea is that the child owns the resource, and it's only lent to the parent while it's used by a quad. So when it's returned to the child, we delete it from the parent.
In transferResource above, we put it into the mailbox, which "disconnects" the data from the texture name, so the delete here doesn't delete the data.

[Antoine Labour]

Created attachment 158416 [details]
Patch

[James Robinson]

Comment on attachment 158416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158416&action=review

Looks pretty good, but I really want whoever lands the next patch adding a parameter to Resource to fix it up so the initialization is readable.

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:112
> +    Resource resource = {textureId, pool, 0, false, false, false, size, format};

I gave this feedback to Alex too, but the pile of 0s and bool literals is unreadable to me. These parameters need readable enum names or the construction of resources has to be done by a builder or something - no mortal can remember the difference between "0, false, false, false" and "0, false, true, false" in the middle of an argument or initializer list.

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:305
> +    for (Vector<ResourceId>::const_iterator it = resources.begin(); it != resources.end(); ++it) {

looks like Vector<ResourceId> should have a typedef, it's repeated several times in this file

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:361
> +    for (Vector<TransferableResource>::const_iterator it = resources.resources.begin(); it != resources.resources.end(); ++it) {

same for Vector<TransferableResource> re typedef

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:369
> +        m_mailboxes.append(it->mailbox);

should we worry about keeping a bound on the # of mailboxes the rootmost compositor gets? it looks like it'll have the set of all mailboxes any child compositors have ever needed. is there any overhead to these on the service side over time? is there any way to discard them?

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:68
> +    struct TransferableResourceList {

this guy gets big pretty fast - sizeof(TransferableResource) is 196b if my math is right, and the first allocation in the vector will allocate space for 16 elements or around 3kb

> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:54
> +    Texture(const IntSize& size_, WGC3Denum format_)

we don't typically do the trailing underscore thing in webkit - ": size(size)" in the initializer list should work on all our build systems, afaik

> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:244
> +    explicit ResourceProviderContext(const Attributes& attrs, ContextSharedData* sharedData)

no explicit for 2-arg c'tors

[Antoine Labour]

Created attachment 158691 [details]
Patch

[Antoine Labour]

(In reply to comment #10)
> (From update of attachment 158416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158416&action=review
> 
> Looks pretty good, but I really want whoever lands the next patch adding a parameter to Resource to fix it up so the initialization is readable.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:112
> > +    Resource resource = {textureId, pool, 0, false, false, false, size, format};
> 
> I gave this feedback to Alex too, but the pile of 0s and bool literals is unreadable to me. These parameters need readable enum names or the construction of resources has to be done by a builder or something - no mortal can remember the difference between "0, false, false, false" and "0, false, true, false" in the middle of an argument or initializer list.

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:305
> > +    for (Vector<ResourceId>::const_iterator it = resources.begin(); it != resources.end(); ++it) {
> 
> looks like Vector<ResourceId> should have a typedef, it's repeated several times in this file

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:361
> > +    for (Vector<TransferableResource>::const_iterator it = resources.resources.begin(); it != resources.resources.end(); ++it) {
> 
> same for Vector<TransferableResource> re typedef

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:369
> > +        m_mailboxes.append(it->mailbox);
> 
> should we worry about keeping a bound on the # of mailboxes the rootmost compositor gets? it looks like it'll have the set of all mailboxes any child compositors have ever needed. is there any overhead to these on the service side over time? is there any way to discard them?

So, first, there's no way to "discard" them. At the same time, they don't hold any resources at all on the GPU process side. They're just signed unguessable unique IDs.
The other thing is, the root compositor sends back the mailbox name when recycling the resources to the child (i.e. it removes it from its deque). Essentially, in steady state, the root compositor will have exactly 1 mailbox name for each resource that was sent to it by children, so it shouldn't grow unbounded (unless it destroys resources without giving them back to a child, but it doesn't do that today).

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:68
> > +    struct TransferableResourceList {
> 
> this guy gets big pretty fast - sizeof(TransferableResource) is 196b if my math is right, and the first allocation in the vector will allocate space for 16 elements or around 3kb

I count 80b and my compiler agrees ;) For the Vector allocating too much capacity by default, I don't think it's a problem since this is a transient data structure anyway (i.e. generate it, stick the data into an IPC, destroy it, and on the other side, receive it from IPC then pass it to another one of these methods, and destroy it). If the capacity vs size overhead becomes high for some reason we could call shrinkToFit but since it involves copying the data, I don't think we necessarily need it.

> 
> > Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:54
> > +    Texture(const IntSize& size_, WGC3Denum format_)
> 
> we don't typically do the trailing underscore thing in webkit - ": size(size)" in the initializer list should work on all our build systems, afaik

Done.

> 
> > Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:244
> > +    explicit ResourceProviderContext(const Attributes& attrs, ContextSharedData* sharedData)
> 
> no explicit for 2-arg c'tors

Done.

[James Robinson]

(In reply to comment #12)
> > > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:369
> > > +        m_mailboxes.append(it->mailbox);
> > 
> > should we worry about keeping a bound on the # of mailboxes the rootmost compositor gets? it looks like it'll have the set of all mailboxes any child compositors have ever needed. is there any overhead to these on the service side over time? is there any way to discard them?
> 
> So, first, there's no way to "discard" them. At the same time, they don't hold any resources at all on the GPU process side. They're just signed unguessable unique IDs.
> The other thing is, the root compositor sends back the mailbox name when recycling the resources to the child (i.e. it removes it from its deque). Essentially, in steady state, the root compositor will have exactly 1 mailbox name for each resource that was sent to it by children, so it shouldn't grow unbounded (unless it destroys resources without giving them back to a child, but it doesn't do that today).
> 

True, but the parent will see many children come and go over the course of its lifetime.  Is there any mechanism by which mailboxes from children that go away are cleared? destroyChild() clears resources, but it doesn't clear mailboxes AFAICT, and it seems like new mailboxes are always created by children.  It's not the biggest deal - they aren't all that big - but it seems like a logical leak unless I'm missing part of it.

[James Robinson]

Comment on attachment 158691 [details]
Patch

R=me

[Antoine Labour]

(In reply to comment #13)
> (In reply to comment #12)
> > > > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:369
> > > > +        m_mailboxes.append(it->mailbox);
> > > 
> > > should we worry about keeping a bound on the # of mailboxes the rootmost compositor gets? it looks like it'll have the set of all mailboxes any child compositors have ever needed. is there any overhead to these on the service side over time? is there any way to discard them?
> > 
> > So, first, there's no way to "discard" them. At the same time, they don't hold any resources at all on the GPU process side. They're just signed unguessable unique IDs.
> > The other thing is, the root compositor sends back the mailbox name when recycling the resources to the child (i.e. it removes it from its deque). Essentially, in steady state, the root compositor will have exactly 1 mailbox name for each resource that was sent to it by children, so it shouldn't grow unbounded (unless it destroys resources without giving them back to a child, but it doesn't do that today).
> > 
> 
> True, but the parent will see many children come and go over the course of its lifetime.  Is there any mechanism by which mailboxes from children that go away are cleared? destroyChild() clears resources, but it doesn't clear mailboxes AFAICT, and it seems like new mailboxes are always created by children.  It's not the biggest deal - they aren't all that big - but it seems like a logical leak unless I'm missing part of it.

Good point, maybe I can trim the deque to the number of child resources when a child dies.

[Antoine Labour]

Created attachment 158932 [details]
Patch

[Antoine Labour]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > > > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:369
> > > > > +        m_mailboxes.append(it->mailbox);
> > > > 
> > > > should we worry about keeping a bound on the # of mailboxes the rootmost compositor gets? it looks like it'll have the set of all mailboxes any child compositors have ever needed. is there any overhead to these on the service side over time? is there any way to discard them?
> > > 
> > > So, first, there's no way to "discard" them. At the same time, they don't hold any resources at all on the GPU process side. They're just signed unguessable unique IDs.
> > > The other thing is, the root compositor sends back the mailbox name when recycling the resources to the child (i.e. it removes it from its deque). Essentially, in steady state, the root compositor will have exactly 1 mailbox name for each resource that was sent to it by children, so it shouldn't grow unbounded (unless it destroys resources without giving them back to a child, but it doesn't do that today).
> > > 
> > 
> > True, but the parent will see many children come and go over the course of its lifetime.  Is there any mechanism by which mailboxes from children that go away are cleared? destroyChild() clears resources, but it doesn't clear mailboxes AFAICT, and it seems like new mailboxes are always created by children.  It's not the biggest deal - they aren't all that big - but it seems like a logical leak unless I'm missing part of it.
> 
> Good point, maybe I can trim the deque to the number of child resources when a child dies.

Done.

[WebKit Review Bot]

Comment on attachment 158932 [details]
Patch

Rejecting attachment 158932 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rceProvider::upload(unsigned int&, uint8_t [4], WebCore::IntRect&, WebCore::IntRect&, WebCore::IntRect&)'
Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:101: note: candidates are: void WebCore::CCResourceProvider::upload(unsigned int, const uint8_t*, const WebCore::IntRect&, const WebCore::IntRect&, const WebCore::IntSize&)
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/CCResourceProviderTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13515594

[Alexandre Elias]

Sorry about that.  Needs rebase on Bug 94154

[Antoine Labour]

Created attachment 158951 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158951 [details]
Patch for landing

Clearing flags on attachment: 158951

Committed r125838: <http://trac.webkit.org/changeset/125838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94283

[Antoine Labour]

Created attachment 158984 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158984 [details]
Patch for landing

Clearing flags on attachment: 158984

Committed r125851: <http://trac.webkit.org/changeset/125851>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.