604969 - talos a11y regression caused by bug 599113

Status: NEW

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: testcase

There was a talos regression in a11y caused by http://hg.mozilla.org/mozilla-central/rev/8188183a6402 of bug 599113. It is not a regression in a11y speed, but rather a general slowdown as it also slows down when removing the a11y aspect.

The testcase is just the page in the a11y page load test that regressed with the a11y aspect removed.

So it looks like doing the extra invalidates are taking more time. Not sure what we can do to mitigate this.

Comment 1

[Timothy Nikkel (:tnikkel)]

Looks like we are spending a small but significant amount of time updating the invalidate thebes region.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You could try reducing the simplification threshold from 20 to say 4?

Comment 3

[Timothy Nikkel (:tnikkel)]

Didn't really help. Most of the region time was in nsRegion::SubRect.

It seems like we'd almost be better off just keeping a list of rects (with some upper bound to the number of them), and then at the next paint doing something a little clever. Say if there are more than 20 rects take the bounding box, this would just be a max over the coords of the rects, without the added overhead of keeping the region invariants incrementally. Or maybe take the bounding box of the first and last rects (on the theory that the first and last rect would occupy the geometric extremes of the area), and then add in the rest somehow.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Where are the calls to nsRegion::SubRect coming from?

Comment 5

[Timothy Nikkel (:tnikkel)]

Just nsRegion::Or, nothing unusual is going on, it is just the non-shortcut case here http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRegion.cpp#851

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't want to overdo this since it will go away when we move to display-list-based invalidation.

Maybe we could have a method on nsRegion that's like Or, but if the rectangle overlaps an existing rectangle, we just replace them both with the bounding-box of the pair?

Comment 7

[Timothy Nikkel (:tnikkel)]

I tried something simpler: if we hit the slow SubRect case just make the new region the bounding rect of the old region's bounding rect and the new rect. That restored about half of the perf we lost.

Comment 8

[Sudarsana Nagineni]

> There was a talos regression in a11y caused by
> http://hg.mozilla.org/mozilla-central/rev/8188183a6402 of bug 599113. 

The performance degradation was observed in flash video playback with this changeset. I checked on latest Fennec build with youtube flash video and the video playback is not smooth. The playback is smooth, when the same video played without this change.

Comment 9

[Timothy Nikkel (:tnikkel)]

Plugins for desktop Fennec don't seem to be working for me at all so I can't investigate that.

I would want to look at nsHTMLScrollFrame/nsXULScrollFrame::InvalidateInternal and the rect to invalidate and the scrollport to see why we are invalidating outside the scrollport.

Comment 10

[Timothy Nikkel (:tnikkel)]

I got it working with vimeo and the pref plugin.disable flipped. (At least I think vimeo was serving me flash, I don't know how to tell flash from say a video tag in fennec.)

I set breakpoints on the new code that changeset 8188183a6402 added and they were never hit.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: quicker less accurate region stuff

You could try this patch to see if it speeds it up for you.

Comment 12

[Sudarsana Nagineni]

(In reply to comment #11)
> Created attachment 486197 [details] [diff] [review]
> quicker less accurate region stuff
> 
> You could try this patch to see if it speeds it up for you.

I’ve not seen any performance improvement in flash video case with this patch.

Comment 13

[Timothy Nikkel (:tnikkel)]

Can you profile? Where is the extra time being taken?

Can you debug? Are we hitting the seperateThebes case in nsHTMLScrollFrame::InvalidateInternal or nsXULScrollFrame::InvalidateInternal?

Comment 14

[Sudarsana Nagineni]

(In reply to comment #13)
> Can you profile? Where is the extra time being taken?
> 
> Can you debug? Are we hitting the seperateThebes case in
> nsHTMLScrollFrame::InvalidateInternal or nsXULScrollFrame::InvalidateInternal?

I've not done profiling on this. But, I quickly checked by adding logs and found that we are spending extra time by calling InvalidateThebesLayerContents as 
seperateThebes is always true in sHTMLScrollFrame::InvalidateInternal while playing the flash video.

Comment 15

[Timothy Nikkel (:tnikkel)]

Thanks for investigating.

Can you reproduce the problem on a flash video page other than youtube, on a page that works with Fennec-on-the-desktop? Youtube doesn't seem to be working on Fennec-on-the-desktop right now, so I had to test vimeo. (You need to flip the pref plugin.disable to plugins to work at all on Fennec-on-the-desktop.)

Comment 16

[Timothy Nikkel (:tnikkel)]

Spun off the Fennec youtube issue into bug 608930.