Closed Bug 495385 Opened 16 years ago Closed 15 years ago

Don't create frames for collapsible whitespace adjacent to block boundaries

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

(Depends on 4 open bugs)

Details

Attachments

(8 files, 1 obsolete file)

raw frame data from the Tp3 page set (CSV format)
18.69 KB, text/plain
Details
frame-stats patch
3.82 KB, patch
Details | Diff | Splinter Review
WIP
46.06 KB, patch
Details | Diff | Splinter Review
fix v2
85.37 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
supplemental fix for
5.07 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
textarea fix v2
4.95 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Tdhtml test results from all boxes
35.44 KB, patch
Details | Diff | Splinter Review
innerHTML testcase
4.04 KB, patch
Details | Diff | Splinter Review
Right now if you have markup like <body> <div>...</div> <span>Hello</span> </body> We create text frames for the whitespace between <body> and <div>, the whitespace between </div> and <span>, and for the whitespace between </span> and </body>. If this text is white-space:normal it can't affect the layout. We should just not create frames for it. Boris already created a way for whitespace-only text nodes in tables to cause frame reconstruction if the text is changed to something that's not just whitespace. We can easily make 'white-space' style changes cause frame reconstruction. We should do this.
This save memory and speed up layout. It would be even more useful if we move to separating "blocks of blocks" from 'blocks of inlines"; without this optimization we'd be forced to create a lot of anonymous "blocks of inlines" containing only whitespace. It should also make frame dumps a lot easier to read.
Some ideas: -- add flags to ConstructFramesFromItemList and ProcessChildren to indicate if the frame(s) will be placed at the beginning and/or end of the parent's child list -- mark FrameConstructionItems with a flag to indicate if they're going to generate a block boundary before and after themselves -- then in ConstructFramesFromItemList we can pass a flag in to ConstructFramesFromItem to indicate if there is a block boundary before or after the item, and ConstructTextFrame can check that flag and avoid creating a frame if it's set and other conditions are met
ProcessChildren() is always called on brand-new frames, so there's no need for a flag there, methinks. > -- mark FrameConstructionItems with a flag to indicate if they're going to > generate a block boundary before and after themselves How would this differ from the existing mHasInlineEnds? Or rather the negation thereof? Though note that for placeholders this boolean might not be exact... but only if the parent is inline, which shouldn't matter here. If we suppress whitespace at the end of a block after an inline, then we need to somehow deal with another inline being appended later. As a first cut, it might be better to not suppress such whitespace (especially because it would go into the same block as the inline once we have blocks of blocks/inlines. More generally, we need to think about what to do when whitespace got suppressed and then inline nodes are inserted next to that textnode....
Yeah, I've been thinking about that. One way might be to have block frames actually hold references to any text nodes whose frames are being suppressed by the boundaries of the block. A given block can potentially suppress up to 4 text frames: before and after its start, and before and after its end. I want to study some page sets to see how much benefit there is from handling each of those cases. It's possible that most of the benefit comes from handling text nodes after the start and after the end, which seems easier to handle than the before cases. In particular, if we only suppress text nodes after a block boundary, we don't have to ever fix things up when appending frames.
This data was obtained via a patch I'll attach in second
The data includes the chrome window, a session restore window, the hidden window, and maybe some other cruft, but that shouldn't matter. Summing the numbers over all pages, we get the following results: If we remove all collapsible whitespace in blocks adjacent to a block boundary, we get rid of 64.9% of all whitespace-only text frames, 30.46% of all text frames, 11.01% of all frames. If we remove all collapsible whitespace in blocks that are at the start of a block or after a block, we get rid of a 47.5% of all whitespace-only text frames, 22.33% of all text frames, 8.08% of all frames. If we get rid of all collapsible whitespace-only frames at the start of a block, we get rid of 21845 frames. If we then get rid of all collapsible whitespace-only frames after a block, we get rid of 25287 frames --- so that part of the optimization is really valuable. If we then get rid of all CWO before a block, we only get rid of 3940 frames --- not valuable. If we then get rid of all CWO at the end of a block, that's 13204 frames --- somewhere in between.
Handling dynamic changes shouldn't be hard in principle. A text node can be suppressed because it's the first or last child of a block, or its prev or next sibling is non-inline. So the only way removing content can make a text node need a frame is if we're removing a non-inline that's before or after the text node. The only way inserting content can make a text node need a frame is if we insert an inline at the start of a block before a text node, at the end of a block after a text node, or between a text node and a non-inline. In other words, when we remove a non-inline element we should create frames for text nodes immediately before and after it. When we insert an inline element we need to create frames for text nodes immediately before and after it.
Do I need to add these frame-less text nodes to the undisplayed content map?
> In other words, when we remove a non-inline element we should create frames for > text nodes immediately before and after it. You mean maybe create frames, right? It might still need to be suppressed, as far as I can tell. > Do I need to add these frame-less text nodes to the undisplayed content map? No. In fact, if you do you'll get asserts.
(In reply to comment #10) > You mean maybe create frames, right? It might still need to be suppressed, as > far as I can tell. Right. > > Do I need to add these frame-less text nodes to the undisplayed content map? > > No. In fact, if you do you'll get asserts. OK thanks. (In reply to comment #8) > In other words, when we remove a non-inline element we should create frames for > text nodes immediately before and after it. When we insert an inline element we > need to create frames for text nodes immediately before and after it. Since XBL can reorder content using insertion points, we should just disable textframe suppression when the parent element has an XBL binding, right?
So disabling when the parent has an XBL binding is not enough, because the parent might be the parent of a <children> too.... So if you're doing this on the DOM level, you need to worry about that situation. Now if you compare to other things in the list the ChildIterator has that might be ok. We also have some non-xbl-related things with multiple insertion points (outer tables, fieldsets), but this should all be a non-issue there so far, I think. I'm not sure I can say anything else useful so far; I'm not sure exactly where you're making your decisions about when to suppress.
(In reply to comment #12) > So disabling when the parent has an XBL binding is not enough, because the > parent might be the parent of a <children> too.... But we can detect that it's part of a binding and block it in that case, I think.
That should work, yeah. At least until people start seriously using XBL2 on the web...
Attached patch WIP — — Splinter Review
Backup in case I get mugged on the way home. This patch seems to suppress the text frames just fine, but I need to write a lot of tests for the interesting difficult cases.
I've got this pretty much working. Try-server results show a 1-2% Talos Tp improvement, and some small memory usage reduction. I found one case where it does change rendering. Suppose we have something like <div style="height:100px; -moz-column-width:50px; -moz-column-rule:2px solid black;"> <div style="border-bottom:200px solid blue;"></div> </div> On trunk, we place the inner <div>, and realize that we're over the height constraint. So we create a new column to contain the trailing whitespace. With my work here, that trailing whitespace doesn't have a frame so we don't create a new column --- the outer div is complete after we've placed the inner div. I think this is actually an improvement, so I don't want to try to preserve the old behaviour. I think the ideal behaviour would be to always avoid starting a new column for lines whose content is entirely collapsed away. This would also fix bug 493649 properly, I think.
Hmm, I just realized we can suppress collapsible whitespace before and after <br> as well ...
Adding the <br>s should let us get rid of another 0.83% of all frames, so I'll do that.
Probably worth having a bit in fcdata and a method on fcitem to indicate whitespace suppression; then we can have the method check for the bit or inline ends. Then if we discover more cases where we can suppress, we can add them easily.
One question is whether we should have one "line break" bit or two (one for each side). Since it's not really clear what we would need for future cases, I'm inclined to just not use an fcdata bit for now (although things are encapsulated in FrameConstructionItem::IsLineBoundary()).
OK, that works too.
I take that back, I'll use an FCDATA bit.
Attached patch fix (obsolete) — — Splinter Review
Some comments: -- The accessibility API exposes "text as rendered". This is not allowed to trim trailing whitespace on a line which ends in a soft break, because you'd lose word breaks, but I think it's fine to trim trailing whitespace before a hard break. Doing the latter in all cases feels like unnecessary complexity, but with this patch we will trim trailing whitespace before a hard break in most cases. So I had to fix the accessibility test to give the right values. -- The majority of the patch is just passing around flags or content indexes. -- border-breaking-004-cols.xhtml fails with this patch. I've altered the test to make it clear that something basically analogous already failed without this patch.
Attachment #382079 - Flags: superreview?(bzbarsky)
Attachment #382079 - Flags: review?(bzbarsky)
>+++ b/layout/base/nsCSSFrameConstructor.h > // Add the frame construction items for the given aContent and aParentFrame > // to the list. This might add more than one item in some rare cases. > void AddFrameConstructionItems(nsFrameConstructorState& aState, > nsIContent* aContent, >+ PRInt32 aContentIndex, > nsIFrame* aParentFrame, > FrameConstructionItemList& aItems); Document that aContentIndex == aContent->GetNodeParent()->IndexOf(aContent), and maybe assert that? Of if it's always allowed to be -1 and that will just lead to not doing this optimization, document that too (and change assert accordingly)? >+ /* If FCDATA_IS_LINE_BREAK is set, the the frame is something that will s/the the/then the/. And same in the previous comment you copied this from. I thought I'd gotten all of those.... :( > FrameConstructionItem* AppendItem(const FrameConstructionData* aFCData, > nsIContent* aContent, > nsIAtom* aTag, > PRInt32 aNameSpaceID, >+ PRInt32 aContentIndex, Again, document and assert the allowed values of aContentIndex. Or do that in the FrameConstructionItem constructor, perhaps. Given the comment on the mContentIndex member, just the assert might be enough, even. >+ // If aParentContent's child at aContentIndex is a text node and >+ // doesn't have a frame, add a frame construction item for it to aItems. >+ void AddTextItemIfNeeded(nsIFrame* aParentFrame, Worth documenting that the new item is appended to aItems, right? >+ LIST_STARTS_AT_LINE_BOUNDARY = 0x01, >+ LIST_ENDS_AT_LINE_BOUNDARY = 0x02, Maybe LIST_PRECEDED_BY_LINE_BOUNDARY and LIST_FOLLOWED_BY_LINE_BOUNDARY? Either way, I guess. >+ LIST_NO_XBL_SIBLINGS = 0x04 This needs some commenting. Or maybe renaming LIST_FLATTENED_TREE_MATCHES_DOM_FOR_PARENT or something? Or both... See more comments on this below. Also, I'd prefer LIST_NO_FLAGS = 0 in there and use wherever we use 0 to pass to this method.... It's probably too much trouble to create a name for this enum type so as to enforce not passing in bare integers, but the 0s were confusing me. Worth documenting for all three of the following methods that aFlags refers to this enum. And maybe move the "construct frames" comment to after the enum (and right before the method it describes). >+++ b/layout/base/nsCSSFrameConstructor.cpp >@@ -2132,17 +2132,17 @@ nsCSSFrameConstructor::ConstructTable(ns > if (aItem.mFCData->mBits & FCDATA_USE_CHILD_ITEMS) { > rv = ConstructFramesFromItemList(aState, aItem.mChildItems, >- innerFrame, childItems); >+ innerFrame, 0, childItems); Hmm. So this is somewhat suboptimal, since it won't suppress whitespace if you have a bunch of block kids of a table, right? Since the flags just depend on the ancestor and where we come in its child list, and since wrapping parts of the list in pseudos can at most introduce more things that terminate in linebreaks (even if not flagged as such), would it make sense to make the flags a member of the list? We could then set the flags appropriately when doing the wrapping into pseudo-frames... and we'd have fewer arguments to cart around. >+ * Return true if the frame construction item pointed to by aIter will be >+ * create a frame adjacent to a line boundary in the frame tree, and that s/will be create/will create/ >+ * line boundary is induced by a content node that it's also adjacent to >+ * in the content tree. You mean its content node is adjacent to, right? >+nsCSSFrameConstructor::AtLineBoundary(FCItemIterator& aIter, >+ // Anonymous, so we can't reliably tell where it is in the content tree Anonymous or location unknown. > nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState& aState, >+ // (We check mAdditionalStateBits because only the generated content >+ // container's frame construction item is marked with mIsGeneratedContent.) And because we might not have an aParentFrame here if the generated content item is inline. >+ if (AtLineBoundary(aIter, aFlags) && item.IsWhitespace() && >+ !styleContext->GetStyleText()->NewlineIsSignificant() && >+ (aFlags & LIST_NO_XBL_SIBLINGS) && >+ !(aState.mAdditionalStateBits & NS_FRAME_GENERATED_CONTENT)) >+ return NS_OK; I think you want the check for NS_FRAME_GENERATED_CONTENT before the check for item.IsWhitespace(). Otherwise you'll flag generated content nodes as FRAMETREE_DEPENDS_ON_CHARS, which I would expect to behave weirdly when their text content is set. I'm surprised that this didn't bite you here... >+ // XXXroc Right now if you start with whitespace and then start adding chars >+ // (e.g. while editing) we reframe on every change, which seems dumb. >+ // Maybe we should use another flag here, or something. The common editing case uses a white-space value that'll prevent us from getting in here anyway, right? >+nsCSSFrameConstructor::AddTextItemIfNeeded(nsIFrame* aParentFrame, >+ nsIContent* aParentContent, >+ PRInt32 aContentIndex, Please assert that aContentIndex is in the right range for aParentContent? GetChildAt will just silently return null if it's not... >+ nsRefPtr<nsStyleContext> sc = mPresShell->StyleSet()-> >+ ResolveStyleForNonElement(aParentFrame->GetStyleContext()); That's wrong. This should be using ResolveStyleContext(aParentFrame, aContent). I knowe we already know it's not an eELEMENT, but duplicating the "get the right parent style context" logic here seems not worth it. >+ aItems.AppendItem(data, content, nsnull, kNameSpaceID_None, content->Tag() and content->GetNameSpaceID(), please, so it doesn't differ from the normal codepath. >+nsCSSFrameConstructor::ReframeTextIfNeeded(nsIContent* aParentContent, >+ PRInt32 aContentIndex, Again, please assert that aContentIndex is in range. >+ // Not text, or not suppressed due to being all-whitespace (if it >+ // was being suppressed, it would have the FRAMETREE_DEPENDS_ON_CHARS s/was/were/ >@@ -6208,22 +6331,16 @@ nsCSSFrameConstructor::ContentAppended(n >- nsIAtom* frameType = parentFrame->GetType(); >- // We should never get here with fieldsets, since they have multiple >- // insertion points. >- NS_ASSERTION(frameType != nsGkAtoms::fieldSetFrame, >- "Unexpected parent"); >- > // Deal with possible :after generated content on the parent > nsIFrame* parentAfterFrame; > parentFrame = > ::AdjustAppendParentForAfterContent(mPresShell->GetPresContext(), That change looks wrong to me. If we ever did get there, for a fieldset with :after, we'd not be looking at the fieldset anymore after the adjustment, would we? We would be looking at its content frame.... Am I missing something? I'd leave the assert here, and move te frameType set to later if you need it later. >@@ -6242,40 +6359,82 @@ nsCSSFrameConstructor::ContentAppended(n >+ nsIFrame* prevSibling = ::FindAppendPrevSibling(parentFrame, parentAfterFrame); Why move this line? >+ if (aNewIndexInContainer > 0) { >+ // If there's a text frame in the normal content list just before s/a text frame/a text node/ >+ // If the parent is a block frame, and we're not in a special case >+ // where frames can be moved around, determine if the list is for the >+ // start or end of the block. >+ PRUint32 flags = 0; >+ if (nsGkAtoms::blockFrame == frameType Is this a type check for performance reasons? That is, is nsLayoutUtils::GetAsBlock significantly slower here? Might be more reliable... >+ // To suppress whitespace-only text frames, we first have to verify that >+ // our container has no anonymous children. >+ if (!mDocument->BindingManager()->GetXBLChildNodesFor(aContainer)) { This really sucks.... I did some digging, and none of the state we already have available in this method answers the question we want answered here, so we do want this code as-is. But please change the comment to say: // To suppress whitespace-only text frames, we first have to verify that // our container's DOM child list matches its flattened tree child list. // This is guaranteed to be true if GetXBLChildNodesFor() returns null. >@@ -6575,34 +6734,52 @@ nsCSSFrameConstructor::ContentInserted(n >+ if (aIndexInContainer > 0) { >+ // If there's a text frame in the normal content list just before s/a text frame/a text node/ >@@ -7166,16 +7343,35 @@ nsCSSFrameConstructor::ContentRemoved(ns >+ // If aChild is still here, we must be reframing it. We don't need >+ // to do anything in that case, because the ContentInserted >+ // notification will take care of updating this text node. In >+ // fact we must not create a frame for it here, or ContentInserted >+ // will create another frame and we'll be hosed. I don't follow that. ContentInserted checks for an existing frame, doesn't it? It seems to me like you should only ReframeTextIfNeeded both aIndexInContainer - 1 and aIndexInContainer if aContainer->GetChildAt(aIndexInContainer) != aChild. In the other case, we're doing a reframe and ContentInserted will deal. That would also mean you don't need the third arg to ReframeTextIfNeeded. But there's one problem with that (and with the current code). This ContentRemoved could be due to a restyle that's changing us to display:none. In that case we do need to create a frame for the following textnode, but its content index will be aIndexInContainer+1... One solution would be to always ReframeTextIfNeeded here, regardless of whether this is a removal or restyle (and condition the index for the second call on whether aChild is at aIndexInContainer). Other ideas? >+++ b/layout/reftests/bugs/409084-1a.html >+++ b/layout/reftests/bugs/409084-1b.html Why did these tests need changes? >+++ b/layout/reftests/bugs/495385-2a.html >+ <div><span>Hello</span> <span>Kitty</span><div id="d4">X</div></div> I assume you wanted space before div? >+++ b/layout/reftests/bugs/495385-2g.html >+ <div><span>Hello</span> <span>Kitty</span><br id="b4"></div> And space before the <br> here. >+++ b/layout/reftests/bugs/495385-4-ref.html >+<body onload="loaded()"> no loaded() in this file. Tests that seem to be missing: 1) Testing that whitespace is not suppressed at start/end of a block with white-space:pre and such (though I guess that 495385-1.html sort of tests this; maybe that's enough). 2) Testing that whitespace is not suppressed between block kids of a block with white-space:pre and such. Also needs dynamic test here (basically several files like 1-ref that set the white-space on <body> dynamically). 3) A test for interaction with :first-letter and :first-line. 4) A test for setting a block to display:none; we should be failing that now. See above comments about ReframeTextIfNeeded. 5) A test for an append to a node with :after. That is, a test like 2d, but with a layout flush right after you open <div id="d2">. Maybe this can just be rolled into 2d.
Comment on attachment 382079 [details] [diff] [review] fix r- because of the display:none issue.
Attachment #382079 - Flags: superreview?(bzbarsky)
Attachment #382079 - Flags: superreview-
Attachment #382079 - Flags: review?(bzbarsky)
Attachment #382079 - Flags: review-
(In reply to comment #24) > >+++ b/layout/base/nsCSSFrameConstructor.h > > // Add the frame construction items for the given aContent and aParentFrame > > // to the list. This might add more than one item in some rare cases. > > void AddFrameConstructionItems(nsFrameConstructorState& aState, > > nsIContent* aContent, > >+ PRInt32 aContentIndex, > > nsIFrame* aParentFrame, > > FrameConstructionItemList& aItems); > > Document that aContentIndex == aContent->GetNodeParent()->IndexOf(aContent), > and maybe assert that? Of if it's always allowed to be -1 and that will just > lead to not doing this optimization, document that too (and change assert > accordingly)? Yes it is always allowed to be -1. OK. > >+ /* If FCDATA_IS_LINE_BREAK is set, the the frame is something that will > > s/the the/then the/. And same in the previous comment you copied this from. I > thought I'd gotten all of those.... :( :-) > > FrameConstructionItem* AppendItem(const FrameConstructionData* aFCData, > > nsIContent* aContent, > > nsIAtom* aTag, > > PRInt32 aNameSpaceID, > >+ PRInt32 aContentIndex, > > Again, document and assert the allowed values of aContentIndex. Or do that in > the FrameConstructionItem constructor, perhaps. Given the comment on the > mContentIndex member, just the assert might be enough, even. OK > >+ // If aParentContent's child at aContentIndex is a text node and > >+ // doesn't have a frame, add a frame construction item for it to aItems. > >+ void AddTextItemIfNeeded(nsIFrame* aParentFrame, > > Worth documenting that the new item is appended to aItems, right? OK. > >+ LIST_STARTS_AT_LINE_BOUNDARY = 0x01, > >+ LIST_ENDS_AT_LINE_BOUNDARY = 0x02, > > Maybe LIST_PRECEDED_BY_LINE_BOUNDARY and LIST_FOLLOWED_BY_LINE_BOUNDARY? > Either way, I guess. I think I'll stick with what I had here, it reads simpler to me. > >+ LIST_NO_XBL_SIBLINGS = 0x04 > > This needs some commenting. Or maybe renaming > LIST_FLATTENED_TREE_MATCHES_DOM_FOR_PARENT or something? Or both... See more > comments on this below. > > Also, I'd prefer LIST_NO_FLAGS = 0 in there and use wherever we use 0 to pass > to this method.... It's probably too much trouble to create a name for this > enum type so as to enforce not passing in bare integers, but the 0s were > confusing me. Yes, makes sense. The problem with using the enum as the parameter type is that callers then have to do explicit casts when they combine flags. > Worth documenting for all three of the following methods that aFlags refers to > this enum. And maybe move the "construct frames" comment to after the enum > (and right before the method it describes). > > >+++ b/layout/base/nsCSSFrameConstructor.cpp > >@@ -2132,17 +2132,17 @@ nsCSSFrameConstructor::ConstructTable(ns > > if (aItem.mFCData->mBits & FCDATA_USE_CHILD_ITEMS) { > > rv = ConstructFramesFromItemList(aState, aItem.mChildItems, > >- innerFrame, childItems); > >+ innerFrame, 0, childItems); > > Hmm. So this is somewhat suboptimal, since it won't suppress whitespace if you > have a bunch of block kids of a table, right? Since the flags just depend on > the ancestor and where we come in its child list, and since wrapping parts of > the list in pseudos can at most introduce more things that terminate in > linebreaks (even if not flagged as such), would it make sense to make the flags > a member of the list? We could then set the flags appropriately when doing the > wrapping into pseudo-frames... and we'd have fewer arguments to cart around. It shouldn't matter for performance, since the HTML parser will construct table nodes in common cases, it would only matter for heavy users of CSS table display types. But yes, it could make things simpler. I'll do it. > >+ * Return true if the frame construction item pointed to by aIter will be > >+ * create a frame adjacent to a line boundary in the frame tree, and that > > s/will be create/will create/ OK. > >+ * line boundary is induced by a content node that it's also adjacent to > >+ * in the content tree. > > You mean its content node is adjacent to, right? Yes > >+nsCSSFrameConstructor::AtLineBoundary(FCItemIterator& aIter, > >+ // Anonymous, so we can't reliably tell where it is in the content tree > > Anonymous or location unknown. Yes. > > nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState& aState, > > >+ // (We check mAdditionalStateBits because only the generated content > >+ // container's frame construction item is marked with mIsGeneratedContent.) > > And because we might not have an aParentFrame here if the generated content > item is inline. OK. > >+ if (AtLineBoundary(aIter, aFlags) && item.IsWhitespace() && > >+ !styleContext->GetStyleText()->NewlineIsSignificant() && > >+ (aFlags & LIST_NO_XBL_SIBLINGS) && > >+ !(aState.mAdditionalStateBits & NS_FRAME_GENERATED_CONTENT)) > >+ return NS_OK; > > I think you want the check for NS_FRAME_GENERATED_CONTENT before the check for > item.IsWhitespace(). Otherwise you'll flag generated content nodes as > FRAMETREE_DEPENDS_ON_CHARS, which I would expect to behave weirdly when their > text content is set. I'm surprised that this didn't bite you here... Right OK. I should actually move item.IsWhitespace() to last to avoid setting the flag as much as possible. > >+ // XXXroc Right now if you start with whitespace and then start adding chars > >+ // (e.g. while editing) we reframe on every change, which seems dumb. > >+ // Maybe we should use another flag here, or something. > > The common editing case uses a white-space value that'll prevent us from > getting in here anyway, right? textareas and inputs, yes. contenteditable, no. I don't think we need to worry about it, though. If we do worry about it, we should use two flags in the text node, meaning "reframe on any change" and "reframe if there currently is no frame". > >+nsCSSFrameConstructor::AddTextItemIfNeeded(nsIFrame* aParentFrame, > >+ nsIContent* aParentContent, > >+ PRInt32 aContentIndex, > > Please assert that aContentIndex is in the right range for aParentContent? > GetChildAt will just silently return null if it's not... OK. > >+ nsRefPtr<nsStyleContext> sc = mPresShell->StyleSet()-> > >+ ResolveStyleForNonElement(aParentFrame->GetStyleContext()); > > That's wrong. This should be using ResolveStyleContext(aParentFrame, > aContent). I knowe we already know it's not an eELEMENT, but duplicating the > "get the right parent style context" logic here seems not worth it. OK. > >+ aItems.AppendItem(data, content, nsnull, kNameSpaceID_None, > > content->Tag() and content->GetNameSpaceID(), please, so it doesn't differ from > the normal codepath. OK. > >+nsCSSFrameConstructor::ReframeTextIfNeeded(nsIContent* aParentContent, > >+ PRInt32 aContentIndex, > > Again, please assert that aContentIndex is in range. OK. > >+ // Not text, or not suppressed due to being all-whitespace (if it > >+ // was being suppressed, it would have the FRAMETREE_DEPENDS_ON_CHARS > > s/was/were/ OK. > >@@ -6208,22 +6331,16 @@ nsCSSFrameConstructor::ContentAppended(n > >- nsIAtom* frameType = parentFrame->GetType(); > >- // We should never get here with fieldsets, since they have multiple > >- // insertion points. > >- NS_ASSERTION(frameType != nsGkAtoms::fieldSetFrame, > >- "Unexpected parent"); > >- > > // Deal with possible :after generated content on the parent > > nsIFrame* parentAfterFrame; > > parentFrame = > > ::AdjustAppendParentForAfterContent(mPresShell->GetPresContext(), > > That change looks wrong to me. If we ever did get there, for a fieldset with > :after, we'd not be looking at the fieldset anymore after the adjustment, would > we? We would be looking at its content frame.... Am I missing something? No, you're right. > I'd > leave the assert here, and move te frameType set to later if you need it > later. Right. > >@@ -6242,40 +6359,82 @@ nsCSSFrameConstructor::ContentAppended(n > >+ nsIFrame* prevSibling = ::FindAppendPrevSibling(parentFrame, parentAfterFrame); > > Why move this line? Just something I did in an intermediate state. I'll put it back. > >+ if (aNewIndexInContainer > 0) { > >+ // If there's a text frame in the normal content list just before > > s/a text frame/a text node/ OK. > >+ // If the parent is a block frame, and we're not in a special case > >+ // where frames can be moved around, determine if the list is for the > >+ // start or end of the block. > >+ PRUint32 flags = 0; > >+ if (nsGkAtoms::blockFrame == frameType > > Is this a type check for performance reasons? That is, is > nsLayoutUtils::GetAsBlock significantly slower here? Might be more reliable... Yes ... I'll change it. > >+ // To suppress whitespace-only text frames, we first have to verify that > >+ // our container has no anonymous children. > >+ if (!mDocument->BindingManager()->GetXBLChildNodesFor(aContainer)) { > > This really sucks.... I did some digging, and none of the state we already > have available in this method answers the question we want answered here, so we > do want this code as-is. But please change the comment to say: > > // To suppress whitespace-only text frames, we first have to verify that > // our container's DOM child list matches its flattened tree child list. > // This is guaranteed to be true if GetXBLChildNodesFor() returns null. OK. It's not that bad perf-wise, is it? Just a hashtable lookup. > >@@ -6575,34 +6734,52 @@ nsCSSFrameConstructor::ContentInserted(n > >+ if (aIndexInContainer > 0) { > >+ // If there's a text frame in the normal content list just before > > s/a text frame/a text node/ OK. > >@@ -7166,16 +7343,35 @@ nsCSSFrameConstructor::ContentRemoved(ns > >+ // If aChild is still here, we must be reframing it. We don't need > >+ // to do anything in that case, because the ContentInserted > >+ // notification will take care of updating this text node. In > >+ // fact we must not create a frame for it here, or ContentInserted > >+ // will create another frame and we'll be hosed. > > I don't follow that. ContentInserted checks for an existing frame, doesn't it? It does? I don't see it. I did have a testcase that caused double frame creation, which I is why I introduced the above code. > It seems to me like you should only ReframeTextIfNeeded both aIndexInContainer > - 1 and aIndexInContainer if aContainer->GetChildAt(aIndexInContainer) != > aChild. In the other case, we're doing a reframe and ContentInserted will > deal. That would also mean you don't need the third arg to > ReframeTextIfNeeded. That makes sense. > But there's one problem with that (and with the current code). This > ContentRemoved could be due to a restyle that's changing us to display:none. > In that case we do need to create a frame for the following textnode, but its > content index will be aIndexInContainer+1... > > One solution would be to always ReframeTextIfNeeded here, regardless of whether > this is a removal or restyle (and condition the index for the second call on > whether aChild is at aIndexInContainer). Other ideas? No. I'll do that, and write a testcase. > >+++ b/layout/reftests/bugs/409084-1a.html > >+++ b/layout/reftests/bugs/409084-1b.html > > Why did these tests need changes? That was the issue in comment #16. On trunk, there is an empty line after the vertically overflowing table, and we create a new (empty) page for that line. With this patch, we no longer create that line, and we don't create the empty page either. This is overall an improvement, but to get the test to pass the simplest thing seemed to add the <br> to ensure there's a line there. > >+++ b/layout/reftests/bugs/495385-2a.html > >+ <div><span>Hello</span> <span>Kitty</span><div id="d4">X</div></div> > > I assume you wanted space before div? Yes. Amazing catch :-) > >+++ b/layout/reftests/bugs/495385-2g.html > >+ <div><span>Hello</span> <span>Kitty</span><br id="b4"></div> > > And space before the <br> here. Yes. > >+++ b/layout/reftests/bugs/495385-4-ref.html > >+<body onload="loaded()"> > > no loaded() in this file. Ooops. > Tests that seem to be missing: > > 1) Testing that whitespace is not suppressed at start/end of a block with > white-space:pre and such (though I guess that 495385-1.html sort of tests > this; maybe that's enough). Yes, that was my intent, but I'll add a static version. I guess I should change the reference to use &nbsp; instead of white-space:pre, too. > 2) Testing that whitespace is not suppressed between block kids of a block > with white-space:pre and such. Also needs dynamic test here (basically > several files like 1-ref that set the white-space on <body> dynamically). OK. > 3) A test for interaction with :first-letter and :first-line. OK. > 4) A test for setting a block to display:none; we should be failing that now. > See above comments about ReframeTextIfNeeded. > 5) A test for an append to a node with :after. That is, a test like 2d, but > with a layout flush right after you open <div id="d2">. Maybe this can > just be rolled into 2d. OK.
> It shouldn't matter for performance, since the HTML parser will construct table > nodes in common cases, it would only matter for heavy users of CSS table > display types. But yes, it could make things simpler. I'll do it. Actually, I'd rather not. It's easy to put the line boundary flags into the frame construction items, but it's not so easy to put the "no XBL siblings flag" in there, unless we put it in the list itself. It seems easier to just keep passing flags as now --- there are only the three functions that get an extra parameter.
> Right OK. I should actually move item.IsWhitespace() to last to avoid setting > the flag as much as possible. I wonder whether we can trigger a failure mode with the patch as-is by using a block generated content frame with just a single counter content inside it or something... Might be worth adding such a test if we don't have one yet. > OK. It's not that bad perf-wise, is it? Just a hashtable lookup. If there's no XBL around at all it's just a bunch of if checks, looks like. If there's any XBL anywhere in the document it's two hashtable lookups... I guess even that shouldn't be too bad, and we might fall into the "bunch of if checks" (5-6 or so) case usually. Well, plus several function calls. With any luck, sicking will make this all better soon anyway. >> I don't follow that. ContentInserted checks for an existing frame, doesn't >> it? >It does? I don't see it. If we're really reframing aChild here, ContentInserted on aChild will end up calling AddTextItemIfNeeded on the following textnode, right? That's what would construct a frame for it... but AddTextItemIfNeeded will check for an existing frame. Or was the comment talking about the fact that if aChild is a textnode being restyled and has the relevant flag then we can end up double-constructing frames for it? You're right that we need to avoid calling ReframeTextIfNeeded on it in that case. I _think_ my proposed approach deals with that. I assume the testcase that caused double frame creation is either one of our existing tests or one of the new ones? ;)
> unless we put it in the list itself I was assuming we'd put it in the list itself. I can go either way, though; it just seemed to me that a lot of callsites ended up with magic numbers that cluttered things up, and the code might look cleaner with the flags on the list. Having a name for "0" might help with the magic-ness, I guess.
(In reply to comment #28) > Or was the comment talking about the fact that if aChild is a textnode being > restyled and has the relevant flag then we can end up double-constructing > frames for it? You're right that we need to avoid calling ReframeTextIfNeeded > on it in that case. I _think_ my proposed approach deals with that. That's right. > I assume the testcase that caused double frame creation is either one of our > existing tests or one of the new ones? ;) Yes.
> But there's one problem with that (and with the current code). This > ContentRemoved could be due to a restyle that's changing us to display:none. > In that case we do need to create a frame for the following textnode, but its > content index will be aIndexInContainer+1... I wrote a test for this, and discovered that it actually works :-). The reason is because RecreateFramesForContent always calls ContentInserted after ContentRemoved, and even though the block doesn't create a frame anymore, ContentInserted still fixes up the text frames, like you say in comment #28. So we don't need to do anything at all in ContentRemoved for reframes. Still, at least we've got the test now :-).
Oh, fun. And the test will catch it if we change RecreateFramesForContent. Good, good. ;)
Attached patch fix v2 — — Splinter Review
With comments addressed.
Attachment #382079 - Attachment is obsolete: true
Attachment #382236 - Flags: superreview?(bzbarsky)
Attachment #382236 - Flags: review?(bzbarsky)
Comment on attachment 382236 [details] [diff] [review] fix v2 >+++ b/layout/base/nsCSSFrameConstructor.cpp >+nsCSSFrameConstructor::AddTextItemIfNeeded(nsIFrame* aParentFrame, >+ nsRefPtr<nsStyleContext> sc = >+ ResolveStyleContext(aParentFrame->GetStyleContext(), content); That first arg should be aParentFrame, not aParentFrame->GetStyleContext(). >@@ -9201,30 +9399,36 @@ nsCSSFrameConstructor::CreateNeededTable >+ // Table psuedo frames always induce line boundaries around their s/psuedo/pseudo/ I guess we still won't suppress the whitespace here because for this child list mParentHasNoXBLChildren will be false, right? Can we SetParentHasNoXBLChildren() to whatever the value is for the child list we're wrapping stuff in? >@@ -9299,30 +9503,37 @@ nsCSSFrameConstructor::ProcessChildren(n >+ // If we have first-letter or first-line style then frames can get >+ // moved around so don't set these flags. I'm been thinking about this some more, by the way, and I'm not sure what moving around would happen exactly... And luckily for us, white-space does not apply to first-line, so we don't have to worry about that. I'm fine with leaving this restriction for now, but we should look into whether it's actually needed. >+ if (aAllowBlockStyles && !haveFirstLetterStyle && !haveFirstLineStyle) { >+ itemsToConstruct.SetLineBoundaryAtStart(PR_TRUE); >+ itemsToConstruct.SetLineBoundaryAtStart(PR_TRUE); Second call should be SetLineBoundaryAtEnd(). >+++ b/layout/base/nsCSSFrameConstructor.h >@@ -113,19 +113,21 @@ public: > // Add the frame construction items for the given aContent and aParentFrame > // to the list. This might add more than one item in some rare cases. >+ // aContentIndex is the index of aContent in its parent's child list, >+ // or -1 if it's not in its parent's child list, or the index is >+ // not known. I'd still like an assert to that effect at the beginning of AddFrameConstructionItemsInternal. >+++ b/layout/reftests/bugs/495385-1c.html >+<!-- Test that 'white-space' creates between blocks when necessary --> "creates text frames" >+++ b/layout/reftests/bugs/495385-1d.html >+<!-- Test that 'white-space' creates between blocks when necessary --> And here. >+++ b/layout/reftests/bugs/495385-1e.html >+<!-- Test that 'white-space' creates between blocks (dynamically) when necessary --> And here. >+++ b/layout/reftests/bugs/495385-1f.html >+<!-- Test that 'white-space' creates between blocks (dynamically) when necessary --> And here. r+sr=bzbarsky with the ProcessChildren and AddTextItemIfNeeded bugs fixed and the nits picked.
Attachment #382236 - Flags: superreview?(bzbarsky)
Attachment #382236 - Flags: superreview+
Attachment #382236 - Flags: review?(bzbarsky)
Attachment #382236 - Flags: review+
(In reply to comment #34) > (From update of attachment 382236 [details] [diff] [review]) > >+++ b/layout/base/nsCSSFrameConstructor.cpp > > >+nsCSSFrameConstructor::AddTextItemIfNeeded(nsIFrame* aParentFrame, > >+ nsRefPtr<nsStyleContext> sc = > >+ ResolveStyleContext(aParentFrame->GetStyleContext(), content); > > That first arg should be aParentFrame, not aParentFrame->GetStyleContext(). Hmm. I do not know how that built. > >@@ -9201,30 +9399,36 @@ nsCSSFrameConstructor::CreateNeededTable > >+ // Table psuedo frames always induce line boundaries around their > > s/psuedo/pseudo/ > > I guess we still won't suppress the whitespace here because for this child list > mParentHasNoXBLChildren will be false, right? Can we > SetParentHasNoXBLChildren() to whatever the value is for the child list we're > wrapping stuff in? Hmm. Yes, I guess so. > >@@ -9299,30 +9503,37 @@ nsCSSFrameConstructor::ProcessChildren(n > >+ // If we have first-letter or first-line style then frames can get > >+ // moved around so don't set these flags. > > I'm been thinking about this some more, by the way, and I'm not sure what > moving around would happen exactly... And luckily for us, white-space does not > apply to first-line, so we don't have to worry about that. > > I'm fine with leaving this restriction for now, but we should look into > whether it's actually needed. First-letter and first-line can wrap whitespace in another frame, defeating the invariant that only frames that would be direct children of blocks are suppressed. I can't actually think of anywhere we rely on that invariant. Although for first-letter it's worse, since the whitespace can end up in a float. Maybe nothing breaks, but it makes the conceptual model more complicated. > >+ if (aAllowBlockStyles && !haveFirstLetterStyle && !haveFirstLineStyle) { > >+ itemsToConstruct.SetLineBoundaryAtStart(PR_TRUE); > >+ itemsToConstruct.SetLineBoundaryAtStart(PR_TRUE); > > Second call should be SetLineBoundaryAtEnd(). Ooops! > >+++ b/layout/base/nsCSSFrameConstructor.h > > >@@ -113,19 +113,21 @@ public: > > // Add the frame construction items for the given aContent and aParentFrame > > // to the list. This might add more than one item in some rare cases. > >+ // aContentIndex is the index of aContent in its parent's child list, > >+ // or -1 if it's not in its parent's child list, or the index is > >+ // not known. > > I'd still like an assert to that effect at the beginning of > AddFrameConstructionItemsInternal. Sorry, forgot about that one. > >+++ b/layout/reftests/bugs/495385-1c.html > >+<!-- Test that 'white-space' creates between blocks when necessary --> > > "creates text frames" > > >+++ b/layout/reftests/bugs/495385-1d.html > >+<!-- Test that 'white-space' creates between blocks when necessary --> > > And here. > > >+++ b/layout/reftests/bugs/495385-1e.html > >+<!-- Test that 'white-space' creates between blocks (dynamically) when necessary --> > > And here. > > >+++ b/layout/reftests/bugs/495385-1f.html > >+<!-- Test that 'white-space' creates between blocks (dynamically) when necessary --> > > And here. > > r+sr=bzbarsky with the ProcessChildren and AddTextItemIfNeeded bugs fixed and > the nits picked. OK thanks!
> Hmm. I do not know how that built. We have both overloads; the other is for use in cases where we already know the parent style context (specifically, inline child item construction).
I forgot to get review on this prerequisite patch. We need to avoid the editor changing the white-space style which (with the main patch in this bug) causes reframing of the anonymous DIV. So this patch makes us not call the editor code that sets the style, instead we just set the style directly on the anonymous content when we create it.
Attachment #382899 - Flags: review?(bzbarsky)
Comment on attachment 382899 [details] [diff] [review] supplemental fix for Why do we need to set the width on the div here? We just need to set white-space, right? And the SetNativeAnonymous() call needs to stay. Also, the "Set up wrapping" comment in GetWrapCols() makes no sense.
Attachment #382899 - Flags: review?(bzbarsky) → review-
I set the width because that's what the existing editor code does in SetWrapWidth. I can't see that it's needed here though. So you want me to take it out? nsCSSFrameConstructor::GetAnonymousContent calls SetNativeAnonymous on all returned content from nsIAnonymousContentCreator::CreateAnonymousContent, so do we really need to it here? I'll remove the "set up wrapping" comment (and fix "// Set wrapping normally otherwise")
> So you want me to take it out? Yeah. It's annoying enough to have to set white-space; I'd rather not have more things that we can sort of get out of sync with editor here. > nsCSSFrameConstructor::GetAnonymousContent calls SetNativeAnonymous on all > returned content from nsIAnonymousContentCreator::CreateAnonymousContent, so > do we really need to it here? As the comment says (not quite explicitly enough), we need to do it before we set the style attr. See bug 234761. I should add a testcase for that to our automated tests...
Attachment #382908 - Flags: superreview+
Attachment #382908 - Flags: review?(bzbarsky)
Attachment #382908 - Flags: review+
Pushed textarea fix: http://hg.mozilla.org/mozilla-central/rev/97d6405478a3 I pushed the main patch, but backed it out due to test failures: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1244886125.1244892815.26892.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1244886125.1244890570.21524.gz I'm not sure why those test failures didn't show up on the try servers, but it may be because ProcessChildren wasn't suppressing text frames at the end of a block in that build, due to the bug Boris spotted, and now we are.
Fixed up the accessibility tests and re-pushed successfully: http://hg.mozilla.org/mozilla-central/rev/c6c685aa0379 1-2% Tp improvement on Linux/Mac. Windows is too noisy to tell.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
While we have the Tp win, we seem to have a DHTML perf loss: http://➡.ws/⊳再 http://➡.ws/裗
So I looked at some logs from before/after the regression. Here's what looks like the most relevant numeric change to me: Before logs: NOISE: |13;scrolling.html;1302.5;1302.5;1297;1383;1383;1300;1305;1308;1297 NOISE: |13;scrolling.html;1301;1318;1298;1374;1372;1303;1299;1374;1298 After logs: NOISE: |13;scrolling.html;1518.5;1529.75;1513;1570;1569;1515;1513;1570;1522 NOISE: |13;scrolling.html;1535;1546.5;1529;1590;1590;1534;1529;1587;1536
So... I just tried profiling that test locally, in a build that certainly has this patch, and I don't see where we could have lost 15% on it. All of frame construction is 10.5% of total time. Reflow is 72%, async scrolling 3%. Painting is 10%....
What platform did you try it on? Could it be the setting of innerHTML?
I tried it on mac, since that's where profiling is easiest. I did include the innerHTML set in the profile; that 10.5% is all spent in there. But I guess we should look on Windows.... I have no idea why this would be platform-dependent, though, and there seems to be a general cross-platform Tdhtml rise.
I'm having a hard time reproducing slowdown on Mac. On Tinderbox here are the Mac Talos results just before the patch: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1245792380.1245797092.8999.gz&fulltext=1 After: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1245794105.1245808614.30765.gz&fulltext=1 NOISE: |13;scrolling.html;2365.5;2367.75;2362;2403;2369;2362;2362;2378;2403 NOISE: |13;scrolling.html;2445;2445;2438;2454;2438;2441;2449;2454;2452 Pretty close, but definitely significant. But layers5 is more significant: NOISE: |7;layers5.html;617.5;615.75;609;620;609;617;619;620;618 NOISE: |7;layers5.html;695.5;678.5;626;699;626;697;696;695;699 I'll look into that.
I can't reproduce any consistent slowdown in layers5 either :-(.
These are the Tdhtml test results from all Tinderboxes, from the run before the patch and the run after the patch.
Regarding layers5 in comment #50, it seems I looked at the wrong box; no other box shows anything like that. So that must be some kind of anomaly. The only test that slowed down on every platform was scrolling.html, so you were right (of course!). I should focus on that. It's hard to believe that the scrolling phase of the test could have slowed down. That does no frame construction --- just setting style.top, reflowing, and painting. I'll try making a testcase that focuses on the innerHTML part, and see if I can reproduce a slowdown there.
Yeah, the innerHTML part is where the issue would have to be; maybe on the talos test machines it's a bigger part of life than on my machine... By gut feeling is that this is something to do with the <br>s in the innerHTML, if so, but all those should be causing is some extra "is this text whitespace?" checks, I'd think....
This seems to have caused bug 500482.
Depends on: 500467
I wrote this testcase which basically just does the innerHTML stuff from scrolling.html. Doesn't reproduce any regression on my Mac. I'll try it on a Mac Mini Windows box at the office.
The only thing I can think of is that the GetPrimaryFrameFor call in AddTextItemIfNeeded is hurting us. We can easily fix that by changing FRAMETREE_DEPENDS_ON_CHARS into two bits: -- CREATE_FRAME_IF_NON_WHITESPACE: means that if the text node changes to non-whitespace, then we need to call RecreateFramesForContent if it doesn't have a frame. This bit can be removed whenever we create a frame for the text. This would be set when we suppress a whitespace-only text frame. We check for this flag in AddTextItemIfNeeded. -- REFRAME_IF_WHITESPACE: means that if the text node changes to all-whitespace, we need to call RecreateFramesForContent. NeedsFrameFor and FrameConstructionItem::IsWhitespace would set both of these flags. This would let us avoid other pathological situations for which I can easily write a testcase (such as reframing a text node at the start of a block every time we add a character to it). I will write such a testcase, file a new bug for it, and then we'll see if that fixes the Tdhtml regression too.
Depends on: 500905
Depends on: 501035
I did that in bug 500556 and checked it in around 6pm June 28 PDT. The Tdhtml graphs around that time are very confusing. It looks like Tdhtml suddenly improved late on the 27th PDT, but I don't see any checkins on the 27th that would obviously have improved anything. For the next day or so, Tdhtml numbers appear very noisy. But it appears on some boxes they went up again during the 28th, and then down again around the time of my checkin. The Tdhtml improvement on the 27th is inexplicable: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b9e75e6c72de&tochange=fe6e0d8924bd Apart from the fact they're all trivial warning fixes, basically none of the affected code is actually significant for Tdhtml. It's also very very suspicious that the Tdhtml improvement was due to a change in scrolling.html, about the same change that this patch appeared to regress: http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unix&logfile=1246155154.1246165244.22845.gz&buildtime=1246155154&buildname=WINNT%206.0%20mozilla-central%20talos&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1246158331.1246168278.22876.gz&fulltext=1 Anyway, the numbers have now stabilized around the level they were at before this patch landed. Whether there was a real problem here or not, I'm calling this done.
Depends on: 514660
Blocks: 489906
Depends on: 525856
Depends on: 536623
Depends on: 537141
Depends on: 512295
This appears to have caused Bug 556305.
Depends on: 556305
Depends on: 570362
Depends on: 677114
Depends on: 839944
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: