226737 - RecoverFloats repeatedly called despite no Floats in document.

Status: RESOLVED WORKSFORME

(Core :: Layout: Block and Inline, defect)

Description

[Ivan Eisen]

If no Floats have been discovered, why go and call
nsBlockReflowState::RecoverFloats.  My trace of a text document shows that
2.18% of the time is spent in RecoverFloats.  I don't have any floats within
the document that was traced.
Why not have a global float encounted switch to be checked
before  nsBlockReflowState::RecoverStateFrom calls RecoverFloats.  The very
first time a float is encountered set this switch.  Else,  don't ever call
the RecoverFloats routine.

Comment 1

[Ivan Eisen]

Attachment: Large text document

Comment 2

[Boris Zbarsky [:bzbarsky]]

This has been discussed in the past, I know... I can't locate the bug where the
discussion happened.

Comment 3

[Ivan Eisen]

I am sure it was there at one time ; however, I also could not locate it.   The
problem does not seem as bad as it once was (some O(N^2) removed?).    

Comment 4

[Ivan Eisen]

I believe this was a long running topic in the performance newsgroup several
years ago.  I found bug 117374 (UpdateSpaceManager no longer needed) which
is related to the newsgroup discussion.

Ivan

Comment 5

[Daniel Holbert [:dholbert]]

I think this is WORKSFORME.

The function in question does still exist, now named BlockReflowInput::RecoverFloats. It does still get called during a pageload of the attached testcase (which I checked by adding some logging and observing the logging), but it doesn't occupy any measurable amount of time -- it doesn't show up in profiles at all (which is great, considering that it took 2% of the time back in comment 0).

I captured a profile of the page repeatedly reconstructing-frames-and-reflowing, with the following loop executed in the web console in a tab with the testcase up:

for (let i = 0; i < 5000; i++) {
  document.documentElement.style.display = "none";
  document.documentElement.offsetTop; // flush layout
  document.documentElement.style.display = "";
  document.documentElement.offsetTop; // flush layout
}

I let this run for 90 seconds and then captured my profile -- that wasn't enough time to get through my full loop of 5000 reflows, but quite a few at least, with 88,260 samples taken with "Reflow" in the backtrace.
Here's the profile that I captured: https://share.firefox.dev/2Zz1nhq

This profile has zero samples with "floats" in the backtrace, so this RecoverFloats function isn't occupying any measurable amount of time. So at this point, there doesn't seem to be any performance-related reason to justify the global switch that was suggested in comment 0.