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

perf(lines): fix potential memory leak in the effect line when setOption with notMerge #16525

Merged
merged 2 commits into from
Feb 17, 2022

Conversation

pissang
Copy link
Contributor

@pissang pissang commented Feb 17, 2022

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • [] others

What does this PR do?

Reproduce demo of memory leak demo: https://uzedc5.csb.app/map-random.html

When we using setOption with notMerge. The model will be recreated but view will be reused. And the graphic elements in the view are also reused so we can apply transition animations easily.

In this case memory leaks happens in

self._updateSymbolPosition(symbol);
. The looped animation of graphic elements will be keeped when updating. It also keeps the closure of during callback. Which includes the old lineData:

image

I still can't figure out why this lineData will be kept in the closure because there is no code using it in the during callback. Current workaroud is move the code of updating animation to a new function.

Another improvement is removing the unnecessary hold of SeriesModel instance in the graphic elements in

private _seriesModel: SeriesModel;
It won't cause memory leak now, but this change can optimize the memory a bit.

Details

Before: What was the problem?

The instances of SeriesData and SeriesModel in memory will increase each time setOption in the above case.

After: How is it fixed in this PR?

The instance numbers of SeriesData and SeriesModel is stable.

@pissang pissang requested review from 100pah and plainheart February 17, 2022 05:31
@echarts-bot
Copy link

echarts-bot bot commented Feb 17, 2022

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

@pissang pissang changed the title perf(symbol): fix potential memory leak in the effect symbol when setOption with notMerge perf(lines): fix potential memory leak in the effect symbol when setOption with notMerge Feb 17, 2022
@pissang pissang changed the title perf(lines): fix potential memory leak in the effect symbol when setOption with notMerge perf(lines): fix potential memory leak in the effect line when setOption with notMerge Feb 17, 2022
@pissang pissang merged commit 949d160 into master Feb 17, 2022
@echarts-bot
Copy link

echarts-bot bot commented Feb 17, 2022

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@pissang pissang deleted the fix-memory-leak branch February 17, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants