Oilpan: Introduce TempRangeScope to avoid needless Range attaches on Document.

Oilpan: Introduce TempRangeScope to avoid needless Range attaches on Document.

In multiple places Ranges are used as short-lived subcomputation results.
These short-lived result objects are registered with the Document even
though they will be unreachable without any changes to the document.
With Oilpan this becomes a performance issue because we require a GC
for the Range objects to die and be removed from the set of ranges
on the Document and we end up spending quite a bit of time running
though ranges that need no updating. With the TempRangeScope we
avoid registering the temporary ranges with the document.

R=haraken@chromium.org, oilpan-reviews@chromium.org

[Mads Ager (chromium), 2014-06-24 11:47:20]

This improves performance on DOM/textarea-edit.html from 1.3 runs/s to 1.7
runs/s on my machine.

[haraken, 2014-06-24 11:57:16]

+yosin, +yutak

Mads:

- Does the performance regression come from the overhead of weak processing for
the attached Ranges?

- This change looks OK to me but is a bit fragile (we might miss attaching
ranges that should be attached). Shall we add an ASSERT to Document's methods
that iterate m_ranges to verify that those methods are not called while we're in
an active TempRangeScope?

https://codereview.chromium.org/330383004/diff/1/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/330383004/diff/1/Source/core/dom/Range.h#newc...
Source/core/dom/Range.h:183: class TempRangeScope {

TempRangeScope => TemporaryRangeScope

Blink prefers a fully qualified name.

[Mads Ager (chromium), 2014-06-24 12:02:33]

On 2014/06/24 11:57:16, haraken wrote:
> +yosin, +yutak
> 
> Mads:
> 
> - Does the performance regression come from the overhead of weak processing
for
> the attached Ranges?

No, the performance regression is coming from repeated iteration over attached
ranges that are no longer reachable. You get the same kind of speedup if you
just comment out all the iteration over AttachedSet in Document.cpp.

> - This change looks OK to me but is a bit fragile (we might miss attaching
> ranges that should be attached). Shall we add an ASSERT to Document's methods
> that iterate m_ranges to verify that those methods are not called while we're
in
> an active TempRangeScope?

That is indeed a good idea to verify that the use of TempRangeScope is correct.
Will do. 

> https://codereview.chromium.org/330383004/diff/1/Source/core/dom/Range.h
> File Source/core/dom/Range.h (right):
> 
>
https://codereview.chromium.org/330383004/diff/1/Source/core/dom/Range.h#newc...
> Source/core/dom/Range.h:183: class TempRangeScope {
> 
> TempRangeScope => TemporaryRangeScope
> 
> Blink prefers a fully qualified name.

[Mads Ager (chromium), 2014-06-24 12:16:51]

Added asserts to all methods that iterate the AttachedRangeSet to make sure that
we are not updating ranges while under a TemporaryRangeScope. This is not enough
to completely verify that we are doing the right thing. If you allocate a Range
under a temporary range scope and them store it in a field of some other object
you can have a range that is not attached and which will therefore not be
updated. So caution is still needed when using the TemporaryRangeScope objects.

https://codereview.chromium.org/330383004/diff/1/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/330383004/diff/1/Source/core/dom/Range.h#newc...
Source/core/dom/Range.h:183: class TempRangeScope {
On 2014/06/24 11:57:16, haraken wrote:
> 
> TempRangeScope => TemporaryRangeScope
> 
> Blink prefers a fully qualified name.

Done.

[haraken, 2014-06-24 12:24:23]

LGTM. Let's wait for an approval of yosin@ or yutak@.

https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:73: if (!TemporaryRangeScope::active())

You can move this check into Document::attachRange, since we're verifying
!TemporaryRangeScope::active() in Document's methods.

https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:91: if (!TemporaryRangeScope::active())

Ditto.

[wibling-chromium, 2014-06-24 13:19:22]

lgtm

An alternative approach would be to have a subclass, e.g. AttachedRange, which
attached itself to the document. This makes it more explicit in the object model
what is happening IMO.
However it might be a more elaborate change since all current Range need to be
changed to AttachedRange, except for the ones that are not in
TemporaryRangeScope.

[wibling-chromium, 2014-06-24 13:23:16]

On 2014/06/24 13:19:22, wibling-chromium wrote:
> lgtm
> 
> An alternative approach would be to have a subclass, e.g. AttachedRange, which
> attached itself to the document. This makes it more explicit in the object
model
> what is happening IMO.
> However it might be a more elaborate change since all current Range need to be
> changed to AttachedRange, except for the ones that are not in
> TemporaryRangeScope.

are not -> are now

[yosin_UTC9, 2014-06-25 01:22:20]

I prefer to rewrite functions using *ephemeral* Range objects to use two
Position objects rather than introducing |TempRangeScope|. |TempRangeScope| may
introducing bugs when we put temporary Range object into heap or returning as
value.

I believe Oiplpan makes our life easier about memory management or object
extents. It seems |TempRangeScope| makes me to consider life time of |Range|
object...

I can help to rewriting them, please file bugs and label "Cr-Blink-Editing".

https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.h#...
Source/core/dom/Range.h:187: static bool active() { return s_nesting != 0; }
nit: Just |return s_nesting|

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:332: TemporaryRangeScope scope;
It seems tempoary new Range object is created by |FrameSelection::firstRange()|
when |m_logicalRange| isn't null. But, cloned range is used for comparision. So,
we don't need to actually create new Range object.

We should change this logic to avoid creating Range object rather than using
|TemporaryRangeScope|.

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
File Source/core/editing/SpellChecker.cpp (right):

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
Source/core/editing/SpellChecker.cpp:749:
m_frame.document()->markers().removeMarkers(wordRange.get(),
DocumentMarker::Spelling);
It seems if we hare removeMakrers() which takes two Position objects, we don't
need to pass Range.

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
Source/core/editing/SpellChecker.cpp:752: TemporaryRangeScope scope;
ditto as L747

[yosin_UTC9, 2014-06-25 01:57:12]

Note: yoichio@ is working to remove VisiblePosition from HTMLTextFormControl.
So, temporary Range object won't be created in HTMLTextFormControl.cpp.

[yoichio, 2014-06-25 02:09:24]

On 2014/06/25 01:57:12, yosi wrote:
> Note: yoichio@ is working to remove VisiblePosition from HTMLTextFormControl.
> So, temporary Range object won't be created in HTMLTextFormControl.cpp.

I will let both visiblePositionForIndex and indexForVisiblePosition be for the
meantime.
That's because that those functions are used by other classes, e.c.
SpellChecker.

[Mads Ager (chromium), 2014-06-25 05:31:03]

On 2014/06/25 01:22:20, yosi wrote:
> I prefer to rewrite functions using *ephemeral* Range objects to use two
> Position objects rather than introducing |TempRangeScope|. |TempRangeScope|
may
> introducing bugs when we put temporary Range object into heap or returning as
> value.

That sounds a lot better. I must admit that I didn't even consider whether the
usages of Ranges here could be completely removed instead. :)

> I believe Oiplpan makes our life easier about memory management or object
> extents. It seems |TempRangeScope| makes me to consider life time of |Range|
> object...

Yes, agreed. I don't like that either and I considered various ways we could
deal with these Range objects differently in the Oilpan infrastructure. If we
can get rid of the temporary Rnage objects instead that would be a lot better.

> I can help to rewriting them, please file bugs and label "Cr-Blink-Editing".

Thank you! I'll have a quick look at it and see if I can get rid of some of
them. I'll file bugs for the rest and we can help eachother get rid of them.

> https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.h
> File Source/core/dom/Range.h (right):
> 
>
https://codereview.chromium.org/330383004/diff/20001/Source/core/dom/Range.h#...
> Source/core/dom/Range.h:187: static bool active() { return s_nesting != 0; }
> nit: Just |return s_nesting|
> 
>
https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Fram...
> File Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Fram...
> Source/core/editing/FrameSelection.cpp:332: TemporaryRangeScope scope;
> It seems tempoary new Range object is created by
|FrameSelection::firstRange()|
> when |m_logicalRange| isn't null. But, cloned range is used for comparision.
So,
> we don't need to actually create new Range object.
> 
> We should change this logic to avoid creating Range object rather than using
> |TemporaryRangeScope|.
> 
>
https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
> File Source/core/editing/SpellChecker.cpp (right):
> 
>
https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
> Source/core/editing/SpellChecker.cpp:749:
> m_frame.document()->markers().removeMarkers(wordRange.get(),
> DocumentMarker::Spelling);
> It seems if we hare removeMakrers() which takes two Position objects, we don't
> need to pass Range.
> 
>
https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
> Source/core/editing/SpellChecker.cpp:752: TemporaryRangeScope scope;
> ditto as L747

[Mads Ager (chromium), 2014-06-25 11:41:42]

I don't think I have enough knowledge about this code to figure out how to
easily avoid these temporary Range objects. Thanks for volunteering to help
figure this out yosin@, I'll file bug reports for it and we can discuss further
there.

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:332: TemporaryRangeScope scope;
On 2014/06/25 01:22:20, yosi wrote:
> It seems tempoary new Range object is created by
|FrameSelection::firstRange()|
> when |m_logicalRange| isn't null. But, cloned range is used for comparision.
So,
> we don't need to actually create new Range object.
> 
> We should change this logic to avoid creating Range object rather than using
> |TemporaryRangeScope|.

It seems that all the comparison logic is all hidden inside the Range object
abstraction at this point. It is not clear to me if there are other comparison
methods that give me what I need here or how to split out the comparison logic
from the Ranges.

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
File Source/core/editing/SpellChecker.cpp (right):

https://codereview.chromium.org/330383004/diff/20001/Source/core/editing/Spel...
Source/core/editing/SpellChecker.cpp:749:
m_frame.document()->markers().removeMarkers(wordRange.get(),
DocumentMarker::Spelling);
On 2014/06/25 01:22:20, yosi wrote:
> It seems if we hare removeMakrers() which takes two Position objects, we don't
> need to pass Range.

It would seem so. However, removeMarkers uses a TextIterator which uses more
temporary range objects to do its job. It is not clear to me what is reasonable
to expose from the TextIterator.

I'll take your offer of helping out with this. I'll file bug reports for this
and we can discuss further there. Thanks.