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

v2.15.0 is leaking memory #1266

Closed
asapach opened this issue Jan 23, 2019 · 6 comments
Closed

v2.15.0 is leaking memory #1266

asapach opened this issue Jan 23, 2019 · 6 comments
Assignees

Comments

@asapach
Copy link

asapach commented Jan 23, 2019

After updating to v2.15.0 eslint started running out of memory on our fairly large project.
I was able to reproduce something similar when running the plugin against a dozen copies of lodash-es. I can create a repro case, but it's rather trivial.
One rule is enough to see the difference between v2.14.0 and v2.15.0:

plugins:
  - import

rules:
  import/default: 2

This seems to be caused by 05c3935. I've run some memory profiles, but I can't pinpoint the exact source of the leak other than that it seems to originate in ExportMap.js. It's possible that the leak was always there, but extra parsing and memory allocations made it more evident.

sergei-startsev added a commit to sergei-startsev/eslint-plugin-import that referenced this issue Jan 27, 2019
@benmosher benmosher self-assigned this Jan 28, 2019
@benmosher
Copy link
Member

Thanks for identifying the commit! This is not the first time ExportMap has had a major memory leak after a minor change.

Memory leaks are always my top priority after a release. I'll try to take a look at lunch today.

sergei-startsev added a commit to sergei-startsev/eslint-plugin-import that referenced this issue Jan 28, 2019
@sergei-startsev
Copy link
Contributor

@benmosher you might also want to take a look at PR #1271

@benmosher
Copy link
Member

@sergei-startsev thanks for that! I think I found the issue though:

image

For some reason, the function closer for getter in captureDependency is retaining source despite not referencing it. I am still untangling why, but semantically, this code should not need to retain source so I think some refactoring should resolve this without disabling the code entirely.

Also: I'm not sure your PR will actually work in practice; if non-no-deprecated rules run first, they will cache ExportMaps without the docs parsed.

@sergei-startsev
Copy link
Contributor

@benmosher The PR idea is not to parse docs if it's not required, I wouldn't like to build SourceCode instances if no-deprecated rule isn't used.

As for cache, we can distinguish docs was disabled and docs wasn't found.

@benmosher
Copy link
Member

Sure, but the SourceCode instances shouldn’t be retained at all to begin with. And I don’t think they cost much CPU-wise.

@benmosher
Copy link
Member

Published as 2.16.0. Thanks so much for both of your help on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants