1405281 - Keystrokes trigger full-page (and slow) paint with contenteditable cells with borders or border-collapse

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[billy78]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

Hi!
I have a big table with 250 rows. Each row contains some cells with buttons, a checkbox and 2 editable cells. You can see the table here: http://users.sch.gr/vandr/table.htm


Actual results:

When I keep pressing a key inside an editable cell or keep pressing backspace to delete the contents, Firefox updates the cell in a slow speed (seems like slow typing). If I remove "border-collapse: collapse;" from "#table1" or remove "border: 1px solid black;" from "#table1 td", the speed is back to normal. Chrome does not experience this slow speed. I have tested many Firefox portable versions. The last one that works correctly is 48.0.2. Delayed typing is happening in all versions after 48.0.2.


Expected results:

Typing should not be delayed.

Comment 1

[Makoto Kato [:m_kato]]

I add regressionwindow-wanted since reporter says "The last one that works correctly is 48.0.2".

Comment 2

[Makoto Kato [:m_kato]]

https://perfht.ml/2kkU5cj

When repeating key down, this operation becomes slow by bug 1330252.  But I guess that root cause is reflow/repaint performance according to profiler.  nsNativeThemeWin::DrawWidgetBackground seems to be slow.

Comment 3

[Daniel Holbert [:dholbert]]

Paint flashing shows that we're doing full-page repaints on every keypress.

Comment 4

[Daniel Holbert [:dholbert]]

We're invalidating the full table via nsTableRowFrame::ReflowChildren calling nsTableFrame::InvalidateTableFrame.

That happens here, inside of a tableFrame->IsBorderCollapse() check:
 https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowFrame.cpp#396-399

It looks like that's only supposed to happen if the cell's BSize (height) changes, or something like that:
 https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/layout/tables/nsTableRowFrame.cpp#354-359

