Closed Bug 1235321 Opened 8 years ago Closed 8 years ago

Firefox hangs opening extremely large commits & files on hg.m.o

Categories

(Core :: Layout, defect)

x86_64
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jorgk-bmo, Assigned: xidorn)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files)

Firefox hangs opening http://hg.mozilla.org/releases/l10n/mozilla-aurora/fr/rev/bb9e69248f29

I'm using FF 43.0.2 (32bit) on Windows 7 (64bit).

While doing some research for another bug I visited
http://hg.mozilla.org/releases/l10n/mozilla-aurora/fr/log/default/extensions/spellcheck/hunspell/fr.dic
and then clicked on the "diff" next to the first entry by Benoit Leseul.

Needless to say that I lost my work in the other tabs.

Maybe Mercurial was unresponsive at the time, but it most certainly shouldn't hang the browser. Chrome can open the link, and you can see it displaying content while more content is being loaded.
Some more information: On my four core CPU, FF seems to use one core pretty much at 100%, so during the hang, the system load is 25%. Another observation: While FF is hanging, I can't start OpenOffice. That also hangs until the FF process is killed.
I can reproduce in Nightly (with e10s disabled).

From a few targeted interruptions with gdb, it seems we're spending all of our time in layout.  (Not too surprising, given that we're loading a gigantic page).

I'd hope that interruptible layout should save us here, but I guess it doesn't. Reclassifying into Core::Layout for now; needs further investigation.
Component: General → Layout
Product: Firefox → Core
Summary: Firefox hangs opening http://hg.mozilla.org/releases/l10n/mozilla-aurora/fr/rev/bb9e69248f29 → Firefox hangs opening extremely large commits on hg.m.o
Version: 43 Branch → Trunk
I saved the page locally, and cut out about half the contents of it so that the page would finish loading in a reasonable time so that I could use the built in gecko profiler to get a profile. Seems to mostly correspond to stacks in lldb during loading the full page.

https://cleopatra.io/#report=2df942da85efeb2c25054b134e4dfec792288ffe

Looks like time is spent getting the index of lineboxes. I think we've had quadratic issues with this before.
You mean getting the index of frames in lineboxes?  See the dep tree of bug 682052.

That said, this is typically mostly an issue for dynamic changes.  In this case, per the profile, the issue is that CalculateHypotheticalBox needs to find the right line, right?  Seems similar to bug 641341 from that dep tree, though I haven't checked to see whether that is in fact the problem here.
Summary: Firefox hangs opening extremely large commits on hg.m.o → Firefox hangs opening extremely large commits & files on hg.m.o
Depends on: 682052, 641341
See Also: → 1254845
I wrote a naive cache for the specific nsBlockInFlowLineIterator constructor involving here, so that we start iterating from the last time it is called.

I tested a local copy of the page mentioned in comment 0, and with this cache, the full load time is reduced from ~11:45 down to ~1:05 with my local debug build. I also tested a local copy of the page I mentioned in my bug 1235727 comment 0, the load time is also reduced from ~55s to ~18s.

This should optimize the time complexity of this kind of usecase from O(N^2) to O(N), so should be a good optimization to take, I suppose.
Attached patch proposed patchSplinter Review
Not sure whether this naive design works. I guess it is possible that a block frame destroys some of its line boxes, or even a new block frame takes the address of a previous block frame. In those cases, there could be a security hole.

Probably we should store the cache in block frame somehow so that we can invalidate it when necessary... But storing inside frame property without guarding with a state bit could potentially affect performance in normal cases.

I wonder whether we can reuse the existing line cursor. The comment of nsBlockFrame::SetupLineCursor() says:
> Set the line cursor to our first line. Only call this if you
> guarantee that the lines' combinedArea.ys and combinedArea.yMosts
> are non-decreasing.
so I don't dare to try...

Anyway, bz, what do you think?
Attachment #8740291 - Flags: feedback?(bzbarsky)
Comment on attachment 8740291 [details] [diff] [review]
proposed patch

The hard part here is invalidating the cache, right?  Yes, blockframes can totally destroy line boxes, or create new ones with the same address or whatnot.

Have you looked into why the existing line cursor optimization is not helping here?
Flags: needinfo?(bugzilla)
Attachment #8740291 - Flags: feedback?(bzbarsky)
The line cursor optimization is mainly used by BuildDisplayList, which uses this cursor to speed up locating the lines to build list for. This optimization is used in nsBlockInFlowListIterator for optimizing caret movement in bug 593211, though I'm not sure whether it would actually work as expected.

To take the advantage of using line cursor optimization, we could probably setup line cursor at the start of nsAbsoluteContainingBlock::Reflow, and clear it at the end of it, and we would also need to update the cursor in either the iterator constructor, or CalculateHypotheticalPosition after calling the constructor. This wouldn't affect the original use of the cursor given that we always clear line cursor when reflow a block.

I'm a bit concerned about the performance impact of setting frame property in a hot loop. Probably not actually a big deal, though.
Flags: needinfo?(bugzilla)
Comment on attachment 8740768 [details]
MozReview Request: Bug 1235321 - Enable using line cursor to optimize reflowing absolute frames. r=bz

https://reviewboard.mozilla.org/r/46017/#review43069

r=me

::: layout/generic/nsBlockFrame.cpp:1397
(Diff revision 1)
>          flags |= AbsPosReflowFlags::eCBWidthChanged;
>        }
>        if (cbHeightChanged) {
>          flags |= AbsPosReflowFlags::eCBHeightChanged;
>        }
> +      SetupLineCursor();

Please add a comment explaining why this call and the ClearLineCursor() call are there.

::: layout/generic/nsBlockFrame.cpp:5525
(Diff revision 1)
>          ++rline;
>        }
>      }
> -    // Didn't find the line
> +    if (mLine != line_end) {
> +      *aFoundValidLine = true;
> +      aFrame->Properties().Set(nsBlockFrame::LineCursorProperty(), mLine.get());

You can optimize out the property set if mLine == cursor, right?  Especially if you make "cursor" an `nsLineBox* const`.
Attachment #8740768 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740768 [details]
MozReview Request: Bug 1235321 - Enable using line cursor to optimize reflowing absolute frames. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46017/diff/1-2/
Attachment #8740768 - Attachment description: MozReview Request: Bug 1235321 - Enable using line cursor to optimize reflowing absolute frames. r?bz → MozReview Request: Bug 1235321 - Enable using line cursor to optimize reflowing absolute frames. r=bz
https://hg.mozilla.org/mozilla-central/rev/23aad06080af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee: nobody → bugzilla
You need to log in before you can comment on or make changes to this bug.