1367740 - Selection::Collapse() should recycle old range if it's not referred by anybody

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Selection::Collapse() releases selection ranges and then, creates a new range.

So, old range needs to do:
* Remove itself from the mutation observer of the root.
* Unregister itself from hashtable which is stored by common ancestor as a property.

new range needs to do:
* Add itself to the mutation observer of the root.
* Register itself to hashtable which is stored by or will be set common ancestor as a property.

However, if old range isn't referred by anybody, it can reuse its range instead of creating new range because:

* The root of range won't be changed since Selection.collapse() doesn't cause moving range to different document.
* In most cases, Collapse() doesn't change its start node nor end node. Therefore, common ancestor won't be changed.

The coming patch depends on the patch for bug 1367683.

It improves the score of attachment 8848015 [details] (bug 1346723) 3%~ from patched build of bug 1367683.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1367740 Selection::Collapse() should reuse old nsRange instance if it's not referred by anybody

Selection::Collapse() removes all ranges first, then, adds a collapsed range which is a new instance of nsRange.

However, new nsRange's initialize cost isn't cheap. It needs to add itself to mutation observer and computes common ancestor.  However, Selection::Collapse() doesn't move the range to different document.  If old range is reusable, we can avoid to remove old range from mutation observer and add new range to mutation observer, and also we can avoid to recompute common ancestor if the node is not changed, e.g., only offset is changed in selected node.

Review commit: https://reviewboard.mozilla.org/r/142748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142748/

Comment 2

[Mats Palmgren (inactive)]

Collapse() starts by calling Clear(), which removes all ranges from the Selection.
http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/layout/generic/nsSelection.cpp#5224
That means we'll *always* do the things you listed above.  I don't see how your
patch changes that.  The only thing you avoid is the Range allocation + ctor:
http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/dom/base/nsRange.cpp#248

I'm confused because the commit message / bug description doesn't seem to reflect
what the patch is doing.  (It's still worth doing though if it improves some test
by 3%, but you need to update the commit message; unless I'm missing something.)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mats Palmgren (:mats) from comment #2)
> Collapse() starts by calling Clear(), which removes all ranges from the
> Selection.
> http://searchfox.org/mozilla-central/rev/
> 35b37316149108d53a02fb8498e250ea8a22ab5d/layout/generic/nsSelection.cpp#5224
> That means we'll *always* do the things you listed above.  I don't see how
> your
> patch changes that.  The only thing you avoid is the Range allocation + ctor:
> http://searchfox.org/mozilla-central/rev/
> 35b37316149108d53a02fb8498e250ea8a22ab5d/dom/base/nsRange.cpp#248

No, Selection::Clear() releases all ranges. So, if there is a referrer of the range(s), the range won't be destructed. That means that DoSetRange() isn't called by the destructor of nsRange:
https://searchfox.org/mozilla-central/rev/c195cc1698da2d3116fd8594db44614bb1304719/dom/base/nsRange.cpp#240,245
And that means that the range instance won't uninstall itself from the mutation observer nor the hashtable which is stored by the common ancestor:
https://searchfox.org/mozilla-central/rev/c195cc1698da2d3116fd8594db44614bb1304719/dom/base/nsRange.cpp#905,935,953

My patch can skip this cost (and initializing new range with same result).

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

So, my patch put off the destruction of the first range after Clear(). That allows we to skip the redundant processing.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8871261 [details]
Bug 1367740 Selection::Collapse() should reuse old nsRange instance if it's not referred by anybody

https://reviewboard.mozilla.org/r/142748/#review147530

OK, I guess the commit message is reasonably clear.

I was mislead by the bug description which lists these items:
> * Unregister itself from hashtable which is stored by common ancestor as a property.
> * Register itself to hashtable which is stored by or will be set common ancestor as a property.

FYI, this patch does NOT avoid those things.  The Unregister part happens
in Selection::Clear when it calls SetSelection(nullptr).  The Register
part happens when AddItemInternal calls SetSelection(this).  (I don't see
an easy way to avoid this either.)

Avoiding the mutation observer stuff and avoiding the GetCommonAncestor()
calls in nsRange::DoSetRange is a nice win though.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mats Palmgren (:mats) from comment #5)
> Comment on attachment 8871261 [details]
> Bug 1367740 Selection::Collapse() should reuse old nsRange instance if it's
> not referred by anybody
> 
> https://reviewboard.mozilla.org/r/142748/#review147530
> 
> OK, I guess the commit message is reasonably clear.
> 
> I was mislead by the bug description which lists these items:
> > * Unregister itself from hashtable which is stored by common ancestor as a property.
> > * Register itself to hashtable which is stored by or will be set common ancestor as a property.
> 
> FYI, this patch does NOT avoid those things.  The Unregister part happens
> in Selection::Clear when it calls SetSelection(nullptr).  The Register
> part happens when AddItemInternal calls SetSelection(this).  (I don't see
> an easy way to avoid this either.)

Ah, indeed. I'll keep it in my mind.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8871261 [details]
Bug 1367740 Selection::Collapse() should reuse old nsRange instance if it's not referred by anybody

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142748/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db2635a42c74
Selection::Collapse() should reuse old nsRange instance if it's not referred by anybody r=mats

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=289bbf40fe524e8933b321f33812c4a11852eed3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db2635a42c74