1464632 - Selecting text and scrolling on github leads to heavy checkerboarding

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180526223142

Steps to reproduce:

1. Go to https://github.com/mozilla/gecko-dev/blob/master/browser/base/content/tabbrowser.xml
2. Seelct some 100 lines of text
3. Scroll the page from the scroll-bar


Actual results:

heavy checkerboarding


Expected results:

no or less checkerboarding.

If you dont select any text and then scroll, there is no checkerboarding.

non-WR:

No selection: https://perfht.ml/2siIJGB
Selection: https://perfht.ml/2GVhGpV

WR:
no selection: https://perfht.ml/2GWSQGs
Selection:  https://perfht.ml/2GTZIEj

Profiles shows displaylist building.

Comment 1

[Matt Woodrow (:mattwoodrow)]

This page is really slow in both display list building *and* rasterization. All the time is being spent in nsRange::IsSelected, within nsContentUtils::ComparePoints.

On my machine, without selection I'm seeing refresh driver ticks take around 10-40ms when scrolling. With selection, it's 200-400ms, a pretty massive regression.

We call nsIFrame::IsSelected for every table cell frame (visible in the displayport) during display list building to decide if we need to create the nsDisplayTableCellSelection. We call nsIFrame::IsSelected for every invalidated nsDisplayText during painting (so only the items that just got scrolled in, though with APZ and paint suppression, this is likely the majority) to decide if we should paint the selection highlight on the text.

I really don't know much about how our selection code works, but it appears to be very inefficient for this use case.

Olli, do you know if we have any plans to store selection state in a way that's quicker to access for a frame/element? And/or who might be the best person to ask more about it?

Setting P3 since this code hasn't changed in a long time and we've got away with being this slow thus far.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

mats might actually have this better in mind.
But I'm not aware of any concrete plans to improve this atm.

I guess we could...cache more about the state of selection, and invalidate the cache whenever anything changes in the document. That might not be too hard to implement.

Comment 3

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #2)
> But I'm not aware of any concrete plans to improve this atm.

Neither am I.

> I guess we could...cache more about the state of selection, and invalidate
> the cache whenever anything changes in the document. That might not be too
> hard to implement.

We could cache selection state in the frames I guess, but note that
we moved the selection state from frames to DOM because it was too
buggy and slow to have in the frame tree.  A cached state might be
slightly easier to maintain though if we reset the state aggressively
when any DOM or Range changes occur.  Still, implementing this with
correctness and performance might be harder than it appears at first.

Comment 4

[Mayank Bansal]

The profiles appear to be changed

non-WR+text selected : https://perfht.ml/2RKYIsS

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: node-index-cache.txt

Attached is a log from some of the operations required to check nsTextFrame::IsFrameSelected on two text frames.

There's a very clear pattern where we call nsINode::ComputeIndexOf repeatedly, always with the same thisptr (0x133488ca0).

The child alternates between 0x1334ee940, and the actual node we're comparing.

The cache in ComputeIndexOf only stores one child index per node, so this alternating means that the cache never gets hit. The cache also assumes that a miss likely means that the target is a nearby sibling, and it iterates back/forward from the previous, but that's not true here, and it's really slow.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1464632 - Add the option to explicitly cache the internal results of nsContentUtils::ComparePoints since nsRange::IsNodeSelected calls it repeatedly with the same value and we don't want to pollute the internal caching of nsINode...

...::ComputeIndexOf. r?mats

MozReview-Commit-ID: HKFmy1QeCs6

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adb083036432
Add the option to explicitly cache the internal results of nsContentUtils::ComparePoints since nsRange::IsNodeSelected calls it repeatedly with the same value and we don't want to pollute the internal caching of nsINode... r=mats

Comment 8

[Andrei Ciure[:aciure]]

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

Comment 9

[Mayank Bansal]

This is much better now.
http://bit.ly/2RoVgTS