I'm about to remove the last non-lazy attaching element. https://bugs.webkit.org/show_bug.cgi?id=45365 I think we can now kill the whole concept of being "attached". attached() does not necessarily mean you have a renderer. It just means that something called attach() or lazyAttach() on you. Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching". (We could also fix the current lazyAttach() code to check to see if a parent is already marked with a child needing style resolve, and stop walking to prevent N^2 behavior.) I'm not sure if it would make sense to keep the "attached" flag around to make sure the attach logic is never run twice. -eric
On Sep 9, 2010, at 12:00 AM, Eric Seidel wrote:
I'm about to remove the last non-lazy attaching element. https://bugs.webkit.org/show_bug.cgi?id=45365
I think we can now kill the whole concept of being "attached".
attached() does not necessarily mean you have a renderer. It just means that something called attach() or lazyAttach() on you.
Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching".
Presumably they should mark their parents as "child needs style recalc", not needing recalc themselves.
(We could also fix the current lazyAttach() code to check to see if a parent is already marked with a child needing style resolve, and stop walking to prevent N^2 behavior.)
I'm not sure if it would make sense to keep the "attached" flag around to make sure the attach logic is never run twice.
Do any attach() methods still do nontrivial work? Is it an error to call it twice, or not properly paired up with detach()? Regards, Maciej
On Thu, Sep 9, 2010 at 12:43 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 9, 2010, at 12:00 AM, Eric Seidel wrote:
I'm about to remove the last non-lazy attaching element. https://bugs.webkit.org/show_bug.cgi?id=45365
I think we can now kill the whole concept of being "attached".
attached() does not necessarily mean you have a renderer. It just means that something called attach() or lazyAttach() on you.
Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching".
Presumably they should mark their parents as "child needs style recalc", not needing recalc themselves.
Turns out they also have to mark themselves as needing recalc. I tried only marking the parents in this latest patch and failed. :) http://trac.webkit.org/browser/trunk/WebCore/dom/Node.cpp#L759
(We could also fix the current lazyAttach() code to check to see if a parent is already marked with a child needing style resolve, and stop walking to prevent N^2 behavior.)
I'm not sure if it would make sense to keep the "attached" flag around to make sure the attach logic is never run twice.
Do any attach() methods still do nontrivial work? Is it an error to call it twice, or not properly paired up with detach()?
I haven't done a full survey of attach() yet, but yes, many elements override attach. It is an error to call attach() twice: http://trac.webkit.org/browser/trunk/WebCore/dom/Node.cpp#L1191
On Sep 9, 2010, at 1:29 AM, Eric Seidel wrote:
On Thu, Sep 9, 2010 at 12:43 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 9, 2010, at 12:00 AM, Eric Seidel wrote:
I'm about to remove the last non-lazy attaching element. https://bugs.webkit.org/show_bug.cgi?id=45365
I think we can now kill the whole concept of being "attached".
attached() does not necessarily mean you have a renderer. It just means that something called attach() or lazyAttach() on you.
Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching".
Presumably they should mark their parents as "child needs style recalc", not needing recalc themselves.
Turns out they also have to mark themselves as needing recalc. I tried only marking the parents in this latest patch and failed. :) http://trac.webkit.org/browser/trunk/WebCore/dom/Node.cpp#L759
Then I somewhat suspect the patch may be buggy buggy and may cause performance regressions. In cases where you insert elements into an attached tree. It very much seems like this would cause unnecessary style recalcs.
(We could also fix the current lazyAttach() code to check to see if a parent is already marked with a child needing style resolve, and stop walking to prevent N^2 behavior.)
I'm not sure if it would make sense to keep the "attached" flag around to make sure the attach logic is never run twice.
Do any attach() methods still do nontrivial work? Is it an error to call it twice, or not properly paired up with detach()?
I haven't done a full survey of attach() yet, but yes, many elements override attach.
It is an error to call attach() twice: http://trac.webkit.org/browser/trunk/WebCore/dom/Node.cpp#L1191
Then it seems like the flag is needed for now. Perhaps some of this work can move to insertedIntoDocument over time. Regards, Maciej
On Thu, Sep 9, 2010 at 1:46 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 9, 2010, at 1:29 AM, Eric Seidel wrote:
On Thu, Sep 9, 2010 at 12:43 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 9, 2010, at 12:00 AM, Eric Seidel wrote:
Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching".
Presumably they should mark their parents as "child needs style recalc", not needing recalc themselves.
Turns out they also have to mark themselves as needing recalc. I tried only marking the parents in this latest patch and failed. :) http://trac.webkit.org/browser/trunk/WebCore/dom/Node.cpp#L759
Then I somewhat suspect the patch may be buggy buggy and may cause performance regressions. In cases where you insert elements into an attached tree. It very much seems like this would cause unnecessary style recalcs.
I'm not sure what you mean. You mean that the change in my patch to run the lazyAttach logic from insertedIntoDocument may cause additional style recalcs? The new element needs style calculated regardless. Marking an element with childNeedsStyleRecalc doesn't recalculate the style for that object to my knowledge.
On Sep 9, 2010, at 12:17 PM, Eric Seidel wrote:
I'm not sure what you mean. You mean that the change in my patch to run the lazyAttach logic from insertedIntoDocument may cause additional style recalcs? The new element needs style calculated regardless. Marking an element with childNeedsStyleRecalc doesn't recalculate the style for that object to my knowledge.
Oh I thought lazyAttach was actually marking nodes as needing a full style recalc. If it's not then I guess that's fine. dave
Overall, this is a great change. It just needs to also take into the account HTMLPluginImageElement. :DG< On Thu, Sep 9, 2010 at 10:23 AM, David Hyatt <hyatt@apple.com> wrote:
On Sep 9, 2010, at 12:17 PM, Eric Seidel wrote:
I'm not sure what you mean. You mean that the change in my patch to run the lazyAttach logic from insertedIntoDocument may cause additional style recalcs? The new element needs style calculated regardless. Marking an element with childNeedsStyleRecalc doesn't recalculate the style for that object to my knowledge.
Oh I thought lazyAttach was actually marking nodes as needing a full style recalc. If it's not then I guess that's fine.
dave
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
You would know this code better than I: http://trac.webkit.org/browser/trunk/WebCore/dom/Node.cpp#L747 It's not clear to me how lazyAttach() ends up calling attach in the end (or if it does). It seems impossible that it would end up calling attach later since lazyAttach normally marks elements as attached(). However since many elements override attach() the lazyAttach() code path might be doing the wrong thing for many elements. Again, I'm not changing when lazyAttach() is called, just making <frame>/<object> be lazy-attach-safe. I'm glad we're having this discussion though as I would like to make a wider deployment of lazyAttach after this lands. I think it could be a PLT win (and certainly a parser benchmark win!) On Thu, Sep 9, 2010 at 10:23 AM, David Hyatt <hyatt@apple.com> wrote:
On Sep 9, 2010, at 12:17 PM, Eric Seidel wrote:
I'm not sure what you mean. You mean that the change in my patch to run the lazyAttach logic from insertedIntoDocument may cause additional style recalcs? The new element needs style calculated regardless. Marking an element with childNeedsStyleRecalc doesn't recalculate the style for that object to my knowledge.
Oh I thought lazyAttach was actually marking nodes as needing a full style recalc. If it's not then I guess that's fine.
dave
On Sep 9, 2010, at 2:00 AM, Eric Seidel wrote:
Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching".
lazyAttach is poorly implemented right now, since it is relying on style recalculation to trigger the attachment when it should have used a new timer. Right now because it is latching onto the style recalc timer and tree it is wasting time doing recalc styles on all of the ancestors of any elements that need attaching. That's just bad. This means the normal attach() process performs better (and is presumably still used by the parser). We should fix lazyAttach to use its own timer and tree walk independent of recalcStyle that just calls attach() instead on children that need it. dave (hyatt@apple.com)
On Thu, Sep 9, 2010 at 9:45 AM, David Hyatt <hyatt@apple.com> wrote:
On Sep 9, 2010, at 2:00 AM, Eric Seidel wrote:
Instead, we could change all elements to mark themselves (and their parents) as needing style recalc during insertedIntoDocument(), effectively "attaching/lazyAttaching".
lazyAttach is poorly implemented right now, since it is relying on style recalculation to trigger the attachment when it should have used a new timer. Right now because it is latching onto the style recalc timer and tree it is wasting time doing recalc styles on all of the ancestors of any elements that need attaching. That's just bad.
I'm not sure a new timer is right. I don't think we need/want a timer at all. Probably just a flag on document which says "something needs an attach, do a full tree walk". Which we check during style resolve and do a tree walk. We don't want to attach any more eagerly than necessary. We have to be attached to resolve style, but not before. I'm not sure how that's necessarily better than being part of the style recalc. But I don't feel like an expert enough here to know.
This means the normal attach() process performs better (and is presumably still used by the parser).
attach() is used in many places, but not all. lazyAttach() is used by the Adoption Agency Algorithm in the parser as well as ContainerNode's insertBefore, appendChild and replaceChild.
We should fix lazyAttach to use its own timer and tree walk independent of recalcStyle that just calls attach() instead on children that need it.
Please explain more. I'm not sure how that is better than marking parents as "childNeedsStyleRecalc" which is what we do now. As far as I know that doesn't cause the parents to recalc. In order to attach we need a style calculation. I agree, it's possible to end up walking the rendering tree too often if we're checking if it needs style recalc after inserting every element anyway. My understanding is that lazyAttach was written to make Acid3 faster by avoiding the malloc associated with attach()/createRenderer() in the cases where elements never end up rendered. It also helps with the Adoption Agency Algorithm as elements get inserted and then moved again and don't necessarily end up rendered in their original location.
participants (4)
-
David Hyatt
-
Dimitri Glazkov
-
Eric Seidel
-
Maciej Stachowiak