90725 - Don't allocate space managers that won't be used

Status: RESOLVED WORKSFORME

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

Description

[Daniel Bratell]

In the table stress case, there is 10000 cells and each one of these allocate a 
nsSpaceManager during reflow. A nsSpaceManager that is never used.

Comment from the code in nsBlockFrame::Reflow:
670   // XXXldb If we start storing the space manager in the frame rather
671   // than keeping it around only during reflow then we should create it
672   // only when there are actually floats to manage.  Otherwise things
673   // like tables will gain significant bloat.

That is about 2% (120ms) of the page load time and 200-300KB of memory space.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking for now, although I may give it to waterson.

Comment 2

[Carlos Alén Silva]

What's the progress on this bug? Is it WONTFIX? Should it be reassigned?

Comment 3

[Wayne Mery (:wsmwk)]

where might one find an archived version of http://www.mozilla.org/newlayout/testcases/stress/wbtblclr.html ?

Comment 4

[Kohei Yoshino [:kohei]]

https://www-archive.mozilla.org/newlayout/testcases/stress/wbtblclr.html

Comment 5

[Daniel Holbert [:dholbert]]

I don't think there's anything that's obviously worth-doing/worth-tracking here at this point.

The premise here seems to have been that we were wastefully allocating nsSpaceManager instances on this wbtblclr.html testcase and it was taking up a measurable amount of time (and memory). (Note that nsSpaceManager was renamed to nsFloatManager in bug 191448, in the time since this bug was filed.)

I just profiled Firefox continuously-reflowing wbtblclr.html, by running this in the web console:

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

I captured the profile after 130 seconds (while my above-quoted JS loop was still executing). There were 46326 samples with reflow in the backtrace, as shown here: https://share.firefox.dev/2ZpGHZn

Of those, only 98 samples ( 0.2% ) had "FloatManager" in the backtrace at all; they were all in nsFloatManager::GetFlowArea. No samples were captured in CreateFloatManager or in nsFloatManager's constructor/destructor. So, this allocation doesn't seem to be taking any measurable amount of time.

There's perhaps a case to be made that we should still avoid allocating these from a memory-savings perspective; but, given that these objects only exist temporarily during reflow (there's no persistent cost), I'm not especially concerned about memory-usage here, in the abstract at least. We also seem to have invariants in our codethat depend on always having these objects allocated (i.e. BlockReflowInput::FloatManager() is infallible), so it would probably require some nontrivial amount of invariant-reworking, additional checks, etc. if we wanted to become extremely-lazy about allocating these.

So, I think we can call this WORKSFORME at this point, since these allocations aren't occupying any measurable amount of CPU time, and given that there's no persistent memory-usage cost.