Simplify TiledContentHost updating code

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

29 Branch
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Posted patch simplify-tiled-update (obsolete) — Splinter Review
It seems overly complex currently.

The current patch seems a bit simple, but I can't think of anything wrong with it.

I'll let you think about this for a bit, let's talk tomorrow :)
Attachment #8629100 - Flags: feedback?(nical.bugzilla)
Attachment #8629102 - Flags: review?(nical.bugzilla)
Attachment #8629100 - Flags: feedback?(bgirard)
Comment on attachment 8629100 [details] [diff] [review]
simplify-tiled-update

Review of attachment 8629100 [details] [diff] [review]:
-----------------------------------------------------------------

The previous-lock thing is unfortunately necessary to avoid locking contention with gralloc buffers that are unlocked "some time after the composition of the next frame in which they are not used". We tried that when implementing tiling for b2g and I tried again when I did the TiledContentHost cleanup (getting the ReadUnlock stuff to work was 90% of the time I spent when cleaning this code up).
But yeah that makes things annoyingly complicated.
Attachment #8629100 - Flags: feedback?(nical.bugzilla) → feedback-
Attachment #8629102 - Flags: review?(nical.bugzilla) → review-
Posted patch simplify-tiled-update v2 (obsolete) — Splinter Review
This addresses the issue that we'd lose the bindings to TextureSources when updating tiles, and makes sure we don't unlock gralloc tiles until we're sure they can actually be re-used.

I've added a lot more comments too to make it clearer.
Attachment #8629100 - Attachment is obsolete: true
Attachment #8629100 - Flags: feedback?(bgirard)
Attachment #8629494 - Flags: feedback?(nical.bugzilla)
Blocks: 1180326
Comment on attachment 8629494 [details] [diff] [review]
simplify-tiled-update v2

Review of attachment 8629494 [details] [diff] [review]:
-----------------------------------------------------------------

f- because of the tile reordering optimization (step 2) going away, but the rest of the patch might be good. Have you tried it on both the HasInternalBuffer and non-HasInternalBuffer configurations? Took me a while to get the ReadLock stuff to work in both cases.

::: gfx/layers/composite/TiledContentHost.cpp
@@ -237,5 @@
> -  }
> -
> -  // Step 2, move the tiles in mRetainedTiles at places that correspond to where
> -  // they should be with the new retained with and height rather than the
> -  // old one.

This patch gets rid of Step two, which means that if the tiles placements change (we have a new row of tiles while scrolling vertically, the layer is resized, etc), we miss the fast path of keeping the Gralloc buffer bound to the gl texture handle.

consider the following tile buffer of 2x2 tiles. 

A B
C D

we scroll up, add a row at the top (tiles E and F) and remove the bottom row (tiles C and D):

E F
A B

In this situation we currently make sure that A and B are moved to the right place in the tile buffer. This means that when A and B do PrepareTextureSource, they go through the fast path, because they are still attached to their respective TextureSources (so PrepareTextureSource is a no-op).

The tile buffer is ordered as follows:
before: [A C B D], then after the transaction: [E A F B] 

With your patch, what will happen in this case, is that E will be at the place in the tile buffer where A used to be, and UseTileTexture on E will brings A's mCompositableRefs to zero, which will unbind the TextureSource (see CompositableTextureRef, TextureHost::RemoveCompositableRef and UnbindTextureSource).
Then, when we do UseTileTexture on A, it has lost its TextureSource and needs to create a new one and do the gralloc-to-gl-handle binding which is expensive.

Basically, one of the rules to keep things optimal is: If your TextureHost is going to be used again the next frame, do not remove it from its CompositableTextureRef slot, otherwise it will loose its TextureSource and have to either bind to a new one that is available (expensive) or create a new TextureSource and bind to it (quite expensive).

I recognize how terrible it is that all of this is implicitly driven by how many CompositableTextureRefs are pointing to a given TextureHost, but this is the only way we have been able to optimally handle the crazy use-cases that we have in the compositor (including the same TextureHost being used in several layers) combined with the way gralloc works.
Attachment #8629494 - Flags: feedback?(nical.bugzilla) → feedback-
I'm not convinced that this is broken.

