1141108 - Flexbox appears to use an inefficient rendering algorithm which causes massive browser load for a relatively small number of nested flex items.

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Luke May]

Attachment: firebox-firefoxFlexboxRenderingBug.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/40.0.2214.111 Chrome/40.0.2214.111 Safari/537.36

Steps to reproduce:

When a relatively small number of flexbox items are used on a page, the performance of Firefox degrades to the point of unusability.  There appears to be an inefficient rendering algorithm used to calculate the sizes of the flex items.  I have created a JSFiddle to demonstrate the issue.  http://jsfiddle.net/yfzeqxgm/1/  .  I also included the same code in an html file and attached it.  I have filed this bug in version 36 as that is the currently relesaed version, however it seems to be present in 37 too.  In version 38 the impact of the bug is lessened, but I think it still remains.


Actual results:

As you increase the number of nested elements, you see an exponential degradation in performance.  If you pop the number of nested elements up to around 100, Firefox performance degrades to the point of crashing.  Chrome and IE have no problem rendering upwards of 200 elements.  


Expected results:

If you run the fiddle with Chrome or IE, you'll see that the rendering of 200 nested elements is no problem at all.  Firefox should behave in a similar fashion.

Comment 1

[Daniel Holbert [:dholbert]]

>  In version 38 the impact of the bug is lessened

Nice, so that's due to bug 1054010.

> but I think it still remains.

Yeah, I see multi-second hangs (in the neighborhood of 5-10 seconds) at a depth of 200. Changing bug from UNCONFIRMED to NEW for that reason.

Comment 2

[Luke May]

This is a very important fix for my current work project (single page webapp), and I've been authorized to offer my time if there's anything I can do to help.  Thanks!

Comment 3

[Daniel Holbert [:dholbert]]

Thanks for offering!

It's possible / likely that there's some helper-CSS that you can apply, to hack around the 2-pass-layout aspects of flexbox[1] & speed things up up. (at least in Firefox 38 with bug 1054010, if not also earlier versions).

I should hopefully get a chance to look into figuring out what CSS might help there in the next day or so.

[1] (not all of which are implemented in Chrome yet -- https://code.google.com/p/chromium/issues/detail?id=426898 in particular -- which may in part explain the perf difference here)

Comment 4

[Jon Keller]

Daniel,
I modified Luke's Fiddle to allow specifying breadth in addition to depth of flexbox nesting: http://jsfiddle.net/keller_jon/p4o2g782/

When adding depth only, Chrome seems to outperform FF, but not by a very noticeable margin.
But when breadth is introduced as well, the difference becomes large when using the current (as opposed to 2009) CSS.

For instance, in a tree of height n where each node has n children, Chrome is in the neighborhood of 1ms per flexbox regardless of n, for values of n=1...6.  n=6 results in about 56,000 flexboxes in the tree.  Firefox 36, on the other hand, gets up to 46ms per flexbox for n=6.

Findings: https://www.dropbox.com/s/3ht0ua9wnzfty8n/Browser_speeds.pdf?dl=0

Comment 5

[Luke May]

Any updates on this Daniel?  It sounded like you may have some tips on how to avoid some of the performance hits without completely abandoning flexbox.  

Is there an approximate timeline for this fix?  Our project is in limbo based on the timeline of this fix.  If the fix is expected to come out this year then we should be ok, but if not then we are likely going to have to abandon flexbox and rework a fairly substantial portion of our code base.

Thanks so much for looking at this!

Comment 6

[Daniel Holbert [:dholbert]]

Yup, I've got a bit of an update here:

I poked around some modified versions of Luke's attached testcase in a debugger, & noticed that bug 1054010's optimization (mentioned in comment 1) wasn't getting applied as thoroughly as I expected (and in particular, the CSS fixes I had in mind to reduce redundant reflows weren't working) -- and then figured out why, and I filed & fixed bug 1142686.

That's fixed in current Nightly 39 builds, and it should be fixed in the next "Firefox Developer Edition" 38 update as well. (those come out every day or so I think)

With current nightly (due to that fix I think), I can give Luke's testcase a depth of 200, and the window responds pretty responsively.  (unlike a build from 3/15, which has multi-second jank on window-resizes at that depth)

Luke, could you test current Nightly and let me know if this seems fixed for you there?

Comment 7

[Daniel Holbert [:dholbert]]

