Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataTable: Memory leak when data updated continuously #5927

Closed
tracktor-git opened this issue Feb 8, 2024 · 23 comments · Fixed by #6011
Closed

DataTable: Memory leak when data updated continuously #5927

tracktor-git opened this issue Feb 8, 2024 · 23 comments · Fixed by #6011
Assignees
Labels
Type: Performance Issue is performance or optimization related
Milestone

Comments

@tracktor-git
Copy link

tracktor-git commented Feb 8, 2024

Describe the bug

Hi there!

In the DataTable component, when constantly updating data (for example, via a websocket), the browser quickly begins to eat up RAM. This problem was already identified earlier, but remained unresolved (issue #4656). Version 9.5.0 does not have this problem.

Reproducer

https://96vrp7-5173.csb.app/

PrimeReact version

10.5.0

React version

18.x

Language

ES6

Build / Runtime

Vite

Browser(s)

Firefox 122.0.1, Maxthon 7.1.8.6000, Chrome latest

Steps to reproduce the behavior

  1. Go to https://96vrp7-5173.csb.app/
  2. Select any item from Dropdown
  3. Go to Chrome console --> Memory tab

Expected behavior

No response

@tracktor-git tracktor-git added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 8, 2024
@melloware
Copy link
Member

Original Ticket: #5420

And this one #5017

And this: #2979

@melloware melloware added Type: Performance Issue is performance or optimization related and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Feb 8, 2024
@kl-nevermore
Copy link
Contributor

kl-nevermore commented Feb 19, 2024

(

const [bindDocumentClickListener, unbindDocumentClickListener] = useEventListener({
)
It should be here,
Many cells are rendered and create man
listeners
After commenting, the memory is stable and gc can be seen

@melloware
Copy link
Member

Awesome work @kl-nevermore I will test today!

@melloware melloware added this to the 10.5.2 milestone Feb 19, 2024
@melloware
Copy link
Member

@tracktor-git i can't reach your Code Sandbox? Is this an editable datatable?

@kl-nevermore
Copy link
Contributor

@melloware
replace basicdoc.js

import { DocSectionCode } from '@/components/doc/common/docsectioncode';
import { DocSectionText } from '@/components/doc/common/docsectiontext';
import { Column } from '@/components/lib/column/Column';
import { DataTable } from '@/components/lib/datatable/DataTable';
import { useState, useEffect } from 'react';
import { ProductService } from '../../../service/ProductService';
import DeferredDemo from '@/components/demo/DeferredDemo';
import colsData from './colsData.json';
import { random, sample, sampleSize, times, isEmpty } from 'lodash';
const ROWS_NUM = 50;
const COLUMNS_TO_UPDATE = 5;

const generateRandomCellValue = (length) => {
    const characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
    return times(length, () => sample(characters)).join('');
};

const generateRows = (rowsNum, columnsData) => {
    const rows = [];

    for (let i = 0; i < rowsNum; i++) {
        let row = {};
        columnsData.forEach((col) => {
            row[col.field] = random(0, 1000000);
        });

        rows.push(row);
    }
    return rows;
};
export function BasicDoc(props) {
    const [products, setProducts] = useState([]);
    const [rows, setRows] = useState([]);
    useEffect(() => {
        //Initialise table with generated rows
        const newRows = generateRows(ROWS_NUM, colsData);
        setRows(newRows);

        const interval = setInterval(() => {
            setRows((currentRows) => {
                if (!isEmpty(currentRows)) {
                    const updatedData = [...currentRows];

                    // Determin random row and row columns to be updated
                    const randomRow = random(0, ROWS_NUM - 1);
                    const randomColumns = sampleSize(colsData, COLUMNS_TO_UPDATE);

                    randomColumns.forEach(({ field }) => {
                        // update cell with random data
                        updatedData[randomRow][field] = generateRandomCellValue(6);
                    });

                    return updatedData;
                }
            });
        }, 1000);

        return () => clearInterval(interval);
    }, []);
    const loadDemoData = () => {
        ProductService.getProductsMini().then((data) => setProducts(data));
    };

    const code = {
        basic: `
<DataTable value={products} tableStyle={{ minWidth: '50rem' }}>
    <Column field="code" header="Code"></Column>
    <Column field="name" header="Name"></Column>
    <Column field="category" header="Category"></Column>
    <Column field="quantity" header="Quantity"></Column>
</DataTable>
        `,
        javascript: `
import React, { useState, useEffect } from 'react';
import { DataTable } from 'primereact/datatable';
import { Column } from 'primereact/column';
import { ProductService } from './service/ProductService';

export default function BasicDemo() {
    const [products, setProducts] = useState([]);

    useEffect(() => {
        ProductService.getProductsMini().then(data => setProducts(data));
    }, []);

    return (
        <div className="card">
            <DataTable value={products} tableStyle={{ minWidth: '50rem' }}>
                <Column field="code" header="Code"></Column>
                <Column field="name" header="Name"></Column>
                <Column field="category" header="Category"></Column>
                <Column field="quantity" header="Quantity"></Column>
            </DataTable>
        </div>
    );
}
        `,
        typescript: `
import React, { useState, useEffect } from 'react';
import { DataTable } from 'primereact/datatable';
import { Column } from 'primereact/column';
import { ProductService } from './service/ProductService';

interface Product {
    id: string;
    code: string;
    name: string;
    description: string;
    image: string;
    price: number;
    category: string;
    quantity: number;
    inventoryStatus: string;
    rating: number;
}

export default function BasicDemo() {
    const [products, setProducts] = useState<Product[]>([]);

    useEffect(() => {
        ProductService.getProductsMini().then(data => setProducts(data));
    }, []);

    return (
        <div className="card">
            <DataTable value={products} tableStyle={{ minWidth: '50rem' }}>
                <Column field="code" header="Code"></Column>
                <Column field="name" header="Name"></Column>
                <Column field="category" header="Category"></Column>
                <Column field="quantity" header="Quantity"></Column>
            </DataTable>
        </div>
    );
}
        `,
        data: `
{
    id: '1000',
    code: 'f230fh0g3',
    name: 'Bamboo Watch',
    description: 'Product Description',
    image: 'bamboo-watch.jpg',
    price: 65,
    category: 'Accessories',
    quantity: 24,
    inventoryStatus: 'INSTOCK',
    rating: 5
},
...
        `
    };

    return (
        <>
            <DocSectionText {...props}>
                <p>
                    DataTable requires a <i>value</i> as data to display and <i>Column</i> components as children for the representation.
                </p>
            </DocSectionText>
            <DeferredDemo onLoad={loadDemoData}>
                <div className="card">
                    <DataTable value={rows} tableStyle={{ minWidth: '50rem' }}>
                        {colsData && colsData.map((col) => <Column key={col.field} field={col.field} header={col.header}></Column>)}
                    </DataTable>
                </div>
            </DeferredDemo>
            <DocSectionCode code={code} service={['ProductService']} />
        </>
    );
}

colsData.json

@melloware
Copy link
Member

melloware commented Feb 19, 2024

OK @kl-nevermore can you try my PR? Those listeners should not be registering unless the cell is editable.

@kl-nevermore
Copy link
Contributor

OK, I'll go home later to test

@kl-nevermore
Copy link
Contributor

It looks like it causes problems once useEventListener is called
and create many closures
image

image

@melloware
Copy link
Member

OK but now at least is it only called when the cell is edtiable right so not for normal datatables?

@kl-nevermore
Copy link
Contributor

No, there will be problems as long as useEventListener created in BodyCell

I also found ToolTip also creates many listeners

@melloware
Copy link
Member

i thought the when clause would not create the listener if its not true though? so when: isEditable() should only create these listeners when a cell is editable?

@tracktor-git
Copy link
Author

@tracktor-git i can't reach your Code Sandbox? Is this an editable datatable?

No, it is not editable. It is sortable, if it's important. I see, you already found the trouble...

@kl-nevermore
Copy link
Contributor

@kl-nevermore
Copy link
Contributor

@melloware

const prevListener = usePrevious(listener);

@melloware
Copy link
Member

melloware commented Feb 19, 2024

Hmm but usePrevious is just holding a ref?

export const usePrevious = (newValue) => {
    const ref = React.useRef(undefined);

    React.useEffect(() => {
        ref.current = newValue;
    });

    return ref.current;
};

so is it that the unbind() is not being called?

React.useEffect(() => {
        // to properly compare functions we can implicitly converting the function to it's body's text as a String
        if (listenerRef.current && ('' + prevListener !== '' + listener || prevOptions !== options)) {
            unbind();
            when && bind();
        }
    }, [listener, options, when]);

@kl-nevermore
Copy link
Contributor

unbind called,but prevListener not gc
Manual GC memory is normal

React.useEffect(() => {
        // to properly compare functions we can implicitly converting the function to it's body's text as a String
        if (listenerRef.current && ('' + prevListener !== '' + listener || prevOptions !== options)) {
            unbind();
            when && bind();
        }
        return () => {
            prevListener  = null
        }
    }, [listener, options, when]);

melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
@melloware
Copy link
Member

OK @kl-nevermore can you review my updated PR?

melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
@melloware
Copy link
Member

I moved the prevListener = null to the useUnmountEffect can you see if it is still working for you?

melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
@melloware
Copy link
Member

Actually I also updated the usePrevious hook to have an unmount of setting current=null so I wonder if that is the real fix?

melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 19, 2024
@kl-nevermore
Copy link
Contributor

  • first render unbind and useUnmountEffect called
  • later render by Interval, although bind will not be executed, it will execute useEventListener and create many prevListeners. useUnmountEffect or unbind will not be executed at this time

@kl-nevermore
Copy link
Contributor

kl-nevermore commented Feb 20, 2024

So I guess we should release manually when not needed like,
because when BodyCell rerender, listenerRef.current is null and not trigger unmountEffect
so prevListener will be more and more
#5927 (comment)
or

if (listenerRef.current && ('' + prevListener !== '' + listener || prevOptions !== options)) {
            unbind();
            when && bind();
}else{
// dispose
}

melloware added a commit to melloware/primereact that referenced this issue Feb 20, 2024
@melloware
Copy link
Member

@kl-nevermore made one more fix if you want to try again.

melloware added a commit to melloware/primereact that referenced this issue Feb 20, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 20, 2024
melloware added a commit to melloware/primereact that referenced this issue Feb 20, 2024
melloware added a commit that referenced this issue Feb 20, 2024
* Fix #5927: BodyCell memory leak when not editable

* Fix #5927: BodyCell memory leak when not editable
@kl-nevermore
Copy link
Contributor

kl-nevermore commented Feb 21, 2024

I had fever yesterday. I just tested it and it fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Performance Issue is performance or optimization related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants