528306 - [FIX]Hook up restyle processing to refresh driver

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Let's do it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

One possible issue there is removing every time we have no restyles to process.  Maybe we should remove on WillRefresh() instead, and readd after restyle processing if there's anything left there.  Otherwise it seems like we might thrash the removes/adds (including stopping/starting the timer) if something keeps posting and flushing over and over.

That's probably a better setup.  Will do that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, now I remember why I didn't do that.  It's because animations will add to the restyle table during their WillRefresh calls, and those can easily happen after the frame constructor's.  So we'll end up with the frame constructor observing the refresh driver though there are no pending restyles.

Will think about this after dinner.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Ready for review

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix shutdown asserts from the refresh driver

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 413413 [details] [diff] [review]
Fix shutdown asserts from the refresh driver

I'm planning to change refresh driver a bit, so need to rev this, and would like to see bug 531585 land first anyway.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1: change when the refresh driver stops its timer

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2: refcount the frame constructor

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3: Run restyles off the refresh driver

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 419593 [details] [diff] [review]
Part 1: change when the refresh driver stops its timer

r=dbaron, but maybe add a comment in Notify explaining why you want to wait until you get to a notification and still don't have observers?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 419594 [details] [diff] [review]
Part 2: refcount the frame constructor

r=dbaron

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 419595 [details] [diff] [review]
Part 3: Run restyles off the refresh driver

r=dbaron, but maybe add a comment to the WillRefresh implementation explaining that the actual work gets done when the refresh driver flushes style

Comment 13

[Boris Zbarsky [:bzbarsky]]

Made those two comment changes, and pushed:
  http://hg.mozilla.org/mozilla-central/rev/88dd8acfbc05
  http://hg.mozilla.org/mozilla-central/rev/6ef8b28b0d9b
  http://hg.mozilla.org/mozilla-central/rev/2e580c431f4e

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/228adabcc071 trying to fix orange in an invalidation test.  Bug 537416 covers that orange.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Looks like this gave a Tdhml win of a few % (0.09 - 3.23 depending on OS and such).