Closed Bug 1283236 Opened 5 years ago Closed 5 years ago

Don't use ForEachNode() in ApplyDoubleBuffering()

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: botond, Assigned: kevin.m.wern)

References

Details

(Keywords: arch, Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
Keywords: arch
Whiteboard: [gfx-noted]
Kevin, would you like to take this? Otherwise, I'm happy to.
Flags: needinfo?(kevin.m.wern)
I can revert that.
Flags: needinfo?(kevin.m.wern)
Assignee: nobody → kevin.m.wern
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/
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.
Flags: needinfo?(botond)
>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?
(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.
Flags: needinfo?(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.
(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.
Flags: needinfo?(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.
Flags: needinfo?(botond)
Do you intend to make changes to the patch, or did you mean to flag me for review?
Yes, I meant to flag it. Sorry.
Attachment #8768628 - Flags: review?(botond)
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 on attachment 8768628 [details]
Bug 1283236 - don't use ForEachNode() in ApplyDoubleBuffering()

https://reviewboard.mozilla.org/r/62766/#review60430
Attachment #8768628 - Flags: review?(botond) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75e220903bc5
don't use ForEachNode() in ApplyDoubleBuffering() r=botond
https://hg.mozilla.org/mozilla-central/rev/75e220903bc5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.