1125593 - Improve performance, remove unnecessary getClientRects calcs in SelectionHandler

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

Attachment: trimUpdateCache.diff

After bug 1123606 is complete, it becomes clear that the only time we need to call _updateCacheForSelection() is immediately before determining handle position in _getHandlePositions().

This can be inlined to that method. Other consumers are redundant and can be eliminated.

The attached patch should be pretty close to the eventual version.

Comment 1

[Mark Capella [:capella]]

Comment on attachment 8554240 [details] [diff] [review]
trimUpdateCache.diff

This is working out locally, and try looks good ...
https://tbpl.mozilla.org/?tree=Try&rev=0f8948764742

Comment 2

[:Margaret Leibovic]

Looking through the places we call _updateCachceForSelection and _positionHandles, it seems like there are currently some places where we call _positionHandles but *not* _updateCacheForSelection, such as when we receive a "TextSelection:Move" event.

Have your profiled this change to confirm that it actually leads to a performance improvement? I agree that this simplifies the logic, but for how often we hit these current call sites, it may not be a performance improvement.

Comment 3

[Mark Capella [:capella]]

Yes, you caught me. I thought about my characterization a little more and the only perf gain I can admit to is avoiding updates on viewport changes. The bug is more properly code cleanup.

Comment 4

[Mark Capella [:capella]]

Forgot, the call to _positionHandles() in "TextSelection:Move" is for this.TYPE_CURSOR handle, which doesn't participate in the _cache fields.

Comment 5

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #3)
> Yes, you caught me. I thought about my characterization a little more and
> the only perf gain I can admit to is avoiding updates on viewport changes.
> The bug is more properly code cleanup.

Sorry for the delayed response here. My concern is that although this is a nice improvement in code readability, it does change the number of times we call _updateCachceForSelection, which could lead to a performance regression.

I would like to see you profile this to prove that it doesn't change the performance characteristics before we go ahead and land this.

Comment 6

[Mark Capella [:capella]]

Comment on attachment 8554240 [details] [diff] [review]
trimUpdateCache.diff

Review of attachment 8554240 [details] [diff] [review]:
-----------------------------------------------------------------

I annotated based on how I see the logical impact of this change. 

I see a net decrease in the number of times we call _updateCacheForSelection(). Where do you see the pain-point?

I'll look some more in the meantime :-)

(I've been known to miss obvious things ... like javascript=1.8 and such)

::: mobile/android/chrome/content/SelectionHandler.js
@@ -126,4 @@
>          if (this._activeType != this.TYPE_NONE) {
>            this._positionHandlesOnChange();
>          }
>          break;

This is a wash ... we defer the _updateCacheForSelection(), until we _positionHandlesOnChange() ---> _getHandlePositions() ---> _updateCacheForSelection().

I wanted to go crazy and actually rename _updateCacheForSelection() to something like "_getSelectionScreenCoords()"

@@ -159,1 @@
>  

Other than code complexity, this is the perf win. We don't need it.

@@ -186,2 @@
>            this._positionHandles();
>  

This is a wash ... we defer the _updateCacheForSelection() until we _positionHandles() ---> _getHandlePositions() ---> _updateCacheForSelection().

Recall that _positionHandlesOnChange() -above- was added alongside _positionHandles() in bug 895463. There we wound up doing the same number of getClientRects(), but we saved on message passing.

@@ -322,2 @@
>      this._positionHandles();
>    },

Same, we defer the _updateCacheForSelection() until we actually _positionHandles().

@@ -374,2 @@
>      let positions = this._getHandlePositions(scroll);
>  

This is a wash ... we defer the _updateCacheForSelection() until we _getHandlePositions(), then reuse and pass to _positionHandles() a little further below.

@@ -1109,5 @@
>  
>      this._contentWindow = null;
>      this._targetElement = null;
>      this._isRTL = false;
> -    this._cache = null;

Safe to just let it reflect the last handle position. (we update right before use). Makes it available for tests after selections have been closed.

@@ -1190,1 @@
>      // the checkHidden function tests to see if the given point is hidden inside an

No change. (scroll position versus handle)

@@ +1198,3 @@
>  
> +    // Determine the handle screen coords
> +    this._updateCacheForSelection();

Grab it when we can't avoid it, only call to getClientRects().

@@ +1231,1 @@
>      if (!samePositions(this._prevHandlePositions, positions)) {

No change.

@@ +1236,5 @@
>    // Position handles, allow for re-position, in case user drags handle
>    // to invalid position, then releases, we can put it back where it started
>    // positions is an array of objects with data about handle positions,
>    // which we get from _getHandlePositions.
> +  _positionHandles: function(positions = this._getHandlePositions()) {

No change.

@@ +1271,3 @@
>          this._positionHandles();
>          break;
>        }

Washes here also. defer until we _positionHandles().

Comment 7

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #6)
> Comment on attachment 8554240 [details] [diff] [review]
> trimUpdateCache.diff
> 
> Review of attachment 8554240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I annotated based on how I see the logical impact of this change. 
> 
> I see a net decrease in the number of times we call
> _updateCacheForSelection(). Where do you see the pain-point?

As I mentioned in comment 2, there are some places where we call _positionHandles where we don't currently call _updateCacheForSelection, most notably in the "TextSelection:Move" handler. And that code gets called a lot as the user drags the handle.

It's not about the net number of call sites in the code, it's about how often the code is actually hit. Hence, that's what we should profile it :)

Comment 8

[Mark Capella [:capella]]

Comment #4 explains that we don't do positionHandles() in TextSelection:Move events for TYPE_SELECTION selections. (Only for caret movements and that doesn't factor in here).

Comment 9

[Mark Capella [:capella]]

btw, I might like to split out a positionCaret() method, but that's extremely optional.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8554240 [details] [diff] [review]
trimUpdateCache.diff

Review of attachment 8554240 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking the time to explain things to me.

Comment 11

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/9ef34ca40f79

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ef34ca40f79