1283236 - Don't use ForEachNode() in ApplyDoubleBuffering()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

In bug 1227231, we converted many loops over the Layer tree to use generic tree traversal algorithms such as ForEachNode().

One of the loops we converted was inside the ApplyDoubleBuffering() function in BasicLayerManager.cpp.

:jrmuizel was telling me today that he doesn't think this change was a readability win for this particular function.

When I reviewed that change (in bug 1227231 comment 8), I said I could go either way on whether or not we convert that function. As Jeff is one of the people who actually need to read and maintain that code, I propose we defer to his opinion, and change it back to a recursive loop.

Kevin, let me know if you have any thoughts / objections.

Comment 1

[Botond Ballo [:botond]]

Kevin, would you like to take this? Otherwise, I'm happy to.

Comment 2

[Kevin Wern]

I can revert that.

Comment 3

[Kevin Wern]

Attachment: Bug 1283236 - don't use ForEachNode() in ApplyDoubleBuffering()

We found that the use of TreeTraversal.h library here (where the root
node is treated as a special case) does not help readability, so revert
to the previous plain implementation

Review commit: https://reviewboard.mozilla.org/r/62766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62766/

Comment 4

[Kevin Wern]

Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b2c40ef3fa

Comment 5

[Kevin Wern]

Hey, Botond!

Just reverted the content of that function.  There is an additional function that I found with a similar idea (Collect3DContextLeaves), should I include that in the patch, as well? 

Additionally, do you think that it would be exploring the readability of other cases (i.e. using a stack, as in GetApzcAtPoint and bug 1227231; making recursive calls within a lambda function, as in WalkTheTree and NotifySubdocumentInvalidation)?  I know that using a stack was a paradigm that sort of crept up as we tackled more functions, and I decided to continue using them when given the choice because the precedent was set with GetApzcAtPoint. For the recursive call case, I'm not sure if there is a well-defined line (I think NotifySubdocumentInvalidation is more readable, for instance).

Obviously, this is only if you think revisiting this is worthwhile.

Comment 6

[Kevin Wern]

>I know that using a stack was a paradigm that sort of crept up as we tackled more functions, and I decided to continue using them when given the choice because the precedent was set with GetApzcAtPoint.

Sorry, to be clear, I mean that when we first used this paradigm, it was with a single function--when we found the other cases, it seemed like you were on the fence about converting those, as well. Do you think the stack paradigm is readable enough to be a more common pattern than what we saw when we first decided to apply it?

Comment 7

[Botond Ballo [:botond]]

(In reply to Kevin Wern from comment #5)
> There is an additional function
> that I found with a similar idea (Collect3DContextLeaves), should I include
> that in the patch, as well? 

I don't see a stack in Collect3DContextLeaves.

> Additionally, do you think that it would be exploring the readability of
> other cases (i.e. using a stack, as in GetApzcAtPoint and bug 1227231;

I believe the only other places where we have a stack are APZCTreeManager::GetApzcAtPoint() and AsyncCompositionManager::ApplyAsyncContentTransformToTree(). I'm one of the people who maintains those functions, and I don't have strong feelings either way about using the stack or not there. I say we leave things as they are (i.e. keep the stack) unless someone else expresses an opinion.

> making recursive calls within a lambda function, as in WalkTheTree and
> NotifySubdocumentInvalidation)?

The reason we make recursive calls there is that we are processing layers which are not part of the normal tree structure (in NotifySubdocumentInvalidation, a layer's mask layers; in WalkTheTree, a layer subtree that was just detached from the main tree) and so the traversal wouldn't otherwise reach them. I think that's fine.

Comment 8

[Botond Ballo [:botond]]

(In reply to Kevin Wern from comment #6)
> Sorry, to be clear, I mean that when we first used this paradigm, it was
> with a single function--when we found the other cases, it seemed like you
> were on the fence about converting those, as well. Do you think the stack
> paradigm is readable enough to be a more common pattern than what we saw
> when we first decided to apply it?

Going forward (e.g. if we do bug 1227233), I think it would make sense to evaluate the overall effect on readability on a case-by-case basis.

Comment 9

[Kevin Wern]

(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to Kevin Wern from comment #5)
> > There is an additional function
> > that I found with a similar idea (Collect3DContextLeaves), should I include
> > that in the patch, as well? 
> 
> I don't see a stack in Collect3DContextLeaves.

Oh, sorry. I thought the readability issue was related to special casing the root node, which occurs in ApplyDoubleBuffering() and was another thing you were on the fence about in the same comment (on Collect3DContextLeaves). That's what the 'similar idea' was. My bad.

Everything else sounds good to me.

Also, any reason why the try run is hanging on 17 jobs?  I was going to check the logs, but it seems like they aren't made available until after the job completes.

Comment 10

[Botond Ballo [:botond]]

(In reply to Kevin Wern from comment #9)
> (In reply to Botond Ballo [:botond] from comment #7)
> > (In reply to Kevin Wern from comment #5)
> > > There is an additional function
> > > that I found with a similar idea (Collect3DContextLeaves), should I include
> > > that in the patch, as well? 
> > 
> > I don't see a stack in Collect3DContextLeaves.
> 
> Oh, sorry. I thought the readability issue was related to special casing the
> root node, which occurs in ApplyDoubleBuffering() and was another thing you
> were on the fence about in the same comment (on Collect3DContextLeaves).
> That's what the 'similar idea' was. My bad.

Oh, right, I remember now. I was indeed on the fence about that. Still am. Would still lean towards not making a change unless someone asks.

> Also, any reason why the try run is hanging on 17 jobs?  I was going to
> check the logs, but it seems like they aren't made available until after the
> job completes.

We sometimes have issues with our test automation infrastructure that cause things like this. I do see a full complement of tests have run for Linux opt, so I'm happy with that.

Comment 11

[Botond Ballo [:botond]]

Do you intend to make changes to the patch, or did you mean to flag me for review?

Comment 12

[Kevin Wern]

Yes, I meant to flag it. Sorry.

Comment 13

[Kevin Wern]

Sorry for the delay in updating.  I couldn't find the edit button on mobile, so I had to wait 'til I got home.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8768628 [details]
Bug 1283236 - don't use ForEachNode() in ApplyDoubleBuffering()

https://reviewboard.mozilla.org/r/62766/#review60430

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75e220903bc5
don't use ForEachNode() in ApplyDoubleBuffering() r=botond

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/75e220903bc5