Avoid allocating temporary ranges in connection with text and character iterators.

Avoid allocating temporary ranges in connection with text and character iterators.

30% performance improvement on DOM/textarea-edit on the oilpan build.
2% performance improvement on DOM/textarea-edit on the non-oilpan build.

R=yosin@chromium.org
BUG=388681
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181551

[yosin_UTC9, 2014-09-08 01:55:19]

LGTM

This patch also makes call sites of |TextIterator| simpler. I love this. (^_^)b

yoichio@, I'm not sure 2% performance improvement also applies to Speedometer
mark or not. For non-Oilpan, this patch avoids to create temporary |Range|
object. It worth to evaluate against Speedometer.

There is one more temporary |Range| creations, it is in loop, at L98 of
|PlainTextRange::createRangeFor()|.

I guess we can apply similar changes for other call sites of
|{Text,Character}Iterator::range()|.

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/PlainTex...
File Source/core/editing/PlainTextRange.cpp (right):

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/PlainTex...
Source/core/editing/PlainTextRange.cpp:91:
resultRange->setEnd(textRunRange->startContainer(), 0, ASSERT_NO_EXCEPTION);
nit: It seems we can replace L91 |textRunRange->startContainer()| to
|it.startContainer()| as L90.

If so, we can get rid of |Range| object creation at |it.range()|.

TODO: We should rename |TextIterator::range()| to |CreateRange()| or another to
indicate this is expensive operation rather than accessing cheap property
access.

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/TextIter...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/TextIter...
Source/core/editing/TextIterator.cpp:1101: if (m_positionOffsetBaseNode) {
nit: It seems early return reduce indentation.

if (!m_posiionOffsetBaseNode)
  return;
int index = ....
...

[Mads Ager (chromium), 2014-09-08 08:50:46]

Thanks!

The remaining temporary range in PlainTextRange is a little more complicated and
I would like to do that one separately.

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/PlainTex...
File Source/core/editing/PlainTextRange.cpp (right):

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/PlainTex...
Source/core/editing/PlainTextRange.cpp:91:
resultRange->setEnd(textRunRange->startContainer(), 0, ASSERT_NO_EXCEPTION);
On 2014/09/08 01:55:19, Yosi_UTC9 wrote:
> nit: It seems we can replace L91 |textRunRange->startContainer()| to
> |it.startContainer()| as L90.
> 
> If so, we can get rid of |Range| object creation at |it.range()|.

Yes, good catch, not sure why I didn't notice that. Thanks!
 
> TODO: We should rename |TextIterator::range()| to |CreateRange()| or another
to
> indicate this is expensive operation rather than accessing cheap property
> access.

Yes, we should.

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/TextIter...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/544083002/diff/1/Source/core/editing/TextIter...
Source/core/editing/TextIterator.cpp:1101: if (m_positionOffsetBaseNode) {
On 2014/09/08 01:55:19, Yosi_UTC9 wrote:
> nit: It seems early return reduce indentation.
> 
> if (!m_posiionOffsetBaseNode)
>   return;
> int index = ....
> ...

Done.

[Mads Ager (chromium), 2014-09-08 08:50:51]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-09-08 08:51:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/544083002/20001

[commit-bot: I haz the power, 2014-09-08 10:20:52]

Committed patchset #2 (id:20001) as 181551