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

Fix memory leak when removing map #13110

Conversation

kamil-sienkiewicz-asi
Copy link
Contributor

Hey, so I digged a little in code to resolve this issue #9126 and it kinda seems like I found what the issue is (for a bare map, nothing was touched after mounting).

Tested it using FinalizationRegistry and heap snapshots

Before:
When Map was added and removed using map.remove() it was piling up in memory heap snapshot.

After my changes:
When Map is removed and another instance of map is being mounted, previous instance is garbage collected.

image

Which wasn't exactly what I wanted.
I suspect that another issue lies in:

src/ui/map.js

 3881  const gl = this.painter.context.gl;

in _authenticate -> getMapSessionAPI -> error callback
which holds this.painter forever as
const mapSessionAPI_ = new MapSessionAPI(); is created in global scope and never freed.

but that code is just below warning about terms of service, so I'm not touching that :p

For future issues: Do I have permission to modify/debug this code with good intentions? All changes that improve mapbox would be put out in public PRs to approve by mapbox team.

@mourner @stepankuzmin let me know if that works and if you could handle issue in code that is under TOS 🤝

@kamil-sienkiewicz-asi kamil-sienkiewicz-asi requested a review from a team as a code owner March 7, 2024 11:08
@CLAassistant
Copy link

CLAassistant commented Mar 7, 2024

CLA assistant check
All committers have signed the CLA.

@kamil-sienkiewicz-asi
Copy link
Contributor Author

@stepankuzmin any followup on that/is something blocking this issue to be resolved? test-render-mac-safari-dev seems like something random broke, can't re-run 🤔

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, nice catch!

@mourner
Copy link
Member

mourner commented Mar 14, 2024

and if you could handle issue in code that is under TOS 🤝

While we're still looking into this (most likely, it's fine for external contributors to edit that in PRs too), let us add a fix while we confirm.

Copy link
Contributor

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow response and thanks for the fix @kamil-sienkiewicz-asi!

@stepankuzmin stepankuzmin changed the title fix-memory-leak-when-removing-map Fix memory leak when removing map Mar 14, 2024
@stepankuzmin stepankuzmin merged commit 22e183b into mapbox:main Mar 14, 2024
26 checks passed
@kamil-sienkiewicz-asi
Copy link
Contributor Author

@stepankuzmin Thanks for the merge, however I can still observe that map is not removed entirely. It's not piling up in memory, however const gl = this.painter.context.gl; is still holding previous map instance after removal. It's freed when new map is initialised.

If you could confirm that I can touch this code under TOS I can work on that in a meanwhile.

@stepankuzmin
Copy link
Contributor

stepankuzmin commented Mar 14, 2024

@kamil-sienkiewicz-asi Hmm, that's weird, I've checked with your example from #9126 (comment) and haven't seen dangling Map instances. But sure, please go ahead and we'll review your PR 👍

@kamil-sienkiewicz-asi
Copy link
Contributor Author

@stepankuzmin #13116 as promised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants