Bug 94751 – [chromium] Use an opaque type for texture priority

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Christopher Cameron]

[chromium] Use an opaque type for texture priority

[Christopher Cameron]

Created attachment 160018 [details]
Patch

[Christopher Cameron]

Adding epenner to review prioritized texture manager changes.

Adding enne as reviewer.

[WebKit Review Bot]

Comment on attachment 160018 [details]
Patch

Attachment 160018 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13558459

[Peter Beverloo (cr-android ews)]

Comment on attachment 160018 [details]
Patch

Attachment 160018 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13574153

[Christopher Cameron]

Created attachment 160031 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160031 [details]
Patch

Attachment 160031 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13570434

[Peter Beverloo (cr-android ews)]

Comment on attachment 160031 [details]
Patch

Attachment 160031 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13568432

[Christopher Cameron]

Created attachment 160056 [details]
Patch

[Eric Penner]

Comment on attachment 160056 [details]
Patch

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

LGTM in general. Although I'm curious what else will go inside the CCPriority.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:-131
> -    size_t m_priority;

Yikes, I'm surprised there isn't a warning somewhere for this. Should have all been changed to int.

[Christopher Cameron]

(In reply to comment #9)
> (From update of attachment 160056 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160056&action=review
> 
> Although I'm curious what else will go inside the CCPriority.

In the short term, I plan to store information about "this is a known distance away from the viewport", so I can evict those textures that are >2-3 screens away from the viewport, without evicting the lingering textures first (but not otherwise altering the scoring scheme).  

Long term, when we have known distances for everything, this can probably go back to just having an int in it.

[Adrienne Walker]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 160056 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=160056&action=review
> > 
> > Although I'm curious what else will go inside the CCPriority.
> 
> In the short term, I plan to store information about "this is a known distance away from the viewport", so I can evict those textures that are >2-3 screens away from the viewport, without evicting the lingering textures first (but not otherwise altering the scoring scheme).

Evicting textures that are 2-3 screens away seems like it could already be done by issuing a smaller texture budget and leaning on the prioritized texture manager to make the decision about which textures should be thrown away or not.

Why do you need a separate mechanism for this?  It seem like if you add additional information that can be used to prioritize textures, then the texture manager should be the one using it.

[Eric Penner]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 160056 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=160056&action=review
> > > 
> > > Although I'm curious what else will go inside the CCPriority.
> > 
> > In the short term, I plan to store information about "this is a known distance away from the viewport", so I can evict those textures that are >2-3 screens away from the viewport, without evicting the lingering textures first (but not otherwise altering the scoring scheme).
> 
> Evicting textures that are 2-3 screens away seems like it could already be done by issuing a smaller texture budget and leaning on the prioritized texture manager to make the decision about which textures should be thrown away or not.
> 
> Why do you need a separate mechanism for this?  It seem like if you add additional information that can be used to prioritize textures, then the texture manager should be the one using it.

Is this perhaps in the context of the impl thread doing some management, since that's where GPU memory manager limits arrive via the graphics context on that thread?

[Christopher Cameron]

(In reply to comment #12)
> (In reply to comment #11)

> > Why do you need a separate mechanism for this?  It seem like if you add additional information that can be used to prioritize textures, then the texture manager should be the one using it.
> 
> Is this perhaps in the context of the impl thread doing some management, since that's where GPU memory manager limits arrive via the graphics context on that thread?

That's exactly it.  This is so that the impl thread can evict "some but not all" textures.  The impl thread will need to guess which textures the main thread would have deleted (were the main thread to have made the choice), since it doesn't have direct access to the prioritized texture manager.

[Adrienne Walker]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> 
> > > Why do you need a separate mechanism for this?  It seem like if you add additional information that can be used to prioritize textures, then the texture manager should be the one using it.
> > 
> > Is this perhaps in the context of the impl thread doing some management, since that's where GPU memory manager limits arrive via the graphics context on that thread?
> 
> That's exactly it.  This is so that the impl thread can evict "some but not all" textures.  The impl thread will need to guess which textures the main thread would have deleted (were the main thread to have made the choice), since it doesn't have direct access to the prioritized texture manager.

Adding additional data to priority isn't going to help the compositor thread.  Priority is a main-thread only concept, at this point.

What about making the texture manager thread-safe and accessible by both threads? I feel like this need has come up before (removing just some textures without a commit) and I suspect impl-side painting is going to need to touch on this as well.

[Dana Jansens]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > 
> > > > Why do you need a separate mechanism for this?  It seem like if you add additional information that can be used to prioritize textures, then the texture manager should be the one using it.
> > > 
> > > Is this perhaps in the context of the impl thread doing some management, since that's where GPU memory manager limits arrive via the graphics context on that thread?
> > 
> > That's exactly it.  This is so that the impl thread can evict "some but not all" textures.  The impl thread will need to guess which textures the main thread would have deleted (were the main thread to have made the choice), since it doesn't have direct access to the prioritized texture manager.
> 
> Adding additional data to priority isn't going to help the compositor thread.  Priority is a main-thread only concept, at this point.
> 
> What about making the texture manager thread-safe and accessible by both threads? I feel like this need has come up before (removing just some textures without a commit) and I suspect impl-side painting is going to need to touch on this as well.

Main thread is blocked when the PTM sorts all the textures by priority. This sorted list shouldn't be modified when the main thread isn't blocked, IIRC? So impl should be able to just read that list and drop the top textures from it until satisfied. And main thread should be able to do the same later and make the same choices?

[Adrienne Walker]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> >
> > What about making the texture manager thread-safe and accessible by both threads? I feel like this need has come up before (removing just some textures without a commit) and I suspect impl-side painting is going to need to touch on this as well.
> 
> Main thread is blocked when the PTM sorts all the textures by priority. This sorted list shouldn't be modified when the main thread isn't blocked, IIRC? So impl should be able to just read that list and drop the top textures from it until satisfied. And main thread should be able to do the same later and make the same choices?

Well, it's more than that, though.  I think the impl thread is eventually going to need to be able to reprioritize on its own somehow.  What if you've scrolled a whole bunch while the main thread was churning on Javascript and you suddenly need to toss out some textures?

[Christopher Cameron]

(In reply to comment #16)

> Well, it's more than that, though.  I think the impl thread is eventually going to need to be able to reprioritize on its own somehow.  What if you've scrolled a whole bunch while the main thread was churning on Javascript and you suddenly need to toss out some textures?

I agree with this, and would say that the main thread should eventually have no interaction with the PTM at all.

At the moment, the main thread's interaction with the PTM is that it
1. Sets priorities based on the viewport
2. Decides what it needs to re-paint (it checks to see if the backing has been destroyed).

For (1), the main thread isn't competent to do this (as enne pointed out)
For (2), I'd imagine that impl-side painting would move this off of the main thread (and of course, then, it doesn't have to worry about not knowing what textures the impl thread has destroyed).

[Dana Jansens]

(In reply to comment #17)
> For (2), I'd imagine that impl-side painting would move this off of the main thread (and of course, then, it doesn't have to worry about not knowing what textures the impl thread has destroyed).

Well, then it will be an update thread instead of main thread, there are the same/similar cross thread issues to deal with though?

[Christopher Cameron]

All of that said, my plan here was to make it be the case that when the state of a tab is changed, this changes the actual priorities of the various textures.

> In the short term, I plan to store information about "this is a known distance away from the viewport", so I can evict those textures that are >2-3 screens away from the viewport, without evicting the lingering textures first (but not otherwise altering the scoring scheme).  

To think about this more... that isn't a good idea -- I should change the scoring scheme now (in particular, make lingering resources be more-important-priority than all offscreen resources, so that I can just adjust the cutoff to effect changes in policy).

So I'll just abandon this patch (unless having an opaque type is considered good for its own sake, which maybe it is...).

[Christopher Cameron]

(In reply to comment #18)
> (In reply to comment #17)
> > For (2), I'd imagine that impl-side painting would move this off of the main thread (and of course, then, it doesn't have to worry about not knowing what textures the impl thread has destroyed).
> 
> Well, then it will be an update thread instead of main thread, there are the same/similar cross thread issues to deal with though?

The big difficulty is in issue (1), where the priorities can be changed by the main thread.

True, issue (2) still exists, but it is not nearly as hard a problem as (1) -- the amount of state that needs to be shared is fairly simple and localized (the impl thread needs to know "what things are other threads currently painting", and the update thread needs to know "don't grab more work for just a second because a reprioritization is in progress").

This all comes with the caveat that I'm not up to speed on impl-side painting.

[Nat Duca]

(In reply to comment #20)

If we have to make a priority call beteween doing things an impl-side-painting sort of way and alternatives that dont enable it, I think we should favor impl-side painting. Its very important that we make that happen.

If I understand right, this work is to allow memory limits be adjustable. This is a feature-level goal, but since we've got a good solution shipping now, it may be that moving impl-side painting is higher priority than making memory limits adjustable in an already working system.

[Christopher Cameron]

Closing this out, since I'll go about this a different way.  Thanks for the feedback!

[Eric Penner]

(In reply to comment #22)
> Closing this out, since I'll go about this a different way.  Thanks for the feedback!

Since I already wrote this, here goes:

I think the only case where multi-threading might help impl-side painting would be when we have many paint jobs in queue, and don't want to pre-allocate textures for all those jobs. So we might want a way to do 'acquireTexture()' and steal textures as late as possible on a paint thread (this also requires zero-copy multi-threaded access to write to the textures). I think ideally though most of it should stay on the impl thread. It gets very confusing if, for example, priorities are set or textures are prioritized on other threads, or something like that.

For partially managing textures on impl thread, that's tricky and not sure what the best solution is there... Best I can think of would be to have flags on each texture (eg 'visible') that are pushed to the impl thread, such that we can delete some textures instead of all of them (and then notify similar to what we do now via 'allBackingTexturesWereDeleted').

[Christopher Cameron]

(In reply to comment #23)

> For partially managing textures on impl thread, that's tricky and not sure what the best solution is there... Best I can think of would be to have flags on each texture (eg 'visible') that are pushed to the impl thread

This was what I was thinking with this patch -- the flag would be stored somewhere in the opaque structure and then sent to the impl thread.

Then Dana pointed out that the priorities are only updated and sorted when the main and impl threads are at their sync-point.  Because this is the case, I can safely read through the priority list in the impl thread to decide what to kill.

[Eric Seidel (no email)]

Comment on attachment 160056 [details]
Patch

Cleared review? from attachment 160056 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).