946167 - Reflow (e.g. from .offsetHeight) is slow with deeply nested flex containers

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[itamari]

Attachment: index.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.57 Safari/537.36

Steps to reproduce:

Sorry for the mess I am not sure 100% what is causing this. The scenario where this happens is quite complex. We've managed to drill it down to a minimum with this basic example on the html file I am attaching. It has a block of some html code (it shouldnt make any sense)





Actual results:

The problem is when you try to get an offset of an HTML element which belongs to some flex container right after you change it's width the browser takes a (very) long time to return the value. we've gotten results as high as 600ms on firefox (using flex).


Expected results:

In other browsers and web-kit this will usually be between 0 to 3 miliseconds
*note: the old flex spec also works fine but using it gives us quite a lot of bugs we are not interested in

Comment 1

[yuvalgo]

Any plans to resolve this soon?

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Reporter, can you provide more information about how to reproduce? (and possibly fix the testcase, since I think it might not actually be able to directly reproduce this)

From skimming the testcase's source, it looks like the intention is for someone to reproduce this by manually running flexPleaseBeFast() in the browser's console -- but that gives me an error for "e" being undefined (I think because you want addEventListener, not addEvent, in the testcase?)

And then, even if I fix that by manually defining "e", I get this output in both Firefox and Chrome (in their Web Developer Consoles) when I invoke flexPleaseBeFast():
> 0
> undefined

So: Can you provide more concrete steps (and/or an updated testcase) for getting metrics like the ones you report in comment 0?

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Also, while you're tweaking the testcase -- if you don't actually need the mootools script for the purpose of reproducing that (I suspect you don't), it'd be worth removing that from the testcase. That load gets blocked anyway, when referenced from a bugzilla-hosted testcase -- that's because the testcase is served over HTTPS, which means it's not allowed to pull in scripts over (insecure) HTTP. (at least, not in recent browser-versions that block mixed content)).

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

STR
1. load the test, open the Console
2. enter the Console commands:
i=400
flexPleaseBeFast(++i)
flexPleaseBeFast(++i)
...

ACTUAL RESULTS
Nightly 29.0a1 (2013-12-14) on Linux64: values in 18..30 range, avg. ~24
Chrome 33 on same host: similar results

Comment 5

[itamari]

Attachment: Testcase #3

Adding a simpler version
changing width and reading size takes around 120 ms

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8349281 [details]
Testcase #3

This test is significantly faster in Firefox (Nightly 29.0a1 (2013-12-14)
on Linux64) than in Chrome33 for me.  Nightly: ~70, Chrome: ~220

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I get numbers similar to what mats reported in Nightly and Chrome 33, from loading attachment 8349281 [details] and invoking flexPleaseBeFast(100).

Firefox 26 (release version) gives me ~96 -- a tad slower than Nightly, but still much faster than Chrome.

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

reporter, if you get results that are slower than what we're seeing, could you provide your explicit steps to reproduce, and your results?

(and if necessary, post a new testcase that demonstrates the orders-of-magnitude-slower-than-webkit/blink performance reported in comment 0? So far it looks like we're significantly faster.)

Comment 9

[Jiri Tobisek]

Reproduction steps demonstrating the same problem:

    console.time("benchmark");
var parentNode = document.body;
for(var i=0; i<15; i++) {
    var childNode = document.createElement("div");
    childNode.style.display = "flex";
    parentNode.appendChild(childNode);
    parentNode = childNode;
}
childNode.innerHTML = "Text";
var height = document.body.children[0].offsetHeight;
console.timeEnd("benchmark");

benchmark: 551.53ms on Firefox 26.0
benchmark: 0.847ms on Chrome 31.0.1650.63

both on Mac OSX 10.9.1
BTW, changing display to "-moz-box": 1.59ms
display = "block": 1.33ms

Comment 10

[Jiri Tobisek]

Firefox 28.0a2 has even worse results. :-(
benchmark: 602.9ms

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Thanks, jiri! I can definitely reproduce the issue with that snippet.

From tweaking that snippet in Firefox, it looks like the benchmark time approximately doubles with each additional layer of depth, but it doesn't explode anywhere near as quickly in Chrome.

I think that sort of doubling behavior may be unavoidable for some flexbox situations, but we've clearly got some room for optimization in at least this case.

Comment 12

[Jiri Tobisek]

Thanks for your swift reaction, Daniel! I have to say I can't agree that rendering 15 DOM elements in 600ms is unavoidable. Please note this is not a special situation, the only special thing about the elements is that they have display:flex. Furthermore, the old display:-moz-box renders just fine in the same scenario (very close to the display:block) in 1.59ms.

You are absolutely right that Chrome suffers from almost identical problems. Fortunately though, they actually fixed the problem and in Chrome 34.0.1788.0 Canary, the rendering time drops from almost 1000ms to 3.6ms on my machine.

Comment 13

[Jiri Tobisek]

As for the question, whether the use case of 15 nested elements is extreme - to clear any doubts, I have prepared yet another reproduction steps:

console.time("benchmark");
    var parentNode = document.body;
    for(var i=0; i<30; i++) {
        var childNode = document.createElement("div");
        if((i>0 && i<=5) || (i>=10 && i<=15) || (i>=25)) {
            childNode.style.display = "flex";
        } else {
            childNode.style.display = "block";
        }
        parentNode.appendChild(childNode);
        parentNode = childNode;
    }
    childNode.innerHTML = "Text";
    var height = document.body.children[0].offsetHeight;
    console.timeEnd("benchmark");

This code creates nested block of 5 flex -> 5 block -> 5 flex -> 5 block elements. This simulates quite reasonably complex web app dom. The benchmark on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0 is anywhere between 600-1000 ms on my machine. I will get the same result if I disperse the flex elements equally (1 flex > 1 block > 1 flex > etc.).

I believe this shows that we are dealing with entirely common situation, not an extreme case.

Comment 14

[Jiri Tobisek]

the ^ if line should be 

if((i>0 && i<=5) || (i>10 && i<=15) || (i>20 && i<=25)) {

of course. :-)

Comment 15

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Jiri Tobisek from comment #12)
> I have to say I can't agree that
> rendering 15 DOM elements in 600ms is unavoidable.

Nobody said that it was. Please don't put words in my mouth.

> You are absolutely right that Chrome suffers from almost identical problems.
> Fortunately though, they actually fixed the problem and in Chrome
> 34.0.1788.0 Canary, the rendering time drops from almost 1000ms to 3.6ms on
> my machine.

I didn't say that either :) but it's interesting to hear that their performance changed recently on this sort of testcase.  FWIW, I was testing Chrome Dev channel (version 33) in comment 11, which was giving me results on the order of a few milliseconds.

Comment 16

[Maximka]

Any progress/estimate on the bug? I am using codemirror (http://codemirror.net) editor with flexbox layouts and typing in firefox working very slow.

Comment 17

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Maximka from comment #16)
> Any progress/estimate on the bug?

I'm not actively working on this at the moment, but I hope to soon.

> I am using codemirror
> (http://codemirror.net) editor with flexbox layouts and typing in firefox
> working very slow.

Do you have a specific codemirror URL (or steps) that hit this bug? I tried the demos at http://codemirror.net/ and didn't see any that seem to use flexbox. (And if I download the codemirror source, the only mention of "flex" seems to be in mode/css/css.js, in a giant list of propertyKeywords, which I'm guessing (?) is just used for syntax-highlighting.)

So, it's possible you're hitting something other than this bug.

Comment 19

[Maximka]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> > I am using codemirror
> > (http://codemirror.net) editor with flexbox layouts and typing in firefox
> > working very slow.
> 
> Do you have a specific codemirror URL (or steps) that hit this bug? I tried
> the demos at http://codemirror.net/ and didn't see any that seem to use
> flexbox. (And if I download the codemirror source, the only mention of
> "flex" seems to be in mode/css/css.js, in a giant list of propertyKeywords,
> which I'm guessing (?) is just used for syntax-highlighting.)
> 
> So, it's possible you're hitting something other than this bug.

CodeMirror does a lot of calls to innerHeight and etc, if codemirror is placed in flex layout, it is working slow because reflow is slow, this is the demo https://rebold-bogard.codio.io/demo.html, it is just nested boxes with codemirror.

Comment 20

[Maximka]

Forgot, demo sources https://codio.com/maximka/FF-bug-demo/tree/demo.html

Comment 21

[Daniel Holbert [:dholbert] (PTO until 2/24)]

This bug's perf issue (at least, the test code in comment 9 and the plnkr URL in bug 1018143) is significantly improved by my patches in bug 1015474. 

In particular, it helps because it makes us preemptively stretch the cross size of a flex item, when the container has a definite size, which does away with the need to reflow the child twice (for that reflow of the container). (Otherwise, we would reflow twice -- once to measure the child's height (before stretching, to establish the stretch amount), and a final reflow after we've stretched it.)

We're still a bit wasteful - with the test code in comment 9, we'll still reflow the child twice the *first* time we see it (before the parent has an established definite height), but on the second time that we reflow the parent, we only need to reflow the child once instead of twice.  So we still blow up a big, but nowhere near as badly.

(I'm working on a patch to do away with the "still a bit wasteful" extra reflows, but I'd just noticed that my build with bug 1015474 & without this new patch was actually pretty good; commenting here to document why. :))

Comment 22

[Daniel Holbert [:dholbert] (PTO until 2/24)]

In particular, with the test code in comment 9
 - Current trunk takes ~5 seconds, with a depth of 15. (and doubles with each additional layer of depth)
 - With bug 1015474 and its dependencies, that takes ~32 ms.

If I adjust the test code to use a depth of 190, then:
 - Current trunk hangs forever (unsurprisingly, given the doubling described above).
 - With bug 1015474 and its dependencies, it takes 9.4 seconds
 - With my WIP patch for this bug, it takes ~280ms to load.

Comment 23

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: testcase 4 (based on comment 9; save locally to adjust var "depth")

Here's the testcase that I've been working with locally in comment 22 (using a depth of 15 here; save locally to adjust).

Comment 24

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: wip patch (now spun off as bug 1054010)

(and here's my WIP patch mentioned above in comment 21 & comment 22.)

Comment 25

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: wip patch (diff -w version) (now spun off as bug 1054010)

Comment 26

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: 946167-crashtest.patch

Here's a crashtest derived from testcase 4 / comment 9, with a depth of 50, which is enough to make current trunk hang long enough to time out the crashtest suite (on my machine at least).

I intend to land this after bug 1015474 is fixed.  (It completes in ~400ms on my machine, in a build with that fixed.)

Comment 27

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed crashtest (on top of patches for bug 1015474, which make it non-painful to load):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b489ff052163

(Still planning on getting the wip patch landed at some point soon, to improve things further; hence, leave-open.)

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b489ff052163

Comment 29

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8458929 [details] [diff] [review]
wip patch (now spun off as bug 1054010)

I've spun off a helper bug, bug 1054010, to finish off the WIP patch that I attached here.

Comment 31

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I'm calling this bug FIXED by bug 1015474, per comment 21 / comment 22 / comment 26.

There's a bit more we can do to improve here, as noted in followup bug 1054010, but from a practical perspective, the severe perf issues here are fixed.

Comment 32

[heavy]

Has this landed in the nightly build yet? I only ask because I have also come across a major performance problem with my web app by using nested flex containers. My web app becomes unusable, literally. Firefox 33 chews up all CPU and doesn't come back. I am also testing on 36.0a1 (2014-10-14) and the performance issue persists. As a point of reference, both Safari and Chrome display the same DOM almost instantaneously.

If this patch is in the nightly, I can safely say the performance issue around nesting flex containers is not resolved, although my case might be slightly different than this case, so I could try to isolate it.

Comment 33

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to heavy from comment #32)
> Firefox 33 chews up all
> CPU and doesn't come back.

This should've been fixed in Firefox 34, so that's not unexpected.

> I am also testing on 36.0a1 (2014-10-14) and the
> performance issue persists.

This, though, is a bit unexpected.  I've got a work-in-progress patch stack to address some known room for improvement, which is tracked by bug 1054010, but (until now) I wasn't aware of there being huge performance problems that would be addressed by that patch-stack. So:

> although my case might be slightly different than this case, so I could try to isolate it.

Yes please -- if you could create a testcase that reproduces the problem, that would be *very* helpful.

Please file a new bug here, with an attached testcase if you can make one (or a link to a live testcase somewhere, if that's easier):
  https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout
...and please CC me on the new bug (and feel free to mention the bug number here).

Thanks!

Comment 34

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(I'm basing my "should've been fixed in Firefox 34" assertion on the "target milestone" field in bug 1015474, which [per comment 31] fixed the performance issue with the testcase on this bug at least.  --> Copying that target-milestone field here, for clarity.)

Comment 35

[heavy]

I created bug 1082780 for the issue I was seeing.