-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: prevent memory leaks from uncleaned uPlot event listeners #9320
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. We now have a clean up in two places 1. Uplot.tsx before chart.destroy() and hooks.destroy
, please consolidate this or is there a reason to clean up in both places separately?
// Initialize cleanup array for tracking all event listeners | ||
(self as ExtendedUPlot)._legendCleanups = []; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is removed, I'm not removing the cleanup from uplot component is because uplot doesn't directly uses getUPlotChartOptions function which has the hooks.destory cleanup. it get's the options through a prop and let's suppose if someone uses this component directly with own options without using the getUPlotChartOptions function. They there won't be any cleanup.
Anyhow after the cleanup we reset the array, so even if the cleanup runs twice, it'll run on a empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not removing the cleanup from uplot component is because uplot doesn't directly uses getUPlotChartOptions function which has the hooks.destory cleanup. it get's the options through a prop and let's suppose if someone uses this component directly with own options without using the getUPlotChartOptions function. They there won't be any cleanup
That's the main point I was trying to raise with "consolidate this". We should not let it anyone do something that it could lead to memory leak and requires separate cleanup work. Let's please consolidate it. If there is a good reason why it needs to be separate, I want to understand why.
Description
Fixes critical memory leaks in uPlot chart rendering that were causing memory spikes during chart re-renders and updates.
Problem
Memory profiling revealed significant memory leaks in
getUplotChartOptions.ts
. The function was adding multiple event listeners that were never cleaned up when charts were destroyed or re-rendered:mouseenter
andmouseleave
events on each legend itemmousemove
event listener ondocument
that accumulated on every chart renderdocument.body
that were never removedEach time a chart was re-rendered or updated, new event listeners were added while old ones remained attached, causing memory to grow continuously.
Solution
Implemented comprehensive event listener cleanup using uPlot's lifecycle hooks:
Changes Made
_legendCleanups
array inExtendedUPlot
interface to track all event listenersdestroy
hook to uPlot options that removes all tracked listenersUplot.tsx
destroy callback to manually trigger all cleanup functions.legend-tooltip
elements from DOMFiles Modified
frontend/src/lib/uPlotLib/getUplotChartOptions.ts
- Main memory leak fixesfrontend/src/components/Uplot/Uplot.tsx
- Enhanced cleanup in React componentImpact
5 Heap Snapshot before Fix: (10 Sec interaction between each snapshot)
6 Heap Snapshot after Fix: (10 Sec interaction between each snapshot)