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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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.
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.)
Flags: needinfo?(masayuki)
(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).
Flags: needinfo?(masayuki)
So, my patch put off the destruction of the first range after Clear(). That allows we to skip the redundant processing.
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.
Attachment #8871261 - Flags: review?(mats) → review+
(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.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/db2635a42c74
Selection::Collapse() should reuse old nsRange instance if it's not referred by anybody r=mats
https://hg.mozilla.org/mozilla-central/rev/db2635a42c74
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.