Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
195363007:
Avoid unnecessary descents into nested boxes with floats. (Closed)
|
Created:
6 years, 9 months ago by rhogan Modified:
6 years, 5 months ago CC:
bemjb+rendering_chromium.org, blink-reviews, dsinclair, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAvoid unnecessary descents into nested boxes with floats.
Steal a bit from RenderBlock to avoid descending through the same
stack of nested boxes multiple times while preparing to lay them out.
This CL creates a 4% improvement in the
Performance/Layout/floats_*_nested.html tests.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170060
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178193
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased #Patch Set 3 : Trying again #
Total comments: 5
Patch Set 4 : Updated #
Total comments: 3
Patch Set 5 : Updated #
Messages
Total messages: 42 (0 generated)
I like the idea. https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBlock.h (right): https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBlock.h:556: mutable unsigned m_descendantsWithFloatsMarkedForLayout : 1; Why is this a member of RenderBlock instead of on RenderBlockFlow?
On 2014/03/20 20:00:10, Bem Jones-Bey (adobe) wrote: > I like the idea. Cool! > > https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... > File Source/core/rendering/RenderBlock.h (right): > > https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... > Source/core/rendering/RenderBlock.h:556: mutable unsigned > m_descendantsWithFloatsMarkedForLayout : 1; > Why is this a member of RenderBlock instead of on RenderBlockFlow? Correct me if I'm wrong here: but it wouldn't actually save any memory doing that since RenderBlockFlow inherits the whole 32-bits anyway. So I imagine it's best to keep them all in the same place so we can keep a proper count of them.
On 2014/03/20 20:06:49, rhogan wrote: > On 2014/03/20 20:00:10, Bem Jones-Bey (adobe) wrote: > > I like the idea. > > Cool! > > > > > > https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... > > File Source/core/rendering/RenderBlock.h (right): > > > > > https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... > > Source/core/rendering/RenderBlock.h:556: mutable unsigned > > m_descendantsWithFloatsMarkedForLayout : 1; > > Why is this a member of RenderBlock instead of on RenderBlockFlow? > > Correct me if I'm wrong here: but it wouldn't actually save any memory doing > that since RenderBlockFlow inherits the whole 32-bits anyway. So I imagine it's > best to keep them all in the same place so we can keep a proper count of them. I actually was mostly thinking in a code readability sense, since my first though upon looking at this was "why does RenderBlock have to know about floats?" But you raise a good point about the memory use, but I would ask for guidance on that from a more experienced C++ person myself. If it will add too much memory overhead to move that member to RenderBlockFlow, I guess we'll just have to leave it in RenderBlock. So lgtm, assuming that we can't move it to RenderBlockFlow for memory reasons. (Though I'm not an owner, so you'll have to wait for one of the others to chime in anyways. :-)
On 2014/03/20 20:17:50, Bem Jones-Bey (adobe) wrote: > On 2014/03/20 20:06:49, rhogan wrote: > > On 2014/03/20 20:00:10, Bem Jones-Bey (adobe) wrote: > > > I like the idea. > > > > Cool! > > > > > > > > > > > https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... > > > File Source/core/rendering/RenderBlock.h (right): > > > > > > > > > https://codereview.chromium.org/195363007/diff/1/Source/core/rendering/Render... > > > Source/core/rendering/RenderBlock.h:556: mutable unsigned > > > m_descendantsWithFloatsMarkedForLayout : 1; > > > Why is this a member of RenderBlock instead of on RenderBlockFlow? > > > > Correct me if I'm wrong here: but it wouldn't actually save any memory doing > > that since RenderBlockFlow inherits the whole 32-bits anyway. So I imagine > it's > > best to keep them all in the same place so we can keep a proper count of them. > > I actually was mostly thinking in a code readability sense, since my first > though upon looking at this was "why does RenderBlock have to know about > floats?" > > But you raise a good point about the memory use, but I would ask for guidance on > that from a more experienced C++ person myself. If it will add too much memory > overhead to move that member to RenderBlockFlow, I guess we'll just have to > leave it in RenderBlock. I don't have a good answer either so it seems fine to go the conservative route (with a FIXME or comment explaining why) unless we get someone to chime in. Robert, are there some performance wins with this change? Also no need for an empty BUG= line in the description.
On 2014/03/21 22:40:46, Julien Chaffraix - PST wrote: > > Robert, are there some performance wins with this change? > > Also no need for an empty BUG= line in the description. I've updated the issue description to state the performance improvement.
looks reasonable, lgtm Levi should comment too as he knows the float code better than I do.
The CQ bit was checked by ojan@chromium.org
The CQ bit was unchecked by ojan@chromium.org
LGTM as well.
The CQ bit was checked by ojan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robhogan@gmail.com/195363007/1
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/rendering/RenderBlockFlow.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/rendering/RenderBlockFlow.cpp
Hunk #1 FAILED at 459.
Hunk #2 succeeded at 1691 (offset -24 lines).
1 out of 2 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderBlockFlow.cpp.rej
Patch: Source/core/rendering/RenderBlockFlow.cpp
Index: Source/core/rendering/RenderBlockFlow.cpp
diff --git a/Source/core/rendering/RenderBlockFlow.cpp
b/Source/core/rendering/RenderBlockFlow.cpp
index
cb3a38f358f9fa854599a51d5ea3505850a9081c..a01af7a465790dd182323d219f4d78ebe554133b
100644
--- a/Source/core/rendering/RenderBlockFlow.cpp
+++ b/Source/core/rendering/RenderBlockFlow.cpp
@@ -459,6 +459,7 @@ inline bool RenderBlockFlow::layoutBlockFlow(bool
relayoutChildren, LayoutUnit &
}
clearNeedsLayout();
+ m_descendantsWithFloatsMarkedForLayout = false;
return true;
}
@@ -1715,6 +1716,10 @@ void
RenderBlockFlow::markAllDescendantsWithFloatsForLayout(RenderBox* floatToRe
if (!everHadLayout() && !containsFloats())
return;
+ if (m_descendantsWithFloatsMarkedForLayout)
+ return;
+ m_descendantsWithFloatsMarkedForLayout = true;
+
MarkingBehavior markParents = inLayout ? MarkOnlyThis :
MarkContainingBlockChain;
setChildNeedsLayout(markParents);
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robhogan@gmail.com/195363007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robhogan@gmail.com/195363007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robhogan@gmail.com/195363007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robhogan@gmail.com/195363007/20001
Message was sent while issue was closed.
Change committed as 170060
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/214523007/ by robhogan@gmail.com. The reason for reverting is: Caused a lot of float crashes in ClusterFuzz https://code.google.com/p/chromium/issues/detail?id=357242.
https://codereview.chromium.org/195363007/diff/230001/Source/core/rendering/R... File Source/core/rendering/RenderBlockFlow.cpp (right): https://codereview.chromium.org/195363007/diff/230001/Source/core/rendering/R... Source/core/rendering/RenderBlockFlow.cpp:1702: m_descendantsWithFloatsMarkedForLayout = !m_descendantsWithFloatsMarkedForLayout && !floatToRemove; The old code would just mark m_descendantWithFloatsMarkedForLayout to true. In this code, you could potentially flip the value of the boolean from true to false (because floatToRemove is true). Is that really intended? If so, I would need some comment explaining the details as I have a hard time understand the boolean logic on this line.
Forgot to look at the tests. I only commented on one but the comments apply to all the tests. https://codereview.chromium.org/195363007/diff/230001/LayoutTests/fast/block/... File LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash-expected.html (right): https://codereview.chromium.org/195363007/diff/230001/LayoutTests/fast/block/... LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash-expected.html:1: <style> DOCTYPE on all tests!!!! (I hate that Clusterfuzz output is using quirks mode) https://codereview.chromium.org/195363007/diff/230001/LayoutTests/fast/block/... LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash-expected.html:22: try { document.documentElement.appendChild(nodes[72]); } catch(e) {} Can we clean that up? All these try { } catch {} are unneeded (or the line could be removed) and obfuscate the test. https://codereview.chromium.org/195363007/diff/230001/LayoutTests/fast/block/... LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash-expected.html:23: setTimeout('clear();', 700); This is never run under DRT as you don't call testRunner.waitUntilDone() so it should be removed. https://codereview.chromium.org/195363007/diff/230001/LayoutTests/fast/block/... LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash-expected.html:27: I really think that this should be a text-only test. It should also have a description.
lgtm https://codereview.chromium.org/195363007/diff/250001/LayoutTests/fast/block/... File LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash.html (right): https://codereview.chromium.org/195363007/diff/250001/LayoutTests/fast/block/... LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash.html:48: setTimeout('clear();', 700); Do we really need a 700ms sleep. Usually forcing a layout and then triggering the bug is enough. If that's not enough, having a smaller interval would be better. https://codereview.chromium.org/195363007/diff/250001/LayoutTests/fast/block/... LayoutTests/fast/block/float/float-mark-descendants-for-layout-crash.html:49: } Let's have something in the output that says what are the condition for passing. Or even PASSED added to the tree at the end of clear().
https://codereview.chromium.org/195363007/diff/250001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.h (right): https://codereview.chromium.org/195363007/diff/250001/Source/core/rendering/R... Source/core/rendering/RenderBlock.h:534: mutable signed m_lineHeight : 25; Can you help me understand why this change from 26 to 25 bits is safe?
On 2014/04/26 01:47:40, pdr wrote: > https://codereview.chromium.org/195363007/diff/250001/Source/core/rendering/R... > File Source/core/rendering/RenderBlock.h (right): > > https://codereview.chromium.org/195363007/diff/250001/Source/core/rendering/R... > Source/core/rendering/RenderBlock.h:534: mutable signed m_lineHeight : 25; > Can you help me understand why this change from 26 to 25 bits is safe? It's not safe. The minimum number of safe bits for a value exposed to CSS is 26 signed and 25 unsigned. If I set line-height: 67108864 then instead of max'ing to 33554321 it will overflow to -1.
Another go?
Yeah! LGTM.
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robhogan@gmail.com/195363007/270001
Message was sent while issue was closed.
Change committed as 178193 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
