EDelay post-insertion notifications until new DOM tree is complete
Created attachment 134626 [details] Patch
Attachment 134626 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 135454 [details] Patch
Created attachment 135658 [details] Patch
Now ready for review. The big idea here is to ensure that inserting a document fragment doesn't have any side-effects _while the DOM tree is being constructed_.
Comment on attachment 135658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135658&action=review I'm uncomfortably excited about this change. :) > Source/WebCore/dom/ContainerNode.cpp:169 > // Due to arbitrary code running in response to a DOM mutation event it's > - // possible that "next" is no longer a child of "this". > - // It's also possible that "child" has been inserted elsewhere. > - // In either of those cases, we'll just stop. > - if (next->parentNode() != this) > - break; > - if (child->parentNode()) > + // possible that "child" has been inserted elsewhere. Is the only case this happens now if you're appending a script element? > Source/WebCore/dom/ContainerNode.cpp:175 > treeScope()->adoptIfNeeded(child); Here and below, it might be worth adding a comment that adoptIfNeeded will never fire sync JS events because we've already removed the child from it's existing parent and so there are no DOM mutations involved here. > Source/WebCore/dom/ContainerNode.cpp:1090 > + // FIXME: Attachment should be the first operation in this function, but some code Nit: s/this function/this loop/
Comment on attachment 135658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135658&action=review >> Source/WebCore/dom/ContainerNode.cpp:169 >> + // possible that "child" has been inserted elsewhere. > > Is the only case this happens now if you're appending a script element? No, this is still checking for the case of gaining a parent due to events fired during reparenting, above. I left this here to avoid a second loop. I think the cleanest thing will be to move this check (and the vector resizing) into collectChildrenAndRemoveFromOldParent, but that can be easily accomplished in a followup change; I wanted to keep the restructuring to a minimum in this one. >> Source/WebCore/dom/ContainerNode.cpp:175 >> treeScope()->adoptIfNeeded(child); > > Here and below, it might be worth adding a comment that adoptIfNeeded will never fire sync JS events because we've already removed the child from it's existing parent and so there are no DOM mutations involved here. TreeScope::adoptIfNeeded doesn't do reparenting (and thus fires no mutation events, ever). Document::adoptNode is what you're thinking of, but that calls removeChild explicitly. I don't think a comment is called for, or if it is, it belongs on adoptIfNeeded, not its callers. >> Source/WebCore/dom/ContainerNode.cpp:1090 >> + // FIXME: Attachment should be the first operation in this function, but some code > > Nit: s/this function/this loop/ I think I still mean "this function": why should this happen after childrenChanged?
Comment on attachment 135658 [details] Patch Clearing flags on attachment: 135658 Committed r113267: <http://trac.webkit.org/changeset/113267>
All reviewed patches have been landed. Closing bug.
Reopen the bug since the patch was rolled out.
After some investigation with Ojan, it looks to be a basic issue with the structure of this code: the code in this patch calls attach() in a separate loop from that which appended the node to the DOM tree. For innerHTML calls with many (thousands) of top-level nodes, this can cause a 2x (or worse) slowdown, presumably due to cache misses the second time through the loop. Still looking for a nice solution.
Carrying out this change seems to cause an unavoidable perf regression when a DocumentFragment containing many Nodes is added, since it's now necessary to touch each Node twice, once when setting its siblings and parent pointers and then again, later, to notify it of insertion, attach a renderer, etc. Balanced against the perf regression, I don't think this refactoring is worthwhile.