[WIP] fix: deepCopy memory leak

[BobbieGoede]

🔗 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

Proof of concept to prevent memory leak based on findings from #2647 and #2629

This adds a changed deepCopy function, this version attempts to merge and copy objects by value. I added simple caching to prevent unnecessary merging as this is CPU heavy and changed the function to use iteration instead of recursion to lower memory usage.

I haven't tested this with dynamic message files or files with cache disabled. I also want to refactor the way messages are cached and merged as mentioned in #2647, but the first priority is tackling the memory leak.

📝 Checklist

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

[BobbieGoede]

This needs some more work, seems like I broke it while cleaning up before committing..

[s00d]

_des = des

I think you can't do it this way, as it destroys the reference to the object. I can suggest using Object.assign(_des, des); but it's still a strange solution. Perhaps it's better to just do

return des;
//...
targetMessage = deepCopyIteratively(message, targetMessage);

?

There's another problem: even after implementing my suggested changes, the translation still doesn't work on the client side. Something else is broken.

By the way, my latest version worked fast, even without a cache. However, there's a significant problem with using a cache: if someone messes up the structure of the translations, it could lead to a leak again, which would be very difficult to trace. Logically, the cache should not be implemented in deepCopyIteratively, but at a higher level, to cache the result of deepCopyIteratively, similar to how it's currently done with loader.cache. This would simplify the code but could theoretically cause leaks.

[BobbieGoede]

@s00d

Perhaps it's better to just do
targetMessage = deepCopyIteratively(message, targetMessage);

Yes something like that is the eventual goal, we also want to stop passing the setter function, this PR is focused on caching and removing the recursion in combination with copying the array entries by value.

By the way, my latest version worked fast, even without a cache.

Are you referring to the changes in this PR intlify/vue-i18n#1675? If so, have you tested this with locale files containing nested keys as well? Testing locally I can confirm it is fast with arrays, but using nested keys ends up being quite slow compared to the original code (as long as you aren't triggering the leak). I'm testing with this file en.json.

However, there's a significant problem with using a cache: if someone messes up the structure of the translations ...

I'm not sure what you mean with this.

Logically, the cache should not be implemented in deepCopyIteratively, but at a higher level, to cache the result of deepCopyIteratively ...

Maybe I misunderstand, do you mean the eventual merged result of all files per locale? This wouldn't work in combination with dynamic locale resources such as those returning API results, but it would work if all locale files were static files. I'm trying to cache the intermediate merges, if a project has a locale configured with ['en.json', 'en-US.json', 'en-US-api.js'], then at least the merging between 'en.json' and 'en-US.json' can be retrieved from cache.

[s00d]

I'm testing with this file en.json.

I tested on this repo. In the pull request, I switched to using Object.assign, and after that, not only did the leak go away, but the speed also significantly increased.

intlify/vue-i18n@07327ec

I'm not sure what you mean with this.

Global maps with objects can also cause leaks; probably, we need to think through a cleaning algorithm
As an option, you can implement cache clearing based on time, at least once a day, or better yet, make it a configurable parameter.

, if a project has a locale configured with ['en.json', 'en-US.json', 'en-US-api.js'], then at least the merging between 'en.json' and 'en-US.json' can be retrieved from cache.

Yes, you are right. I forgot about that.