| Summary: | Move :before and :after into the DOM | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Elliott Sprehn <esprehn> | ||||||||||||||||||||||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Elliott Sprehn <esprehn> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abucur, ap, cmarcelo, dbates, dglazkov, dtrebbien, eric, gyuyoung.kim, hyatt, inferno, jchaffraix, kling, koivisto, mifenton, mihnea, morrita, ojan, peter+ews, pfeldman, pravind.2k4, rakuco, simon.fraser, webcomponents-bugzilla, webkit.review.bot | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 98836, 103705, 104462 | ||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 100161 | ||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
Note: Trivially this could be implemented by making a subclass of Node that override previousSibling/nextSibling to go through parentNode removing the need for book keeping code to maintain the prev/next ptrs. Unfortunately those aren't virtual methods :( Created attachment 166332 [details]
WIP
WIP patch that attempts to move :before and :after into the DOM. This reuses the existing renderers for generated content so we can do this in multiple steps. The current issues are infinite loops in RenderCounter, broken CSS inheritance in CSS tables, and a bug with counters and styleRecalc causing crashes. On the plus side this patch fixes tons of layout bugs with generated content which is awesome.
Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14089003 Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14090003 Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14074049 Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14075051 Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14085020 http://trac.webkit.org/changeset/129423 moved usesBeforeAfterRules() off of Document which is where all the build failures came from. Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14085021 Comment on attachment 166332 [details] WIP Attachment 166332 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14078031 Created attachment 166559 [details]
Patch
Created attachment 166585 [details]
Patch
Comment on attachment 166585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166585&action=review > Source/WebCore/dom/Element.cpp:1214 > + beforeContent->setNeedsStyleRecalc(); Is this safe during recalcStyle? Will this crawl up and tell our parents, and possibyl our previous siblings they need style-recalc again? > Source/WebCore/dom/Element.cpp:1860 > +PassRefPtr<PseudoElement> Element::beforePseudoElement() const > +{ > + return hasRareData() ? elementRareData()->m_generatedBefore : 0; > +} Why PassRefPtr? These functxions are not creating a new object, or expecting the caller to take ownership, are they? > Source/WebCore/dom/PseudoElement.cpp:79 > + child->setStyle(renderStyle()); So this assumes that we don't ever modify this style... since otherwise we'd modify our parent's style, or? Don't we normally just unconditionally inherit styles? I guess these are really siblings of the element in question instead of children... > Source/WebCore/dom/PseudoElement.cpp:105 > + renderer = new (arena) RenderTextFragment(document(), static_cast<const TextContentData*>(content)->text().impl()); You might benifit from toTextContentData helpers which would ASSERT as well as do the cast. > Source/WebCore/dom/PseudoElement.cpp:112 > + if (const StyleImage* styleImage = static_cast<const ImageContentData*>(content)->image()) > + image->setImageResource(RenderImageResourceStyleImage::create(const_cast<StyleImage*>(styleImage))); Why are these casts safe? > Source/WebCore/dom/PseudoElement.cpp:121 > + renderer = new (arena) RenderCounter(document(), *static_cast<const CounterContentData*>(content)->counter()); > + break; > + case CONTENT_QUOTE: > + renderer = new (arena) RenderQuote(document(), static_cast<const QuoteContentData*>(content)->quote()); Again to*() helpers which also ASSERT the type might be useful. > Source/WebCore/rendering/RenderObject.cpp:135 > - // RenderImageResourceStyleImage requires a style being present on the image but we don't want to > - // trigger a style change now as the node is not fully attached. Moving this code to style change > - // doesn't make sense as it should be run once at renderer creation. > - image->m_style = style; > + image->setStyle(style); Are we now triggering a style change and causing badness? Comment on attachment 166585 [details]
Patch
Why aren't we just using the Shadow DOM plumbing? A lot of this code seems to duplicate it.
Comment on attachment 166585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166585&action=review >> Source/WebCore/dom/Element.cpp:1214 >> + beforeContent->setNeedsStyleRecalc(); > > Is this safe during recalcStyle? Will this crawl up and tell our parents, and possibyl our previous siblings they need style-recalc again? I assumed it was safe because just a few lines below we do this on our children: if ((forceCheckOfNextElementSibling || forceCheckOfAnyElementSibling)) element->setNeedsStyleRecalc(); I didn't see a difference either way in the tests I ran, but it seems like a recalc of our style should mean a recalc of the pseudo since they're derived from them. >> Source/WebCore/dom/Element.cpp:1860 >> +} > > Why PassRefPtr? These functxions are not creating a new object, or expecting the caller to take ownership, are they? I'll switch to bare ptrs on the return types. >> Source/WebCore/dom/PseudoElement.cpp:79 >> + child->setStyle(renderStyle()); > > So this assumes that we don't ever modify this style... since otherwise we'd modify our parent's style, or? Don't we normally just unconditionally inherit styles? I guess these are really siblings of the element in question instead of children... This code is just copy and paste from RenderObjectChildList's updateBeforeAfterStyle so it might be wrong. If the PseudoElement had font-size: 50%; and then I inherit for the text here, do I get 25% or do I keep the 50%? >> Source/WebCore/dom/PseudoElement.cpp:112 >> + image->setImageResource(RenderImageResourceStyleImage::create(const_cast<StyleImage*>(styleImage))); > > Why are these casts safe? They're safe because the switch() checks the type, this switch is actually copy and pasted from RenderObjectChildList. They're pretty gross though... I'll put together a separate patch that fixes this while I continue here. >> Source/WebCore/rendering/RenderObject.cpp:135 >> + image->setStyle(style); > > Are we now triggering a style change and causing badness? Ack. This is a bad merge. I'll revert, I have a bunch of merge badness in this file it seems. (In reply to comment #14) > (From update of attachment 166585 [details]) > Why aren't we just using the Shadow DOM plumbing? A lot of this code seems to duplicate it. Which "a lot" are you talking about? (In reply to comment #16) > (In reply to comment #14) > > (From update of attachment 166585 [details] [details]) > > Why aren't we just using the Shadow DOM plumbing? A lot of this code seems to duplicate it. > > Which "a lot" are you talking about? Code in Element, Node, NodeRenderingContext, adding stuff to RareNodeData. Can we see if we can maybe reframe :before/:after in terms of shadow DOM? I would be happy to help. Created attachment 166797 [details]
Patch
The crash was some kind of heap corruption that comes from generated content in display: run-in; so Ive disabled support for that for now. This wont work quite right with Shadow DOM yet either, but as Dimitri mentioned we might take a different approach then, but Id like to get this in before that since Shadow DOM is all fluxy right now anyway. Either way this patch now passes all the tests except for RunIn ones (and a few where the patch fixed bugs)
Created attachment 167144 [details]
Patch
This now passes all the tests except for ones that this fixes (yay). I don't have the build files updated for non-Chromium ports yet, but if someone wants to review this while I get that and the test output sorted...
Comment on attachment 167144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167144&action=review > Source/WebCore/dom/ElementRareData.h:46 > + void clearGeneratedContent(); This doesn't seem to get called anywhere. Where do you delete generated content nodes when a selector no longer applies? Presumably we have some tests that cover this. Comment on attachment 167144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167144&action=review >> Source/WebCore/dom/ElementRareData.h:46 >> + void clearGeneratedContent(); > > This doesn't seem to get called anywhere. Where do you delete generated content nodes when a selector no longer applies? Presumably we have some tests that cover this. Woops, that method name is leftover from a previous iteration of the code. In Element::recalcStyle if we already have a :before or :after we call recalcStyle on it which would detach() the PseudoElement if there's no more content resulting in a PseudoElement without a renderer(). You'll see there's a check there: afterContent->recalcStyle(change); if (!afterContent->renderer()) setAfterPseudoElement(0); // free the after content. We also remove it in Element::detach. Created attachment 167431 [details]
Patch
(In reply to comment #22) > Created an attachment (id=167431) [details] > Patch I've identified some places where our tests are lacking or bugs this fixes that we need more tests (ex. animations work on :before and :after now) so I'll work on that, but code is good for review! Comment on attachment 167431 [details] Patch Attachment 167431 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14168949 Comment on attachment 167431 [details] Patch Attachment 167431 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14171984 Comment on attachment 167431 [details] Patch Attachment 167431 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14179692 New failing tests: fast/css-generated-content/before-content-continuation-chain.html fast/css-generated-content/table-row-group-to-inline.html http/tests/workers/terminate-during-sync-operation.html Comment on attachment 167431 [details] Patch Attachment 167431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14168957 New failing tests: fast/css-generated-content/before-content-continuation-chain.html fast/forms/time-multiple-fields/time-multiple-fields-appearance-pseudo-elements.html fast/css-generated-content/table-row-group-to-inline.html Comment on attachment 167431 [details] Patch Attachment 167431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14171997 New failing tests: fast/css-generated-content/before-content-continuation-chain.html fast/forms/time-multiple-fields/time-multiple-fields-appearance-pseudo-elements.html fast/css-generated-content/table-row-group-to-inline.html Created attachment 167604 [details]
Patch
(In reply to comment #29) > Created an attachment (id=167604) [details] > Patch This passes all tests except a few more than have to do with generated content on inputs: [4/13] fast/forms/date-multiple-fields/date-multiple-fields-appearance-pseudo-elements.html failed unexpectedly (image diff) [5/13] fast/forms/month-multiple-fields/month-multiple-fields-appearance-pseudo-elements.html failed unexpectedly (image diff) [9/13] fast/forms/week-multiple-fields/week-multiple-fields-appearance-pseudo-elements.html failed unexpectedly (image diff) These tests are bad because they're checking that you can add :before and :after content to the special <input> elements which is wrong. We shouldn't even support generated content on those things yet (and we don't support it for most input types). Comment on attachment 167604 [details] Patch Attachment 167604 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14205824 New failing tests: fast/forms/month-multiple-fields/month-multiple-fields-appearance-pseudo-elements.html fast/forms/week-multiple-fields/week-multiple-fields-appearance-pseudo-elements.html fast/forms/date-multiple-fields/date-multiple-fields-appearance-pseudo-elements.html Comment on attachment 167604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review Overall this looks good. I need to give it a few more passes before I fully understand it and can comment intelligently. > Source/WebCore/dom/PseudoElement.cpp:76 > + if (RenderObject* renderer = this->renderer()) { Early return probably is cleaner than indenting this whole block. > Source/WebCore/dom/PseudoElement.cpp:106 > + if (child->isText()) { > + child->setStyle(renderStyle()); > + } else if (child->isImage()) { I think single-line ifs don't have { } in webkit. > LayoutTests/ChangeLog:10 > + Note: The output of table-row-group-to-inline.html is wrong (but always was) until Bug 86261 is fixed. Could you explain the changes we see in the image results? (In reply to comment #32) > (From update of attachment 167604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review > ... > > LayoutTests/ChangeLog:10 > > + Note: The output of table-row-group-to-inline.html is wrong (but always was) until Bug 86261 is fixed. > > Could you explain the changes we see in the image results? I'll update the ChangeLog. There's always been a bug where we leave anonymous RenderTable wrappers when removing nodes, that's why you see the output of the test on two lines in the old -expected.png, but now the way generated content gets inserted is the same as DOM nodes so instead of: RenderText RenderTable RenderText which used to put this on two lines we end up with: RenderTable RenderText RenderText Which puts it all on one line. It *should* be all on one line, but it shouldn't be pushed down a line. To fix this we need to clean up anonymous table wrappers. Comment on attachment 167604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review > Source/WebCore/dom/Element.cpp:1253 > + if (PseudoElement* afterContent = afterPseudoElement()) { > + afterContent->setNeedsStyleRecalc(); > + afterContent->recalcStyle(change); > + if (!afterContent->renderer()) > + elementRareData()->setAfterPseudoElement(0); // free afterContent > + } else if (RefPtr<PseudoElement> afterContent = createPseudoElementIfNeeded(AFTER)) { This block is mostly a copy/paste of the above. it seems with some smarts we could rip the one non-sharable setAfterPseudoElement line out of these and share the rest in a helper function. Maybe something liek this? if (PseudoElement* newAfterContent = updateBeforeAfterContent(afterPseudoElement(), AFTER)) setAfterPseudoElement(newAfterContent); > Source/WebCore/dom/Element.cpp:1850 > + RenderStyle* style = renderer()->getCachedPseudoStyle(pseudoId); > + if (style && PseudoElement::pseudoRendererIsNeeded(style)) > + return PseudoElement::create(this, pseudoId); nit: I might have continued the pattern of early return, and made this be the negative, return 0, and the final clause of the funtion return the created element. > Source/WebCore/dom/Element.cpp:1858 > +PseudoId Element::pseudoId() const > +{ > + return hasRareData() ? elementRareData()->m_pseudoId : NOPSEUDO; > +} I wonder how hot any of these are and if they'll need to be inlined. > Source/WebCore/dom/Node.cpp:476 > + if (isElementNode() && !previousSibling()) { I think previousSibling is cheaper than isElementNode() and should probably be first. > Source/WebCore/dom/Node.cpp:490 > + if (isElementNode() && !nextSibling()) { Same. > Source/WebCore/dom/Node.cpp:494 > + if (isBeforePseudoElement() && parent->firstChild()) if firstChild is faster, we might move that first. I dont' remember it's comparitive speed on Element. It's slow on RenderObject. :) > Source/WebCore/dom/NodeRenderingContext.cpp:97 > + // FIXME: This is wrong when the next sibling was actually moved around by shadow insertion points. Can you link to a bug, or give hints to proper behavior? > Source/WebCore/dom/PseudoElement.cpp:62 > + bool hasContent = style->contentData() && style->contentData()->type() != CONTENT_NONE; > + bool hasRegionThread = !style->regionThread().isEmpty(); > + return hasContent || hasRegionThread; You might explain why :) > Source/WebCore/dom/PseudoElement.cpp:89 > + RenderObject* child = createRendererForContent(content); > + if (!child) > + continue; > + if (renderer->isChildAllowed(child, renderStyle())) > + renderer->addChild(child); > + else > + child->destroy(); unconditional-create/conditional-destroy patterns like this are inefficent. I believe this is how existing code works, so this is fine... but hopefully the destroy() branch is very uncommon. :) > Source/WebCore/dom/PseudoElement.cpp:110 > + // Images must inherit the pseudo so the width/height styles on the element don't affect it. > + RefPtr<RenderStyle> style = RenderStyle::create(); > + style->inheritFrom(renderStyle()); > + child->setStyle(style.release()); It's unclear to me why images are special. Is the non-inherit path an optimization? Does text need to not-inherit? If so, why? Do those reasons not apply to images? > Source/WebCore/dom/PseudoElement.cpp:121 > + RenderObject* renderer = this->renderer(); > + for (RenderObject* child = renderer->nextInPreOrder(renderer); child; child = child->nextInPreOrder(renderer)) > + updateChildStyle(child); Should probably explain why. // Because PseudoElements are not part of the DOM tree, they don't participate in style recalc normally, so we have to fake our own for our child elements. or some such... > Source/WebCore/dom/PseudoElement.cpp:132 > + switch (content->type()) { > + case CONTENT_NONE: > + ASSERT_NOT_REACHED(); > + return 0; Is this switch copied from somewhere? Can we share code? > Source/WebCore/dom/PseudoElement.cpp:134 > + renderer = new (arena) RenderTextFragment(document(), static_cast<const TextContentData*>(content)->text().impl()); Too bad we don't have toTextContentData(), those tend to be cleaner/safer than raw static_casts, as they ASSERT and are fewer chars... > Source/WebCore/dom/PseudoElement.h:58 > +inline PassRefPtr<PseudoElement> PseudoElement::create(Element* parent, PseudoId pseudoId) I don't think you need to mark things in headers as inline? I thought that was implicit? Also, why declare this after the class instead of inline in teh declaration? > Source/WebCore/rendering/HitTestResult.cpp:265 > + if (n && n->isPseudoElement()) > + n = n->parentOrHostNode(); How does shadow avoid this? Created attachment 167888 [details]
Patch
Changes per review by eseidel
Comment on attachment 167888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167888&action=review > Source/WebCore/dom/Element.cpp:1020 > + updatePseudoElement(BEFORE); Wow, this is so much nicer. (In reply to comment #34) > (From update of attachment 167604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review > > > > Source/WebCore/dom/Element.cpp:1858 > > +PseudoId Element::pseudoId() const > > +{ > > + return hasRareData() ? elementRareData()->m_pseudoId : NOPSEUDO; > > +} > > I wonder how hot any of these are and if they'll need to be inlined. Not sure. I originally had a lot of work in this patch for that but most cases don't need that perf difference. hasRareData() is a bitfield check and this method isn't virtual so I'd hope it's reasonably fast. > > > Source/WebCore/dom/Node.cpp:476 > > + if (isElementNode() && !previousSibling()) { > > I think previousSibling is cheaper than isElementNode() and should probably be first. isElementNode() is a bitfield check in the DOM so it's fast. > > Source/WebCore/dom/PseudoElement.cpp:89 > > ... > > + child->destroy(); > > unconditional-create/conditional-destroy patterns like this are inefficent. I believe this is how existing code works, so this is fine... but hopefully the destroy() branch is very uncommon. :) This should be very uncommon. > > > Source/WebCore/dom/PseudoElement.cpp:110 > > + // Images must inherit the pseudo so the width/height styles on the element don't affect it. > > + RefPtr<RenderStyle> style = RenderStyle::create(); > > + style->inheritFrom(renderStyle()); > > + child->setStyle(style.release()); > > It's unclear to me why images are special. Is the non-inherit path an optimization? Does text need to not-inherit? If so, why? Do those reasons not apply to images? I added a comment. Images in generated content should always be intrinsically sized. If we didn't inherit then the width/height on the generated content node itself would change the sizing of the image inside it. > > Source/WebCore/dom/PseudoElement.cpp:132 > > + switch (content->type()) { > > + case CONTENT_NONE: > > + ASSERT_NOT_REACHED(); > > + return 0; > > Is this switch copied from somewhere? Can we share code? Well, it's copied from the old RenderObjectChildList but modified a bit. In a follow up patch I want to move renderer creation to virtual methods on the ContentData classes and get rid of this. There's no reason for this kind of perf optimization here. > > > Source/WebCore/dom/PseudoElement.cpp:134 > > + renderer = new (arena) RenderTextFragment(document(), static_cast<const TextContentData*>(content)->text().impl()); > > Too bad we don't have toTextContentData(), those tend to be cleaner/safer than raw static_casts, as they ASSERT and are fewer chars... Yeah, after this lands I'll create a patch that does that. > > > Source/WebCore/dom/PseudoElement.h:58 > > +inline PassRefPtr<PseudoElement> PseudoElement::create(Element* parent, PseudoId pseudoId) > > I don't think you need to mark things in headers as inline? I thought that was implicit? > > Also, why declare this after the class instead of inline in teh declaration? You must have inline or you'll get duplicate symbol declarations I think. We always put it in there. It should just be in the class though (I fixed it). > > > Source/WebCore/rendering/HitTestResult.cpp:265 > > + if (n && n->isPseudoElement()) > > + n = n->parentOrHostNode(); > > How does shadow avoid this? Shadow does some magic for event retargeting, but it's designed to only happen during event dispatch and we need it for all kinds of things like selection or clicking links (ie you should never hit the content). We might be able to fix shadow DOM, but I'd like to wait for that. Comment on attachment 167888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167888&action=review > Source/WebCore/dom/Element.h:488 > + void setAfterPseudoElement(PassRefPtr<PseudoElement>); Woops, these methods don't exist anymore. I'll remove them. Comment on attachment 167888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167888&action=review > Source/WebCore/dom/Element.h:488 > + void setAfterPseudoElement(PassRefPtr<PseudoElement>); Woops, these methods don't exist anymore. I'll remove them. Comment on attachment 167888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167888&action=review The only question is the perf. :) > Source/WebCore/dom/Element.cpp:1820 > + existing->setNeedsStyleRecalc(); > + existing->recalcStyle(change); Why do we need to setNeedsStyleRecalc()? And do you need to upgrade the change to Detach or Force? It seems any style change could be a pseudo change, so we do need this setNeedsStyleRecalc or we need to pass Force. You should document this now that we understand what's going on. > Source/WebCore/dom/PseudoElement.cpp:107 > + if (child->isText()) > + child->setStyle(renderStyle()); > + else if (child->isImage()) { Instead add an early return, which is if (!child->isText() || !child->isImage()) with a note, saying that it must not be our rendererer so we don't manage the style. Then ahve a "optimized" path for text. And then fallback for images to the normal inherit path. > Source/WebCore/dom/PseudoElement.cpp:122 > + // PseudoElements are not part of the DOM tree and don't participte in the recalcStyle flow so we must > + // propagate the style downward manually. note to you: YOu said you wanted to update this. Created attachment 168113 [details]
Patch
Final review comments
(In reply to comment #40) > (From update of attachment 167888 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167888&action=review > > The only question is the perf. :) I changed it so there's a fast path that's inline and non-virtual and a slow path for things with custom callbacks which is very rare (search for setHasCustomCallbacks). With this patch there's no difference for Parser/html5-full-render. Created attachment 168116 [details]
Patch for landing
Created attachment 168118 [details]
Patch for landing
Created attachment 168119 [details]
Patch for landing
Created attachment 168121 [details]
Patch for landing
(In reply to comment #46) > Created an attachment (id=168121) [details] > Patch for landing Seems webkit patch got confused when I renamed the bug or I lost the r+ bit somehow so I had to manually put in the reviewer in and fix the bug name. Assuming this goes green on all the bots I just need a cq+. :) Comment on attachment 168121 [details] Patch for landing Clearing flags on attachment: 168121 Committed r131004: <http://trac.webkit.org/changeset/131004> All reviewed patches have been landed. Closing bug. I needed to roll this back because it broke inspector (and hence is likely to break the web). Try opening console and typing there - it is not editable. http://trac.webkit.org/changeset/131050 (In reply to comment #50) > I needed to roll this back because it broke inspector (and hence is likely to break the web). Try opening console and typing there - it is not editable. > > http://trac.webkit.org/changeset/131050 Interesting. How do I get the web inspector to open for the web inspector? This seems like it's related to editing and generated content, but I can't find any reference to contentEditable in the inspector code. Looks like this patch caused a 7.91MB increase in memory usage on the Parser/html5-full-render:Malloc test while it was in the tree. ( http://webkit-perf.appspot.com/graph.html#tests=[[6268923,2001,7288486]]&sel=none&displayrange=7&datatype=running ) (In reply to comment #52) > Looks like this patch caused a 7.91MB increase in memory usage on the Parser/html5-full-render:Malloc test while it was in the tree. ( http://webkit-perf.appspot.com/graph.html#tests=[[6268923,2001,7288486]]&sel=none&displayrange=7&datatype=running ) Woah that can't be right. Two RefPtrs in rare data shouldn't be causing 8mb of memory usage... how do I profile this? (In reply to comment #53) > (In reply to comment #52) > > Looks like this patch caused a 7.91MB increase in memory usage on the Parser/html5-full-render:Malloc test while it was in the tree. ( http://webkit-perf.appspot.com/graph.html#tests=[[6268923,2001,7288486]]&sel=none&displayrange=7&datatype=running ) > > Woah that can't be right. Two RefPtrs in rare data shouldn't be causing 8mb of memory usage... how do I profile this? I looked at the HTML5 spec http://www.whatwg.org/specs/web-apps/current-work/ and it has 137447 elements and 186374 text noes. This patch means creating another 4459 Element's for the PseudoElements, or a 1.37% growth in the number of Nodes and a 3.2% growth in the number of Elements. Presumably our memory should be scaling with those factors. There must be some bug in my code, I can't imagine that Element is 1.8k per instance. (In reply to comment #54) > (In reply to comment #53) > > (In reply to comment #52) > > > Looks like this patch caused a 7.91MB increase in memory usage on the Parser/html5-full-render:Malloc test while it was in the tree. ( http://webkit-perf.appspot.com/graph.html#tests=[[6268923,2001,7288486]]&sel=none&displayrange=7&datatype=running ) > > > > Woah that can't be right. Two RefPtrs in rare data shouldn't be causing 8mb of memory usage... how do I profile this? > > I looked at the HTML5 spec http://www.whatwg.org/specs/web-apps/current-work/ and it has 137447 elements and 186374 text noes. > > This patch means creating another 4459 Element's for the PseudoElements, or a 1.37% growth in the number of Nodes and a 3.2% growth in the number of Elements. Presumably our memory should be scaling with those factors. > > There must be some bug in my code, I can't imagine that Element is 1.8k per instance. If you are developing on Mac, I suggest using Instruments.app with the "Allocations" tool and getting a full profile of loading the test file. (And make sure you're using a release build, as debug builds have quite different memory usage characteristics.) (In reply to comment #54) > This patch means creating another 4459 Element's for the PseudoElements, or a 1.37% growth in the number of Nodes and a 3.2% growth in the number of Elements. Presumably our memory should be scaling with those factors. > > There must be some bug in my code, I can't imagine that Element is 1.8k per instance. I would guess that this is the cost of creating a ton of ElementRareDatas on nodes that otherwise wouldn't need them. ElementRareData is actually quite large. Any pages that uses a lot of generated content nodes, but otherwise doesn't hit ElementRareData, will definitely use a lot more memory. Created attachment 168835 [details]
Patch
Fix use-after-free bugs from not explictly calling detch and fix Range code that was confused by the new nodes which broke the DevTools inspector. This isn't meant for landing, I'm going to instead break this apart into parts where we can land most of it without turning it on and then land the final part to avoid having to roll it in and out repeatedly.
(In reply to comment #57) > Created an attachment (id=168835) [details] > Patch > > Fix use-after-free bugs from not explictly calling detch and fix Range code that was confused by the new nodes which broke the DevTools inspector. This isn't meant for landing, I'm going to instead break this apart into parts where we can land most of it without turning it on and then land the final part to avoid having to roll it in and out repeatedly. I've found the leak: I keep the PseudoElement in a RefPtr in the ElementRareData and just assign 0 to it to remove the last ref, but because of TreeShared::deref() semantics even though the m_refCount is 0 the PseudoElement doesn't actually get freed because it still has a parent. Instead to follow the TreeShared design we should just forcibly delete the PseudoElements ignoring their ref count (or I guess we could unset it's parent and then deref?). I now see a 2MB increase in memory usage on that test in Instruments and it's stable between runs so it's not leaking anymore, but it also seems to allocate 13k PseudoElements per run even though there should only be 2.5k :/ (In reply to comment #58) > ... > I now see a 2MB increase in memory usage on that test in Instruments and it's stable between runs so it's not leaking anymore, but it also seems to allocate 13k PseudoElements per run even though there should only be 2.5k :/ Figured this out, there really are 13k pseudos in that test's output but there's only 2.5k in the source html5.html file! There's a bug in html5-full-render.html when chunking up the source HTML using substring where it should be using substr. html5-full-render.html:16 has (w/ chunkSize = 500000): spec.substring(chunkIndex * chunkSize, chunkSize); but this is wrong because substring is (from, to) so it should be: spec.substr(chunkIndex * chunkSize, chunkSize); to get chunks of the specified size. Right now the html5-full-render.html actually loads massive chunks of the HTML5 spec that wrap around to the start repeatedly over and over yielding the following chunk sizes: Chunk 1: 500000 Chunk 2: 0 Chunk 3: 500000 Chunk 4: 1000000 Chunk 5: 1500000 Chunk 6: 2000000 Chunk 7: 2500000 Chunk 8: 3000000 Chunk 9: 3500000 Chunk 10: 4000000 Chunk 11: 4500000 Chunk 12: 5000000 Chunk 13: 5500000 So while each chunk was supposed to be 500000, we end up loading chunks anywhere from 2-11x that in size. @eseidel: It looks like you were the one who landed this test with the chunking in https://trac.webkit.org/changeset/96647 I can fix the bug, but the test will become less stressful since we'll be loading 5x less chars: 33.5m vs 6.5m chars. The good news is that there's only a ~150 byte overhead per pseudo with this patch. What do you want to do here Eric? :) Lets fix the perf test first, in a separate bug. :) Then, lets re-land this with the known memory effect, documented in the ChangeLog. Thanks! (In reply to comment #59) > The good news is that there's only a ~150 byte overhead per pseudo with this patch. That's 150 bytes more than we used to use or 150 bytes total per pseudo? Would you mind instrumenting your local build to spit out the number of psuedoelements and then testing out a few major sites (e.g. test the html spec, gmail, facebook, etc)? That way we can get a sense of what sort of memory impact to expect. (In reply to comment #61) > (In reply to comment #59) > > The good news is that there's only a ~150 byte overhead per pseudo with this patch. > > That's 150 bytes more than we used to use or 150 bytes total per pseudo? Would you mind instrumenting your local build to spit out the number of psuedoelements and then testing out a few major sites (e.g. test the html spec, gmail, facebook, etc)? That way we can get a sense of what sort of memory impact to expect. That's 150 bytes more than we used to use for each one. Gmail is ~40, Docs is ~30 pseudos. Facebook uses 175 on first load of my timeline and uses more and more as I scroll down and content loads. Twitter uses 134 on first load and uses more as I scroll down and content loads. So that's +25k memory for Facebook, +19k for Twitter, and about +6k for Docs and Gmail. 6500 pseudos is equal to approximately 1MB of increased RAM usage. This is not any different than if you inserted the equivalent number of DOM nodes and touched anything that allocates rare data including: classList, childNodes, getElementsBy(TagName|ClassName) or a bunch of other things on them. We could move them out of the RareData and into static maps to reduce memory usage, but I think it'd make more sense to do that in a separate patch after this lands. Created attachment 170532 [details]
Patch
(In reply to comment #63) > Created an attachment (id=170532) [details] > Patch I posted this just to get it up there and get eyes on the changes/simplifications. I want to land this in parts so we don't need to roll the whole thing out again if something goes wrong. Comment on attachment 170532 [details] Patch Attachment 170532 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14562541 New failing tests: editing/selection/hit-test-anonymous.html Comment on attachment 170532 [details] Patch Attachment 170532 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14564534 New failing tests: editing/selection/hit-test-anonymous.html Created attachment 176852 [details]
Patch
This is all done! |
Created attachment 160788 [details] Concept We should move the generated content nodes into the DOM. This has the nice side effect of vastly simplifying the inside of the render tree in favor of centralized code for insert/remove from the DOM. From inside the render tree we should then be able to ignore if a node is from :before or an actual <span>.