Closed
Bug 1283236
Opened 6 years ago
Closed 6 years ago
Don't use ForEachNode() in ApplyDoubleBuffering()
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
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.
Reporter | ||
Comment 1•6 years ago
|
||
Kevin, would you like to take this? Otherwise, I'm happy to.
Flags: needinfo?(kevin.m.wern)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → kevin.m.wern
Assignee | ||
Comment 3•6 years ago
|
||
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/
Assignee | ||
Comment 4•6 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b2c40ef3fa
Assignee | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
>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?
Reporter | ||
Comment 7•6 years ago
|
||
(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)
Reporter | ||
Comment 8•6 years ago
|
||
(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.
Assignee | ||
Comment 9•6 years ago
|
||
(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)
Reporter | ||
Comment 10•6 years ago
|
||
(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)
Reporter | ||
Comment 11•6 years ago
|
||
Do you intend to make changes to the patch, or did you mean to flag me for review?
Assignee | ||
Comment 12•6 years ago
|
||
Yes, I meant to flag it. Sorry.
Assignee | ||
Updated•6 years ago
|
Attachment #8768628 -
Flags: review?(botond)
Assignee | ||
Comment 13•6 years ago
|
||
Sorry for the delay in updating. I couldn't find the edit button on mobile, so I had to wait 'til I got home.
Reporter | ||
Comment 14•6 years ago
|
||
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+
Comment 15•6 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/75e220903bc5 don't use ForEachNode() in ApplyDoubleBuffering() r=botond
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75e220903bc5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•