nsFrameConstructorState::ProcessFrameInsertions is slow

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on 1 bug)

Trunk
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

When opening the treeview in Gecko profiler and resizing the window, 
nsLayoutUtils::DoCompareTreePosition(nsIFrame*,nsIFrame*...) takes over 85% of the time.
Under it GetPlaceholderFrameFor is about 50%.

I'm testing a simple placeholder cache if it helps with this case.
If not, perhaps we should keep placeholder member variable in frames?
The cache doesn't seem to help.
ok, so the problem is nsFrameConstructorState::ProcessFrameInsertions.
We end up calling CompareTreePosition a lot, and that calls FillAncestors.
Simple fix is to cache the result of FillAncestors for firstNewFrame,
but for other things...
One possibility would be to not do the linear search but binary or tree search or so when
going trough childList.
Summary: nsLayoutUtils::DoCompareTreePosition is slow → nsFrameConstructorState::ProcessFrameInsertions is slow
patch coming
Assignee: nobody → bugs
And the testcase is to create a profile using gecko profiler and alt+click the root arrow
(that expands the whole tree-view).
Depends on: 928645
Bug 928645 is annoying, but I think we should fix this even before that.
Or in other words, I don't have cycles to look at bug 928645.
Posted patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=7795afa9001c

The hack for the old behavior isn't pretty, but easy to remove once
the other bug is fixed.
Attachment #819328 - Attachment is obsolete: true
Attachment #819356 - Flags: review?(dbaron)
Comment on attachment 819356 [details] [diff] [review]
patch

Actually, since the code is originally from bug 217604, perhaps you
roc could review.
(I'm sure you remember what you did 10 years ago ;) )
Attachment #819356 - Flags: review?(dbaron) → review?(roc)
Comment on attachment 819356 [details] [diff] [review]
patch

Review of attachment 819356 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1270,5 @@
> +      nsIFrame* insertionPoint = nullptr;
> +      int32_t imin = 0;
> +      int32_t imax = children.Length() - 1;
> +      int32_t imid = 0;
> +      while (imax >= imin) {

I think it's a little clear if imax is exclusive; so set imax to children.Length() and make appropriate changes below.
There was a reason why it was imax, not max ;)
But ok.
Posted patch v2Splinter Review
This way then.
(to me this is harder to understand since we don't handle
boundary points the same way anymore. [min, max] vs [min, max) )
Attachment #819356 - Attachment is obsolete: true
Attachment #819356 - Flags: review?(roc)
Attachment #820294 - Flags: review?(roc)
Comment on attachment 820294 [details] [diff] [review]
v2

Review of attachment 820294 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1271,5 @@
> +      int32_t imin = 0;
> +      int32_t imid = 0;
> +      int32_t max = children.Length();
> +      while (max > imin) {
> +        imid = imin + ((max - imin) / 2);

declare imid here.
Attachment #820294 - Flags: review?(roc) → review+
Posted patch imidSplinter Review
https://hg.mozilla.org/mozilla-central/rev/6bb23207bdbb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.