And visually, that doesn't seem to be happening here... So it's possible the logic is wrong somehow (or maybe the BSize "changes" but we can't ever actually honor it, so it always looks like a change, or something like that

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (reporter's testcase)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Makoto Kato [:m_kato] from comment #1)
> I add regressionwindow-wanted since reporter says "The last one that works
> correctly is 48.0.2"

I'm seeing full-page repaints* and jank (from my perspective) even in much older builds, e.g. Firefox 44 Nightlies.  So I don't think this is really a regression in 49, except possibly from e10s. (I do see the performance difference when comparing 48 vs 49 release, and I can confirm that about:support shows "Multiprocess Windows: 0/1" in 48 vs "1/1" in 49.)

48 still does full-page repaints, but these massive repaints are a bit faster when there's only one process, I think, so that's why this *seems* like a regression there. But really we've always been inefficient with this scenario, and it's just more noticeable when e10s is enabled.

* (you can see how much is being repainted by flipping the about:config pref nglayout.debug.paint_flashing to true)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> I'm seeing full-page repaints* and jank (from my perspective) even in much
> older builds, e.g. Firefox 44 Nightlies.

(Note: in case it wasn't clear, Firefox 44 *Nightly* [and older Nightlies as well] had e10s enabled by default, unlike release builds.  That's why this is janky in Nightly builds that far back, despite being fast in release builds for much longer.)

Comment 8

[Bobby Holley (:bholley)]

I've confirmed dholbert's analysis of full-page repaints on each keystroke. It seems from comment 4 that there may be a layout bug to fix here so that we invalidate less. Jonathan, you recently set a dependency on bug 540256. Does that mean you understand what's going on and what we'd need to do, or is it speculative? (Jonathan isn't accepting NIs, but we can ask when he's back).

Another weird thing about the testcase is that painting calls into DrawWidgetBackground, which seems to be incredibly slow on mac. Increasing the window size (and thus the buttons to be painting) increases the jank, so I made exactly five rows visible in my profiling for consistent measurements. On mac, each keystroke janks for 150ms in FrameLayerBuilder [1], whereas on Linux it only janks for 30 ms [2]. What's more, the initial paint of the page on mac spends only 66ms painting the native widgets [3], which suggests our dynamic path is doing something suboptimal. Matt, any insight into this?

Also, I thought we painted buttons ourselves with clever CSS rather than using native widgets. I guess that changed?


[1] https://perfht.ml/2MqYcwy
[2] https://perfht.ml/2KtbKu0
[3] https://perfht.ml/2KrBOTe

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Bobby Holley (:bholley) from comment #8)
> Jonathan, you recently set a dependency on bug 540256.
> Does that mean you understand what's going on and what we'd need to do, or
> is it speculative?

I marked that during the All Hands Layout Planning meeting because someone (I don't recall who, sorry) said we should maybe fix bug 540256 first to make the code sane, and if that didn't fix this bug then maybe it would help make it easier to fix. So yeah, purely speculative.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Jonathan Watt [:jwatt] (mostly away until Tue 10th) from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #8)
> > Jonathan, you recently set a dependency on bug 540256.
> > Does that mean you understand what's going on and what we'd need to do, or
> > is it speculative?
> 
> I marked that during the All Hands Layout Planning meeting because someone
> (I don't recall who, sorry) said we should maybe fix bug 540256 first to
> make the code sane, and if that didn't fix this bug then maybe it would help
> make it easier to fix. So yeah, purely speculative.

Alright thanks. I'll move that to a see-also given that it's not an obvious hard dependency and we may be able to do a targeted fix here. Adding this to the perf docket.

Comment 11

[Bobby Holley (:bholley)]

Tracking this for now as the over-invalidation issue for layout. If there's something we can do to the FrameLayerBuilder slowness described in comment 8 we can fork off a separate bug for it, which may in turn decrease the priority of this bug.

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Bobby Holley (:bholley) from comment #8)
> I've confirmed dholbert's analysis of full-page repaints on each keystroke.
> It seems from comment 4 that there may be a layout bug to fix here so that
> we invalidate less. Jonathan, you recently set a dependency on bug 540256.
> Does that mean you understand what's going on and what we'd need to do, or
> is it speculative? (Jonathan isn't accepting NIs, but we can ask when he's
> back).
> 
> Another weird thing about the testcase is that painting calls into
> DrawWidgetBackground, which seems to be incredibly slow on mac. Increasing
> the window size (and thus the buttons to be painting) increases the jank, so
> I made exactly five rows visible in my profiling for consistent
> measurements. On mac, each keystroke janks for 150ms in FrameLayerBuilder
> [1], whereas on Linux it only janks for 30 ms [2]. What's more, the initial
> paint of the page on mac spends only 66ms painting the native widgets [3],
> which suggests our dynamic path is doing something suboptimal. Matt, any
> insight into this?

I believe this is based on the area that we're painting, since the painting that we do shouldn't change for load vs invalidations (and the profiles look very similar, just slower).

I think for first paint we're only drawing what's actually visible right now, and on later paints we're painting the whole displayport (async scrollable area).

> 
> Also, I thought we painted buttons ourselves with clever CSS rather than
> using native widgets. I guess that changed?

It's been talked about a lot, I don't think we're actually doing it anywhere though unfortunately.

It's super nice to have our own rendering, but using the platform code keeps us in sync with updates and user settings without any real work on our end.

Comment 13

[Matt Woodrow (:mattwoodrow)]

The invalidations look to be happening because we change the size of basically everything twice during reflow. We change it, and then change it back to the old value, but we issue invalidations both times.

Callstacks: https://pastebin.mozilla.org/9088639


This is a problem that is supposed to be fixed by DLBI, where we ignore invalidations due to reflow, and then do a comparison on the actual things that we paint at paint time.

Unfortunately border-collapse tables don't have display items per-primitive, so we can't compare their sizes. They just have a single display item for all the borders, and it's much harder to detect changes to individual cell/row/column sizes at that granularity.

There's a couple of rewrite-the-world approaches to fixing this, one is to actually create per-cell display items even with border-collapse and figure out how to share drawing of common borders. That'd give us DLBI 'for free', and we could basically delete all InvalidateTableFrame calls.

The other is to have the nsDisplayBorderCollapse item actually keep a copy of all the cell/row/column geometry, and implement a code to diff that between paints. That'd be implementing a very complex version of nsDisplayItem::ComputeInvalidationRegion.


It's also possible that we can just be smarter during table reflow and not touch these things twice, but I know very little about that code and haven't looked in detail as of yet. There's a large class of 'table invalidation code is known bad' bugs, and we should probably decide if we want to keep spot-fixing or try one of the larger rewrites.

Comment 14

[Bobby Holley (:bholley)]

Thanks Matt, this is really helpful analysis, and gives us a couple of possible avenues to pursue.

My rough intuition is that we should have a look here and see if there's an easy way to avoid touching things twice during table reflow. If so that seems like a decent spot fix, and we might as well land it. If not, then we should probably file this as testcase for fixing table invalidation, which seems worth doing at some point but may not be the most urgent thing on the list.

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> I think for first paint we're only drawing what's actually visible right
> now, and on later paints we're painting the whole displayport (async
> scrollable area).

Is there a reason for this? If we're able to scope the first paint to the viewport, it seems like we ought to be able to do the same for subsequent paints (at least when we detect that the invalidation rect matches the entire page). Is there an easy optimization we could do, and if so should we get a bug on file?

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Bobby Holley (:bholley) from comment #14)
> My rough intuition is that we should have a look here and see if there's an
> easy way to avoid touching things twice during table reflow. If so that
> seems like a decent spot fix, and we might as well land it. If not, then we
> should probably file this as testcase for fixing table invalidation, which
> seems worth doing at some point but may not be the most urgent thing on the
> list.

I agree with this, we should spot fix as long as it's easy to do. Once we get a decent list of hard-to-fix problems, then we can prioritize the work.

> 
> (In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> > I think for first paint we're only drawing what's actually visible right
> > now, and on later paints we're painting the whole displayport (async
> > scrollable area).
> 
> Is there a reason for this? If we're able to scope the first paint to the
> viewport, it seems like we ought to be able to do the same for subsequent
> paints (at least when we detect that the invalidation rect matches the
> entire page). Is there an easy optimization we could do, and if so should we
> get a bug on file?

The hard bit here is that the decision about how much area to pre-render is made during display-list builidng, but invalidation is done post-invalidation (quite a bit later).

During page load, it was decided that it was worth trying to get the currently visible area displayed ASAP, even if it means that immediate async scrolling might checkerboard (and the total amount of work done is more).

There's definitely things we can do here, like guessing slow/large paints in advance (based on RDL inputs?), or possibly by just painting the changes to the visible area, and having the async-scrollable area out of date (until we catch up and get it painted).

We have/had an implementation of the latter, where fennec painted tiles for the visible area first, sent them to the compositor, and then continued painting the remainder.

Lots of tradeoffs for these though (including UX ones), and none are particularly simple unfortunately. I'm not in a huge rush to implement any of them right now :)

Comment 16

[Bobby Holley (:bholley)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> The hard bit here is that the decision about how much area to pre-render is
> made during display-list builidng, but invalidation is done
> post-invalidation (quite a bit later).

I assume you mean post-DL-building? I suppose this makes sense, given DLBI.

> Lots of tradeoffs for these though (including UX ones), and none are
> particularly simple unfortunately. I'm not in a huge rush to implement any
> of them right now :)

Fair enough - thanks for the explanation!