Nightlies are available at https://nightly.mozilla.org -- though you may want to wait for the Developer Edition update. (unlike Nightly, Developer Edition doesn't share your existing Firefox profile & is hence a bit safer/easier to test alongside an existing Firefox install).

As soon as your Developer Edition has a datestamp of 3/21 or later (in Help|About, or Firefox|About on Mac), I'd expect it to include bug 1142686's fix.

Comment 8

[Daniel Holbert [:dholbert]]

(Jon, it may be useful to update your findings from comment 4 as well - I'm curious how we compare there with bug 1142686's fix.  6x6 seems to hang both browsers, and I didn't bother waiting for measurements there -- but with 5x5, I get an initial render time of 2642ms in Chrome vs. 2450ms in current Firefox nightly.)

Comment 9

[Daniel Holbert [:dholbert]]

RE your timeline question:

(In reply to Luke May from comment #5)
> Is there an approximate timeline for this fix?  Our project is in limbo
> based on the timeline of this fix.  If the fix is expected to come out this
> year then we should be ok

Optimistically, if your results match mine & this is fixed in Developer Edition version 38: that'll become a Firefox release version in May (on 2015-05-12), according to https://wiki.mozilla.org/RapidRelease/Calendar

There's a chance the fix would be be backed out of 38 if we discover that it causes non-trivial regressions, in which case it'd be bumped to 39 or possibly 40 if it's really bad.  (which would just be 6 weeks of delay, per version number)

So, it sounds like that fits your ballpark "this year" schedule. (again, assuming that it's actually fixed as I think it is)

Comment 10

[Luke May]

This is awesome Daniel!  From my initial tests everything appears to be working great!  Outstanding!  You have saved us some major time and effort here.  Thank you so much!  We will try to get some more exhaustive testing done soon.

Comment 11

[Daniel Holbert [:dholbert]]

Hooray, glad to hear it! I'm going to tentatively resolve this as FIXED by bug 1142686, then.

Comment 12

[Daniel Holbert [:dholbert]]

(Thanks for filing the bug & for including a handy standalone testcase, too! That definitely makes it easier to get to the bottom of things.)

Comment 13

[Jon Keller]

Thanks, Daniel! I apologize for being MIA for a while (vacation, then got sick during the trip home). I'll re-test today or tomorrow and put some results up.

Comment 14

[Daniel Holbert [:dholbert]]

No need to apologize! :) Also, no need to get comprehensive results from my perspective (unless you want to). I just want to make sure it's not as bad as it was in comment 4 anymore.

Comment 15

[Jon Keller]

Ok, testing done, and it's great.
4x4 tree, 342 flexboxes: FF38 before improvement=914ms, FF38 after improvement=363ms, Chrome=267ms.
5x5 tree, 3907 flexboxes: FF38 before improvement=17s, FF38 after improvement=2.8s, Chrome=3.1s.
6x6 tree, 56k flexboxes: FF38 before improvement=681s, FF38 after improvement=44s, Chrome=62s.

For Luke's 1-dimensional trees, I tested various depths up to 60 and your code change gives an improvement of 0 to 6ms, but this scenario wasn't problematic to begin with.

Thanks so much for your work on this, Daniel!

Comment 16

[Luke May]

Hey Daniel, did you pull your fix from the Nightlies?  It appears to still work on the version of firefox I have that is dated 3/21, but on versions 3/22 and 3/26 the fix appears to no longer be present.  I'm guessing you pulled it to do more thorough testing perhaps?

Comment 17

[Daniel Holbert [:dholbert]]

Nope, though I did land a different fix, which disable the core optimization here when there's a percent-height child. (which requires that we do two reflows of the flex item -- once to measure its content [before we know its height, & before the child can know what to resolve its percent against], and once again with the final resolved height [at which point the child has something to resolve its percent against].

This will return some pathological scenarios to Firefox 37 level performance, but I expect that should only happen when there's nesting with percent heights (or perhaps something similar) all the way down.

That fix landed on 3/21, so it makes sense that you'd see its effects in the 3/22 nightly.  However, I don't see any trouble with the attached testcase or the jsfiddles that have been linked here. Are these testcases the ones that you're seeing a behavior-difference on?  If not, could you make another testcase? (or try removing percent heights and see if that fixes it)

Comment 18

[Daniel Holbert [:dholbert]]

(The patch that disabled the optimization when there are percent-height children was on bug 1128354, btw.)

Comment 19

[Daniel Holbert [:dholbert]]

(One other possible workaround [if you really need percent heights] would be to turn off min-height:auto and flex-basis:auto -- those are the two main reasons we need the first just-content-measuring reflow for each flex item. So, e.g. try setting "min-height:0" and provide a non-"auto" flex-basis (e.g. in the 3rd entry in the "flex" shorthand property), if you aren't already doing that.)

Comment 20

[Luke May]

To clarify, my comment was not in regards to the fiddle I provided in this bug. we are using `flex: 1 1 100%;` in several places, so that could indeed be the cause of the issue I am seeing.  I'm not sure why`flex: 1 1 100%;` is being used so much, because I think the same results can be achieved (stretching a div to the size of the viewport or its parent) without the percentage basis.  We also use `flex-basis: auto;` in quite a few places as well, but that is explicitly needed because of the nature of our product (web-based designer), so the user can dynamically change the content.  

As far as the min-width/min-height stuff goes (we are almost exclusively setting min-width and min-height to 0px for virtually every flex item.  The reasoning for this is not well founded, but we do it simply because it has fixed some of the firefox issues we have seen due to a fairly recent advancement in the flexbox spec Mozilla released into the wild.  I do not know much of the specifics of it other than the few articles I read, and I tried it and it seemed to work.

I can try to whip up another fiddle to specifically isolate this case.  We are seeing performance hits similar to those prior to the fix you pushed on 3/21.

Comment 21

[Daniel Holbert [:dholbert]]

OK, thanks for clarifying.

(In reply to Luke May from comment #20)
> we are using `flex: 1 1 100%;` in several places, so that could indeed
> be the cause of the issue I am seeing.  I'm not sure why`flex: 1 1 100%;` is
> being used so much, because I think the same results can be achieved
> (stretching a div to the size of the viewport or its parent) without the
> percentage basis.

Agreed.  You'd likely be better off using "flex: 1 1 0px" in those places.  I don't think 

> We also use `flex-basis: auto;` in quite a few places as
> well, but that is explicitly needed because of the nature of our product

Understood. To be clear, I think this would only cause a slowdown if
 (1) it's in a *vertical* flexbox (so we have to measure the height up-front, which requires layout)
 (2) this flex item with flex-basis:auto has a percent-height child (which means we can't optimize away the second reflow)

> As far as the min-width/min-height stuff goes (we are almost exclusively
> setting min-width and min-height to 0px for virtually every flex item.  The
> reasoning for this is not well founded, but we do it simply because it has
> fixed some of the firefox issues we have seen due to a fairly recent
> advancement in the flexbox spec Mozilla released into the wild.

Cool -- yeah, we're the first to implement "min-width:auto"/"min-height:auto", which is generally useful but can have some unexpected behavior (see dependencies on bug 1043520). You're running up against that, and "min-[width|height]:0" restores the old behavior.  Note that Chrome is close to having an implementation of this, too ( https://code.google.com/p/chromium/issues/detail?id=426898 ) and Microsoft has it implemented in their next-gen engine, so any min-sizing trouble you saw in Firefox will soon be present in those other browsers too.

> I can try to whip up another fiddle to specifically isolate this case.  We
> are seeing performance hits similar to those prior to the fix you pushed on
> 3/21.

That would be great. For sanity/bug-management's sake, would you mind filing a new (separate) bug for that testcase?  Handy URL (which will auto-CC me on the bug):
https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout&cc=:dholbert

Otherwise, the state of this bug would get a bit confusing & hard to track. (since the linked/attached testcases are now fixed)

Comment 22

[Daniel Holbert [:dholbert]]

> Agreed.  You'd likely be better off using "flex: 1 1 0px" in those places.  I don't think

Sorry, didn't finish my thought there. Meant to say: I don't think those should produce different results, if you're setting that same style on all of the flex items.  (On the other hand, if you're mixing a "flex: 1 1 100%" flex-item with flex items with different flexibilities/flex-basis values, you may see a behavior change from switching 100% to 0px. Even so, flex:1 1 100% is probably not the best way to express what you're going for there.)

Comment 23

[Luke May]

Awesome, I'll open a new bug once I get the fiddle going.  Thanks Daniel!

Comment 24

[Daniel Holbert [:dholbert]]

Thanks! I'll keep an eye out for it.

I'm bumping this bug to VERIFIED FIXED, given verification in comment 10, comment 15, and beginning of comment 20.