Closed Bug 1357142 Opened 7 years ago Closed 7 years ago

Avoid sync frame construction in PresShell::RecreateFramesFor

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

Bug 1357142: Kill PresShell::RecreateFramesFor.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1357142: Always append descendants to RestyleTracker.
59 bytes, text/x-review-board-request
bzbarsky
: review+
heycam
: review+
Details 
I wrote a patch for bug 1355343 that flushed styles synchronously for that.

But actually callers seem quite happy with a bit more aggressive solution.
Here's a try run of the patch with some other stylo-only patches on top of it (please ignore the stylo bustage, which is due to those).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5063cf38c8b0a3da17e5ce51fac167d0117b8d4

Here's a try run with only this patch, just started:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1575efa9c6fe19acd688664e2b82d7c1ad42f6eb
I'll comment here before going to sleep. This happens to be bogus when a ForceDescendants hint is present on a restyle root of the parent, read the conversation with bz starting at <http://logs.glob.uno/?c=mozilla%23servo&s=17+Apr+2017&e=17+Apr+2017#c651939>.

It's late here, so I probably won't have time to fix this now, but modulo that this should be fine. There are multiple solutions for that, that range from:

 * Killing the optimization of skipping pushing to mDescendants when the root has ForceDescendants.
 * Forcing the explicitly posted reconstructs to be their own style roots (I need to dig into the invariants of mRestyleRoots to know how feasible is this).
 * Adding reconstructed frames without a frame to the undisplayed content map (potentially tricky, too).
I think I'd be happier with bz reviewing this, as he would know better what the implications are for switching these to do the reframing async, and I'd just be going off the try results...
Flags: needinfo?(bzbarsky)
Attachment #8858893 - Flags: review?(cam) → review?(bzbarsky)
Attachment #8859180 - Flags: review?(cam) → review?(bzbarsky)
Flags: needinfo?(bzbarsky)
Comment on attachment 8859180 [details]
Bug 1357142: Always append descendants to RestyleTracker.

https://reviewboard.mozilla.org/r/131212/#review135880

I guess this is ok, but I'd really like heycam to give this a once-over too.  I don't have a good feel for how often this case is hit and under what circumstances.

In particular, if we have a chain of descendants with RestyleAllDescendants on them all, is the new code is going to make the behavior O(N^2) in length of the chain?  If not, why not?

Could we restrict this to the two cases we know are a problem (things with XBL bindings and anon content)?
Attachment #8859180 - Flags: review?(bzbarsky) → review+
Attachment #8859180 - Flags: review?(cam)
Comment on attachment 8858893 [details]
Bug 1357142: Kill PresShell::RecreateFramesFor.

https://reviewboard.mozilla.org/r/130880/#review135886

r=me, though I have low confidence in my ability to catch issues in the editor code here.  :(

::: dom/base/nsObjectLoadingContent.cpp:2696
(Diff revision 1)
>    } else if (aOldType != mType) {
>      // If our state changed, then we already recreated frames
>      // Otherwise, need to do that here
>      nsCOMPtr<nsIPresShell> shell = doc->GetShell();
>      if (shell) {
> -      shell->RecreateFramesFor(thisContent);
> +      shell->PostRecreateFramesFor(thisContent->AsElement());

This isn't right in the aSync case.  That case really does want the frame created right now.

What you probably want to do is copy what the other branch of the if does and `doc->FlushPendingNotifications(FlushType::Frames)` if aSync.

Maybe the simplest way to do that is to early-return if `newState == aOldState && mType == aOldType`, then do the existing if branches but hoist the aSync bit outside the if so it's shared or something.

::: dom/xbl/nsXBLResourceLoader.cpp:253
(Diff revision 1)
>          if (shell) {
>            nsIFrame* childFrame = content->GetPrimaryFrame();
>            if (!childFrame) {
>              // Check to see if it's in the undisplayed content map.
> +            //
> +            // FIXME(emilio): What about display: contents stuff? Looks like

File a followup, please, reference it here.
Attachment #8858893 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Comment on attachment 8859180 [details]
> Bug 1357142: Always append descendants to RestyleTracker.
> 
> https://reviewboard.mozilla.org/r/131212/#review135880
> 
> In particular, if we have a chain of descendants with RestyleAllDescendants
> on them all, is the new code is going to make the behavior O(N^2) in length
> of the chain?  If not, why not?

It's not, we check if they haven't been restyled already before re-inserting them in the style roots array.

> Could we restrict this to the two cases we know are a problem (things with
> XBL bindings and anon content)?

I guess I could, but per the above I'm not sure it's totally worth, I'll leave that up to heycam.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #8)
> ::: dom/base/nsObjectLoadingContent.cpp:2696
> (Diff revision 1)
> >    } else if (aOldType != mType) {
> >      // If our state changed, then we already recreated frames
> >      // Otherwise, need to do that here
> >      nsCOMPtr<nsIPresShell> shell = doc->GetShell();
> >      if (shell) {
> > -      shell->RecreateFramesFor(thisContent);
> > +      shell->PostRecreateFramesFor(thisContent->AsElement());
> 
> This isn't right in the aSync case.  That case really does want the frame
> created right now.
> 
> What you probably want to do is copy what the other branch of the if does
> and `doc->FlushPendingNotifications(FlushType::Frames)` if aSync.
> 
> Maybe the simplest way to do that is to early-return if `newState ==
> aOldState && mType == aOldType`, then do the existing if branches but hoist
> the aSync bit outside the if so it's shared or something.

I think the aSync parameter doesn't make any sense. It's only true when called for eType_Plugin. When that happens, we actually want the frame when `doSpawnPlugin == true`. But in that case, we'll end up at [1] before trying to do anything with frames, so I think I could remove that.

I guess I could do the flush and file a followup for that, since I'd like to double-check it.

[1]: http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/dom/base/nsObjectLoadingContent.cpp#664

> ::: dom/xbl/nsXBLResourceLoader.cpp:253
> (Diff revision 1)
> >          if (shell) {
> >            nsIFrame* childFrame = content->GetPrimaryFrame();
> >            if (!childFrame) {
> >              // Check to see if it's in the undisplayed content map.
> > +            //
> > +            // FIXME(emilio): What about display: contents stuff? Looks like
> 
> File a followup, please, reference it here.

Will do.
> It's only true when called for eType_Plugin.

Yes.

> When that happens, we actually want the frame when `doSpawnPlugin == true`.

Yes.  But more to the point, if no frame we do NOT want to set doSpawnPlugin to true, or call into InstantiatePluginInstance at all, as thing stand.  We want to cancel the load and return instead, which is what we do in that eType_Plugin case if no frame after the NotifyStateChanged call.

> so I think I could remove that

Then you need to handle the case where after that flush there is still no frame.  This is not going to be a simple change.  If you want to try it, that's ok, but definitely don't scope-creep this bug on it.  Just preserve the current semantics for now.
Comment on attachment 8859180 [details]
Bug 1357142: Always append descendants to RestyleTracker.

https://reviewboard.mozilla.org/r/131212/#review136048

::: layout/base/RestyleTracker.h:349
(Diff revision 1)
> -      // If cur has an eRestyle_ForceDescendants restyle hint, then we
> -      // know that we will get to all descendants.  Don't bother
> -      // recording the descendant to restyle in that case.
> -      if (curData && !(curData->mRestyleHint & eRestyle_ForceDescendants)) {
> +      // Even if cur has a ForceDescendants restyle hint, we're not guaranteed
> +      // to reach aElement in the case the PresShell posts a restyle event from
> +      // PostRecreateFramesFor, so we need to track it here.
> +      MOZ_ASSERT(curData, "expected to find a RestyleData for cur");
> -        curData->mDescendants.AppendElement(aElement);
> +      curData->mDescendants.AppendElement(aElement);

I think this is OK.  It's not that common to have a ForceDescendants hint; it's really only used when we have some global style data change, like a change to devPixelsPerPx, or when fonts change and we have to restyle due to ex/ch units.  So most of the time we don't utilize this optimization.
Attachment #8859180 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/210986a3b355
Kill PresShell::RecreateFramesFor. r=bz
https://hg.mozilla.org/integration/autoland/rev/c37a6aba9245
Always append descendants to RestyleTracker. r=bz,heycam
https://hg.mozilla.org/mozilla-central/rev/210986a3b355
https://hg.mozilla.org/mozilla-central/rev/c37a6aba9245
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1359995
Depends on: 1361041
Blocks: 1449807
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: