Speed up updating inner value of HTMLTextFormControlElements.

Speed up updating inner value of HTMLTextFormControlElements.

Upon setting the inner string value of an element like <textarea/>, we
append a solitary trailing <br> if the new value ends with a line
break character (for layout and content editable purposes.)

Upon replacing that string value with another, check if the shadowed
inner element being updated has such a trailing <br> element and remove
it first, before proceeding with setting the new value. Doing so enables
fast replacement of text nodes rather than invoking the general path of
replacing a set of siblings with a text node.

This is of most benefit with Oilpan enabled, as it reduces the heap
allocations made when handling this update operation. For a deep-loop
microbenchmark like blink_perf.DOM.textarea-dom, a 10% speedup is
observed (linux, windows).

Linux64-chrome numbers on textarea-dom:

- Oilpan enabled:
  + ToT: 14.6 runs/s
  + CL:  16.6 runs/s

- Oilpan disabled:
  + ToT: 16.5 runs/s
  + CL:  16.9 runs/s

R=tkent,haraken,esprehn
BUG=

Committed: https://crrev.com/175ce93e9e4dd69a6191d28f7c21a63f2541f006
Cr-Commit-Position: refs/heads/master@{#350989}

[haraken, 2015-09-20 00:31:26]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, keishi@chromium.org, tkent@chromium.org,
yosin@chromium.org

[haraken, 2015-09-20 00:31:27]

Thanks for looking at this! This change makes sense to me.

+yosin, tkent, keishi

[yosin_UTC9, 2015-09-20 12:42:40]

Thanks for paying attention to TEXTAREA!

Rather than adding kludge, I would like to change use the fact that TEXTAREA
always contain
plain text, although it isn't true in current implementation, because
"white-space" property
is defined in "html.css" is associated to TEXTAREA rather than "inner-editor".

If TEXTAREA always have "white-space: pre-wrap", TEXTAREA can be represented by
one Text node,
and we don't need to bother with BR element.

So, we should make TEXTAREA element always have "white-space: pre-wrap". For
INPUT, it should be
"white-space: pre".

I'm not sure single Text node represent can be faster than multiple nodes
especially for large
text. But, I believe optimizing single Text node representation is easier than
multiple node
case.

Examples:
- data:text/html,<textarea style="white-space:normal"></textarea>
 yield BR
 This is a reason why I would like to make inner-editor to have "white-space:
pre-wrap"
- data:text/html,<textarea></textarea>
 no BR; until textarea.value = "foo\n"
- data:text/html,<div contenteditable="true" style="white-space: pre-wrap;
-webkit-user-modify: read-write-plaintext-only"></div>
 works like text area TEXTAREA

https://codereview.chromium.org/1355013002/diff/1/Source/core/html/HTMLTextFo...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1355013002/diff/1/Source/core/html/HTMLTextFo...
Source/core/html/HTMLTextFormControlElement.cpp:636: if (textIsChanged ||
innerEditor->hasChildren()) {
nit: Could you change this to early return style?

[sof, 2015-09-20 13:05:05]

This is addressing a perf blocker for Oilpan, not reworking
HTMLTextFormControlElement internals for longer-term benefits.

https://codereview.chromium.org/1355013002/diff/1/Source/core/html/HTMLTextFo...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1355013002/diff/1/Source/core/html/HTMLTextFo...
Source/core/html/HTMLTextFormControlElement.cpp:636: if (textIsChanged ||
innerEditor->hasChildren()) {
On 2015/09/20 12:42:40, Yosi_back_on_Sep_25 wrote:
> nit: Could you change this to early return style?

Done.

[esprehn, 2015-09-21 03:37:25]

Not lgtm. We should not do different things in terms of dom mutation in oilpan
and non oilpan mode.

If optimisation is good we should always do it.

[sof, 2015-09-21 05:26:49]

On 2015/09/21 03:37:25, esprehn wrote:
> Not lgtm. We should not do different things in terms of dom mutation in oilpan
> and non oilpan mode.
> 
> If optimisation is good we should always do it.

What dom mutation are you referring to? This is inside a shadow tree.

[esprehn, 2015-09-21 08:20:43]

On 2015/09/21 at 05:26:49, sigbjornf wrote:
> On 2015/09/21 03:37:25, esprehn wrote:
> > Not lgtm. We should not do different things in terms of dom mutation in
oilpan
> > and non oilpan mode.
> > 
> > If optimisation is good we should always do it.
> 
> What dom mutation are you referring to? This is inside a shadow tree.

I don't see how shadow trees are related, you're only calling removeChild when
oilpan is on. That isn't okay, oilpan should not toggle engine behavior in the
DOM or rendering system. It should only alter object lifetime and destruction
ordering.

[sof, 2015-09-21 08:34:49]

On 2015/09/21 08:20:43, esprehn wrote:
> On 2015/09/21 at 05:26:49, sigbjornf wrote:
> > On 2015/09/21 03:37:25, esprehn wrote:
> > > Not lgtm. We should not do different things in terms of dom mutation in
> oilpan
> > > and non oilpan mode.
> > > 
> > > If optimisation is good we should always do it.
> > 
> > What dom mutation are you referring to? This is inside a shadow tree.
> 
> I don't see how shadow trees are related, you're only calling removeChild when
> oilpan is on. That isn't okay, oilpan should not toggle engine behavior in the
> DOM or rendering system. It should only alter object lifetime and destruction
> ordering.

If it is in a shadow tree, this is not externally observable wrt dom mutations,
right? (I'm sure you are aware of that.)

Oilpan changes object lifetimes, surely?

[esprehn, 2015-09-21 08:49:24]

On 2015/09/21 at 08:34:49, sigbjornf wrote:
> ...
> 
> If it is in a shadow tree, this is not externally observable wrt dom
mutations, right? (I'm sure you are aware of that.)

That doesn't matter, we don't put behavior changes for the engine behind ifdefs
and oilpan shouldn't be impacting the way the engine operates. If this is a good
idea we should always do it, not just in oilpan mode.

You wouldn't put a separate layout path in LayoutFlexBox only for oilpan mode
even if it passed the tests so it wasn't "observable".

> 
> Oilpan changes object lifetimes, surely?

Yes I said that.

[sof, 2015-09-21 08:57:00]

On 2015/09/21 08:49:24, esprehn wrote:
> On 2015/09/21 at 08:34:49, sigbjornf wrote:
> > ...
> > 
> > If it is in a shadow tree, this is not externally observable wrt dom
> mutations, right? (I'm sure you are aware of that.)
> 
> That doesn't matter, we don't put behavior changes for the engine behind
ifdefs
> and oilpan shouldn't be impacting the way the engine operates. If this is a
good
> idea we should always do it, not just in oilpan mode.
> 
> You wouldn't put a separate layout path in LayoutFlexBox only for oilpan mode
> even if it passed the tests so it wasn't "observable".
> 

Modulo footprint and other code considerations vs payoff, I don't see how you
would blanket refuse that. Oilpan (or not) will be here soon enough, so the
codebase won't be in this dual state for long now.

> > 
> > Oilpan changes object lifetimes, surely?
> 
> Yes I said that.

Sure you did - so you have to adopt your code to that. Pretending that it
doesn't matter seems like a curious position.

[sof, 2015-09-21 11:57:07]

If textarea-dom is adjusted slightly to work over strings that vary, e.g.,

 for (var i = 0; i < childCount; ++i)
    nodes.push(document.createTextNode(i.toString() + 'A quick brown fox jumps
over the lazy dog.\n'));

then a slight speedup in runs/s can be seen on linux without Oilpan. So if
that's an acceptable tweak to make to that test, the need for making this
conditional on ENABLE(OILPAN) should fall away (afaik -- I'm not aware of other
tests that hammer this code.)

[sof, 2015-09-22 10:46:56]

On 2015/09/21 11:57:07, sof wrote:
> If textarea-dom is adjusted slightly to work over strings that vary, e.g.,
> 
>  for (var i = 0; i < childCount; ++i)
>     nodes.push(document.createTextNode(i.toString() + 'A quick brown fox jumps
> over the lazy dog.\n'));
> 
> then a slight speedup in runs/s can be seen on linux without Oilpan. So if
> that's an acceptable tweak to make to that test, the need for making this
> conditional on ENABLE(OILPAN) should fall away (afaik -- I'm not aware of
other
> tests that hammer this code.)

Done + updated description.

[sof, 2015-09-22 20:22:49]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 20:23:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355013002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355013002/40001

[commit-bot: I haz the power, 2015-09-22 21:59:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 21:59:23]

Dry run: This issue passed the CQ dry run.

[tkent, 2015-09-24 03:16:39]

tkent@chromium.org changed reviewers:
+ esprehn@chromium.org

[tkent, 2015-09-24 03:16:40]

https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/te...
File PerformanceTests/DOM/textarea-dom.html (right):

https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/te...
PerformanceTests/DOM/textarea-dom.html:19: nodes.push(document.createTextNode('A
quick brown fox jumps over the ' + i + 'th lazy dog.\n'));
Please land this separately.

[sof, 2015-09-25 08:50:24]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-25 08:50:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355013002/60001

[sof, 2015-09-25 08:51:35]

https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/te...
File PerformanceTests/DOM/textarea-dom.html (right):

https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/te...
PerformanceTests/DOM/textarea-dom.html:19: nodes.push(document.createTextNode('A
quick brown fox jumps over the ' + i + 'th lazy dog.\n'));
On 2015/09/24 03:16:40, tkent wrote:
> Please land this separately.

Done + adjusted description accordingly.

[tkent, 2015-09-25 09:03:36]

lgtm

https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:641:
cache->handleTextFormControlChanged(this);
It's safe to change the order of AX notification and setInnerText() because AX
notification is asynchronous.

[sof, 2015-09-25 09:34:05]

https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:636: if
(!textIsChanged && !innerEditor->hasChildren())
The RHS negation of (&&) was incorrect; fixed.

https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:641:
cache->handleTextFormControlChanged(this);
On 2015/09/25 09:03:36, tkent wrote:
> It's safe to change the order of AX notification and setInnerText() because AX
> notification is asynchronous.

It makes sense to have this notification post update even if? Moved it down a
bit.

[haraken, 2015-09-25 09:37:47]

LGTM

Elliott: If you think that this is just gaming micro-benchmarks and shouldn't be
landed, I'm fine with not landing the CL. We can just justify the regression.

[esprehn, 2015-09-25 18:34:37]

lgtm, this seems like a general purpose optimization where we reuse the Text
node directly instead of destroying it (and the <br>).

https://codereview.chromium.org/1355013002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1355013002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:641: //
requiring less allocations. Generally helpful, but avoids dead object
I would remove the comment about less allocations and the mention of Oilpan.
This seems like a reasonable thing to do in general.

"fast path of updating a text node."

[sof, 2015-09-26 06:20:23]

https://codereview.chromium.org/1355013002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/1355013002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:641: //
requiring less allocations. Generally helpful, but avoids dead object
On 2015/09/25 18:34:37, esprehn wrote:
> I would remove the comment about less allocations and the mention of Oilpan.
> This seems like a reasonable thing to do in general.
> 
> "fast path of updating a text node."

Done.

[sof, 2015-09-26 06:20:30]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-09-26 06:20:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1355013002/#ps100001
(title: "Pare down comment")

[commit-bot: I haz the power, 2015-09-26 06:20:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355013002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355013002/100001

[commit-bot: I haz the power, 2015-09-26 07:35:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-26 07:36:14]

Patchset 6 (id:??) landed as
https://crrev.com/175ce93e9e4dd69a6191d28f7c21a63f2541f006
Cr-Commit-Position: refs/heads/master@{#350989}