My code swaps the current set of tiles into oldRetainedTiles which should guarantee that there's always at least one CompositableRef to each the previous tiles for the rest of the process.

We then construct the new tile buffer into the empty mRetainedTiles so we shouldn't be removing any references during this process.

If any of the tiles exist in both the old and new sets, then GrallocTextureHostOGL::mGLTextureSource should still be set (since the number of compositable refs never dropped to 0), and we should hit the early return from PrepareTextureSource.

Once we finish setting up the new tiles, we let oldRetainedTiles go out of scope, releasing all the unused tiles and letting the re-used ones drop to 1 compositable ref.
Actually, I see one bug. The new code adds a read lock to every tile in the transaction, and releases all of them on the host side (at one of 3 possible times: after upload, after next transaction is received, after following composite).

The current lock implementations get initialized with a lock count of 1, and nothing will release that initial ref. Initializing them with a lock count of 0 should fix that.
Posted patch Simplify locking/unlocking (obsolete) — Splinter Review
I think this piece is safe(ish) and simpler than the old code.
Attachment #8630097 - Flags: review?(nical.bugzilla)
Attachment #8629494 - Attachment is obsolete: true
Unfortunately this is no longer a code reduction, but more :(

I think it's easier to understand because the two different forms of recycling (where we recycle the TextureSource+binding, and where we just recycle the TextureSource) are handled explicitly.

It also should perform better, because in the scrolling example you gave above we'd manage to recycle the TextureSources for C and D would be recycled onto E and F, where they wouldn't have been previously.

The recycling search is potentially now O(n) instead of O(1), but mFirstPossibility should largely mitigate that and tile sets are fairly small.
Attachment #8630100 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8629102 [details] [diff] [review]
Remove painted region

Review of attachment 8629102 [details] [diff] [review]:
-----------------------------------------------------------------

Re-adding r? because you didn't add any comments for this last time.

It appears to be a fairly simple unused variable removal, unless you're using again in a different bug?
Attachment #8629102 - Flags: review- → review?(nical.bugzilla)
Comment on attachment 8629102 [details] [diff] [review]
Remove painted region

Review of attachment 8629102 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, it's dead code now.
Attachment #8629102 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8630097 [details] [diff] [review]
Simplify locking/unlocking

Review of attachment 8630097 [details] [diff] [review]:
-----------------------------------------------------------------

If you think that's simpler, I'm fine with this (I spent too long working on this code to have a unbiased opinion about it). This kind of changes to the ReadUnlock stuff is a bit too tricky for me to review properly but I am pretty sure that it'll show up quickly on tbpl if anything's wrong in that area.
r+ with the comment about LockedUntilComposite addressed.

::: gfx/layers/composite/TextureHost.h
@@ +422,5 @@
> +   * the composite following the last time it was used, and should
> +   * not be reused until that time to avoid contention. This is
> +   * true for Gralloc backed textures.
> +   */
> +  virtual bool LockedUntilComposite() { return false; }

I think that this is pretty much equivalent to !HasInternalBuffer(). I don't think there is any kind of direct-gpu-texture that you can unlock without waiting for the frame of delay (or stalling the gpu), so you should just use !HasInternalBuffer instead (or have this function return !HasInternalBuffer if you prefer).

::: gfx/layers/composite/TiledContentHost.cpp
@@ +182,5 @@
> +    TileHost& tile = mRetainedTiles[i];
> +    // Tile with an internal buffer get unlocked as soon as we've uploaded,
> +    // so won't have a lock at this point.
> +    if (tile.mTextureHost && tile.mSharedLock) {
> +      // Some texture host types aren't truely unlocked (by the driver)

nit: truely -> truly (I think?)
Attachment #8630097 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #11)
> nit: truely -> truly (I think?)

You're very much correct, not sure where that came from.
Posted patch Simplify locking/unlocking v2 (obsolete) — Splinter Review
Fixed review comments. Removed the changes to the initial lock counts, as it appears that TiledContentClient treats a lock count of 1 as 'this tile can be reused'.

Carrying r=nical.
Attachment #8630097 - Attachment is obsolete: true
Attachment #8630608 - Flags: review+
Now with more qref.
Attachment #8630608 - Attachment is obsolete: true
Attachment #8630610 - Flags: review+
Extended the comments after talking with Jeff, and fixed a few bugs.
Attachment #8630100 - Attachment is obsolete: true
Attachment #8630100 - Flags: feedback?(nical.bugzilla)
Attachment #8630761 - Flags: review?(nical.bugzilla)
Comment on attachment 8630761 [details] [diff] [review]
Rewrite TextureSource recycling

Review of attachment 8630761 [details] [diff] [review]:
-----------------------------------------------------------------

I think that this is almost good. Looks like you don't unbind the TextureSources from the TextureHosts that are not used during that frame (after you have recycled things). This means that a TextureSource that wasn't reused for this frame would stay attached to its TextureHost (himself not used either) and the TextureHost's gralloc buffer will then remain locked. The corresponding TextureClient is most likely sitting in a pool on the other side and we need it to be unlocked to reuse it (and on ics the only way to know if it is still locked is to wait on the lock).

UnbindTextureSource simply nulls out the TextureHost's reference to the TextureSource (so it's not clear at first why it is important), but it's that reference that keeps the TextureSource alive. Destroying the TextureSource destroys its gl handle, and that unlocks the gralloc buffer that is still attached to it.
Comment on attachment 8630761 [details] [diff] [review]
Rewrite TextureSource recycling

Review of attachment 8630761 [details] [diff] [review]:
-----------------------------------------------------------------

By the way, this patch will make snorp sad, because he is writing a patch that relies on keeping track of the state of tiles across transactions (to add a fade-in animation during progressive tiling).
I mentioned this bug to him yesterday, but adding ni? to be sure. Snorp, please base your work on top of this patch :)
Flags: needinfo?(snorp)
(In reply to Nicolas Silva [:nical] from comment #16)
> Comment on attachment 8630761 [details] [diff] [review]
> Rewrite TextureSource recycling
> 
> Review of attachment 8630761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that this is almost good. Looks like you don't unbind the
> TextureSources from the TextureHosts that are not used during that frame
> (after you have recycled things). This means that a TextureSource that
> wasn't reused for this frame would stay attached to its TextureHost (himself
> not used either) and the TextureHost's gralloc buffer will then remain
> locked. The corresponding TextureClient is most likely sitting in a pool on
> the other side and we need it to be unlocked to reuse it (and on ics the
> only way to know if it is still locked is to wait on the lock).
> 
> UnbindTextureSource simply nulls out the TextureHost's reference to the
> TextureSource (so it's not clear at first why it is important), but it's
> that reference that keeps the TextureSource alive. Destroying the
> TextureSource destroys its gl handle, and that unlocks the gralloc buffer
> that is still attached to it.

Don't we get this for free, thanks to CompositableTextureHostRef?

Once we've finished recycling, any TextureHost/TextureSource pairs that weren't re-used (and weren't recycled) should now only exist in the oldRetainedTiles array.

When that array goes out of scope, we'll release the reference to the TextureHost, and that will result in a call to TextureHost::ReleaseCompositableRef. That function calls UnbindTextureSource for us if this was the last compositable ref (which it should be).
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)> 
> Don't we get this for free, thanks to CompositableTextureHostRef?
 
Actually, you are right.
Yeah, I will rebase my stuff on this patch.
Flags: needinfo?(snorp)
Comment on attachment 8630761 [details] [diff] [review]
Rewrite TextureSource recycling

Review of attachment 8630761 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this patch makes things any simpler, but it has the argument of a potential perf improvement through recycling TextureSources more intelligently so I give a somewhat reluctant r+ for that.
Attachment #8630761 - Flags: review?(nical.bugzilla) → review+
You need to log in before you can comment on or make changes to this bug.