Bug 86813 – WebKit spends ~20% of time in HTMLTextAreaElement::defaultValue() when opening a review page

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

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%).

[Ryosuke Niwa]

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?

[Ojan Vafai]

(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.

[Ryosuke Niwa]

(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.

[Eric Seidel (no email)]

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.

[Ryosuke Niwa]

(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?

[Filip Pizlo]

(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.

[Eric Seidel (no email)]

We could be in the middle of a <textArea> when the parser pauses and scripts run (from a load or whatever), sure.

[Ryosuke Niwa]

(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.

[Eric Seidel (no email)]

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.

[Ojan Vafai]

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.

[Antti Koivisto]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=87034 to track treeScope() slowness per Dimitri's request.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 143097 [details]
Fixes the bug

[Eric Seidel (no email)]

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!

[Ryosuke Niwa]

Created attachment 143101 [details]
Patch for landing

[Ryosuke Niwa]

I'm going to hold off on landing this patch until the bug 86994 is fixed (hopefully later today).

[WebKit Review Bot]

Comment on attachment 143101 [details]
Patch for landing

Clearing flags on attachment: 143101

Committed r118149: <http://trac.webkit.org/changeset/118149>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Kent Tamura]

(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.

[Ryosuke Niwa]

(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?

[Kent Tamura]

(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.