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 #2616

Merged
merged 3 commits into from Dec 13, 2023
Merged

fix: memory leak #2616

merged 3 commits into from Dec 13, 2023

Conversation

BobbieGoede
Copy link
Collaborator

@BobbieGoede BobbieGoede commented Dec 12, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

It is unlikely that this fixes all present memory leaks in the module, but this small change appears to have a measurable impact on the memory usage.

I ran the reproductions with the latest release rc.9, and took memory snapshots with chrome. First without any requests, then running siege for 10 seconds with 10 concurrent users (siege -t 10s -c 10 "http://localhost:3000/") after which I took another snapshot. After waiting for a short moment I took another snapshot in case any more memory was cleared πŸ€·β€β™‚οΈ. Then I installed my local modified @nuxtjs/i18n and went through the same steps.

My testing isn't exactly thorough, but it was a reliable way to see the memory usage go up and stay up, it's possible memory will still increase after longer usage and uptime, it's something that takes time and patience.

Below a comparison of the snapshots before and after installing the modified package:
memory-usage-comparison

I also tested #2034 (comment) (or https://github.com/danielroe/i18n-memory-leak) after making this comparison image, memory usage went from ~121mb to ~33mb.

While profiling I kept seeing nuxtI18Options and baseUrl, based on thismy theory is that the nuxt context is not being disposed of correctly as it is being set on nuxtI18nOptions in the i18n.ts plugin.

For example here:

nuxtI18nOptions.baseUrl = extendBaseUrl(nuxtI18nOptions.baseUrl, {
differentDomains,
nuxt: nuxtContext,
localeCodeLoader: defaultLocale,
normalizedLocales
})

nuxtI18nOptions.baseUrl is being set to a function returned from extendBaseUrl, the returned function makes use of the passed nuxt context (options.nuxt), nuxtI18nOptions is not defined in the scope of the plugin so I can imagine it would be hard to know when to dispose/free the memory? Based on this assumption I instead create a new object using nuxtI18nOptions, which since it's scoped to the plugin should be easier to safely dispose/free.

I don't know enough about memory management or garbage collection in Javascript, but I have experience optimizing memory usage for mobile games in C#, to provide a background for my reasoning. If someone has good resources on this please share them so I can form more informed conclusions πŸ˜…

Update

Based on the testing I did, I changed the way the Nuxt context is used/retrieved. Instead of passing it to functions/closures and potentially causing leaks the useNuxtApp composable is used where possible.

There is still a small and slow increase in memory usage for every few 1000s of requests, will look into that as well.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@BobbieGoede BobbieGoede self-assigned this Dec 13, 2023
@BobbieGoede BobbieGoede marked this pull request as draft December 13, 2023 10:04
@BobbieGoede BobbieGoede marked this pull request as ready for review December 13, 2023 11:25
@dargmuesli
Copy link
Collaborator

Wow, I just found the same cause and wanted to report it! πŸ˜‚ Good thing this PR was a bit quicker than me πŸ˜‰ I'll try it in my project now.

Copy link
Collaborator

@dargmuesli dargmuesli left a comment

Choose a reason for hiding this comment

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

Tried and it seems to resolve the issue! ❀️

There seems to be another leak in some head / pinia related files in my project though πŸ˜… but that's unrelated to nuxt-i18n.

@kazupon
Copy link
Collaborator

kazupon commented Dec 13, 2023

That's amazing work! ❀️

The causing of memory leaks is very difficult since the code must be checked.
I appreciate your efforts πŸ™‡

I don't know enough about memory management or garbage collection in Javascript, but I have experience optimizing memory usage for mobile games in C#, to provide a background for my reasoning. If someone has good resources on this please share them so I can form more informed conclusions

As far as I know, I think you can control GC in Javascript by using WeakRef or FinalizationRegistry to get around it.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef

However, unlike C# and other languages, JavaScript does not currently provide a mechanism to release resources like the dispose / descructor pattern.
So, I think the only way to intentionally control memory release in JavaScript is to use those APIs.

Once again, thank you so much!

@kazupon kazupon merged commit 64fa2f6 into nuxt-modules:main Dec 13, 2023
7 checks passed
@BobbieGoede
Copy link
Collaborator Author

There seems to be another leak in some head / pinia related files in my project though πŸ˜… but that's unrelated to nuxt-i18n.

@dargmuesli
Happy to hear it's working, thanks for testing! I do think there is a smaller memory leak related to the head tags, still need to check if it is present without @nuxtjs/i18n, it may be caused by nuxt itself.

As far as I know, I think you can control GC in Javascript by using WeakRef or FinalizationRegistry to get around it.

@kazupon
I still need to learn more about those APIs, as well as WeakSet and WeakMap, maybe it would be easier to prevent leaks with those πŸ˜…

DarthGigi pushed a commit to DarthGigi/i18n that referenced this pull request Apr 16, 2024
* fix: `nuxtI18nOptions` being set and not cleaned up

* fix: use local `useNuxtApp` instead of passing nuxt context as argument

* fix: remove `nuxtI18nOptions` copy
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.

None yet

3 participants