Bug 84308 – [chromium] Adding PrioritizedTexture and replacing ContentsTextureManager

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

[Eric Penner]

[chromium] TextureManager V2

[Eric Penner]

Created attachment 137810 [details]
Patch

[Dana Jansens]

Comment on attachment 137810 [details]
Patch

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

Woot! Some questions below as I try to grok all this. Will give it another read tomorrow.

> Source/WebCore/ChangeLog:5
> +

Needs descriptive text.

> Source/WebCore/ChangeLog:21
> +        (WebCore::TexturePriorityManager::TexturePriorityManager):

As a bikeshed, I'd say this would be better named PrioritizedTextureManager, since it's a manager of textures (via priorities), not a manager of texture priorities.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:175
> +        memoryBytes += (*it)->memorySizeBytes();

I think I'd rather "memoryBytes = newMemoryBytes" so we don't have to worry about computing it differently in two places by accident, even tho its pretty simple right now.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:43
> +class PrioritizedTexture {

why not in its own file like ManagedTexture?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:84
> +    // This texture is available for use and will stay available at least until textures
> +    // are prioritized again. This guarantees we will be able to update/use this texture
> +    // at least once before it is taken away.
> +    bool isAvailable() const { return m_isAvailable; }
> +
> +    // After a texture becomes available, it is dirty. This acknoledges that
> +    // this texture is dirty and any old updates are lost.
> +    // isAvailable() must be true to call this.
> +    void resetToValid();
> +
> +    // Is the texture still valid since resetToValid was called (still has the same size, format and data).
> +    // This might be true even if isAvailable() is false. In this case the texture
> +    // shouldn't be updated because it's allocation could get taken away before it is uploaded/used.
> +    bool isStillValid() const;
> +    bool isStillValidAs(IntSize, unsigned format) const;

It seems like this all wraps what ManagedTexture::isValid does, but its split up to support recycling? When would you want to check isStillValid() but not need isAvailable() to be true too?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:152
> +class TexturePriorityManager {

I would really like if this could subclass TextureManager. Or maybe there was some common interface they could both inherit? It would be nice if the code did not need to know which manager your were using when not dealing explicitly with priorities. Do you think that's possible or is it an inherent part of using this manager than you always be aware it is prioritized?

It would likely mean the texture would share some interface with ManagedTexture too if we could do this. Can we reuse any of the ManagedTexture logic, just wrapping the reserve/evict decisions in priorities?

[Eric Penner]

Sorry, didn't know this was view-able yet. I'll update the description etc. shortly.

[Dana Jansens]

Ohh, marking R? throws it at EWS which CCs people :)

[Eric Penner]

Comment on attachment 137810 [details]
Patch

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

>> Source/WebCore/ChangeLog:5
>> +
> 
> Needs descriptive text.

Will add.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:175
>> +        memoryBytes += (*it)->memorySizeBytes();
> 
> I think I'd rather "memoryBytes = newMemoryBytes" so we don't have to worry about computing it differently in two places by accident, even tho its pretty simple right now.

Will do.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:43
>> +class PrioritizedTexture {
> 
> why not in its own file like ManagedTexture?

I viewed it as more of an inner class co-dependent with the manager (unlike the original TextureManager which could live on it's own without ManagedTexture), but it was too big to actually declare inside TexturePriorityManager. I can move it to a new file though if you think that's better.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:84
>> +    bool isStillValidAs(IntSize, unsigned format) const;
> 
> It seems like this all wraps what ManagedTexture::isValid does, but its split up to support recycling? When would you want to check isStillValid() but not need isAvailable() to be true too?

Yeah that really needs a better name. isAvailable was originally isAbovePriorityThreshold() and maybe I should change it back.

Basically isAvailable means you can write to the texture (similar to reserve/protect returning true). 
isStillValid means you still have the last contents you wrote in there (although it might get evicted before you can paint/upload it again).

ResetToValid and Allocate can also become one I think. We just need some signal that the client understands the texture was dirty. I will think this over more and update the doc so we can finalize the interface.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:152
>> +class TexturePriorityManager {
> 
> I would really like if this could subclass TextureManager. Or maybe there was some common interface they could both inherit? It would be nice if the code did not need to know which manager your were using when not dealing explicitly with priorities. Do you think that's possible or is it an inherent part of using this manager than you always be aware it is prioritized?
> 
> It would likely mean the texture would share some interface with ManagedTexture too if we could do this. Can we reuse any of the ManagedTexture logic, just wrapping the reserve/evict decisions in priorities?

I was thinking that this is really just an extension of the old texture manager to support priorities, except in a different file so we can replace the old one over a few CLs.

So my thinking would be if we need any functionality from the old TextureManager we just add it here. Then we can remove references to the old one until it is gone.

[Eric Penner]

Comment on attachment 137810 [details]
Patch

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

>>> Source/WebCore/ChangeLog:5
>>> +
>> 
>> Needs descriptive text.
> 
> Will add.

I'll add a description, but here's how the design evolved so far:

Basic Idea:

The basic idea is that both the texture requests and the allocations are sorted. Textures are sorted so they can be prioritized.  Allocations are sorted so that they can be recycled/evicted in the right order (lowest priority first). Prioritizing textures doesn't fully evict the old textures and allocate the new ones, it just marks which textures are *allowed* to be allocated. Low priority textures aren't immediately evicted, but rather keep their allocation until a high priority texture comes and takes it or it is evicted by memory reductions.

Data structures and pointers vs tokens/handles:

I initially used a simple HashSet and sorted the textures in prioritizeTextures. However I switched to std::set (ordered) as we might want to be able to lower priorities dynamically later on.  It's easy enough to switch back to using HashSet/Vector though. 

I also initially used handles/tokens like the original texture manager, but since there is now two things to track and keep sorted, I found it easier to link textures with allocations via pointers.

[Dana Jansens]

ok cool thanks for the explanations!

[Eric Penner]

Created attachment 137938 [details]
Patch

[Eric Penner]

Created attachment 139248 [details]
Patch

[Dana Jansens]

Comment on attachment 139248 [details]
Patch

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

These are the things that I was able to fix. But there's some things that are less trivial and seem like design problems.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:100
> +    // FIXME: This texture was previously always requested in ::Update which was likely
> +    // only called based on visibility. So this is wrong and we need to detect visibility.

nit: extra period at eol and update() lowercase

This wasn't based on visibility, but it should be!

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:232
> +    void setTexturePriorities(const PriorityCalculator&) { }

virtual

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:187
> +    if (m_currentAllocation)
> +        m_isValid = false;
> +    m_currentAllocation = allocation;

So if you set a new allocation, you would have a new texture id, but the contents are still valid?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:304
> +bool PrioritizedTextureManager::reserve(PrioritizedTexture* texture, bool useSpareMemory)

Why does useSpareMemory need to exist? Shouldn't prioritization already solve this issue?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:307
> +        return true;

When this function returns true, it should set m_setValid on the texture to true, so that the webkit-side code knows the texture should be pushed to impl.

allocate() is done on impl, and should not have anything to do with isValid, if it is supposed to mean the same thing as it did for ManagedTextures. All the unchanged. use of isValidAs() seems to suggest that it should mean the same.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:431
> +    texture->setIsPriority(false);

texture->setIsValid(false) here as well, since isValid is not linked to being allocated, but instead linked to being reserved by the texture manager.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:135
> +class TextureAllocation {

I'd like to have this as a nested class PrioritizedTexture::Allocation.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:495
> +    const IntRect& visibleRect = visibleLayerRect();
> +    if (visibleRect.isEmpty() || m_tiler->hasEmptyBounds())
>          return;
> +    IntRect priorityRect = idlePaintRect(visibleRect);

This should return if priorityRect is empty instead of visibleRect. idlePaintRect() checks if animating and returns something non-empty even if visibleLayerRect() is empty in that case.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:60
> +    virtual void setTexturePriorities(const PriorityCalculator&) OVERRIDE;

Other methods on TiledLayerChromium such as updateLayer() and needsIdlePaint() take a layer rect, and it's inconsistent, and causing issues in the tests, that this does not take the visibleLayerRect as a parameter, and uses the value on the class object instead.

I think we should make this call a helper method on the class that takes a layerRect. It will make tests more clear/consistent at the least. I feel like it's really weird to do the following, and have it only work because the layer's visibleLayerRect == visibleRect given to needsIdlePaint:

layer->setTexturePriorities(priorityCalculator);
textureManager->prioritizeTextures();
EXPECT_EQ(shouldPrepaint, layer->needsIdlePaint(visibleRect));

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-241
> -    m_contentsTextureManager->unprotectAllTextures();

From one frame to the next, if a texture is no longer used, its priority should get demoted shouldn't it? Should it do contentsTextureManager->clearPriorities() here? Or is it always enough to do this in updateLayers()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:472
> +    prioritizeTextures(updateList);

This gets undone by the ContentLayerChromium::update() calling updateTileSizeAndTilingOption(). We need to do this updateTileSize business when we set priorities instead. Same for ImageLayerChromium. We should probably add ContentLayerChromium::setTexturePriorities() and have it call this, and do the same for ImageLayerChromium.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:309
>  

To exhibit the old behaviour of this test, we need to start with enough memory that it expects to be able to prepaint, then reduce the memory limits between frames, here, so that the idle paint here is unable to paint.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:454
> +    layer1->setVisibleLayerRect(IntRect(0, 0, 100, 200));
> +    layer1->setTexturePriorities(priorityCalculator);

layer2->

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:676
> +            layer->setTexturePriorities(priorityCalculator);

Since the tiles don't actually get painted, we drop the Tile objects for them in pushPropertiesTo(). This makes needsIdlePaint ASSERT because it expects all tiles to exist. We need to setTexturePriorities() and prioritizeTextures() again after the pushPropertiesTo.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-830
> -    textureManager->unprotectAllTextures(); // CCLayerTreeHost::commitComplete() normally does this, but since we're mocking out the manager we have to do it.

Should textureManager->clearPriorities() here. The child still has priorities on its textures. We should probably clearPriorities() before doing prioritizeTextures() always..

In this case it's passing because the two layers are getting the same priorities I believe. But if the child had a higher priority, then I think this would/should fail when not clearing priority on the child.

[Dana Jansens]

Created attachment 139724 [details]
Patch

Addresses my own comments from above, but there's some stuff unresolved.. and maybe this isn't quite right.

[Dana Jansens]

Comment on attachment 139724 [details]
Patch

Here's the tests that are failing with my changes:

[  FAILED  ] 9 tests, listed below:
[  FAILED  ] PrioritizedTextureTest.requestTextureInPreferredLimit
[  FAILED  ] PrioritizedTextureTest.requestTextureExceedingPreferredLimit
[  FAILED  ] PrioritizedTextureTest.requestTextureExceedingMaxLimit
[  FAILED  ] PrioritizedTextureTest.changeMemoryLimits
[  FAILED  ] PrioritizedTextureTest.textureManagerPartialUpdateTextures
[  FAILED  ] PrioritizedTextureTest.textureManagerPrioritiesAreEqual
[  FAILED  ] CCLayerTreeHostTestAtomicCommit.runMultiThread
[  FAILED  ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
[  FAILED  ] Canvas2DLayerChromiumTest.testFullLifecycleThreaded

The CCLayerTreeHostTestAtomicCommit tests are failing because when we steal a tile for the 2nd frame, it does not stay in the context. I think this has to do with the allocations. Also the tests are flakey and I will upload another patch that makes them less-so, so you can at least run them deterministically in gdb. And I'll fix them in another CL also since it's just wrong.

The PrioritizedTextureTests are failing.. at least in part.. due to the whole reserve() vs allocate() vs isValid confusion, which is the root of the CCLTH test fails I think as well. I will try to explain what I see as problematic.

Old code would do reserve() on main thread, and that would reserve a chunk of the memory limit. From then, the texture is considered "valid" and we would queue it for upload etc and push the texture id over to impl.

New code would do reserve() and that would change the memory value but not mark the texture as valid - it was expected to be allocate()d to become "valid". So no textures would get pushed to impl since they are not valid! It assumed this meant they weren't reserved either. isPriorityAs might be the correct check on main thread instead if that is how it is supposed to behave.

I assumed that reserve() should mark the texture "isValid = true" for main thread. It can treat the texture as being for real and from there. But then the PrioritizedTextureTests don't do the right things anymore because they are expecting valid to be linked to allocate(). Now that I write this, I think maybe I should have changed all main thread isValidAs() checks into isPriorityAs() checks instead?

Stealing is also not working for the CCLTH tests. When you steal a texture in the old code, it would move the m_token to the new texture.
In the new code, stealFrom does not set m_isValid on the newly created texture, nor does it set isPriority. The texture gets dropped from the layer, so its priority will be reset anyhow, but in reality I think it should have the same (or higher?) priority in that state - it's in use by the impl thread and we don't want to drop it.

It seems wrong that stealing a texture on the main thread is changing allocations - since allocations are something done on the impl thread. It feels like the main thread should just set appropriate state/hints and the impl thread will allocate appropriately.. maybe TextureAllocations isn't the right notion? It should be "virtual allocations" kinda? I'm not sure here what is the right thing to do.

What do you think of that all?

[Dana Jansens]

Created attachment 139728 [details]
Patch

[Dana Jansens]

Comment on attachment 139728 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:515
> +            unsigned priority;
> +            if (visibleLayerRect.isEmpty())
> +                priority = priorityCalc.priorityFromDistance(0);
> +            else
> +                priority = priorityCalc.priorityFromDistance(visibleLayerRect, m_tiler->tileBounds(i, j));
> +            tile->managedTexture()->setPriority(priority);

Wondering if this shouldn't be deciding whether to partial update or double-buffer update here. If we double-buffer then this tile will basically have 2 full-sized textures until commit completes.

[Eric Penner]

Comment on attachment 139248 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:187
>> +    m_currentAllocation = allocation;
> 
> So if you set a new allocation, you would have a new texture id, but the contents are still valid?

Intent is to always set valid to false when the allocation changes.  Is something here not expected?

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:304
>> +bool PrioritizedTextureManager::reserve(PrioritizedTexture* texture, bool useSpareMemory)
> 
> Why does useSpareMemory need to exist? Shouldn't prioritization already solve this issue?

Prioritization still doesn't solve the case where we are close to OOM and can't even fit visible tiles into memory. By setting all visible tiles at the same priority, that priority will all fail to pass the threshold and we can fall back on the current call-order behavior (with occlusions etc to reduce memory usage and maybe still avoid OOM).  

I also liked not needing to add layer ordering into the priorities since that is only really needed for visible tiles, and even then it's not enough to avoid early OOM when occlusions are present.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:307
>> +        return true;
> 
> When this function returns true, it should set m_setValid on the texture to true, so that the webkit-side code knows the texture should be pushed to impl.
> 
> allocate() is done on impl, and should not have anything to do with isValid, if it is supposed to mean the same thing as it did for ManagedTextures. All the unchanged. use of isValidAs() seems to suggest that it should mean the same.

I intended for the semantics to change slightly and for textures to have isPriority to indicate they can/should be updated, and isValid after the texture is updated and has valid content. The reason we need two flags is that the texture could briefly become isPriority==false and then true again later, unlike in the old texture manager. 

This was the cause of many failing unit tests as well but I still felt it made more sense that way since the update is when the texture actually gets valid content.  I fixed the TiledLayer unit tests by insuring the updater runs before pushing tiles.   I thought chrome behaves similar and this would be okay, but possibly I'm missing something here, in which case let's discuss further.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:431
>> +    texture->setIsPriority(false);
> 
> texture->setIsValid(false) here as well, since isValid is not linked to being allocated, but instead linked to being reserved by the texture manager.

I think that ends up happening already via unlink. But putting it here is clear.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:135
>> +class TextureAllocation {
> 
> I'd like to have this as a nested class PrioritizedTexture::Allocation.

SGTM

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:495
>> +    IntRect priorityRect = idlePaintRect(visibleRect);
> 
> This should return if priorityRect is empty instead of visibleRect. idlePaintRect() checks if animating and returns something non-empty even if visibleLayerRect() is empty in that case.

SGTM.  Or we can pass the rect like you mentioned in your other comment.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:60
>> +    virtual void setTexturePriorities(const PriorityCalculator&) OVERRIDE;
> 
> Other methods on TiledLayerChromium such as updateLayer() and needsIdlePaint() take a layer rect, and it's inconsistent, and causing issues in the tests, that this does not take the visibleLayerRect as a parameter, and uses the value on the class object instead.
> 
> I think we should make this call a helper method on the class that takes a layerRect. It will make tests more clear/consistent at the least. I feel like it's really weird to do the following, and have it only work because the layer's visibleLayerRect == visibleRect given to needsIdlePaint:
> 
> layer->setTexturePriorities(priorityCalculator);
> textureManager->prioritizeTextures();
> EXPECT_EQ(shouldPrepaint, layer->needsIdlePaint(visibleRect));

SGTM. That was indeed causing problems for tests. I was fixing them by setting the visible rect, but it's odd if we pass the rect sometimes but not others.

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:676
>> +            layer->setTexturePriorities(priorityCalculator);
> 
> Since the tiles don't actually get painted, we drop the Tile objects for them in pushPropertiesTo(). This makes needsIdlePaint ASSERT because it expects all tiles to exist. We need to setTexturePriorities() and prioritizeTextures() again after the pushPropertiesTo.

That was happening on other tests too and I fixed them in a similar way. I'm wondering though, should we drop the tiles now? Possibly we should only drop the tiles outside the preprint rect so we don't keep recreating/dropping them?

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-830
>> -    textureManager->unprotectAllTextures(); // CCLayerTreeHost::commitComplete() normally does this, but since we're mocking out the manager we have to do it.
> 
> Should textureManager->clearPriorities() here. The child still has priorities on its textures. We should probably clearPriorities() before doing prioritizeTextures() always..
> 
> In this case it's passing because the two layers are getting the same priorities I believe. But if the child had a higher priority, then I think this would/should fail when not clearing priority on the child.

This sounds right. This was one of the remaining failing tests as I recall so maybe that was the issue.

[Dana Jansens]

Some notes from our VC. Thanks for walking me through my confusion.

- reserve() is ok as is. there's room to maybe change this but does make sense.
- isValid() shouldnt be set by reserve (need to undo my change to this).
- can isValid() be set without impl thread interaction?
  - rename to hasValidAllocation() or something?
  - TLC does resetToValid() when it appends to the updater. Then it only gets unset by the impl thread while main thread is blocked for texture update
- if tile isPriority() false, then don't cause layer to skipIdlePaint when reserve() fails in TLC::updateTiles(). (we don't always reserve() in priority order here).
- double buffering, decision made during prioritization
  - stealing with prioritization doesnt work well right now.

[Adrienne Walker]

Comment on attachment 139728 [details]
Patch

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

PrioritizedTexture.h, PrioritizedTexture.cpp, PrioritizedTextureTest.cpp don't appear to be included in this latest patch.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:123
> +    m_frontTexture->setDimensions(m_size, GraphicsContext3D::RGBA);

Why does this call (and the parallel one on the TiledLayerChromium) need to be made? It looks like you already set the size and format during PrioritizedTexture creation.

[Dana Jansens]

Created attachment 141835 [details]
Patch

Oh darn, sorry!

Included those files, rebased to ToT, and undid my change to isValid() so it means what you originally intended again Eric, as we discussed.

I made the TiledLayerChromiumTests pass again, by calling updateTextures() as appropritate to make the tiles valid before pushing them. There's still some failing tests, and especially, the whole "partial update/double buffered update" scenario is not addressed yet.

[Adrienne Walker]

Comment on attachment 141835 [details]
Patch

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

160k patches generate a lot of questions.

> Source/WebCore/ChangeLog:84
> +        (WebCore::PrioritizedTexture::isValid):
> +        (WebCore::PrioritizedTexture::isValidAs):
> +        (WebCore::PrioritizedTexture::isPriority):
> +        (WebCore::PrioritizedTexture::isPriorityAs):

Having to pass format and size to isValid has always seemed like an API wart to me and I'm saddened to see it be contagious.  In the few places where we are actually changing the size of a texture from frame to frame, could we just explicitly invalidate it via setDimensions and then not have to have "am I still the right size" versions of these API functions?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:40
> +    return -1;

I'm kind of surprised that some compiler doesn't yell at you for this implicit conversion.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:55
> +    return (x + y);

Can this overflow?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:84
> +    , m_manager(0)

m_manager(manager)?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:279
> +    // Only allow textures if they are greater than the cutoff. This means all
> +    // textures of the same priority are accepted or rejected together, rather than
> +    // being partially allowed randomly.

How does this work in practice? In general, are most textures of different priority?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:285
> +            m_memoryAboveThresholdBytes += (*it)->memorySizeBytes();

Above what threshold?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:298
> +    // Put allocations in eviction/recycling order.
> +    for (AllocationVector::reverse_iterator it = sortedAllocations.rbegin(); it != sortedAllocations.rend(); ++it) {
> +        m_allocations.remove(*it);
> +        m_allocations.add(*it);
> +    }

Why not just reverse sort m_allocations?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:408
> +    texture->setCurrentAllocation(0);

When do allocations get deleted other than at ~PrioritizedTextureManager time? I kind of expected the allocation to be deleted here.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:44
> +    static unsigned uiPriority();
> +    static unsigned visiblePriority();

Why are these public? Should they just be in the anonymous namespace with manhattanDistance?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:46
> +    unsigned priorityFromDistance(unsigned pixels) const;

What does priorityFromDistance(pixels) mean? It only seems to be called as priorityFromDistance(0) which seems a lot like priorityFromVisibility(true).

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:51
> +class PrioritizedTexture {

Separate files for PrioritizedCalculator, PrioritizedTexture, PrioritizedTextureManager, please.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:76
> +    unsigned priority() const { return m_priority; }

This may just be a personal preference, but my brain finds it awfully hard to deal with a set of numbers that are within a few hundred of four billion compared to dealing with a set of numbers that are all within a few hundred of zero.  Is it too much to ask to sort priorities the other way, so that 0 is the highest priority and ~0 is the lowest?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:81
> +    bool isPriority() const;

setPriority, priority, isPriority, setIsPriority.  I think you need to differentiate these better.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:87
> +    // Reserve will succeed either if isPriority is true, or if useSpareMemory is true and
> +    // there is spare memory available. Reserve will just fill up to the maximum limit
> +    // as textures are reserved.
> +    bool reserve(bool useSpareMemory);

Why does prepainting need this end-around the priority system? This smells a lot like our old texture management system.  Would it be possible to modify the priority function for prepainted tiles such they get sorted properly with respect to other things in the system? Maybe I just think it'd be much easier to reason about which textures were valid if everything went through the same path.

Along these same lines, what does "idle paint rect" mean in a texture priority world? Isn't "idle paint rect" just a binary priority system? Theoretically, shouldn't we just assign priorities to tiles and then prepaint anything that is a priority this frame?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:99
> +    // isValid() must be true to call these functions.

You should assert that in these functions.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:104
> +    // FIXME: This can result in hidden additional memory use over the max limit!

Remind me, how did this not happen before?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:256
> +    tile->managedTexture()->setDimensions(m_tiler->tileSize(), m_textureFormat);

Why does this call need to be made? It looks like you already set the size and format during PrioritizedTexture creation.  Don't we destroy all the tiles (and their textures) if the tile size changes?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:497
> +    IntRect priorityRect = idlePaintRect(visibleLayerRect);

What if idlePaintRect is disjoint with visibleLayerRect? It seems like we might want to do that in a fling situation.  I see that the texture priorities are cleared every frame so that you don't need to iterate through all the tiles on the layer, but it does seem like you should iterate through the union of these two rects.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:513
> +            if (visibleLayerRect.isEmpty())
> +                priority = priorityCalc.priorityFromDistance(0);

This needs a comment.  ...if the layer isn't visible at all, then the idle paint rect gets a higher priority?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:674
> +    // the rect by it's own size until the total area covered by the new rect
> +    // is >= 4X it's original area. When zoomed in, this will stop after one

s/'//g

[Eric Penner]

Comment on attachment 141835 [details]
Patch

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

>> Source/WebCore/ChangeLog:84
>> +        (WebCore::PrioritizedTexture::isPriorityAs):
> 
> Having to pass format and size to isValid has always seemed like an API wart to me and I'm saddened to see it be contagious.  In the few places where we are actually changing the size of a texture from frame to frame, could we just explicitly invalidate it via setDimensions and then not have to have "am I still the right size" versions of these API functions?

I just did it to minimize changes. But I like your idea and will try to get rid of it.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:40
>> +    return -1;
> 
> I'm kind of surprised that some compiler doesn't yell at you for this implicit conversion.

I like your idea of lower number is high priority. Then this can be 0.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:55
>> +    return (x + y);
> 
> Can this overflow?

It can, but only for ~2^31 sizes right? (unless I'm missing something?).

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:84
>> +    , m_manager(0)
> 
> m_manager(manager)?

manager->registerTexture() sets the manager and it asserts if there is already a manager set. In cases where either the manager or texture could do something, the manager does all work (and checks validity).

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:279
>> +    // being partially allowed randomly.
> 
> How does this work in practice? In general, are most textures of different priority?

This is meant to solve two issues:
- In the case where we don't have enough memory for all visible tiles, then none will be allowed and call-order can dictate what gets allocated.
- With the above, we don't need layer order to be baked into priorities. I'm hoping the number of priorities can actually be made pretty low (eg. quantized to one priority for each row of pre-painting). If that's the case then this will settle at a useful level (like "two rows of pre-painting" rather than scattering the extra memory randomly).

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:285
>> +            m_memoryAboveThresholdBytes += (*it)->memorySizeBytes();
> 
> Above what threshold?

It's the priority threshold which we calculated above, which dynamically changes. This will be between preferred/max limit depending on what maxMemoryPriorityThreshold is. For example if maxMemoryPriorityThreshold is set to visiblePriority() and there is more visible tiles, then it will go up.  We've discussed removing the preferred/max limits but it still serves a purpose until the GpuMemoryManager can adjust the max limit to achieve the same behavior.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:298
>> +    }
> 
> Why not just reverse sort m_allocations?

SGTM

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:408
>> +    texture->setCurrentAllocation(0);
> 
> When do allocations get deleted other than at ~PrioritizedTextureManager time? I kind of expected the allocation to be deleted here.

This can also be used to recycle old textures in ::allocateIfNeeded. TextureAllocations correspond to texture lifetime, so they are deleted very lazily to maximize the chance they can be recycled. Even if the texture is deleted, the allocation hangs around with 0 priority.

Additional note: they don't even get deleted if they are below the priority threshold. Since painting can be so slow sometimes I didn't want a drastic change in priorities to clear out all the old textures. Instead, the old textures will only be deleted as the new textures are painted and allocateIfNeeded is called.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:44
>> +    static unsigned visiblePriority();
> 
> Why are these public? Should they just be in the anonymous namespace with manhattanDistance?

I've been meaning to add a call to texturemanager->setMaxMemoryPriorityThreshold(PriorityCalculator::visiblePriority()). Or maybe it could also use the instance rather than a static for that. I'll try to get rid of it.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:46
>> +    unsigned priorityFromDistance(unsigned pixels) const;
> 
> What does priorityFromDistance(pixels) mean? It only seems to be called as priorityFromDistance(0) which seems a lot like priorityFromVisibility(true).

If you don't have two rects and know the pixel distance? I'll remove it if that doesn't become a needed use case.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:51
>> +class PrioritizedTexture {
> 
> Separate files for PrioritizedCalculator, PrioritizedTexture, PrioritizedTextureManager, please.

Will do.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:76
>> +    unsigned priority() const { return m_priority; }
> 
> This may just be a personal preference, but my brain finds it awfully hard to deal with a set of numbers that are within a few hundred of four billion compared to dealing with a set of numbers that are all within a few hundred of zero.  Is it too much to ask to sort priorities the other way, so that 0 is the highest priority and ~0 is the lowest?

Way *way* more clear I agree. I was straining thinking of it counting down as well.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:81
>> +    bool isPriority() const;
> 
> setPriority, priority, isPriority, setIsPriority.  I think you need to differentiate these better.

(set)priority is clear I think.  I've changed the isPriority() back and forth. Initially we called it "shouldPaint()" but that's kind of external to this class. isAbovePriorityThreshold ?

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:87
>> +    bool reserve(bool useSpareMemory);
> 
> Why does prepainting need this end-around the priority system? This smells a lot like our old texture management system.  Would it be possible to modify the priority function for prepainted tiles such they get sorted properly with respect to other things in the system? Maybe I just think it'd be much easier to reason about which textures were valid if everything went through the same path.
> 
> Along these same lines, what does "idle paint rect" mean in a texture priority world? Isn't "idle paint rect" just a binary priority system? Theoretically, shouldn't we just assign priorities to tiles and then prepaint anything that is a priority this frame?

Two reasons for this:
- The problem scenario is "lots of visible tiles with lots of occlusions".  I didn't want this to cause new pages to OOM because they can't use the memory savings from occlusions. But there is no easy/clean way to get that occluded memory prioritized in advance.
- Having this also frees us from needing to prioritize based on layer order, which reduces the complexity of setting priorities, and the priority calculations etc.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:99
>> +    // isValid() must be true to call these functions.
> 
> You should assert that in these functions.

SGTM. Will add it.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:104
>> +    // FIXME: This can result in hidden additional memory use over the max limit!
> 
> Remind me, how did this not happen before?

It still caused the same memory fluctuations which is bad, but since you call reserve() it could fail and prevent you from going over the limit.  Talking with Dana we realized that double-buffering can be determined in advance during prioritizeTextures (unlike occlusions). So I'll do that as soon as I get back at this. Possibly this "steal" function can go away and we can just create two textures when we need them. TBD..

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:256
>> +    tile->managedTexture()->setDimensions(m_tiler->tileSize(), m_textureFormat);
> 
> Why does this call need to be made? It looks like you already set the size and format during PrioritizedTexture creation.  Don't we destroy all the tiles (and their textures) if the tile size changes?

Doesn't the PrioritizedTexture get created in textureUpdater()->createTexture() right here? Isn't this the texture creation?

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:497
>> +    IntRect priorityRect = idlePaintRect(visibleLayerRect);
> 
> What if idlePaintRect is disjoint with visibleLayerRect? It seems like we might want to do that in a fling situation.  I see that the texture priorities are cleared every frame so that you don't need to iterate through all the tiles on the layer, but it does seem like you should iterate through the union of these two rects.

True, I'll add that in case we make them disjoint.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:513
>> +                priority = priorityCalc.priorityFromDistance(0);
> 
> This needs a comment.  ...if the layer isn't visible at all, then the idle paint rect gets a higher priority?

Hmm, I don't recall this. Dana did you add this for non-visible animated layers?  I think we should find a better solution to not having a visibleRect in the long run (like calculating the non-clipped version).

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:674
>> +    // is >= 4X it's original area. When zoomed in, this will stop after one
> 
> s/'//g

Not once but twice!  :)

[Dana Jansens]

Comment on attachment 141835 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:513
>>> +                priority = priorityCalc.priorityFromDistance(0);
>> 
>> This needs a comment.  ...if the layer isn't visible at all, then the idle paint rect gets a higher priority?
> 
> Hmm, I don't recall this. Dana did you add this for non-visible animated layers?  I think we should find a better solution to not having a visibleRect in the long run (like calculating the non-clipped version).

Yeh if there's not visible layer rect, then there's no distance to be computed. But we're thinking we should have a visible layer rect always anyhow.. so this will just vanish. (Was planning to make it compute distance from viewport instead in the future.. should have added a FIXME, ah well.)

I should do away with the empty visibleLayerRect business tomorrow I think.

[Adrienne Walker]

(In reply to comment #20)
> (From update of attachment 141835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141835&action=review
>
> >> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:55
> >> +    return (x + y);
> > 
> > Can this overflow?
> 
> It can, but only for ~2^31 sizes right? (unless I'm missing something?).

Or locations, yeah.  I think I've just fixed too many subtle problems related to overflowing integers in WebKit that I feel compelled to call it out.

> >> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:285
> >> +            m_memoryAboveThresholdBytes += (*it)->memorySizeBytes();
> > 
> > Above what threshold?
> 
> It's the priority threshold which we calculated above, which dynamically changes. This will be between preferred/max limit depending on what maxMemoryPriorityThreshold is. For example if maxMemoryPriorityThreshold is set to visiblePriority() and there is more visible tiles, then it will go up.  We've discussed removing the preferred/max limits but it still serves a purpose until the GpuMemoryManager can adjust the max limit to achieve the same behavior.

How does it differ from current memory bytes?

> >> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:408
> >> +    texture->setCurrentAllocation(0);
> > 
> > When do allocations get deleted other than at ~PrioritizedTextureManager time? I kind of expected the allocation to be deleted here.
> 
> This can also be used to recycle old textures in ::allocateIfNeeded. TextureAllocations correspond to texture lifetime, so they are deleted very lazily to maximize the chance they can be recycled. Even if the texture is deleted, the allocation hangs around with 0 priority.
> 
> Additional note: they don't even get deleted if they are below the priority threshold. Since painting can be so slow sometimes I didn't want a drastic change in priorities to clear out all the old textures. Instead, the old textures will only be deleted as the new textures are painted and allocateIfNeeded is called.

Oh, I see.  They hang around until reduceMemory.  That sounds great.

> >> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:81
> >> +    bool isPriority() const;
> > 
> > setPriority, priority, isPriority, setIsPriority.  I think you need to differentiate these better.
> 
> (set)priority is clear I think.  I've changed the isPriority() back and forth. Initially we called it "shouldPaint()" but that's kind of external to this class. isAbovePriorityThreshold ?

It's clear only if isPriority/setIsPriority don't exist.  ;)

What about shouldBeAllocated? isAbovePriorityThreshold sounds reasonable too.

> >> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:87
> >> +    bool reserve(bool useSpareMemory);
> > 
> > Why does prepainting need this end-around the priority system? This smells a lot like our old texture management system.  Would it be possible to modify the priority function for prepainted tiles such they get sorted properly with respect to other things in the system? Maybe I just think it'd be much easier to reason about which textures were valid if everything went through the same path.
> > 
> > Along these same lines, what does "idle paint rect" mean in a texture priority world? Isn't "idle paint rect" just a binary priority system? Theoretically, shouldn't we just assign priorities to tiles and then prepaint anything that is a priority this frame?
> 
> Two reasons for this:
> - The problem scenario is "lots of visible tiles with lots of occlusions".  I didn't want this to cause new pages to OOM because they can't use the memory savings from occlusions. But there is no easy/clean way to get that occluded memory prioritized in advance.
> - Having this also frees us from needing to prioritize based on layer order, which reduces the complexity of setting priorities, and the priority calculations etc.

Ok, I see where you're coming from.  Occlusion only being known after paint definitely makes things a lot more complicated here.

Why would you ever call reserve(false), then? Is this trying to bump up the priorities of prepainting more than a layer who got skipped because all of its visible textures didn't make the priority cut?

> >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:256
> >> +    tile->managedTexture()->setDimensions(m_tiler->tileSize(), m_textureFormat);
> > 
> > Why does this call need to be made? It looks like you already set the size and format during PrioritizedTexture creation.  Don't we destroy all the tiles (and their textures) if the tile size changes?
> 
> Doesn't the PrioritizedTexture get created in textureUpdater()->createTexture() right here? Isn't this the texture creation?

Oh, quite right.  I misread this.

> >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:497
> >> +    IntRect priorityRect = idlePaintRect(visibleLayerRect);
> > 
> > What if idlePaintRect is disjoint with visibleLayerRect? It seems like we might want to do that in a fling situation.  I see that the texture priorities are cleared every frame so that you don't need to iterate through all the tiles on the layer, but it does seem like you should iterate through the union of these two rects.
> 
> True, I'll add that in case we make them disjoint.

I guess describing it as disjoint isn't sufficient.  I suppose this could also cause problems if the idlePaintRect didn't entirely contain visibleLayerRect.

[Dana Jansens]

Comment on attachment 141835 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:81
>>>> +    bool isPriority() const;
>>> 
>>> setPriority, priority, isPriority, setIsPriority.  I think you need to differentiate these better.
>> 
>> (set)priority is clear I think.  I've changed the isPriority() back and forth. Initially we called it "shouldPaint()" but that's kind of external to this class. isAbovePriorityThreshold ?
> 
> It's clear only if isPriority/setIsPriority don't exist.  ;)
> 
> What about shouldBeAllocated? isAbovePriorityThreshold sounds reasonable too.

I like shouldBeAllocated.

[Eric Penner]

Comment on attachment 141835 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:285
>>>> +            m_memoryAboveThresholdBytes += (*it)->memorySizeBytes();
>>> 
>>> Above what threshold?
>> 
>> It's the priority threshold which we calculated above, which dynamically changes. This will be between preferred/max limit depending on what maxMemoryPriorityThreshold is. For example if maxMemoryPriorityThreshold is set to visiblePriority() and there is more visible tiles, then it will go up.  We've discussed removing the preferred/max limits but it still serves a purpose until the GpuMemoryManager can adjust the max limit to achieve the same behavior.
> 
> How does it differ from current memory bytes?

Current memory bytes is the actual texture memory allocated, where as memory above threshold is the amount that would be allocated if you called allocate on all textures. I added some documentation. Maybe both don't need to be exposed, but it's useful to have both internally.

>>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:298
>>> +    }
>> 
>> Why not just reverse sort m_allocations?
> 
> SGTM

Done.

>>>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:87
>>>> +    bool reserve(bool useSpareMemory);
>>> 
>>> Why does prepainting need this end-around the priority system? This smells a lot like our old texture management system.  Would it be possible to modify the priority function for prepainted tiles such they get sorted properly with respect to other things in the system? Maybe I just think it'd be much easier to reason about which textures were valid if everything went through the same path.
>>> 
>>> Along these same lines, what does "idle paint rect" mean in a texture priority world? Isn't "idle paint rect" just a binary priority system? Theoretically, shouldn't we just assign priorities to tiles and then prepaint anything that is a priority this frame?
>> 
>> Two reasons for this:
>> - The problem scenario is "lots of visible tiles with lots of occlusions".  I didn't want this to cause new pages to OOM because they can't use the memory savings from occlusions. But there is no easy/clean way to get that occluded memory prioritized in advance.
>> - Having this also frees us from needing to prioritize based on layer order, which reduces the complexity of setting priorities, and the priority calculations etc.
> 
> Ok, I see where you're coming from.  Occlusion only being known after paint definitely makes things a lot more complicated here.
> 
> Why would you ever call reserve(false), then? Is this trying to bump up the priorities of prepainting more than a layer who got skipped because all of its visible textures didn't make the priority cut?

It's no trying to bump up priorities or anything.  Reserve(false) is just giving up early rather than trying to use every last texture when you fulfill all textures of that priority. 

I will make it so it will only succeed for priorities that are equal to the cutoff (rather than letting any texture succeed). This means it just helps disambiguate the priority sort (most importantly when all visible tiles don't fit).

>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:497
>>>> +    IntRect priorityRect = idlePaintRect(visibleLayerRect);
>>> 
>>> What if idlePaintRect is disjoint with visibleLayerRect? It seems like we might want to do that in a fling situation.  I see that the texture priorities are cleared every frame so that you don't need to iterate through all the tiles on the layer, but it does seem like you should iterate through the union of these two rects.
>> 
>> True, I'll add that in case we make them disjoint.
> 
> I guess describing it as disjoint isn't sufficient.  I suppose this could also cause problems if the idlePaintRect didn't entirely contain visibleLayerRect.

I realized this was an oversight. I've changed it we iterate through *all* tiles in the map like we discussed (adding the pre-paint rect tiles first).  We should keep all tiles around if we have memory for them.

>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:513
>>>> +                priority = priorityCalc.priorityFromDistance(0);
>>> 
>>> This needs a comment.  ...if the layer isn't visible at all, then the idle paint rect gets a higher priority?
>> 
>> Hmm, I don't recall this. Dana did you add this for non-visible animated layers?  I think we should find a better solution to not having a visibleRect in the long run (like calculating the non-clipped version).
> 
> Yeh if there's not visible layer rect, then there's no distance to be computed. But we're thinking we should have a visible layer rect always anyhow.. so this will just vanish. (Was planning to make it compute distance from viewport instead in the future.. should have added a FIXME, ah well.)
> 
> I should do away with the empty visibleLayerRect business tomorrow I think.

Since the "small animated layers" case is still here I'm just bumping it to priorityFromDistance(512) for now.  I've got some ideas how we can get distances for each texture, but can discuss that later.

[Eric Penner]

Created attachment 145018 [details]
Patch

[WebKit Review Bot]

Attachment 145018 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/PrioritizedTextureTest.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Penner]

All the unit tests are fixed and addressed all previous comments. I need to actually run it a bit now to see if there is any issues the unit tests didn't catch. I've tuned back to the amount of pre-painting to only slightly more than before, then we can tune it up after landing this.

Double-buffering memory usage is still an issue, however the memory will get reclaimed on the next frame so it's overall probably still better than before. Possibly we can make fixing that issue another patch since it was an existing problem?

[Eric Penner]

Created attachment 145152 [details]
Added_assertInvariants_function

[Adrienne Walker]

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

This is an open question, but does there need to be priority with respect to invalidation? I mean this in the sense of: should we prioritize recycling allocations for textures on invalidated tiles that aren't going to get painted this frame, since those textures and allocations are unused and useless? Is this something to do in the future, along with better prioritization for animated textures?

> Source/WebCore/ChangeLog:6
> +PrioritizedTextures have a priority such that all textures can be prioritized.

nit: indent all of these blocks properly.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:130
> +        // FIXME: Canvas can't currently give up it's texture as it can't currently
> +        //        restore it when when it gets it's texture back. For this reason

s/it's/its/g

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:80
> +    bool isValid() const;
> +    bool isValidAs(IntSize, unsigned format) const;

What happened to removing the "As" versions of isValid and shouldBeAllocated? (re: https://bugs.webkit.org/show_bug.cgi?id=84308#c20)

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:92
> +    // FIXME: This can result in hidden additional memory use over the max limit!

I'm fine with fixing this previously existing issue in another patch.  Can you file a bug, if there isn't one?

> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.h:56
> +    static inline int highestPriority() { return std::numeric_limits<int>::min(); }
> +    static inline int lowestPriority() { return std::numeric_limits<int>::max(); }
> +    static inline bool priorityIsLower(int a, int b) { return a > b; }
> +    static inline bool priorityIsHigher(int a, int b) { return a < b; }

Don't duplicate this from PriorityCalculator.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:514
> +void TiledLayerChromium::setTexturePrioritiesInRect(const PriorityCalculator& priorityCalc, const IntRect& visibleRect)
> +{
> +    IntRect prepaintRect = idlePaintRect(visibleRect);
> +    if (prepaintRect.isEmpty())
>          return;

I don't think you should have an early out here.  If the prepaint rect is empty (maybe a large layer *just* went off screen), you should still prioritize its tiles.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:520
> +        createTextureUpdaterIfNeeded();

Can you just move this call to createTile? If we're going to lazily create the texture updater, we might as well let callers be lazy too.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:588
> -    updateBounds();
> -
>      if (m_tiler->hasEmptyBounds())
>          return;

Why are all these updateBounds() calls removed? It seems like you'd want to have updated bounds information before checking if the tiler had empty bounds.

[Eric Penner]

Created attachment 145156 [details]
minor_tweak

[Dana Jansens]

(In reply to comment #24)
> >>> What if idlePaintRect is disjoint with visibleLayerRect? It seems like we might want to do that in a fling situation.  I see that the texture priorities are cleared every frame so that you don't need to iterate through all the tiles on the layer, but it does seem like you should iterate through the union of these two rects.
> >> 
> >> True, I'll add that in case we make them disjoint.
> > 
> > I guess describing it as disjoint isn't sufficient.  I suppose this could also cause problems if the idlePaintRect didn't entirely contain visibleLayerRect.
> 
> I realized this was an oversight. I've changed it we iterate through *all* tiles in the map like we discussed (adding the pre-paint rect tiles first).  We should keep all tiles around if we have memory for them.

I think this means that if the visible rect is not contained in the prepaint rect, and we did not yet create tiles for the visible rect, we won't create them here. Maybe this is okay, was it intended?

[Eric Penner]

> 
> This is an open question, but does there need to be priority with respect to invalidation? I mean this in the sense of: should we prioritize recycling allocations for textures on invalidated tiles that aren't going to get painted this frame, since those textures and allocations are unused and useless? Is this something to do in the future, along with better prioritization for animated textures?
> 

I had a feature previously where textures can actively "give back" their texture allocation. I'm not sure this would help though as we will want to repaint the texture eventually according to whatever its priority is, at which point we will want the texture again.

> > Source/WebCore/ChangeLog:6
> > +PrioritizedTextures have a priority such that all textures can be prioritized.
> 
> nit: indent all of these blocks properly.
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:130
> > +        // FIXME: Canvas can't currently give up it's texture as it can't currently
> > +        //        restore it when when it gets it's texture back. For this reason
> 
> s/it's/its/g
> 
> > Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:80
> > +    bool isValid() const;
> > +    bool isValidAs(IntSize, unsigned format) const;
> 
> What happened to removing the "As" versions of isValid and shouldBeAllocated? (re: https://bugs.webkit.org/show_bug.cgi?id=84308#c20)
>

Hoping you would forget? :) Actually I just didn't want to do this until all tests were fixed and then forgot. Now I'll do a pass and converting them all and make sure it doesn't break anything.

> > Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:92
> > +    // FIXME: This can result in hidden additional memory use over the max limit!
> 
> I'm fine with fixing this previously existing issue in another patch.  Can you file a bug, if there isn't one?
> 

Yep.

> > Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.h:56
> > +    static inline int highestPriority() { return std::numeric_limits<int>::min(); }
> > +    static inline int lowestPriority() { return std::numeric_limits<int>::max(); }
> > +    static inline bool priorityIsLower(int a, int b) { return a > b; }
> > +    static inline bool priorityIsHigher(int a, int b) { return a < b; }
> 
> Don't duplicate this from PriorityCalculator.
>

yep.
 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:514
> > +void TiledLayerChromium::setTexturePrioritiesInRect(const PriorityCalculator& priorityCalc, const IntRect& visibleRect)
> > +{
> > +    IntRect prepaintRect = idlePaintRect(visibleRect);
> > +    if (prepaintRect.isEmpty())
> >          return;
> 
> I don't think you should have an early out here.  If the prepaint rect is empty (maybe a large layer *just* went off screen), you should still prioritize its tiles.
> 

Totally, good catch.

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:520
> > +        createTextureUpdaterIfNeeded();
> 
> Can you just move this call to createTile? If we're going to lazily create the texture updater, we might as well let callers be lazy too.
> 

Okay

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:588
> > -    updateBounds();
> > -
> >      if (m_tiler->hasEmptyBounds())
> >          return;
> 
> Why are all these updateBounds() calls removed? It seems like you'd want to have updated bounds information before checking if the tiler had empty bounds.

I don't know how this change got in. I'll move it back.

[Eric Penner]

> > I don't think you should have an early out here.  If the prepaint rect is empty (maybe a large layer *just* went off screen), you should still prioritize its tiles.
> > 
> 
> Totally, good catch.


Dang, so I've realized there is a problem with this that I should have noticed. Whereas before the LRU would would keep the recently protected textures, now we need a priority to order them. When we have no visible rect, we don't have a priority to set, so off-screen layer textures will be reclaimed in no particular order. There are a few ways we can get a distance without the visible rect, but I was hoping that wouldn't need to go into this patch.

One work-around I can think of is:
- clearPriorities() settings to priority to min(lingeringPriority(), priority()-1) instead of setting to lowestPriority()

[Dana Jansens]

Comment on attachment 145156 [details]
minor_tweak

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

I love CCLTH::updateLayers() so much after this.

I'll take another pass thru the new files tomorrow. :)

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:126
> +        if (m_frontTexture)
> +            m_frontTexture->setTextureManager(textureManager);

Should we be resizing the texture if m_size != the texture's size here? It seems like this size would affect what we give to other textures, and if the size changes that would have the wrong effect, as well as make the shouldBeAllocatedAs() call below fail.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:50
> +        m_memorySizeBytes = TextureManager::memoryUseBytes(size, format);

Should we move memoryUseBytes to PrioritizedTextureManager?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:129
> +    unsigned textureId = m_currentAllocation ? m_currentAllocation->textureId() : 0;
> +    context->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId);

Can just use textureId() and drop the local var?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:138
> +    unsigned textureId = m_currentAllocation ? m_currentAllocation->textureId() : 0;
> +    context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, textureId, 0);

Same here?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:158
> +} // WebCore

nit: "namespace WebCore" I think?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:53
> +    // Texture properties. Changing these invalidates this texture until
> +    // textures are prioritized again.

Or until it is allocated again? Does this mean to say that we only allocate during prioritization? But we allocate during texture upload right? *confuse face*

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:90
> +    // isValid() must be true to call these functions.
> +    unsigned textureId();
> +    void bindTexture(GraphicsContext3D*, TextureAllocator*);
> +    void framebufferTexture2D(GraphicsContext3D*, TextureAllocator*);

Is this comment right? The latter two functions call allocate() which sets valid to true.

> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.cpp:57
> +    // Each remaining allocation is a leaked opengl texture. We don't have the allocator
> +    // to delete the textures at this time so clearMemory() needs to be called before this.
> +    while (m_allocations.size() > 0)
> +        destroyAllocation(*m_allocations.begin(), 0);

Should we ASSERT() instead?

> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.cpp:104
> +    TextureVector& sortedTextures = m_tempTextureVector;
> +    AllocationVector& sortedAllocations = m_tempAllocationVector;

Why are these class members?

> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.cpp:106
> +    sortedTextures.clear();
> +    sortedAllocations.clear();

Looks like we clear these at the end, so they should always be empty coming in here?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380
> +

nit: extra whitespace

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:663
> +

nit: extra whitespace

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:671
> +            if (tile && tile->isDirty() && tile->managedTexture()->shouldBeAllocatedAs(m_tiler->tileSize(), m_textureFormat))

Should this check valid too?

I guess the old code said "if we got evicted but we're in the rect then try get that texture prepainted again". This code says "if we got evicted we stay that way"? To keep the same behaviour I think it would look like: if (tile && shouldBeAllocated && (isDirty || !isValid)) ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:544
> +    PriorityCalculator calculator;

We create an instance of PriorityCalculator without any arguments and then pass it around as a const reference. Can its methods just all be static and we don't need to pass it around?

[Eric Penner]

> > This is an open question, but does there need to be priority with respect to invalidation? I mean this in the sense of: should we prioritize recycling allocations for textures on invalidated tiles that aren't going to get painted this frame, since those textures and allocations are unused and useless? Is this something to do in the future, along with better prioritization for animated textures?
> > 
> 
> I had a feature previously where textures can actively "give back" their texture allocation. I'm not sure this would help though as we will want to repaint the texture eventually according to whatever its priority is, at which point we will want the texture again.
> 

On second thought, for textures which are not going to be pre-painted and are just hanging around out of view, then this could indeed help to reduce work and increase recycling. However, Android might need some custom behavior regarding evicting these tiles, so I wouldn't want to add something like that just yet.

[Eric Penner]

Comment on attachment 145156 [details]
minor_tweak

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

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:126
>> +            m_frontTexture->setTextureManager(textureManager);
> 
> Should we be resizing the texture if m_size != the texture's size here? It seems like this size would affect what we give to other textures, and if the size changes that would have the wrong effect, as well as make the shouldBeAllocatedAs() call below fail.

Along with removing the "As" functions, what if I set the dimensions here and remove the "As" below.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:50
>> +        m_memorySizeBytes = TextureManager::memoryUseBytes(size, format);
> 
> Should we move memoryUseBytes to PrioritizedTextureManager?

How about as part of removing the other TextureManager?

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:129
>> +    context->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId);
> 
> Can just use textureId() and drop the local var?

yep

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:138
>> +    context->framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, textureId, 0);
> 
> Same here?

ditto

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:53
>> +    // textures are prioritized again.
> 
> Or until it is allocated again? Does this mean to say that we only allocate during prioritization? But we allocate during texture upload right? *confuse face*

It needs to get prioritized again. Calling allocate requires shouldBeAllocated to be true, which won't happen until prioritizeTextures().  This is a limitation over the old texture manager, but it makes sense because we need to take the memory from something else to fill the new request.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:90
>> +    void framebufferTexture2D(GraphicsContext3D*, TextureAllocator*);
> 
> Is this comment right? The latter two functions call allocate() which sets valid to true.

Yeah I changed that for convenience. I'll change the comment to "shouldBeAllocated()" must be true.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.cpp:57
>> +        destroyAllocation(*m_allocations.begin(), 0);
> 
> Should we ASSERT() instead?

I think this makes unit tests fail... I think the old texture manager has the same problem and doesn't ASSERT on shutdown.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.cpp:104
>> +    AllocationVector& sortedAllocations = m_tempAllocationVector;
> 
> Why are these class members?

Depends on the implementation but it often results in no further memory allocations after first use (depends if it shrinks aggressively). Possibly there is some other functions I need to call to make sure it has that effect though. I haven't looked into the details for this particular vector class.

>> Source/WebCore/platform/graphics/chromium/PrioritizedTextureManager.cpp:106
>> +    sortedAllocations.clear();
> 
> Looks like we clear these at the end, so they should always be empty coming in here?

It's a temp that could be used for other things, so best to be safe.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:671
>> +            if (tile && tile->isDirty() && tile->managedTexture()->shouldBeAllocatedAs(m_tiler->tileSize(), m_textureFormat))
> 
> Should this check valid too?
> 
> I guess the old code said "if we got evicted but we're in the rect then try get that texture prepainted again". This code says "if we got evicted we stay that way"? To keep the same behaviour I think it would look like: if (tile && shouldBeAllocated && (isDirty || !isValid)) ?

Yep that sounds right to me.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:544
>> +    PriorityCalculator calculator;
> 
> We create an instance of PriorityCalculator without any arguments and then pass it around as a const reference. Can its methods just all be static and we don't need to pass it around?

Right now we could. I had planned for it to hold some state though which would need the instance (eg. what if we revisit layer-order again).

[Dana Jansens]

Comment on attachment 145156 [details]
minor_tweak

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

>>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:126
>>> +            m_frontTexture->setTextureManager(textureManager);
>> 
>> Should we be resizing the texture if m_size != the texture's size here? It seems like this size would affect what we give to other textures, and if the size changes that would have the wrong effect, as well as make the shouldBeAllocatedAs() call below fail.
> 
> Along with removing the "As" functions, what if I set the dimensions here and remove the "As" below.

Sounds right to me.

>>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:50
>>> +        m_memorySizeBytes = TextureManager::memoryUseBytes(size, format);
>> 
>> Should we move memoryUseBytes to PrioritizedTextureManager?
> 
> How about as part of removing the other TextureManager?

Sure that's cool.

>>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:53
>>> +    // textures are prioritized again.
>> 
>> Or until it is allocated again? Does this mean to say that we only allocate during prioritization? But we allocate during texture upload right? *confuse face*
> 
> It needs to get prioritized again. Calling allocate requires shouldBeAllocated to be true, which won't happen until prioritizeTextures().  This is a limitation over the old texture manager, but it makes sense because we need to take the memory from something else to fill the new request.

Ah ok I see. Thanks.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:544
>>> +    PriorityCalculator calculator;
>> 
>> We create an instance of PriorityCalculator without any arguments and then pass it around as a const reference. Can its methods just all be static and we don't need to pass it around?
> 
> Right now we could. I had planned for it to hold some state though which would need the instance (eg. what if we revisit layer-order again).

Hm, but then we'd have to touch every argument list to remove the const anyways right? Maybe that's preferable though?

[Eric Penner]

Created attachment 145365 [details]
Patch

[Eric Penner]

Created attachment 145371 [details]
Patch

[Eric Penner]

Comment on attachment 145371 [details]
Patch

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

> Source/WebCore/ChangeLog:84
> +        (WebCore::PrioritizedTexture::shouldBeAllocatedAs):

These are gone. I'll redo the changelog.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:673
> +            if (tile && tile->managedTexture()->shouldBeAllocated() && tile->isDirty())

This is the only remaining problem left from the comments. Unfortunately adding the check for the texture being invalid here causes one of the double-buffering unit tests to break. I'll have to investigate before this can land.

[Eric Penner]

Created attachment 145473 [details]
fix_double_buffering_memory_usage

[Eric Penner]

Created attachment 145475 [details]
fixed_last_issue

[Nat Duca]

Comment on attachment 145156 [details]
minor_tweak

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

Its very hard to see what the PrioritizedTextureManager calls versus what the user of the PrioritizedTexture call. Can we make PrioritizedTexture have a PrioritizedTextureInternal m_internal which is the internal state of the prioritized texture that is only called by the manager? And, and and, only have the interal one be registered to the textureManager?

Any reason we have PriorityCalculator and not CCPriorityCalculator? Same goes for the new code not being in CC and the new classes not being called CCxxx. The old naming predates our cc convention but I understood that we were trying to adopt the new convention...

I see this as an important feature-level refactoring. But, as followon, I think we need to consider making a manager for resources that is aware of both:
* A more-unified resource manager that spans both threads
* Aware of the fact that resources get moved over to the "other side" (and can't be updated the same way once they're on the impl side)
* Aware of the difference between "I need a texture and its the same as last time" versus "I need a texture so that I can draw into it"
* Draws a distinction between a request and an allocation. I think PrioritizedTexture is actually a PrioritizedTextureRequest, but the way we use it implies that it is a retained resource whereas a "request" implies something ephemeral a la appendQuads//QuadLists.
* A request/reservation system that uses passes to make the managment process easier to understand
* Moving more of the "how much to paint this pass" logic out of the tiled layers and into an overall managment class (resource manager, etc)

This having been said, and lets keep refining our solution in this area.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:65
> +    if (manager)

We might want to mention that resgiterTexture calls back to us to set m_manager = manager... that was very not clear to me.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:64
> +    int priority() const { return m_priority; }

Out of curiosity, rationale behind int vs float?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:77
> +    // Is the texture still valid since allocate was called (still has the same size, format and data).

for clarity, move these functions to before line 52 so the comment about invalidate makes sense to novices?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:550
> +    m_contentsTextureManager->prioritizeTextures();

Why

[Eric Penner]

Comment on attachment 145156 [details]
minor_tweak

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

>> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:65
>> +    if (manager)
> 
> We might want to mention that resgiterTexture calls back to us to set m_manager = manager... that was very not clear to me.

I'll add a comment. The general idea is to let the manager do as much as possible in cases where the both class could do something.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:544
>>>> +    PriorityCalculator calculator;
>>> 
>>> We create an instance of PriorityCalculator without any arguments and then pass it around as a const reference. Can its methods just all be static and we don't need to pass it around?
>> 
>> Right now we could. I had planned for it to hold some state though which would need the instance (eg. what if we revisit layer-order again).
> 
> Hm, but then we'd have to touch every argument list to remove the const anyways right? Maybe that's preferable though?

Only on the functions for which it needs to be non-const. I think non-const stuff would happen outside the layers.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:550
>> +    m_contentsTextureManager->prioritizeTextures();
> 
> Why

Are you wondering why we need the prioritizeTextures() call at all
and why they aren't just always prioritized?

It's important that all requests happen before you determine which
requests are successful (prioritizeTextures()). Late requests could
mean that you paint something that you later decide you can't fit into
memory because a late but high-priority request occurred after it was
painted. The two benefits of the current design in my mind is:
1.) You never paint stuff wastefully (never gets pushed to impl)
2.) The entire memory budget is in active use at all times (could
    become visible on the composite thread).

[Dana Jansens]

Comment on attachment 145156 [details]
minor_tweak

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

>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:544
>>>>> +    PriorityCalculator calculator;
>>>> 
>>>> We create an instance of PriorityCalculator without any arguments and then pass it around as a const reference. Can its methods just all be static and we don't need to pass it around?
>>> 
>>> Right now we could. I had planned for it to hold some state though which would need the instance (eg. what if we revisit layer-order again).
>> 
>> Hm, but then we'd have to touch every argument list to remove the const anyways right? Maybe that's preferable though?
> 
> Only on the functions for which it needs to be non-const. I think non-const stuff would happen outside the layers.

Ohh.. good point, thanks!

[Eric Penner]

(In reply to comment #43)
> (From update of attachment 145156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145156&action=review
> 
> Its very hard to see what the PrioritizedTextureManager calls versus what the user of the PrioritizedTexture call. Can we make PrioritizedTexture have a PrioritizedTextureInternal m_internal which is the internal state of the prioritized texture that is only called by the manager? And, and and, only have the interal one be registered to the textureManager?
> 

That would be a pretty easy change, but I tried to achieve the same (in the external interface anyway) by making all the "manager only" methods private and making the manager a friend class. I think (right now anyway) the internal class would contain everything, since the manager accesses just about every field right now. The public texture methods however are pretty restricted.

> Any reason we have PriorityCalculator and not CCPriorityCalculator? Same goes for the new code not being in CC and the new classes not being called CCxxx. The old naming predates our cc convention but I understood that we were trying to adopt the new convention...
> 

Should I make all classes CCxxx? I pretty ignorantly just copied the old texture manager when I started this, and didn't give this convention any thought.

> I see this as an important feature-level refactoring. But, as followon, I think we need to consider making a manager for resources that is aware of both:
> * A more-unified resource manager that spans both threads
> * Aware of the fact that resources get moved over to the "other side" (and can't be updated the same way once they're on the impl side)
> * Aware of the difference between "I need a texture and its the same as last time" versus "I need a texture so that I can draw into it"
> * Draws a distinction between a request and an allocation. I think PrioritizedTexture is actually a PrioritizedTextureRequest, but the way we use it implies that it is a retained resource whereas a "request" implies something ephemeral a la appendQuads//QuadLists.
> * A request/reservation system that uses passes to make the managment process easier to understand
> * Moving more of the "how much to paint this pass" logic out of the tiled layers and into an overall managment class (resource manager, etc)
> 

This all sounds like good stuff. The one thing I'm not sure about is whether this process will ever be possible via a temporary pass (like QuadList etc) or whether the current approach of keeping a persistent request makes more sense. You need to request to keep all your old textures the same way you request new textures, and the persistent textures need to be stored somewhere. That said, I always prefer the temporary pass approach when it's possible.

[Dana Jansens]

(In reply to comment #46)
> This all sounds like good stuff. The one thing I'm not sure about is whether this process will ever be possible via a temporary pass (like QuadList etc) or whether the current approach of keeping a persistent request makes more sense. You need to request to keep all your old textures the same way you request new textures, and the persistent textures need to be stored somewhere. That said, I always prefer the temporary pass approach when it's possible.

I'm trying to do something similar right now for render surface/pass textures, since we need some way to keep the textures across frames, but we need to remove any pointers from the quads/passes. I'm planning to do this with a HashMap of textures, where you request/find/use them always by an integer id that is persistent across frames (the owning layer's ID in this case for now).

[Eric Penner]

(In reply to comment #47)
> (In reply to comment #46)
> > This all sounds like good stuff. The one thing I'm not sure about is whether this process will ever be possible via a temporary pass (like QuadList etc) or whether the current approach of keeping a persistent request makes more sense. You need to request to keep all your old textures the same way you request new textures, and the persistent textures need to be stored somewhere. That said, I always prefer the temporary pass approach when it's possible.
> 
> I'm trying to do something similar right now for render surface/pass textures, since we need some way to keep the textures across frames, but we need to remove any pointers from the quads/passes. I'm planning to do this with a HashMap of textures, where you request/find/use them always by an integer id that is persistent across frames (the owning layer's ID in this case for now).

Fundamentally I'm not sure that is different though. It sounds like a Handle vs. Pointer argument at that point. If one were to wrap a class around the Handle for convenience, wouldn't it be the same?

[Dana Jansens]

(In reply to comment #48)
> (In reply to comment #47)
> > (In reply to comment #46)
> > > This all sounds like good stuff. The one thing I'm not sure about is whether this process will ever be possible via a temporary pass (like QuadList etc) or whether the current approach of keeping a persistent request makes more sense. You need to request to keep all your old textures the same way you request new textures, and the persistent textures need to be stored somewhere. That said, I always prefer the temporary pass approach when it's possible.
> > 
> > I'm trying to do something similar right now for render surface/pass textures, since we need some way to keep the textures across frames, but we need to remove any pointers from the quads/passes. I'm planning to do this with a HashMap of textures, where you request/find/use them always by an integer id that is persistent across frames (the owning layer's ID in this case for now).
> 
> Fundamentally I'm not sure that is different though. It sounds like a Handle vs. Pointer argument at that point. If one were to wrap a class around the Handle for convenience, wouldn't it be the same?

Yes, you're absolutely right. It's like a "soft pointer" that is serializable, so that it can refer to data in a different process, but its not fundamentally different.

Nat, can you maybe elaborate one how you'd like to see this as a temporary pass a bit more?

[Nat Duca]

> Nat, can you maybe elaborate one how you'd like to see this as a temporary pass a bit more?

If you're needing something for a patch you've got in progress, can you set up a hangout with you eric and I? But, lets land Eric's change first and foremost.

[Dana Jansens]

(In reply to comment #50)
> If you're needing something for a patch you've got in progress, can you set up a hangout with you eric and I? But, lets land Eric's change first and foremost.

Yeh let's do that!

To be clear I'm really happy with this CL :) Nice job on it Eric, I think everything I've brought up has been addressed very satisfactorily.

[Eric Penner]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > > (In reply to comment #46)
> > > > This all sounds like good stuff. The one thing I'm not sure about is whether this process will ever be possible via a temporary pass (like QuadList etc) or whether the current approach of keeping a persistent request makes more sense. You need to request to keep all your old textures the same way you request new textures, and the persistent textures need to be stored somewhere. That said, I always prefer the temporary pass approach when it's possible.
> > > 
> > > I'm trying to do something similar right now for render surface/pass textures, since we need some way to keep the textures across frames, but we need to remove any pointers from the quads/passes. I'm planning to do this with a HashMap of textures, where you request/find/use them always by an integer id that is persistent across frames (the owning layer's ID in this case for now).
> > 
> > Fundamentally I'm not sure that is different though. It sounds like a Handle vs. Pointer argument at that point. If one were to wrap a class around the Handle for convenience, wouldn't it be the same?
> 
> Yes, you're absolutely right. It's like a "soft pointer" that is serializable, so that it can refer to data in a different process, but its not fundamentally different.
> 
> Nat, can you maybe elaborate one how you'd like to see this as a temporary pass a bit more?

If it is a handle thing, this could be made to use handles pretty easily. However I pictured both of these classes always existing in the same thread/process that does painting.

Insert-meme: "It's a paint prioritizer essentially"

[Eric Penner]

(In reply to comment #52)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (In reply to comment #47)
> > > > (In reply to comment #46)
> > > > > This all sounds like good stuff. The one thing I'm not sure about is whether this process will ever be possible via a temporary pass (like QuadList etc) or whether the current approach of keeping a persistent request makes more sense. You need to request to keep all your old textures the same way you request new textures, and the persistent textures need to be stored somewhere. That said, I always prefer the temporary pass approach when it's possible.
> > > > 
> > > > I'm trying to do something similar right now for render surface/pass textures, since we need some way to keep the textures across frames, but we need to remove any pointers from the quads/passes. I'm planning to do this with a HashMap of textures, where you request/find/use them always by an integer id that is persistent across frames (the owning layer's ID in this case for now).
> > > 
> > > Fundamentally I'm not sure that is different though. It sounds like a Handle vs. Pointer argument at that point. If one were to wrap a class around the Handle for convenience, wouldn't it be the same?
> > 
> > Yes, you're absolutely right. It's like a "soft pointer" that is serializable, so that it can refer to data in a different process, but its not fundamentally different.
> > 
> > Nat, can you maybe elaborate one how you'd like to see this as a temporary pass a bit more?
> 
> If it is a handle thing, this could be made to use handles pretty easily. However I pictured both of these classes always existing in the same thread/process that does painting.
> 
> Insert-meme: "It's a paint prioritizer essentially"

Rather, the same thread/process that decides what to paint. :)

[Nat Duca]

(In reply to comment #46)
> > Any reason we have PriorityCalculator and not CCPriorityCalculator? Same goes for the new code not being in CC and the new classes not being called CCxxx. The old naming predates our cc convention but I understood that we were trying to adopt the new convention...
> > 
> 
> Should I make all classes CCxxx? I pretty ignorantly just copied the old texture manager when I started this, and didn't give this convention any thought.
Yep. CCxxx inside the cc/ folder.

You can do bulk renames this way:
git diff origin/master > /tmp/foo
edit /tmp/foo to do renaming
git reset --hard origin/master
patch -p1 < /tmp/foo

[Dana Jansens]

(In reply to comment #53)
> Rather, the same thread/process that decides what to paint. :)

Yeh me too, I don't think it would make sense to use this class (and certainly not the same instance of it) for RenderPass intermediate textures the way we are structuring things currently. And contents would be reserved where the painting decision is made.

[Eric Penner]

Created attachment 146674 [details]
Patch

[Eric Penner]

Other than any big concerns, I think this should land so we can start getting some benefits from it. The amount of pre-painting is currently just slightly larger, but we can jack that up after landing.

In this patch:
- Huge renaming, all the files as well as many method/classes names for clarity.
- Removed isValid() entirely as it isn't needed. haveBackingTexture() now 
  guarantees texture validity as textures are only acquired upon request.

[James Robinson]

Comment on attachment 146674 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:54
> +    // Texture properties. Changing these causes the backing texture to be lost.
> +    void setTextureManager(CCPrioritizedTextureManager*);

if "be lost" means actually losing the backing and having to repaint then this won't work - we need to be able to call setTextureManager() on a layer without actually losing the contents. the reason is we have cases when rebuilding the compositing tree where a LayerChromium has a setLayerTreeHost(0) call then a setLayerTreeHost(host) where "host" is the same as the original host. we handle this by calling setTextureManager() on setLayerTreeHost(non-NULL) and making sure that setTM() is a no-op if the manager is the same, but destroys the texture if the new host is different.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:67
> +    bool canAquireBackingTexture() const;

typo "Aquire" -> "Acquire"

[Eric Penner]

Comment on attachment 146674 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:54
>> +    void setTextureManager(CCPrioritizedTextureManager*);
> 
> if "be lost" means actually losing the backing and having to repaint then this won't work - we need to be able to call setTextureManager() on a layer without actually losing the contents. the reason is we have cases when rebuilding the compositing tree where a LayerChromium has a setLayerTreeHost(0) call then a setLayerTreeHost(host) where "host" is the same as the original host. we handle this by calling setTextureManager() on setLayerTreeHost(non-NULL) and making sure that setTM() is a no-op if the manager is the same, but destroys the texture if the new host is different.

I think it is only lost if the manager is changed to a different manager. Do you mean it should be able to keep the texture when the manager is temporarily NULL?

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:67
>> +    bool canAquireBackingTexture() const;
> 
> typo "Aquire" -> "Acquire"

Dang. Find an replace has failed me ;)

[Eric Penner]

I think I added a problem with the isValid removal. I've got to run but I'll test a lot with chrome so this can land monday. Please let me know if you see other problems (I'll look into the setManager case as well).

[WebKit Review Bot]

Comment on attachment 146674 [details]
Patch

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

New failing tests:
compositing/masks/multiple-masks.html
compositing/masks/masked-ancestor.html
compositing/masks/direct-image-mask.html
compositing/reflections/masked-reflection-on-composited.html
compositing/reflections/nested-reflection-mask-change.html
compositing/masks/simple-composited-mask.html

[WebKit Review Bot]

Created attachment 146682 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Penner]

Created attachment 146751 [details]
Patch

[Eric Penner]

Created attachment 146752 [details]
Patch

[Eric Penner]

Comment on attachment 146752 [details]
Patch

This makes setManager a no-op when it's the same manager and fixes a bug in swapping textures. If there are images diffs at this point then I'll have to look into that, as this shouldn't even change our painting behavior.

[WebKit Review Bot]

Comment on attachment 146752 [details]
Patch

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

New failing tests:
compositing/masks/multiple-masks.html
compositing/masks/masked-ancestor.html
compositing/masks/direct-image-mask.html
compositing/reflections/masked-reflection-on-composited.html
compositing/reflections/nested-reflection-mask-change.html
compositing/masks/simple-composited-mask.html

[WebKit Review Bot]

Created attachment 146790 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Penner]

Created attachment 146905 [details]
Patch

[Eric Penner]

Comment on attachment 146905 [details]
Patch

It looks like the visible rect is set late for replicas and masks (which would explain the image diffs). This uses the same traversal as painting to insure all the right layers get their priorities set.

[Adrienne Walker]

(In reply to comment #69)
> (From update of attachment 146905 [details])
> It looks like the visible rect is set late for replicas and masks (which would explain the image diffs). This uses the same traversal as painting to insure all the right layers get their priorities set.

That sounds like a bug to me.  We should set those visible layer rects in CCLayerTreeHostCommon and not in CCLayerTreeHost.

[Dana Jansens]

(In reply to comment #70)
> (In reply to comment #69)
> > (From update of attachment 146905 [details] [details])
> > It looks like the visible rect is set late for replicas and masks (which would explain the image diffs). This uses the same traversal as painting to insure all the right layers get their priorities set.
> 
> That sounds like a bug to me.  We should set those visible layer rects in CCLayerTreeHostCommon and not in CCLayerTreeHost.

Can it use the visibleLayerRect of the owning layer?

[Eric Penner]

(In reply to comment #71)
> (In reply to comment #70)
> > (In reply to comment #69)
> > > (From update of attachment 146905 [details] [details] [details])
> > > It looks like the visible rect is set late for replicas and masks (which would explain the image diffs). This uses the same traversal as painting to insure all the right layers get their priorities set.
> > 
> > That sounds like a bug to me.  We should set those visible layer rects in CCLayerTreeHostCommon and not in CCLayerTreeHost.
> 
> Can it use the visibleLayerRect of the owning layer?

Here's the comment from CCLayerTreeHost::paintMasksForRenderSurface where setVisibleLayerRect is called.

    // Note: Masks and replicas only exist for layers that own render surfaces. If we reach this point
    // in code, we already know that at least something will be drawn into this render surface, so the
    // mask and replica should be painted.

Looks like we should definitely move it out of there and set it when other visible rects are set.

[Dana Jansens]

(In reply to comment #72)
> (In reply to comment #71)
> > (In reply to comment #70)
> > > (In reply to comment #69)
> > > > (From update of attachment 146905 [details] [details] [details] [details])
> > > > It looks like the visible rect is set late for replicas and masks (which would explain the image diffs). This uses the same traversal as painting to insure all the right layers get their priorities set.
> > > 
> > > That sounds like a bug to me.  We should set those visible layer rects in CCLayerTreeHostCommon and not in CCLayerTreeHost.
> > 
> > Can it use the visibleLayerRect of the owning layer?
> 
> Here's the comment from CCLayerTreeHost::paintMasksForRenderSurface where setVisibleLayerRect is called.
> 
>     // Note: Masks and replicas only exist for layers that own render surfaces. If we reach this point
>     // in code, we already know that at least something will be drawn into this render surface, so the
>     // mask and replica should be painted.
> 
> Looks like we should definitely move it out of there and set it when other visible rects are set.

Ohh, ya I totally agree. Ignore my above question. I was looking for a calculateVisibleRect..

[James Robinson]

(In reply to comment #59)
> (From update of attachment 146674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146674&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:54
> >> +    void setTextureManager(CCPrioritizedTextureManager*);
> > 
> > if "be lost" means actually losing the backing and having to repaint then this won't work - we need to be able to call setTextureManager() on a layer without actually losing the contents. the reason is we have cases when rebuilding the compositing tree where a LayerChromium has a setLayerTreeHost(0) call then a setLayerTreeHost(host) where "host" is the same as the original host. we handle this by calling setTextureManager() on setLayerTreeHost(non-NULL) and making sure that setTM() is a no-op if the manager is the same, but destroys the texture if the new host is different.
> 
> I think it is only lost if the manager is changed to a different manager. Do you mean it should be able to keep the texture when the manager is temporarily NULL?

That's right.  TextureManager / ManagedTexture in the current code maintain an ad-hoc weak pointer representation to also deal with the case of a layer leaving the tree and then the tree being destroyed (along with its TextureManager) before the layer does.  I haven't looked to see if you have something like this in this patch (bit hard to tell in a 184k patch) but I suspect something similar will be needed.

[Eric Penner]

(In reply to comment #74)
> (In reply to comment #59)
> > (From update of attachment 146674 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146674&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:54
> > >> +    void setTextureManager(CCPrioritizedTextureManager*);
> > > 
> > > if "be lost" means actually losing the backing and having to repaint then this won't work - we need to be able to call setTextureManager() on a layer without actually losing the contents. the reason is we have cases when rebuilding the compositing tree where a LayerChromium has a setLayerTreeHost(0) call then a setLayerTreeHost(host) where "host" is the same as the original host. we handle this by calling setTextureManager() on setLayerTreeHost(non-NULL) and making sure that setTM() is a no-op if the manager is the same, but destroys the texture if the new host is different.
> > 
> > I think it is only lost if the manager is changed to a different manager. Do you mean it should be able to keep the texture when the manager is temporarily NULL?
> 
> That's right.  TextureManager / ManagedTexture in the current code maintain an ad-hoc weak pointer representation to also deal with the case of a layer leaving the tree and then the tree being destroyed (along with its TextureManager) before the layer does.  I haven't looked to see if you have something like this in this patch (bit hard to tell in a 184k patch) but I suspect something similar will be needed.

Hey James, I think it now supports everything needed, but let me run through what it does support to see if it matches all the expectations you are aware of, and doesn't cause any additional painting or introduce any avoidable bugs.

Since both the textures and the backings are needed in the manager, I didn't use handles (tokens) like the old texture manager but rather doubly linked the backings/textures. When either one of these is deleted the link is broken, so this achieves the same as the handles used in the old texture manager, and all the benefits of a weak pointer etc.

Setting the manager to be the same manager is now a no-op, but setting the manager to NULL will mean the texture backing is lost. I understood your previous comment to imply we just need a no-op when setting the manager to be *the same manager*, but not NULL. If we need to temporarily set the manager to NULL while keeping the backing, then I think there could be a problem, but I think it should be easy to fix if you point me to where this can happen.

[Eric Penner]

(In reply to comment #75)
> (In reply to comment #74)
> > (In reply to comment #59)
> > > (From update of attachment 146674 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=146674&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:54
> > > >> +    void setTextureManager(CCPrioritizedTextureManager*);
> > > > 
> > > > if "be lost" means actually losing the backing and having to repaint then this won't work - we need to be able to call setTextureManager() on a layer without actually losing the contents. the reason is we have cases when rebuilding the compositing tree where a LayerChromium has a setLayerTreeHost(0) call then a setLayerTreeHost(host) where "host" is the same as the original host. we handle this by calling setTextureManager() on setLayerTreeHost(non-NULL) and making sure that setTM() is a no-op if the manager is the same, but destroys the texture if the new host is different.
> > > 
> > > I think it is only lost if the manager is changed to a different manager. Do you mean it should be able to keep the texture when the manager is temporarily NULL?
> > 
> > That's right.  TextureManager / ManagedTexture in the current code maintain an ad-hoc weak pointer representation to also deal with the case of a layer leaving the tree and then the tree being destroyed (along with its TextureManager) before the layer does.  I haven't looked to see if you have something like this in this patch (bit hard to tell in a 184k patch) but I suspect something similar will be needed.
> 
> Hey James, I think it now supports everything needed, but let me run through what it does support to see if it matches all the expectations you are aware of, and doesn't cause any additional painting or introduce any avoidable bugs.
> 
> Since both the textures and the backings are needed in the manager, I didn't use handles (tokens) like the old texture manager but rather doubly linked the backings/textures. When either one of these is deleted the link is broken, so this achieves the same as the handles used in the old texture manager, and all the benefits of a weak pointer etc.
> 
> Setting the manager to be the same manager is now a no-op, but setting the manager to NULL will mean the texture backing is lost. I understood your previous comment to imply we just need a no-op when setting the manager to be *the same manager*, but not NULL. If we need to temporarily set the manager to NULL while keeping the backing, then I think there could be a problem, but I think it should be easy to fix if you point me to where this can happen.

I should also add that either the texture/manager can go away at any time and this just results in the texture being unregistered or manager being set to NULL.

[Eric Penner]

Created attachment 146993 [details]
Patch

[Eric Penner]

I (quite embarrassingly) removed the call to prioritizeTextures in the last CL, so only visible textures would get memory via requestLate(). This CL adds it back and also cleans up requestLate() a bit.

[Dana Jansens]

Comment on attachment 146993 [details]
Patch

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

yay green EWS. i like the "backing" terminology change.

The behaviour change I see with regard to the weak pointer business is that ManagedTexture keeps its textureId when it loses its TextureManager (when TextureManager is destroyed). But when you set a new TextureManager then it is lost/cleared. I don't think it's a feature of the current TM code though, to keep its textureId in this sxtate, it seems like it'd be a bug to destroy the TM without deleting all the textures. If you didn't it looks like a memory leak, where the textureId will never be evicted/deleted. So, if I understand correctly, I think this meets the requirements you are stating as well, JamesR.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:162
> +    m_memoryAboveThresholdBytes = newMemoryBytes;

What do you think of m_memoryAbovePriorityCutoffBytes?

It seems like we have "threshold" and "priority cutoff" to refer to the same thing, and the latter seems more clear to me.

[Vangelis Kokkevis]

Given how close we are to branching for M21 could we hold off on landing this until after the branch? It's a fairly big change and we will likely take some time for the dust to settle.

[Eric Penner]

(In reply to comment #80)
> Given how close we are to branching for M21 could we hold off on landing this until after the branch? It's a fairly big change and we will likely take some time for the dust to settle.

I wanted to be sure it was ready for M21 but no rush here on when to land it.

[Eric Penner]

(In reply to comment #79)
> (From update of attachment 146993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146993&action=review
> 
> yay green EWS. i like the "backing" terminology change.
> 
> The behaviour change I see with regard to the weak pointer business is that ManagedTexture keeps its textureId when it loses its TextureManager (when TextureManager is destroyed). But when you set a new TextureManager then it is lost/cleared. I don't think it's a feature of the current TM code though, to keep its textureId in this sxtate, it seems like it'd be a bug to destroy the TM without deleting all the textures. If you didn't it looks like a memory leak, where the textureId will never be evicted/deleted. So, if I understand correctly, I think this meets the requirements you are stating as well, JamesR.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:162
> > +    m_memoryAboveThresholdBytes = newMemoryBytes;
> 
> What do you think of m_memoryAbovePriorityCutoffBytes?
> 
> It seems like we have "threshold" and "priority cutoff" to refer to the same thing, and the latter seems more clear to me.

Thanks for pointing this out. I meant to convert everything to use cutoff but I guess I missed this.

[Eric Penner]

Created attachment 149416 [details]
Patch

[Eric Penner]

Sorry for the delay. This includes:
- some more renaming that was suggested
- removing ::swap function in favor of OwnPtr::swap
- converting ScrollBar code.

One extra thing about this patch is that all deleted textures get added to the pool of memory for recycling, so we will reach limit much faster than by just scrolling around with the previous memory manager. So we should insure we don't use the 'contents remainder' memory for anything (https://bugs.webkit.org/show_bug.cgi?id=89901 looks like it will take care of this quite nicely).

[Eric Penner]

Created attachment 149425 [details]
Patch

[Eric Penner]

Created attachment 149430 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149430 [details]
Patch

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

[Dana Jansens]

Comment on attachment 146993 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:341
> +#ifndef NDEBUG

nit: Oh, btw all these #ifndef NDEBUG should be #if !ASSERT_DISABLED. Asserts can be enabled in release builds with this.

[Eric Penner]

Created attachment 149608 [details]
Patch

[Adrienne Walker]

Comment on attachment 149608 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.h:48
> +

nit: no extra whitespace here.  Keep all the LayerChromium overrides together, please.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:359
> -            if (!tile)
> -                tile = createTile(i, j);
> +            ASSERT(tile); // Did setTexturePriorites get skipped?

Please be robust to null tiles.  See bug 89143.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-580
> -        tile->partialUpdate = false;

It's kind of unexpected not to clear the partial update flag in resetUpdateState.  Maybe you should call resetUpdateState at the start of setTexturePriorities rather than in updateLayerRect?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:719
> +            if (tile) {

style nit: early out. if !tile continue

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:490
> +    paintLayerContents(updateList, PrioritizePaintTextures, updater);

It feels a little awkward to me to shoehorn prioritization into paintLayerContents.  It seems like we'd grossly overestimate overdraw.

Is there some better way to do this? An iterator that includes masks and replicas, maybe? Or something else?

[Eric Penner]

Comment on attachment 149608 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:359
>> +            ASSERT(tile); // Did setTexturePriorites get skipped?
> 
> Please be robust to null tiles.  See bug 89143.

Good point. Must have overlooked this one.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-580
>> -        tile->partialUpdate = false;
> 
> It's kind of unexpected not to clear the partial update flag in resetUpdateState.  Maybe you should call resetUpdateState at the start of setTexturePriorities rather than in updateLayerRect?

Makes sense. I'll move it there, since it must always get called first now.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:719
>> +            if (tile) {
> 
> style nit: early out. if !tile continue

continue; got it.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:490
>> +    paintLayerContents(updateList, PrioritizePaintTextures, updater);
> 
> It feels a little awkward to me to shoehorn prioritization into paintLayerContents.  It seems like we'd grossly overestimate overdraw.
> 
> Is there some better way to do this? An iterator that includes masks and replicas, maybe? Or something else?

Already on it. Yes, I really didn't like reverting to this. The other big preventing a custom pass has been fixed but now strangely setTexturePriorities is being called twice(!) when I use the other layer traversal logic.  I'm tracking it down, after which this can go away. Then we can also merge the two paintLayerContents below and only have one! :)

[Dana Jansens]

Comment on attachment 149608 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:490
>>> +    paintLayerContents(updateList, PrioritizePaintTextures, updater);
>> 
>> It feels a little awkward to me to shoehorn prioritization into paintLayerContents.  It seems like we'd grossly overestimate overdraw.
>> 
>> Is there some better way to do this? An iterator that includes masks and replicas, maybe? Or something else?
> 
> Already on it. Yes, I really didn't like reverting to this. The other big preventing a custom pass has been fixed but now strangely setTexturePriorities is being called twice(!) when I use the other layer traversal logic.  I'm tracking it down, after which this can go away. Then we can also merge the two paintLayerContents below and only have one! :)

twice? You should be calling it only when the iterator representsItself(), is that it? And call for mask/replicamask when representsTargetRenderSurface().

[Eric Penner]

(In reply to comment #92)
> (From update of attachment 149608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149608&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:490
> >>> +    paintLayerContents(updateList, PrioritizePaintTextures, updater);
> >> 
> >> It feels a little awkward to me to shoehorn prioritization into paintLayerContents.  It seems like we'd grossly overestimate overdraw.
> >> 
> >> Is there some better way to do this? An iterator that includes masks and replicas, maybe? Or something else?
> > 
> > Already on it. Yes, I really didn't like reverting to this. The other big preventing a custom pass has been fixed but now strangely setTexturePriorities is being called twice(!) when I use the other layer traversal logic.  I'm tracking it down, after which this can go away. Then we can also merge the two paintLayerContents below and only have one! :)
> 
> twice? You should be calling it only when the iterator representsItself(), is that it? And call for mask/replicamask when representsTargetRenderSurface().

Arg! I rewrote the traversal code and that got dropped. That sounds like it! Thankfully a unit test caught it.

[Eric Penner]

Created attachment 149805 [details]
Patch

[Eric Penner]

(In reply to comment #94)
> Created an attachment (id=149805) [details]
> Patch

- Fixed issues Enne mentioned above
- Fixed unit tests that broke due to that change
- Added some "clean up" in ::reduceMemory() to insure we don't waste too much memory waiting for recycling.

[Eric Penner]

Created attachment 149816 [details]
rebase

[Adrienne Walker]

Comment on attachment 149816 [details]
rebase

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

R=me.  Let's land this puppy.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:505
> +void CCLayerTreeHost::prioritizeTextures(const LayerList& updateList)

So much better.  Thank you!

[Dana Jansens]

Comment on attachment 149816 [details]
rebase

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:520
> +            it->setTexturePriorities(calculator);
> +        else if (it->maskLayer())
> +            it->maskLayer()->setTexturePriorities(calculator);
> +        else if (it->replicaLayer() && it->replicaLayer()->maskLayer())
> +            it->replicaLayer()->maskLayer()->setTexturePriorities(calculator);

You'll call this on these layers twice. You want something like "else if representsTargetSurface && maskLayer" for each of the else cases.

[Eric Penner]

Comment on attachment 149816 [details]
rebase

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:520
>> +            it->replicaLayer()->maskLayer()->setTexturePriorities(calculator);
> 
> You'll call this on these layers twice. You want something like "else if representsTargetSurface && maskLayer" for each of the else cases.

I read the iterator doc but I didn't completely understand where masks and replicas come in.  Are they guaranteed to exist only in representsTargetSurface iterators?

[Eric Penner]

Comment on attachment 149816 [details]
rebase

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:520
>>> +            it->replicaLayer()->maskLayer()->setTexturePriorities(calculator);
>> 
>> You'll call this on these layers twice. You want something like "else if representsTargetSurface && maskLayer" for each of the else cases.
> 
> I read the iterator doc but I didn't completely understand where masks and replicas come in.  Are they guaranteed to exist only in representsTargetSurface iterators?

In any case, I'll add this since you clearly know more about this than me! :)

[Dana Jansens]

Comment on attachment 149816 [details]
rebase

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:520
>>> +            it->replicaLayer()->maskLayer()->setTexturePriorities(calculator);
>> 
>> You'll call this on these layers twice. You want something like "else if representsTargetSurface && maskLayer" for each of the else cases.
> 
> I read the iterator doc but I didn't completely understand where masks and replicas come in.  Are they guaranteed to exist only in representsTargetSurface iterators?

No, it's that the iterator can land on a layer both as representsContributingSurface and representsTargetSurface (and will for all surface owning layers except the root, where it is never a contributing surface). So if you want to call it only once on the layer, then you need to pick one. Since you want to include the root here, I think, you should use the representsTargetSurface

[Eric Penner]

Created attachment 149861 [details]
fix_iterator_for_masks

[WebKit Review Bot]

Comment on attachment 149861 [details]
fix_iterator_for_masks

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

[Dana Jansens]

Comment on attachment 149861 [details]
fix_iterator_for_masks

ya lgtm!

[Eric Penner]

Created attachment 149864 [details]
I_AM_SOFA_KING_WE_TA_DID

[WebKit Review Bot]

Comment on attachment 149864 [details]
I_AM_SOFA_KING_WE_TA_DID

Rejecting attachment 149864 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 149864 [details]
I_AM_SOFA_KING_WE_TA_DID

Rejecting attachment 149864 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Dana Jansens]

Lol Sofa King. The patch needs enne's name as reviewer in all the changelogs (looks like the Reviewed by NOBODY got removed from the first one). Then I can cq it for ya.

[Adrienne Walker]

Comment on attachment 149864 [details]
I_AM_SOFA_KING_WE_TA_DID

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

> Source/WebCore/ChangeLog:5
> +

Need a Reviewed by NOBODY (OOPS!). line here too.

Alternatively, add a "Reviewed by Adrienne Walker." line here and in the other ChangeLog.  You can then just leave the R field blank and CQ?.

[Eric Penner]

Created attachment 149995 [details]
fix_review

[Eric Penner]

(In reply to comment #110)
> Created an attachment (id=149995) [details]
> fix_review

Sorry guys, I didn't see the replies in my inbox or I would have done this early this morning. Hopefully it's still green!

[Eric Penner]

Created attachment 150000 [details]
rebase

[Adrienne Walker]

Comment on attachment 150000 [details]
rebase

R=meagain.

[WebKit Review Bot]

Comment on attachment 150000 [details]
rebase

Rejecting attachment 150000 [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:
um/tests/CCTiledLayerTestCommon.h:108: note: candidates are: WebKitTests::FakeTiledLayerChromium::FakeTiledLayerChromium(WebCore::CCPrioritizedTextureManager*)
Source/WebKit/chromium/tests/CCTiledLayerTestCommon.h:106: note:                 WebKitTests::FakeTiledLayerChromium::FakeTiledLayerChromium(const WebKitTests::FakeTiledLayerChromium&)
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/TiledLayerChromiumTest.o] Error 1
make: *** Waiting for unfinished jobs....

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

[WebKit Review Bot]

Comment on attachment 150000 [details]
rebase

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

[Eric Penner]

Created attachment 150054 [details]
rebase_again

[Eric Penner]

(In reply to comment #116)
> Created an attachment (id=150054) [details]
> rebase_again

Clean rebase != will build :(

Two unit tests got in that I just fixed. Unfortunately I can't get unit tests to build even on gclient branch right now (there is an odd unrelated link error I'm sorting out). But if it's green then this can land.

[WebKit Review Bot]

Comment on attachment 150054 [details]
rebase_again

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

New failing tests:
TiledLayerChromiumTest.nonIntegerContentsScaleIsNotDistortedDuringPaint
TiledLayerChromiumTest.nonIntegerContentsScaleIsNotDistortedDuringInvalidation

[WebKit Review Bot]

Created attachment 150077 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Penner]

Created attachment 150105 [details]
fix_tests_and_by_fix_I_mean_delete

[Eric Penner]

I tried quite hard to fix these tests but it was quite difficult as they were using a real BitmapCanvasLayerTextureUpdater class rather than a fake class like the rest of the tests (I now need to update textures for them to work, but this crashes etc with the real class). 

It seems that the main point of the tests is to insure we can do layer->content->layer space conversions correctly, so I'm wondering if we should just add some math functions in TiledLayerChromium and test those instead of doing a big integration test.

TiledLayerChromiumTest.nonIntegerContentsScaleIsNotDistortedDuringPaint
TiledLayerChromiumTest.nonIntegerContentsScaleIsNotDistortedDuringInvalidation

[Dana Jansens]

Hmm, there's no easy way to test TiledLayerChromium::updateLayerRect behaviour now, then.

[Dana Jansens]

Created attachment 150184 [details]
Diff to fix tests

False alarm, tests were not too bad to fix. Attached the diff here for reference. As per chat with Eric last night, I'm going to land this thing.

[Dana Jansens]

Created attachment 150188 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 150188 [details]
Patch for landing

Clearing flags on attachment: 150188

Committed r121574: <http://trac.webkit.org/changeset/121574>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nat Duca]

Way to go, Eric!

[Adrienne Walker]

Comment on attachment 145156 [details]
minor_tweak

Removing R? flag on old patch.