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 using multiple webpack instances #1205

Merged
merged 5 commits into from Nov 4, 2020
Merged

Fix memory leak when using multiple webpack instances #1205

merged 5 commits into from Nov 4, 2020

Conversation

valerio
Copy link
Contributor

@valerio valerio commented Nov 3, 2020

Closes #1203

This PR should fix a memory leak issue when using multiple webpack instances that get discarded over time.

I initially converted the webpackInstances array to a WeakMap (plus a global index) but that wasn't enough to fix the leak, since the cached TSInstance held in instances.ts was still retaining the webpack compiler and never being freed.

In the end, I removed the webpack instance array and changed the TS instance cache by using a combined WeakMap + Map so that we maintain the same behaviour but keep the lifetime of TSInstances tied to the webpack compiler that spawned them.

There is one small change in how loaderOptions.instance names are generated:

  • old names: 0_default_e3b0c44298fc1c14
  • new names: default_e3b0c44298fc1c14

They now don't contain the unique index for the webpack compiler that generated them, this shouldn't change the way instances are cached, since they are now tied to a specific webpack instance by reference (the key of the WeakMap) instead of using the index.

I've tested that the leak is fixed using this repo: https://github.com/valerio/ts-loader-leak
Memory now gets periodically freed between runs:

➜  ts-loader-leak git:(main) node index.js 1
webpack will run, process should run out of memory
current heap usage { heapMBs: 101 }
current heap usage { heapMBs: 151 }
// omitted for brevity
current heap usage { heapMBs: 882 }
current heap usage { heapMBs: 937 }
current heap usage { heapMBs: 991 }
current heap usage { heapMBs: 98 } <-- memory was freed 
current heap usage { heapMBs: 154 }
current heap usage { heapMBs: 208 }
current heap usage { heapMBs: 257 }
current heap usage { heapMBs: 312 }
current heap usage { heapMBs: 366 }
current heap usage { heapMBs: 155 } <-- memory was freed 
current heap usage { heapMBs: 202 }
current heap usage { heapMBs: 257 }
current heap usage { heapMBs: 312 }
current heap usage { heapMBs: 359 }

I thought about adding a test case in here but I don't think it would fit with the current setup, it also might be non-deterministic.

@johnnyreilly
Copy link
Member

This is fantastic work! Do you want to update the package.json to a new version and add an entry to the CHANGELOG.md? Let's ship this!

@valerio valerio changed the title Fix/use weak references for webpack and ts compilers Fix memory leak when using multiple webpack instances Nov 3, 2020
@valerio
Copy link
Contributor Author

valerio commented Nov 3, 2020

This is fantastic work! Do you want to update the package.json to a new version and add an entry to the CHANGELOG.md? Let's ship this!

Done and done! 🚀
And thanks for the speedy review! 😄

@johnnyreilly johnnyreilly merged commit e90f8ad into TypeStrong:master Nov 4, 2020
@liorcode
Copy link

liorcode commented Nov 4, 2020

Seems this change breaks my build:

        Module build failed (from ../node_modules/sled-test-runner/node_modules/thread-loader/dist/cjs.js):
        Thread Loader (Worker 2)
        Invalid value used as weak map key
            at PoolWorker.fromErrorObj (node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:262:12)
            at node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:204:29
            at WeakMap.set (<anonymous>)
            at Object.getTypeScriptInstance (node_modules/ts-loader/dist/instances.js:37:23)
            at Object.loader (node_modules/ts-loader/dist/index.js:16:41)  

Had to pin the ts-loader version to 8.0.7 to work around it

@johnnyreilly
Copy link
Member

johnnyreilly commented Nov 4, 2020

Oh no! @valerio would you be able to take a look please?

@liorcode is it ThreadLoader being in the mix that causes the issue? If you remove ThreadLoader does that fix the issue? (side note; you might find your build is faster without ThreadLoader: https://blog.johnnyreilly.com/2018/12/you-might-not-need-thread-loader.html )

If you could share a minimal reproduction repo that would be helpful

@liorcode
Copy link

liorcode commented Nov 4, 2020

I am actually using a 3rd party service which itself uses thread loader, so unfortunately I can't control it, but thanks!

@valerio
Copy link
Contributor Author

valerio commented Nov 4, 2020

        Module build failed (from ../node_modules/sled-test-runner/node_modules/thread-loader/dist/cjs.js):
        Thread Loader (Worker 2)
        Invalid value used as weak map key
            at PoolWorker.fromErrorObj (node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:262:12)
            at node_modules/sled-test-runner/node_modules/thread-loader/dist/WorkerPool.js:204:29
            at WeakMap.set (<anonymous>)
            at Object.getTypeScriptInstance (node_modules/ts-loader/dist/instances.js:37:23)
            at Object.loader (node_modules/ts-loader/dist/index.js:16:41)  

That's a strange issue indeed, some values like Symbol or string can't be used as a weak map key, maybe ThreadLoader is hooking into webpack and replacing the compiler instance with something else?

I'll see if I can reproduce, @liorcode a reproduction repo would definitely help if you have an easy way to set that up!

@Krasnopir
Copy link

Have same problems after update.


ERROR in ./apps/admin-client/src/index.tsx
Module build failed (from ./node_modules/thread-loader/dist/cjs.js):
Thread Loader (Worker 0)

Invalid value used as weak map key
at PoolWorker.fromErrorObj (/usr/src/app/node_modules/thread-loader/dist/WorkerPool.js:344:12)
at WeakMap.set (<anonymous>)
at Object.getTypeScriptInstance (/usr/src/app/node_modules/ts-loader/dist/instances.js:37:23)
at Object.loader (/usr/src/app/node_modules/ts-loader/dist/index.js:16:41)

my webpack config


        {
          test: /\.tsx?$/,
          exclude: [/node_modules/],
          use: [
            { loader: 'cache-loader' },
            {
              loader: 'thread-loader',
              options: {
                workers: require('os').cpus().length - 1,
                poolTimeout: isProduction ? 500 : Infinity,
              },
            },
            {
              loader: 'ts-loader',
              options: {
                happyPackMode: true, 
              },
            },
          ],
        },

@valerio
Copy link
Contributor Author

valerio commented Nov 4, 2020

I've managed to reproduce this, opened an issue here: #1206

I'll see if I can come up with a fix for this as well!

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.

Memory leak when running with multiple webpack (compiler) instances
4 participants