Skip to content

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Nov 2, 2021

What's the problem this PR addresses?

ZipFS memory management was entirely manual - we always had to call one of ZipFS.prototype.{save,discard}AndClose to avoid memory leaks. This was very unfortunate because we had to design other parts of the architecture with this in mind. For example, the fetchResults had a releaseFs function that would release the ZipFS memory and that had to be called immediately after using the fetch results. This meant that it was hard for us to reuse fetchResults between different functions and the performance was taking a hit because of this (e.g. the link step had to decompress all archives again).

How did you fix it?

Made ZipFS memory management entirely automatic using FinalizationRegistry to automatically free the memory allocated by libzip when the ZipFS instances are garbage-collected.

Now, unfortunately FinalizationRegistry only got introduced in Node 14.6.0 and we still have to support Node 12.x over Yarn 3.x's lifetime. Because of this, I don't see how this PR could be merged earlier than Yarn 4.x.
We're introducing this in Yarn 4.

Breaking Changes:

  • api.holdFetchResult inside Installer['installPackage'] got removed since it doesn't make sense anymore. It got introduced in 3.1 so most likely nobody's using it.
  • Removed releaseFs from FetchResult. It shouldn't affect people that returned it or called it since it was optional anyways. It's a type-only breaking change.
  • Made fetchPackageFromCache return an object with the results rather than a tuple so that it isn't order dependent. Still kept the tuple via Object.assign magic so that it isn't a runtime breaking change since all third-party plugins that add fetchers that use the cache would have to be updated. Yet again, a type-only breaking change for now. I don't intend to remove the fallback until at least Yarn 5.x.
  • The fact that releaseFs won't be called anymore.
  • FinalizationRegistry has to be defined for ZipFS to work.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz added the major label Nov 2, 2021
@paul-soporan paul-soporan mentioned this pull request Nov 17, 2021
13 tasks
@paul-soporan paul-soporan force-pushed the paul/feat/automatic-zipfs-memory-management branch 6 times, most recently from 6159ec4 to 09c4dee Compare August 10, 2022 23:41
@paul-soporan paul-soporan marked this pull request as ready for review August 11, 2022 13:07
@paul-soporan paul-soporan requested a review from arcanis as a code owner August 11, 2022 13:07
@paul-soporan paul-soporan force-pushed the paul/feat/automatic-zipfs-memory-management branch from 8485932 to 96e23e4 Compare August 11, 2022 21:34
@paul-soporan paul-soporan marked this pull request as draft August 17, 2022 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants