Closed Bug 1427677 Opened 6 years ago Closed 6 years ago

Remove nsContentUtils::HasDistributedChildren, and simplify insertion point computation.

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1427677: Get rid of nsContentUtils::HasDistributedChildren.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1427677: Remove nsBindingManager::FindNestedInsertionPoint.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
The whole function doesn't have much sense. I killed its only Shadow DOM use in bug 1427511.

So now it only has two callers on nsCSSFrameConstructor, which basically only want to know whether the children of the same node can have different flattened tree parents.

So let's check that directly instead, and simplify a bit other surrounding code while at it.
Attachment #8939475 - Attachment is obsolete: true
Attachment #8939475 - Flags: review?(bzbarsky)
(Second commit is not quite right, <details> and <frameset> and mathml stuff can do sync frame construction from content insertion, will move that one to another bug).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05a59643bf9c09b302ff40fecc1409b79b734ff&selectedJob=153782956

(assertions are from the second commit which is gone from the patch set in this bug, and I'll fix separately)
See Also: → 1427908
Blocks: 1425759
Comment on attachment 8939474 [details]
Bug 1427677: Get rid of nsContentUtils::HasDistributedChildren.

https://reviewboard.mozilla.org/r/209816/#review216358

r+ assuming we're OK with removing the optimization this code was trying to implement...  That optimization is why all the complexity is there!

::: commit-message-98b7e:7
(Diff revision 2)
> +
> +The whole function doesn't have much sense.
> +
> +I killed its only DOM use in bug 1427511.
> +
> +Now it only has two callers on nsCSSFrameConstructor, which basically only want

s/on/in/

::: layout/base/nsCSSFrameConstructor.cpp:7431
(Diff revision 2)
> -  InsertionPoint insertionPoint = GetInsertionPoint(aContainer, nullptr);
> -  if (!insertionPoint.mParentFrame && !insertionPoint.mMultiple) {
> -    return insertionPoint; // Don't build the frames.
> -  }
>  
> -  if (insertionPoint.mMultiple || aStartChild->GetXBLInsertionPoint()) {
> +  // If the nodes of the container may be distributed to different insertion

"If the children"?

::: layout/base/nsCSSFrameConstructor.cpp:7433
(Diff revision 2)
> -    return insertionPoint; // Don't build the frames.
> -  }
>  
> -  if (insertionPoint.mMultiple || aStartChild->GetXBLInsertionPoint()) {
> -    // If we have multiple insertion points or if we have an insertion point
> -    // and the operation is not a true append or if the insertion point already
> +  // If the nodes of the container may be distributed to different insertion
> +  // points, insert separately and bail out, letting ContentInserted handle the
> +  // mess.

So this code was trying to optimize the case of appends to an element with an XBL binding with a single insertion point to avoid constructing one at a time for the appended elements.

This patch is effectively removing that optimization, right?

::: layout/base/nsCSSFrameConstructor.cpp:7440
(Diff revision 2)
>        IssueSingleInsertNofications(aContainer, aStartChild, aEndChild,
>                                     aInsertionKind);
> -      insertionPoint.mParentFrame = nullptr;
> +      return { };
> -    }
> +  }
> +
> +  InsertionPoint ip = GetInsertionPoint(aContainer, aStartChild);

What's the point of calling GetInsertionPoint here?  We know the insertion will be under { GetContentInsertionFrameFor(aContainer), aContainer }, because by this point aStartChild's flattened tree parent better be aContainer.

In particular, it's not at all clear why aStartChild should be privileged here (and it's not; the flattened parent for all the kids is aContainer, right?).
Attachment #8939474 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939476 [details]
Bug 1427677: Remove nsBindingManager::FindNestedInsertionPoint.

https://reviewboard.mozilla.org/r/209820/#review216360
Attachment #8939476 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939474 [details]
Bug 1427677: Get rid of nsContentUtils::HasDistributedChildren.

https://reviewboard.mozilla.org/r/209816/#review216358

> So this code was trying to optimize the case of appends to an element with an XBL binding with a single insertion point to avoid constructing one at a time for the appended elements.
> 
> This patch is effectively removing that optimization, right?

Yes, per IRC conversation, I'll measure and maybe talk to the UI folks.

> What's the point of calling GetInsertionPoint here?  We know the insertion will be under { GetContentInsertionFrameFor(aContainer), aContainer }, because by this point aStartChild's flattened tree parent better be aContainer.
> 
> In particular, it's not at all clear why aStartChild should be privileged here (and it's not; the flattened parent for all the kids is aContainer, right?).

Per IRC conversation, this is not quite right, aContainer could be an XBL children element, or fallback content that isn't assigned because the relevant `<children>` / `<slot>` has assigned nodes.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s feb428352403 -d ece8a96dfaa4: rebasing 441221:feb428352403 "Bug 1427677: Get rid of nsContentUtils::HasDistributedChildren. r=bz"
merging layout/base/nsCSSFrameConstructor.cpp
merging layout/base/nsCSSFrameConstructor.h
warning: conflicts while merging layout/base/nsCSSFrameConstructor.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2cae0b7e4b1c
Get rid of nsContentUtils::HasDistributedChildren. r=bz
https://hg.mozilla.org/integration/autoland/rev/e1fedb493a2f
Remove nsBindingManager::FindNestedInsertionPoint. r=bz
(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> Given this try run I'm just going to go ahead and land it :)
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3c63a7f0d59f5fca8d23043d798490e008d6c221

Huh, before this comment a talos run saying "Talos doesn't show anything particularly interesting" should've been posted, for some reason it wasn't...

  https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=512ef35ec2fee264f2b096994463aff26a4d91b4&newProject=try&newRevision=2a83b7ddd8a7b39773489b23ff777e908b95e353&framework=1
I would really appreciate it if you checked with the front-end folks and/or tested a worst-case scenario.  Our talos coverage is horrible and I do not trust it to usefully catch performance regressions.
Flags: needinfo?(emilio)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> I would really appreciate it if you checked with the front-end folks and/or
> tested a worst-case scenario.  Our talos coverage is horrible and I do not
> trust it to usefully catch performance regressions.

My point with the link from comment 13 is that I don't think that the optimization is making any effect at all. I can check more carefully, and if it really is effective in more cases I'll do that.
Flags: needinfo?(emilio)
ni? myself to do that.
Flags: needinfo?(emilio)
Backout by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/960017548cea
Backout two changesets until I prove they have no negative performance implication. r=backout
Boris, comment 13 contains a try run asserting in the path we pick the optimization, not sure if that's enough for you.

I also get a working browser with:

+  if (nsXBLBinding* binding = aContainer->GetXBLBinding()) {
+    if (binding->GetDefaultInsertionPoint() ||
+        binding->HasFilteredInsertionPoints()) {
+      MOZ_ASSERT(!insertionPoint.mParentFrame);
+    }
+  }

In the current code.

So the only case we're potentially de-optimizing IIUC is when the XBL binding has no insertion point at all, and thus renders the actual children (please correct me if I'm wrong).

I'm not sure it's worth to check for that case explicitly from my patches (there's <label>, which uses that), but I can definitely do it. It'd be a matter of changing the GetXBLBinding check with GetBindingWithContent.

Would that be fine with you?
Flags: needinfo?(emilio) → needinfo?(bzbarsky)
Such a try run is still ridiculously green...

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

So it's just a matter of whether we care enough about the case the binding has zero insertion points.
Furthermore, implementing that optimization in the "obvious" way, like:

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

Causes orange because we really rely on _not_ going through the contentAppended path if we have an XBL insertion point...
OK, so it looks like ever since bug 653881 got fixed this optimization has been ineffective.  That's because the insertion point is the parent of the <children>, as Emilio pointed out on IRC, and hence it always has kids (the <children> itself) after bug 653881.  We even added an "XXX this optimization no longer works" comment in that bug, which then got removed in bug 1415149 along with some other surrounding code.

Given that this optimization is not working anyway, let's go ahead and reland these patches.
Flags: needinfo?(bzbarsky)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0a4fe34f8d6c
Get rid of nsContentUtils::HasDistributedChildren. r=bz
https://hg.mozilla.org/integration/autoland/rev/d5dc9bcdd84b
Remove nsBindingManager::FindNestedInsertionPoint. r=bz
https://hg.mozilla.org/mozilla-central/rev/0a4fe34f8d6c
https://hg.mozilla.org/mozilla-central/rev/d5dc9bcdd84b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: