Bug 96114 – [chromium] Make prioritized texture manager not touch backings array on the main thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Christopher Cameron]

[chromium] Make prioritized texture manager not touch backings array on the main thread

[Christopher Cameron]

Created attachment 162787 [details]
Patch

[Christopher Cameron]

This is another smaller chunk towards bug 95057.

Adding jamesr, epenner, and enne.

[Eric Penner]

Comment on attachment 162787 [details]
Patch

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

I really like this. It also helps break things up into more clear stages IMO, by not needing to keep backings ready for eviction at all times.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:176
>      texture->setAbovePriorityCutoff(true);

I think there might be a subtle issue with removing this. If you requestLate() then this texture is chosen to be allocated in front of all other texture that have the same priority, even though the group of equal-priority textures didn't make the cut (this only really happens when we are totally OOM).

I think this could be handled by making the backing sort use this flag to sort backings correctly.

[Christopher Cameron]

Thanks!

(In reply to comment #3)
> (From update of attachment 162787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162787&action=review
> I think there might be a subtle issue with removing this. If you requestLate() then this texture is chosen to be allocated in front of all other texture that have the same priority, even though the group of equal-priority textures didn't make the cut (this only really happens when we are totally OOM).

You're right -- I'll record the "requestLate" info the CCPrioritizedTexture, use it in the sorting, and verify it in the assert.

[Eric Penner]

(In reply to comment #4)
> Thanks!
> 
> (In reply to comment #3)
> > (From update of attachment 162787 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162787&action=review
> > I think there might be a subtle issue with removing this. If you requestLate() then this texture is chosen to be allocated in front of all other texture that have the same priority, even though the group of equal-priority textures didn't make the cut (this only really happens when we are totally OOM).
> 
> You're right -- I'll record the "requestLate" info the CCPrioritizedTexture, use it in the sorting, and verify it in the assert.

Would it also work to use the "abovePriorityCutoff" flag to distinguish between equal priorities during the backing sort?

[Christopher Cameron]

(In reply to comment #5)
> Would it also work to use the "abovePriorityCutoff" flag to distinguish between equal priorities during the backing sort?

Yeah, that's the way I should go.  I'm adding a unit test to verify this.

[Eric Penner]

Comment on attachment 162787 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:319
>  {

If you have still have a chance, maybe just remove this function and move the code into unregister?

[Christopher Cameron]

(In reply to comment #7)
> (From update of attachment 162787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162787&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:319
> >  {
> 
> If you have still have a chance, maybe just remove this function and move the code into unregister?

For returnBackingTexture (right?), I wanted to do this, but it's also called in CCPrioritizedTexture::setDimensions, so it seemed easier to leave it here.

[Eric Penner]

Comment on attachment 162787 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:319
>>>  {
>> 
>> If you have still have a chance, maybe just remove this function and move the code into unregister?
> 
> For returnBackingTexture (right?), I wanted to do this, but it's also called in CCPrioritizedTexture::setDimensions, so it seemed easier to leave it here.

Ahh! Quite right! Still need this then.

[Christopher Cameron]

Created attachment 162924 [details]
Patch

[Adrienne Walker]

Comment on attachment 162924 [details]
Patch

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

I was a little worried that this patch was going to put us back into the same old "split TextureManager" situation that we were in before (where we had one allocating/deleting class and one managing class), but this is a lot smaller and more elegant.  It's still one managing class, with plenty of asserts to make sure that we're working on data structures on the right thread.  Maybe I'm just a little bit wary of baking too much main thread vs. impl thread logic here when there's impl thread painting in the future.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:384
> +    if (m_needsSortBackings && forceResort) {
> +        prioritizeBackings();
> +        ASSERT(!m_needsSortBackings);
> +    }

I don't follow this.  assertInvariants is for making sure that the sorting is correct.  It seems like you should either only assert when it's sorted or sort every time.  Why do you need to sometimes sort here?

[Christopher Cameron]

Thanks!

(In reply to comment #11)
> I don't follow this.  assertInvariants is for making sure that the sorting is correct.  It seems like you should either only assert when it's sorted or sort every time.  Why do you need to sometimes sort here?

This is idiosyncratic.  I wanted to add code to test the sorting algorithm, and code to verify that things are sorted at various points in the execution.  The flag "forceResort" is only used in a test which verifies the sorting scheme.  The alternative (which may be better) would be to have make prioritizeBackings public, and have the test explicitly call that (which then implicitly verifies the order in assertInvariants).  Would that be more straightforward.  The only downside of exposing a (basically) internal function just for testing.

[Eric Penner]

(In reply to comment #12)
> Thanks!
> 
> (In reply to comment #11)
> > I don't follow this.  assertInvariants is for making sure that the sorting is correct.  It seems like you should either only assert when it's sorted or sort every time.  Why do you need to sometimes sort here?
> 
> This is idiosyncratic.  I wanted to add code to test the sorting algorithm, and code to verify that things are sorted at various points in the execution.  The flag "forceResort" is only used in a test which verifies the sorting scheme.  The alternative (which may be better) would be to have make prioritizeBackings public, and have the test explicitly call that (which then implicitly verifies the order in assertInvariants).  Would that be more straightforward.  The only downside of exposing a (basically) internal function just for testing.

An another option (could go either way really) we could always make the sort explicit in the commit and remove the lazy sort flag. It would make it harder to accidentally not sort IMO, but also reduces flexibility a bit I guess.

[Christopher Cameron]

(In reply to comment #13)
> (In reply to comment #12)
> An another option (could go either way really) we could always make the sort explicit in the commit and remove the lazy sort flag. It would make it harder to accidentally not sort IMO, but also reduces flexibility a bit I guess.

I liked this idea too, but it kept on blowing up when I had the sequence

  main thread               | impl thread
  -----------------------------------------------------------------
  beginFrame                |
  - prioritizeTextures [A]  |
  - (block on impl thread)  | beginFrameCompleteOnImplThread
                            | - prioritizeBackings [B]
                            | - un-block main thread
  beginFrame                |
  - prioritizeTextures [C]  |
  - (block on impl thread)  | scheduledActionUpdateMoreResources
                            | - acquireBackingIfNeeded (**)

At the point of the (**), the backings vector is sorted based on the priorities at point [A], but when we check the backings' orders, they are now out-of-order with respect to the priorities at point [C].  And note that at step [C], we may have unlinked backings from textures, so the inconsistency is not just in the priority values.

Now, we will need to (very soon) push the priority values to the backings themselves, in order to delete only-some textures on the impl thread.  So one solution is to push this data to the impl thread at the "prioritizeBackings" phase.

The down-side to this is that it opens up the possibility for a performance regression: As of now, the PTM can, by virtue of this race, recycle the backing for a texture that it knows will be deleted in the next frame that will be committed.  With this change, the PTM would lose this ability.  As it happens, this happens a lot every time we load a page (in particular, for a simple page, we hit the recycle queue 42 times on page load with this behavior, and 31 times if we regress this behavior).

Anyway, I've put together a patch which
- pushes the priorities to the PTM at only commit time in the ordinary run of things
- re-pushes the priorities when we fail to find a hit in the recycle pool *and* the the main thread has returned some backing(s) to the recycle pool since the last push

I'll attach the patch when it's cleaned up enough.  Of note is that we're (necessarily) pushing more into the "separate main and impl" territory that enne mentioned (and that will become unnecessary when we have impl-side painting).

[Adrienne Walker]

(In reply to comment #12)
> Thanks!
> 
> (In reply to comment #11)
> > I don't follow this.  assertInvariants is for making sure that the sorting is correct.  It seems like you should either only assert when it's sorted or sort every time.  Why do you need to sometimes sort here?
> 
> This is idiosyncratic.  I wanted to add code to test the sorting algorithm, and code to verify that things are sorted at various points in the execution.  The flag "forceResort" is only used in a test which verifies the sorting scheme.  The alternative (which may be better) would be to have make prioritizeBackings public, and have the test explicitly call that (which then implicitly verifies the order in assertInvariants).  Would that be more straightforward.  The only downside of exposing a (basically) internal function just for testing.

I think if you want to test the sorting algorithm, then you should write some tests that test whatever cases you're worried about explicitly.

I think the other thing that worries me is assertInvariants(true) modifies state, so debug builds now have different behavior than release builds.  So, if there's some point where we need to have sorted backings but haven't, then assertInvariants(true) might mask that.

It's also just hard to look at these assertInvariants(true) and assertInvariants(false) from a code reading perspective and have any easy understanding of what value needs to be passed when.

I like the patch other than that bit.

[Christopher Cameron]

Created attachment 163177 [details]
Patch

[Christopher Cameron]

(In reply to comment #15)
> I think the other thing that worries me is assertInvariants(true) modifies state, so debug builds now have different behavior than release builds.  So, if there's some point where we need to have sorted backings but haven't, then assertInvariants(true) might mask that.

Very very good point.  I updated the patch to mirror the priority state on the Backing structures, and made the impl thread operate on only that state (instead of the main thread), and as a consequence, the assertInvariants argument went away.

[Eric Penner]

Comment on attachment 163177 [details]
Patch

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

I know we'll need to copy priorities to backings so that's good, but I think it would be ideal if we could limit things such that we can think of everything as always being 'up-to-date' rather than thinking about 'AtLastPriorityUpdate' values as that introduces thinking about threading etc again.

Is there anything wrong with just keeping the old lazy scheme and sorting/copying priorities over during the lazy sort step? Those values should always be up-to-date. My first preference would be to just insure sortBackings is *always* called externally when it needs to be called (after any main thread involvement), but your case above illustrates that's not as simple as I thought.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:225
> +    ASSERT(CCProxy::isImplThread() && CCProxy::isMainThreadBlocked());

Are the backings possibly not fully sorted when this is called? Could this miss available backings if they were high priority but were since returned?

In this sense I preferred the old lazy scheme which always detected and kept the backings sorted, or in alternate just insuring sortBackings is called everywhere it needs to be called so this would not be needed.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:108
> +        if (priorityA == priorityB) {

Minor nit, Google usually prefers minimum nesting with early returns, eg:
if (priorityA != priorityB)
   return priorityIsLower(priorityA, priorityB)
if (aboveCuffoffA != aboveCutoffB)
   return aboveCutoffB;
return a < b;

[Christopher Cameron]

Comment on attachment 163177 [details]
Patch

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

Thanks!  Patch update incoming.

So I have a preference the non-lazy scheme, since it makes somewhat clear when the backings' priorities will be updated in the commit flow, and it was also easier to unit-test (I could do the explicit prioritize and then test the output).  If you feel strongly about going back to the fully-lazy scheme, I can change it back.

All of that said, I'm somewhat curious why we create-then-delete like 12 textures when loading a simple page (maybe if we make that behavior not the norm we can get rid of the aggressive intra-frame recycling).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:225
>> +    ASSERT(CCProxy::isImplThread() && CCProxy::isMainThreadBlocked());
> 
> Are the backings possibly not fully sorted when this is called? Could this miss available backings if they were high priority but were since returned?
> 
> In this sense I preferred the old lazy scheme which always detected and kept the backings sorted, or in alternate just insuring sortBackings is called everywhere it needs to be called so this would not be needed.

The backings are guaranteed to be sorted at all times, however, their sorting may be with respect to the last snapshot, but it may be that the main thread has made some changes to the priorities that have not yet been committed.

In acquireBackingTextureIfNeeded, we call this twice -- if the first call doesn't get us a backing and the main thread has since returned some backings, we re-pull the priorities from the textures (to avoid that regression).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:108
>> +        if (priorityA == priorityB) {
> 
> Minor nit, Google usually prefers minimum nesting with early returns, eg:
> if (priorityA != priorityB)
>    return priorityIsLower(priorityA, priorityB)
> if (aboveCuffoffA != aboveCutoffB)
>    return aboveCutoffB;
> return a < b;

Fixed.

[Christopher Cameron]

Created attachment 163189 [details]
Patch

[Eric Penner]

(In reply to comment #19)
> The backings are guaranteed to be sorted at all times, however, their sorting may be with respect to the last snapshot, but it may be that the main thread has made some changes to the priorities that have not yet been committed.

The reason I still prefer the lazy scheme is that I don't see any reason why we couldn't just sort the backings again in this case. If the main thread changed priorities then it must have also called prioritizeTextures(), and it would never change any priorities further after that, so we it seems like we should definitely sort the backings.

If we did go back to the lazy scheme, would it be even easier if the 'needsBackingSort' flag was set during PrioritizeTextures and unset during PrioritizeBackings? Then we could add ASSERT(!needsBackingSort) to make sure the main thread doesn't change anything between the two sorting steps, and also insure that either a.) sortBackings is called externally always, or b.) sortBackingsIfNeeded is always called first internally.

[Christopher Cameron]

Created attachment 163246 [details]
Patch

[Christopher Cameron]

(In reply to comment #21)
> (In reply to comment #19)
> I still prefer the lazy scheme

So I went ahead and re-implemented the lazy scheme on top of the new priority pulling logic, and I agree with you -- it's much easier to reason about.

[Eric Penner]

Comment on attachment 163246 [details]
Patch

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

LGTM from my perspective. Enne?  I hope I haven't advocated anything you disagree with! :)

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:111
>          CCPrioritizedTexture* owner() { return m_owner; }

Up to you, but I would just call these hasOwner()/requestPriority()/abovePriorityCutoff() since there is only one of these we need to care about in this class.

[Eric Penner]

Comment on attachment 163246 [details]
Patch

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

> Source/WebKit/chromium/tests/CCPrioritizedTextureTest.cpp:521
> +    DebugScopedSetImplThreadAndMainThreadBlocked implThreadAndMainThreadBlocked;

I'm confused as to why this is passing. Shouldn't the backings be unsorted due to requestLate, and result in an ASSERT below because they aren't sorted?

Hmm, perhaps the backing list is out of date but still sorted in the old valid order (since certain values are copied to backings now). In any case I think we need to tweak this so it fails, or test this another way.

[Christopher Cameron]

Comment on attachment 163246 [details]
Patch

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

>> Source/WebKit/chromium/tests/CCPrioritizedTextureTest.cpp:521
>> +    DebugScopedSetImplThreadAndMainThreadBlocked implThreadAndMainThreadBlocked;
> 
> I'm confused as to why this is passing. Shouldn't the backings be unsorted due to requestLate, and result in an ASSERT below because they aren't sorted?
> 
> Hmm, perhaps the backing list is out of date but still sorted in the old valid order (since certain values are copied to backings now). In any case I think we need to tweak this so it fails, or test this another way.

Ah, so this wasn't actually testing anything!  The backings are out-of-date (and therefore still sorted)!

I updated the test to sort the backings explicitly, and the verify that the textures' backings have the right "isAbovePriorityThreshold".  In order to do this, I made CCPrioritizedTextureTest be a friend of the CCPTM and CCPT classes, so it can reach in and inspect this state.  I also made some functions (assertInvariants and updateBackingsPriorities private again).

[Christopher Cameron]

Created attachment 163272 [details]
Patch

[James Robinson]

Comment on attachment 163272 [details]
Patch

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

This looks really nice to me.

I think one tradeoff to keep in mind in general with these classes is that they seem to always end up being fairly deeply aware of our commit flow, our threaded use of the class, or both.  I think being aware of the commit flow - i.e. what things happen in what order from what threads - tends to be the least intrusive of all of these.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:397
> +        if ((*it)->hadOwnerAtLastPriorityUpdate())

I think putting (*it) in an appropriately-named-and-typed temporary would be a big readability win for this section

[Christopher Cameron]

Comment on attachment 163272 [details]
Patch

Thanks!  I'll make a separate pass changing more of the (*it) to CCPrioritizedTexture::Backing*s or CCPrioritizedTexture*s (as it is, the style of the file is operating on the iterators)

[WebKit Review Bot]

Comment on attachment 163272 [details]
Patch

Clearing flags on attachment: 163272

Committed r128253: <http://trac.webkit.org/changeset/128253>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.