diff/patch slower in 10.4.5 when removing >1 keyed items from the list #2619

xkam · 2020-07-11T18:47:59Z

Looks like the diff/patch sometimes does unnecessary re-ordering of the child nodes instead of just remove two or more consecutive items from the middle of the list. Removing 1 item is OK. The version 10.4.4 was OK - issue started in 10.4.5.

We make a datagrid component which implements virtualized scrollling (i.e. adding/removing rows on the fly) and this issue totally kills the performance - i.e. full component repaint instead of just the affected rows.

Reproduction

Here is the static html page to reproduce the issue -

<!DOCTYPE html>
<html>
<head>
    <script src="https://unpkg.com/[email protected]/dist/preact.umd.js"></script>
    <script src="https://unpkg.com/@activewidgets/[email protected]/data/index.common.js"></script>
    <script src="https://cdn.activewidgets.com/[email protected]/bundle.js"></script>

<style>
    body { font: 14px/1.2143 'Segoe UI', 'Avenir', 'Helvetica Neue', 'Tahoma', sans-serif;}
    .ax-headers-view { font-weight: bold; color: #666; border-bottom: 1px solid #aaa;}
</style>
</head>
<body>

    <div id="app"></div>

<script type="module">

    let {h, render} = preact,
        {Datagrid} = ActiveWidgets.Preact;

    let el = document.getElementById('app'),
        app = h(Datagrid, {rows, columns});

    render(app, el);

// --------------------------------------------

    let observer = new MutationObserver(mutations => {

        if (mutations.length > 50){
            console.log('!!!'); // put breakpoint here
        }

        function rows(rec){
            return rec.type == 'childList' && rec.target.className.indexOf('fixed') > 0;
        }

        function out(rec){

            let s = '';

            rec.addedNodes.forEach(n => {
                s += 'add ' + n.children[0].children[0].innerText;
            });

            rec.removedNodes.forEach(n => {
                s += 'remove ' + n.children[0].children[0].innerText;
            });

            return s;
        }

        console.log(mutations.filter(rows).map(out));
    });

    observer.observe(el, {childList: true, subtree: true});

</script>
</body>
</html>

Steps to reproduce

Open the browser developer tools console and then slowly scroll the datagrid down either with mousewheel or scrollbar. The console should show added/removed rows, like

["add GROSR"]
["remove AROUT"]
["add HANAR"]
["remove BERGS"]
["add HILAA"]
["remove BLAUS"]

Scroll faster - as soon as you scroll more than 2 rows per 0.1s you will see something like this -

["remove BONAP", "add BONAP", "remove BOTTM", "add BOTTM", "remove BSBEV", "add BSBEV", "remove CACTU", "add CACTU", "remove CENTC", "add CENTC", "remove CHOPS", "add CHOPS", "remove COMMI", "add COMMI", "remove CONSH", "add CONSH", "remove DRACD", "add DRACD", "remove DUMON", "add DUMON", "remove EASTC", "add EASTC", "remove ERNSH", "add ERNSH", "remove FAMIA", "add FAMIA", "remove FISSA", "add FISSA", "remove FOLIG", "add FOLIG", "remove FOLKO", "add FOLKO", "remove FRANK", "add FRANK", "remove FRANR", "add FRANR", "remove FRANS", "add FRANS", "remove FURIB", "add FURIB", "remove GALED", "add GALED", "remove GODOS", "add GODOS", "remove GOURL", "add GOURL", "remove GREAL", "add GREAL", "remove GROSR", "add GROSR", "remove HANAR", "add HANAR", "remove HILAA", "add HILAA", "remove HUNGC", "add HUNGC", "remove HUNGO", "add HUNGO", "remove BOLID", "remove BLONP"]

Expected Behavior

If you replace 10.4.5 with 10.4.4 then adding/removing rows works as expected, including more than 1 row at a time -

["add FAMIA", "add FISSA", "add FOLIG"]
["add FOLKO", "add FRANK", "add FRANR", "add FRANS"]
["add FURIB", "add GALED", "add GODOS", "add GOURL"]
["remove ANATR", "remove ALFKI"]
["add GREAL", "add GROSR", "add HANAR", "add HILAA", "add HUNGC"]
["remove BLONP", "remove BLAUS", "remove BERGS", "remove AROUT", "remove ANTON"]
["add HUNGO", "add ISLAT", "add KOENE", "add LACOR"]
["remove BSBEV", "remove BOTTM", "remove BONAP", "remove BOLID"]
["add LAMAI", "add LAUGB", "add LAZYK", "add LEHMS", "add LETSS"]
["remove COMMI", "remove CHOPS", "remove CENTC", "remove CACTU"]
["add LILAS", "add LINOD", "add LONEP"]
["remove EASTC", "remove DUMON", "remove DRACD", "remove CONSH"]

Tested with Chrome 83.0.4103.116 and Firefox 77.0.1

The text was updated successfully, but these errors were encountered:

marvinhagemeister · 2020-07-11T20:44:25Z

hm the only change that seems relevant is this #2551 . The other PRs look harmless.

Comparing the flamegraphs of 10.4.5 and 10.4.4 shows that diffChildren or placeChildren sometimes take up more time. It's not consistent for me so there seems to be some scenario that is worse than others. It's getting late here will continue some other time.

xkam · 2020-07-12T06:19:01Z

Just want to clarify that performance impact is not that js execution is taking more time, but that it triggers DOM style/layout/repaint (for 1000s of elements in our case) where previously there was none.

marvinhagemeister · 2020-07-12T06:27:51Z

Good point! I wouldn't go so far as to say there being "none" before in 10.4.4 looking at the metrics on my end, but admittedly there is lot less time spent in rendering, painting and scripting with 10.4.4 compared to 10.4.5.

JoviDeCroock · 2020-07-13T10:19:45Z

Something that could help is improving the check here essentially this branch is very specific to this one issue. We should be able to define this behaviour better but this fix has just been added.

xkam · 2020-07-13T11:40:01Z

Moved to #2622

developit · 2020-07-13T13:25:50Z

@xkam nice repro! A separate issue would be fantastic for that.

xkam · 2020-07-14T10:38:45Z

I can finally reproduce this issue in an isolated example -

<!DOCTYPE html>
<html>
<head>
</head>
<body>

    <div id="app"></div>
    <div id="log"></div>

<script type="module">

    import {h, render, Component} from 'https://unpkg.com/[email protected]/dist/preact.module.js';

    let el = document.getElementById('app'),
        log = document.getElementById('log');


    let observer = new MutationObserver(function(mutations){

        function items(r){
            return r.type == 'childList';
        }

        function out(r){

            let s = '';

            r.addedNodes.forEach(n => {
                s += 'add ' + n.innerText;
            });

            r.removedNodes.forEach(n => {
                s += 'remove ' + n.innerText;
            });

            return s;
        }

        log.innerHTML = mutations.filter(items).map(out);
    });



    class Item extends Component {

        shouldComponentUpdate(props){
            return props.value !== this.props.value;
        }

        render() {
            return h('span', {}, this.props.value);
        }
    }

    function item(i){
        return h(Item, {value: i, key: i});
    }


    let data = [1,2,3,4,5,6,7,8,9];
    let app = h('div', {}, data.map(item));
    render(app, el);


    observer.observe(el, {childList: true, subtree: true});


//  remove 3 items from the middle
    data.splice(3, 3);
    let app2 = h('div', {}, data.map(item));
    render(app2, el);

</script>
</body>
</html>

Here is the output with 10.4.4 -

123789
remove 6,remove 5,remove 4

And this one is with 10.4.5 -

123789
remove 7,add 7,remove 8,add 8,remove 9,add 9,remove 6,remove 5,remove 4

The only difference with #2622 is the presence of shouldComponentUpdate callback.

JoviDeCroock · 2020-11-17T12:08:16Z

Hey, would you mind testing this with: https://pkg.csb.dev/preactjs/preact/commit/9ff1e5f6/preact I think this fixes the issue.

xkam · 2020-11-19T20:15:49Z

No, the issue is still there, tested both with full datagrid and the isolated code above. With 9ff1e5f I still get

remove 7,add 7,remove 8,add 8,remove 9,add 9,remove 6,remove 5,remove 4

JoviDeCroock · 2023-11-12T15:26:57Z

This is fixed in latest https://codesandbox.io/s/withered-snowflake-6lm3zs?file=/src/index.mjs

Repro from issue #2619

marvinhagemeister added the important label Jul 11, 2020

marvinhagemeister added the performance label Jul 11, 2020

marvinhagemeister changed the title ~~diff/patch broken in 10.4.5 when removing >2 keyed items from the list~~ diff/patch slower in 10.4.5 when removing >2 keyed items from the list Jul 11, 2020

marvinhagemeister mentioned this issue Jul 11, 2020

App statistics: Help us make Preact even faster #2618

Open

3 tasks

xkam mentioned this issue Jul 13, 2020

diff/patch unnecessarily re-inserts children when removing >1 item and items are class components #2622

Closed

xkam changed the title ~~diff/patch slower in 10.4.5 when removing >2 keyed items from the list~~ diff/patch slower in 10.4.5 when removing >1 keyed items from the list Jul 13, 2020

marvinhagemeister mentioned this issue Oct 3, 2020

Unnecessary unmounting/mounting children with keys #2783

Closed

JoviDeCroock mentioned this issue Nov 17, 2020

Search full list for oldDomNodes #2833

Closed

JoviDeCroock mentioned this issue Dec 8, 2021

improve performance of keyed children #3359

Closed

JoviDeCroock closed this as completed Nov 12, 2023

andrewiggins added a commit that referenced this issue Nov 13, 2023

Add test for removing children of memoed components

fabb5da

Repro from issue #2619

andrewiggins mentioned this issue Nov 13, 2023

Add test for removing children of memoed components #4210

Merged

JoviDeCroock pushed a commit that referenced this issue Nov 14, 2023

Add test for removing children of memoed components (#4210)

1e14862

Repro from issue #2619

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff/patch slower in 10.4.5 when removing >1 keyed items from the list #2619

diff/patch slower in 10.4.5 when removing >1 keyed items from the list #2619

xkam commented Jul 11, 2020

marvinhagemeister commented Jul 11, 2020

xkam commented Jul 12, 2020

marvinhagemeister commented Jul 12, 2020

JoviDeCroock commented Jul 13, 2020

xkam commented Jul 13, 2020 •

edited

Loading

developit commented Jul 13, 2020 •

edited

Loading

xkam commented Jul 14, 2020

JoviDeCroock commented Nov 17, 2020

xkam commented Nov 19, 2020

JoviDeCroock commented Nov 12, 2023

diff/patch slower in 10.4.5 when removing >1 keyed items from the list #2619

diff/patch slower in 10.4.5 when removing >1 keyed items from the list #2619

Comments

xkam commented Jul 11, 2020

Reproduction

Steps to reproduce

Expected Behavior

marvinhagemeister commented Jul 11, 2020

xkam commented Jul 12, 2020

marvinhagemeister commented Jul 12, 2020

JoviDeCroock commented Jul 13, 2020

xkam commented Jul 13, 2020 • edited Loading

developit commented Jul 13, 2020 • edited Loading

xkam commented Jul 14, 2020

JoviDeCroock commented Nov 17, 2020

xkam commented Nov 19, 2020

JoviDeCroock commented Nov 12, 2023

xkam commented Jul 13, 2020 •

edited

Loading

developit commented Jul 13, 2020 •

edited

Loading