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

Bugfix/unmount memory leak #3708

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

g4rb4g3
Copy link
Contributor

@g4rb4g3 g4rb4g3 commented Dec 1, 2024

Description

Fixes an issue that memory would not be freed up when the MapView was unmounted on iOS.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new bugfix on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

Screenshot OR Video

without this memory consumption increases whenever a new MapView is mounted
Bildschirmfoto 2024-11-29 um 13 01 16

with this fix memory is freed up when a MapView is unmounted
Bildschirmfoto 2024-12-01 um 15 31 35

Component to reproduce the issue you're fixing

I've updated the ShowMap.tsx and added an (un)mount button there. Whenever you press this button and a MapView is mounted memory usage increases but never decreases when unmounting unless this fix is in place.

@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens December 1, 2024 15:44 — with GitHub Actions Inactive
@g4rb4g3
Copy link
Contributor Author

g4rb4g3 commented Dec 1, 2024

I just double checked on Android and it seems like this is an iOS issue only.
Whenever the map is unmounted the memory consumption goes down and raises again when the map is mounted again.
image

Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

Thanks, looks great to me.

Note that those examples shows as a documentation, we don't want to pollute them.

Maybe we can add new example for testing just this, what do you think?

https://rnmapbox.github.io/docs/examples/Map/ShowMap

@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens January 7, 2025 09:03 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens January 7, 2025 09:03 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens January 7, 2025 09:03 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens January 7, 2025 09:04 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 deployed to CI with Mapbox Tokens January 7, 2025 09:04 — with GitHub Actions Active
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens January 7, 2025 09:04 — with GitHub Actions Inactive
@g4rb4g3 g4rb4g3 temporarily deployed to CI with Mapbox Tokens January 7, 2025 09:04 — with GitHub Actions Inactive
@g4rb4g3
Copy link
Contributor Author

g4rb4g3 commented Jan 7, 2025

Sure, I just pushed an update moving the example to a new file.

@g4rb4g3 g4rb4g3 requested a review from mfazekas January 7, 2025 09:36
@mfazekas
Copy link
Contributor

mfazekas commented Jan 9, 2025

@g4rb4g3 you'll need to run yarn generate

@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3 g4rb4g3 requested a deployment to CI with Mapbox Tokens January 9, 2025 14:39 — with GitHub Actions Waiting
@g4rb4g3
Copy link
Contributor Author

g4rb4g3 commented Jan 9, 2025

@g4rb4g3 you'll need to run yarn generate

Doing so gives me an error about an invalid babel version, but it is exactly the one from package.json

➜  maps git:(bugfix/unmount-memory-leak) ✗ yarn generate
yarn run v1.22.19
$ node ./scripts/autogenerate.mjs
(node:41108) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
Generating MapboxStyles.d.ts
Generating RNMBXStyle.swift
Generating RNMBXStyleFactory.kt
Generating styleMap.ts
$ yarn tsc --build plugin
$ /Users/manuel_auer/github/maps/node_modules/.bin/tsc --build plugin
$ cd example; jest __tests__/dumpExamplesJson.ts
 FAIL  __tests__/dumpExamplesJson.ts
  ● Test suite failed to run

    [BABEL] /Users/manuel_auer/github/maps/example/node_modules/react-native/jest/react-native-env.js: Requires Babel "^7.22.0 || >8.0.0-alpha <8.0.0-beta", but was loaded with "7.19.1". If you are sure you have a compatible version of @babel/core, it is likely that something in your build process is loading the wrong version. Inspect the stack trace of this error to look for the first entry that doesn't mention "@babel/core" or "babel-core" to see what is calling Babel. (While processing: "/Users/manuel_auer/github/maps/node_modules/@babel/plugin-syntax-import-attributes/lib/index.js")

So I updated it to 7.22.0 to run generate but did not commit it, maybe you can double check on that?

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.

2 participants