Flex performance with nested flex containers is very poor compared to other browser

RESOLVED DUPLICATE of bug 1209697

Status

()

defect
RESOLVED DUPLICATE of bug 1209697
4 years ago
11 months ago

People

(Reporter: warcraftthreeft, Assigned: dholbert)

Tracking

(Blocks 1 bug)

38 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

Test Case Size: 10 http://jsfiddle.net/xbg9xk7d/3/
Test Case Size: 20 http://jsfiddle.net/xbg9xk7d/4/
Test Case Size: 50 http://jsfiddle.net/xbg9xk7d/5/

You can change the value to like 15 or whatever value in the setup code to test different ideas. This seems to be a good way to test though. Might not want to open the 50 example except on other browsers.

Javascript Setup Code:
var Separator = function(separator, previous, next)
{
    var vertical = separator.parent().css('flex-direction') == 'row';
    separator.addClass(vertical ? 'vertical-separator' : 'horizontal-separator');
    var mouseDown = function(mousedownEvent)
    {
        // If the user left clicks on a separator then start the resizing operation
        if (mousedownEvent.which == 1)
        {
            var cursor = vertical ? 'col-resize' : 'row-resize';
            var page = vertical ? 'pageX' : 'pageY';
            var dimension = vertical ? 'flex-basis' : 'flex-basis';
            var oldPreviousDimension = parseInt(separator.prev().css(dimension), 10);
            var oldNextDimension = parseInt(separator.next().css(dimension), 10);
            $('html, body').css('cursor', cursor);
            var SetDimension = function(offset)
            {
                if (previous)
                {
                    separator.prev().css(dimension, oldPreviousDimension + offset + 'px');
                }
                if (next)
                {
                    separator.next().css(dimension, oldNextDimension - offset + 'px');
                }
            }
            var mouseMove = function(mousemoveEvent)
            {
                SetDimension(mousemoveEvent[page] - mousedownEvent[page]);
            }
            $(window).mousemove(mouseMove);
            // mouseup and blur catch every possible case where the mouse is released. This ensures the events are unregistered correctly.
            $(window).one('mouseup blur', function(mouseupEvent)
            {
                $(window).off('mousemove', mouseMove);
                SetDimension(mouseupEvent[page] - mousedownEvent[page]);
                $('html, body').css('cursor', '');
                separator.one('mousedown', mouseDown);
            });
        }
        else
        {
            separator.one('mousedown', mouseDown);
        }
    };
    separator.one('mousedown', mouseDown);
}

var currentTag = $('body');
for (var i = 0; i < 50; ++i)
{
    currentTag.append('<ul><li></li><li></li><li></li></ul>');
    if (i % 2 == 0)
    {
        // Row
        currentTag.find('ul').addClass('horizontal');
    }
    else
    {
        // Column
        currentTag.find('ul').addClass('vertical');
    }
    currentTag = currentTag.find('li:last-child');
}
$('.horizontal > li:nth-child(2)').each(function()
{
    $(this).next().css('flex-basis', $(this).parent().width() / 2 + 'px');
    Separator($(this), false, true);
});
$('.vertical > li:nth-child(2)').each(function()
{
    $(this).next().css('flex-basis', $(this).parent().height() / 2 + 'px');
    Separator($(this), false, true);
});

CSS:
*
{
    margin: 0;
    padding: 0;
}
html
{
	height: 100%;
	display: flex;
	flex-flow: row nowrap;
}
body
{
	flex: 1 1 0px;
	min-width: 0;
	display: flex;
	flex-flow: row nowrap;
}
ul
{
    list-style: none;
}
.horizontal
{
    flex: 1 1 0px;
	min-height: 0;
    display: flex;
    flex-flow: row nowrap;
}
.horizontal > li:first-child
{
    flex: 1 1 0px;
    min-width: 0;
    background-color: #f00;
}
.horizontal > li:last-child
{
    flex: 0 0 0px;
    min-width: 0;
    background-color: #0f0;
    display: flex;
    flex-flow: row nowrap;
}
.vertical
{
    flex: 1 1 0px;
	min-width: 0;
    display: flex;
    flex-flow: column nowrap;
}
.vertical > li:first-child
{
    flex: 1 1 0px;
    min-height: 0;
    background-color: #0ff;
}
.vertical > li:last-child
{
    flex: 0 0 0px;
    min-height: 0;
    background-color: #f0f;
    display: flex;
    flex-flow: row nowrap;
}
.horizontal-separator
{
	flex: 0 0 5px;
	cursor: row-resize;
	background-color: #fff;
	user-select: none;
	-webkit-user-select: none;
	-moz-user-select: none;
	-ms-user-select: none;
}
.vertical-separator
{
	flex: 0 0 5px;
	display: inline-block;
	cursor: col-resize;
	background-color: #fff;
	user-select: none;
	-webkit-user-select: none;
	-moz-user-select: none;
	-ms-user-select: none;
}


Actual results:

It's very slow, to the point of locking up the browser, compared to every other browser. Other browsers are unaffected by the number of nested flex containers, but Firefox gets progressively slower fast.


Expected results:

Should perform like Chrome, IE11 or Microsoft Edge which have no issue with nested flex containers.
Component: Untriaged → Layout
Product: Firefox → Core
Thank you for the bug report.

Daniel, I'm still seeing slowness here even on nightly, so this looks different from bug 1142686...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dholbert)
I'll take a look in the next day or so. In the meantime, here's a partly reduced testcase with no JS required.
Assignee: nobody → dholbert
the testcase opens without any jank for me.. Should this be closed?
Flags: needinfo?(dholbert)
That's Daniel's call.  Did you test the original testcases, or the one from comment 2?
Flags: needinfo?(dholbert)
Nice! Thanks for the heads-up -- this does indeed seem to be fixed!

The testcase from comment 2 renders pretty much instantly for me, and "Test Case Size: 50" (the biggest one from comment 0) loads in a few seconds.

I'm seeing if I can track down when/where this was fixed... I'll close after I've done that.
Fix range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f985243bb630b2c78cd57731c8d8ab191aa09527&tochange=bf15d4078c2a6db7df37ab466d28a1e075c9eb4d

--> fixed by bug 1209697
Status: NEW → RESOLVED
Closed: 11 months ago
Flags: needinfo?(dholbert)
Resolution: --- → DUPLICATE
Duplicate of bug: 1209697
In old (before-the-fix) builds, the attached testcase (from comment 2) takes at least 5 minutes to load (I gave it that long and it was still hanging).  So I think it's suitable as a crashtest [i.e. it'd trigger failures due to timeouts if we regressed this.]

Hence, I'll go ahead and push it as a crashtest.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ed5b8d9f61
Add crashtest for (now-fixed) flexbox hang bug. (no review, test-only)
You need to log in before you can comment on or make changes to this bug.