Reproduction steps: 1. Open https://bugs.webkit.org/attachment.cgi?id=142634 (Caution: 5MB!) 2. Instrument the Web Process while WebKit opens it Observe that we spend 19.5% of time in HTMLTextAreaElement::defaultValue() inside HTMLTextAreaElement::childrenChanged, which in turn is called by ContainerNode::parserAddChild (18.5%).
Obviously, we should be using StringBuilder in this function. But even then we still have O(n^2) algorithm here because parserAddChild is going to call this function whenever new child is added :( 373 String HTMLTextAreaElement::defaultValue() const 374 { 375 String value = ""; 376 377 // Since there may be comments, ignore nodes other than text nodes. 378 for (Node* n = firstChild(); n; n = n->nextSibling()) { 379 if (n->isTextNode()) 380 value += toText(n)->data(); 381 } 382 383 return value; 384 } Perhaps we can lazily update the default value when the parser is adding children?
(In reply to comment #0) > Reproduction steps: > 1. Open https://bugs.webkit.org/attachment.cgi?id=142634 (Caution: 5MB!) > 2. Instrument the Web Process while WebKit opens it > > Observe that we spend 19.5% of time in HTMLTextAreaElement::defaultValue() inside HTMLTextAreaElement::childrenChanged, which in turn is called by ContainerNode::parserAddChild (18.5%). Did you link to the wrong page? There's no textareas on that page, just plain text.
(In reply to comment #2) > Did you link to the wrong page? There's no textareas on that page, just plain text. Oh sorry, I meant https://bugs.webkit.org/attachment.cgi?id=142634&action=review.
We should be able to tell that we're still during a parse and do it during the finishedParsingChildren method, or whatever it's named these days. the "we just parsed the end tag" method.
(In reply to comment #4) > We should be able to tell that we're still during a parse and do it during the finishedParsingChildren method, or whatever it's named these days. the "we just parsed the end tag" method. Can scripts run while parsing textarea content?
(In reply to comment #5) > (In reply to comment #4) > > We should be able to tell that we're still during a parse and do it during the finishedParsingChildren method, or whatever it's named these days. the "we just parsed the end tag" method. > > Can scripts run while parsing textarea content? Even if they can't currently, it feels safer to assume that they could. Imagine the horror if someone made it so that scripts could run while parsing textarea content and we had put in an optimization that relied on them not running.
We could be in the middle of a <textArea> when the parser pauses and scripts run (from a load or whatever), sure.
(In reply to comment #7) > We could be in the middle of a <textArea> when the parser pauses and scripts run (from a load or whatever), sure. I see. That makes things a little tricker I guess because some Web-facing API relies on the fact we update values whenever children changes. So there's some book keeping to do.
Lots of web-facing apis rely on the fact that we layout/resolve-style before we return them values. :) Similarly we would need to keep a dirty bit here and know to refresh our cache whenever those are called.
I can't even load the page on my laptop. :( If I could, I'd try to understand what textareas are so slow. Only think I can think of is that we load the old review page in an iframe that contains the full diff as text in the textarea. I could imagine that being slow. It's silly through really. We don't need to load the diff in that iframe at all anymore. Noone uses the old review page anymore. We should fix the inefficiency on the C++ side first of course, but after that, we should just change the review page to not be dumb.
Can anyone think of a valid use for String::operator+=()? If not we should just remove it and make all current clients use StringBuilder instead.
It appears that just using StringBuilder instead of operator+= will fix this perf. hit. Now we're spending roughly 8% of time in treeScope() :( Someone needs to make treeScope O(1). It's showing up every time I profile some DOM operation.
Filed https://bugs.webkit.org/show_bug.cgi?id=87034 to track treeScope() slowness per Dimitri's request.
We could also lazy construct string at finishParsingChildren (paired with beginParsingChildren) but maybe we can do something more general like not replacing all children in the shadow DOM when some nodes match.
(In reply to comment #14) > We could also lazy construct string at finishParsingChildren (paired with beginParsingChildren) but maybe we can do something more general like not replacing all children in the shadow DOM when some nodes match. Turns out that this requires a considerable amount of refactoring. Let's just replace String by StringBuilder for now.
Created attachment 143097 [details] Fixes the bug
Comment on attachment 143097 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=143097&action=review LGTM. > Source/WebCore/ChangeLog:13 > + Also avoid the redundant call to setValue in childrenChaned when the value is dirty. childrenChaned!
Created attachment 143101 [details] Patch for landing
I'm going to hold off on landing this patch until the bug 86994 is fixed (hopefully later today).
Comment on attachment 143101 [details] Patch for landing Clearing flags on attachment: 143101 Committed r118149: <http://trac.webkit.org/changeset/118149>
All reviewed patches have been landed. Closing bug.
This patch improved the score of tests added by the bug 86994 by 1000-3000%: http://webkit-perf.appspot.com/graph.html#tests=[[2831883,2001,3001],[2831883,2001,173262],[2831883,2001,963028],[2831883,2001,957069],[2831883,2001,2389050],[2965621,2001,32196],[2965621,2001,957069],[2965621,2001,173262],[2965621,2001,2389050],[2965621,2001,3001],[2965621,2001,963028],[2831883,2001,32196]]&sel=1337223431149,1337828231149&displayrange=7&datatype=running
(In reply to comment #22) > This patch improved the score of tests added by the bug 86994 by 1000-3000%: > http://webkit-perf.appspot.com/graph.html#tests=[[2831883,2001,3001],[2831883,2001,173262],[2831883,2001,963028],[2831883,2001,957069],[2831883,2001,2389050],[2965621,2001,32196],[2965621,2001,957069],[2965621,2001,173262],[2965621,2001,2389050],[2965621,2001,3001],[2965621,2001,963028],[2831883,2001,32196]]&sel=1337223431149,1337828231149&displayrange=7&datatype=running Cool! textarea-dom.html: Huge improvement textarea-parsing.html: 4% improvement textarea-edit.html: Not affected.
(In reply to comment #23) > Cool! > textarea-dom.html: Huge improvement > textarea-parsing.html: 4% improvement > textarea-edit.html: Not affected. Do you know where we're spending time in the latter two tests?
(In reply to comment #24) > (In reply to comment #23) > > Cool! > > textarea-dom.html: Huge improvement > > textarea-parsing.html: 4% improvement > > textarea-edit.html: Not affected. > > Do you know where we're spending time in the latter two tests? I haven't. Try replacing the textarea in textarea-edit.html with a <div contentEditable=true>. It's slightly fastter than the <textarea>, but it's still very slow.