1302522 - Find bar highlighter performance evaluation

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Markus Stange [:mstange]]

I investigated the performance of the new find toolbar highlighter. Here are my finding along with proposed solutions.


Problem: mix-blend-mode slows things down and does not actually do anything.
This is my fault. I hadn't anticipated that the anonymous content container is stacking context. That means that the blending happens only between the contents of the anonymous content overlay. But in order for this to work as intended, it would need to blend with the page content. But due to the extra stacking context this can not (and currently does not) work.

Proposed solutions:
Remove the mix-blend-mode property. Doing so will not change rendering.
If the current appearance is good enough, then that's all that needs to be done.
If the current appearance is not good enough, and we want actual holes, then:
 - Either we need to add a platform capability to cut out actual holes in the current stacking context, e.g. using -moz-appearance: highlight-hole;?
 - Or the JS needs to cut the black dimming element into small pieces that assemble the region without holes.
 - Or we need to add a new API that lets you set a cutout region so that we can efficiently forward it to the layers / compositor.


Problem: The DOMMouseScroll handler delays async scrolling.
DOMMouseScroll handlers can call event.preventDefault() to disable scrolling, so APZ needs to wait for the first wheel event of a scroll gesture to be processed by content before it can start scrolling in the compositor.

Proposed solution: Listen for the "scroll" event instead of listening for DOMMouseScroll and mousewheel. (We don't even support the mousewheel event; the modern equivalent for DOMMouseScroll is "wheel". But please use "scroll".)


Problem: The grow/shrink animation needs to paint when it starts.

Proposed solution: Set will-change:transform on the outlineNode.


Problem: The grow/shrink animation is not handled by the compositor.
This is because there is an animation of the top/left properties running at the same time, which can not be OMTA'd, so Gecko demotes the whole animation to be a main thread animation.

Proposed solution: Get rid of the sliding animation.
There's no good way to get it to be OMTAable, the intermediate states are somewhat undefined (e.g. what happens if you slide to an occurence with a different font / font size? There will be a discrete step there anyway), and it means that the emphasis of the grow/shrink animation is at some point between the last and the next occurrence of the search term.
If you get rid of the slide animation (i.e. remove all the "transition" properties from outlineNode), then you will have a smooth off-main-thread grow/shrink animation which is centered on the search term that you want the user to focus on.


Problem: The whole page is invalidated on every scroll, which causes extreme checkerboarding during fast scrolling.
This happens because the whole maskNode is being replaced on every scroll, including the element that has the black overlay. Replacing the DOM element means that we invalidate the whole thing.

Proposed solutions:
 - Don't replace the maskNode if nothing changes.
 - Even if things change, try to keep the invalidations localized to that part of the page. Ideally this should be done by recycling elements, but it seems recycling is not really possible with the AnonymousContent API. What you could do instead is to partition the page into tiles (e.g. 512px x 512px), have a separate AnonymousContent node per tile, and then only replace tiles that have changed.
 - If we add an efficient cutout region API to AnonymousContent, then you'd only need to call that API when the mask locations change. This would be the most efficient solution of them all but also more work.

Comment 1

[Markus Stange [:mstange]]

You can verify invalidations using

Comment 2

[Markus Stange [:mstange]]

the pref nglayout.debug.paint_flashing - after the fix, scrolling should not cause the page to flash any more.

Comment 3

[Mike de Boer [:mikedeboer]]

Thanks so much Markus! This is not a complete answer yet, but I wanted to provide some feedback right away.

(In reply to Markus Stange [:mstange] from comment #0)
> I investigated the performance of the new find toolbar highlighter. Here are
> my finding along with proposed solutions.

\o/

> Proposed solutions:
> Remove the mix-blend-mode property. Doing so will not change rendering.
> If the current appearance is good enough, then that's all that needs to be
> done.
> If the current appearance is not good enough, and we want actual holes, then:
>  - Either we need to add a platform capability to cut out actual holes in
> the current stacking context, e.g. using -moz-appearance: highlight-hole;?
>  - Or the JS needs to cut the black dimming element into small pieces that
> assemble the region without holes.
>  - Or we need to add a new API that lets you set a cutout region so that we
> can efficiently forward it to the layers / compositor.

OK, because removing mix-blend-mode and somehow ending up with non-blank white rectangles is not an option. So you alternatives would need to be investigated and perhaps implemented.
Another option I have consider quite often is add platform support to style ranges as pseudo-block elements with border-radius, background-color and box-shadow support.

> Proposed solution: Listen for the "scroll" event instead of listening for
> DOMMouseScroll and mousewheel. (We don't even support the mousewheel event;
> the modern equivalent for DOMMouseScroll is "wheel". But please use
> "scroll".)

No prob, consider it done.

> Proposed solution: Get rid of the sliding animation.
> There's no good way to get it to be OMTAable, the intermediate states are
> somewhat undefined (e.g. what happens if you slide to an occurence with a
> different font / font size? There will be a discrete step there anyway), and
> it means that the emphasis of the grow/shrink animation is at some point
> between the last and the next occurrence of the search term.
> If you get rid of the slide animation (i.e. remove all the "transition"
> properties from outlineNode), then you will have a smooth off-main-thread
> grow/shrink animation which is centered on the search term that you want the
> user to focus on.

Consider it done.

> Proposed solutions:
>  - Don't replace the maskNode if nothing changes.

It's all about the answer to this question: What's the definition of 'nothing changed in the page'?

>  - Even if things change, try to keep the invalidations localized to that
> part of the page. Ideally this should be done by recycling elements, but it
> seems recycling is not really possible with the AnonymousContent API. What
> you could do instead is to partition the page into tiles (e.g. 512px x
> 512px), have a separate AnonymousContent node per tile, and then only
> replace tiles that have changed.

Quite a bit of work, but not impossible. I'd like to know if we'll keep the HTML/ DOM approach before I start this work, though.

>  - If we add an efficient cutout region API to AnonymousContent, then you'd
> only need to call that API when the mask locations change. This would be the
> most efficient solution of them all but also more work.

Questions here are along the lines of 'Who can do this?' and 'Is this important enough to spend time on right now by this person?'

All in all I still think it's kinda neat we can do all this with 'just' HTML/ CSS, but if it remains 'impossible' to squeeze enough perf out of it... I don't know.

Comment 4

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> > Proposed solutions:
> > Remove the mix-blend-mode property. Doing so will not change rendering.
> > If the current appearance is good enough, then that's all that needs to be
> > done.
> > If the current appearance is not good enough, and we want actual holes, then:
> >  - Either we need to add a platform capability to cut out actual holes in
> > the current stacking context, e.g. using -moz-appearance: highlight-hole;?
> >  - Or the JS needs to cut the black dimming element into small pieces that
> > assemble the region without holes.
> >  - Or we need to add a new API that lets you set a cutout region so that we
> > can efficiently forward it to the layers / compositor.
> 
> OK, because removing mix-blend-mode and somehow ending up with non-blank
> white rectangles is not an option.

That's the thing - even with mix-blend-mode you have white rectangles. You currently have white rectangles shipping on Nightly. The mix-blend-mode solution I suggested does not work. (Because of that extra stacking context.) So removing mix-blend-mode today will not change anything except make things faster.

> So you alternatives would need to be
> investigated and perhaps implemented.

I'm currently investigating.

> Another option I have consider quite often is add platform support to style
> ranges as pseudo-block elements with border-radius, background-color and
> box-shadow support.

Well, you could have those styles on the maskRect overlay elements, even if they aren't used for actually cutting out the black dimming color, couldn't you?

> > Proposed solution: Get rid of the sliding animation.
> > There's no good way to get it to be OMTAable, the intermediate states are
> > somewhat undefined (e.g. what happens if you slide to an occurence with a
> > different font / font size? There will be a discrete step there anyway), and
> > it means that the emphasis of the grow/shrink animation is at some point
> > between the last and the next occurrence of the search term.
> > If you get rid of the slide animation (i.e. remove all the "transition"
> > properties from outlineNode), then you will have a smooth off-main-thread
> > grow/shrink animation which is centered on the search term that you want the
> > user to focus on.
> 
> Consider it done.

Yay.

> > Proposed solutions:
> >  - Don't replace the maskNode if nothing changes.
> 
> It's all about the answer to this question: What's the definition of
> 'nothing changed in the page'?

You only need to check whether the geometry of the rectangles changed.

> >  - If we add an efficient cutout region API to AnonymousContent, then you'd
> > only need to call that API when the mask locations change. This would be the
> > most efficient solution of them all but also more work.
> 
> Questions here are along the lines of 'Who can do this?' and 'Is this
> important enough to spend time on right now by this person?'

Not sure, I'm currently seeing how quickly I can whip up a prototype.

> All in all I still think it's kinda neat we can do all this with 'just'
> HTML/ CSS, but if it remains 'impossible' to squeeze enough perf out of
> it... I don't know.

Yeah, it would be preferable to be able to make this fast enough with pure HTML and CSS. Being able to recycle elements would make it easier - the AnonymousContent API is working against us here.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #4)
> That's the thing - even with mix-blend-mode you have white rectangles. You
> currently have white rectangles shipping on Nightly. The mix-blend-mode
> solution I suggested does not work. (Because of that extra stacking
> context.) So removing mix-blend-mode today will not change anything except
> make things faster.

Argh! My eyes have deceived me all this time, darnit!

> Well, you could have those styles on the maskRect overlay elements, even if
> they aren't used for actually cutting out the black dimming color, couldn't
> you?

Yes, but next step would be to get the inverse of all those rects to be dimmed.

> You only need to check whether the geometry of the rectangles changed.

Yeah, dunno how expensive that'd be in JS, but will check.

> Yeah, it would be preferable to be able to make this fast enough with pure
> HTML and CSS. Being able to recycle elements would make it easier - the
> AnonymousContent API is working against us here.

Hmm, there might be a way to sidestep it, but will require some thought.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1302522 - remove CSS properties that slow down scrolling with find toolbar dimming enabled and listen to a singular scroll event.

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

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8791528 [details]
Bug 1302522 - remove CSS properties that slow down scrolling with find toolbar dimming enabled and listen to a singular scroll event.

https://reviewboard.mozilla.org/r/78936/#review77630

Are you going to make the will-change:transform in a different bug?

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #7)
> Are you going to make the will-change:transform in a different bug?

But I removed the 'transform' property altogether, so wouldn't that be a no-op?

Comment 9

[Markus Stange [:mstange]]

The animation that you start with setAnimationForElement uses the transform property. You want layers to be prepared for that animation to start, so you tell it ahead of time that transform is a property that you're going to change. Does that make sense?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #9)
> The animation that you start with setAnimationForElement uses the transform
> property. You want layers to be prepared for that animation to start, so you
> tell it ahead of time that transform is a property that you're going to
> change. Does that make sense?

Ah! Yesyes, I'll add it here. Thanks for the clarification.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8791528 [details]
Bug 1302522 - remove CSS properties that slow down scrolling with find toolbar dimming enabled and listen to a singular scroll event.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96e13a175752
remove CSS properties that slow down scrolling with find toolbar dimming enabled and listen to a singular scroll event. r=mstange

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96e13a175752