Fix a memory leak in Error objects#6965
Conversation
|
This would need the same change in Also, not collecting the stack was on purpose, as we only want the stacks in case of errors, and collecting all stacks when we don't need them is pretty slow. I haven't benchmarked it, though... See #4782 for some discussion (although that one was "skip collecting stack at all", not "eagerly vs lazily evaluate") |
Do you mean this location? It's not required here, because only the |
|
@mjesun ideas on why this isn't detected by |
Codecov Report
@@ Coverage Diff @@
## master #6965 +/- ##
=======================================
Coverage 66.97% 66.97%
=======================================
Files 250 250
Lines 10381 10381
Branches 4 4
=======================================
Hits 6953 6953
Misses 3427 3427
Partials 1 1Continue to review full report at Codecov.
|
just guessing, but maybe these Error objects are eventually disposed when all test suites finished running?
I think this isn't even needed in this case, because Error objects are not stored, they are only dispatched as event, which is only temporary. |
An alternative would be to get rid of references to |
|
If it's not a problem in circus, maybe you can switch webpack over to using it? Unless you use jasmine globals it should be a straight swap. I can send a PR. EDIT: this couples the test run to jasmine, so no circus for now :( https://github.com/webpack/webpack/blob/ee7d9485fa30d1d3d5adbfdfb52c8f57ae8b439e/test/helpers/createLazyTestEnv.js |
|
What is jest-circus? Is there any documentation about that? |
|
We don't really have much docs on it, we probably should... @cpojer @aaronabramov do we have some docs, or a short pitch, on the what and why of circus? |
Nope... that's weird TBH. Jasmine executes inside the container, so it should be detected by the leaker. It might be that the stack traces have weak references, but then there shouldn't be any leak at all... |
|
If this helps here is the retainers path:
|
|
I think the |
|
Is there anything else you want me to change in the PR? |
|
I think we should roll back the changes to circus (which is failing CI), seeing as you say it's not needed. We do stick that error into the state, but perhaps that state is more properly cleaned up than it is with Jasmine? I'm also a bit afraid that this will cause a performance hit, but maybe not? |
sokra
force-pushed
the
bugfix/memory-leak-error
branch
from
030c969 to
fa68244
Compare
I did some microbenchmarking and figured out that the overhead for eager-evaluation is about 130µs in node 8, a bit lower in node 10. It's only done a few times per test case, so the overhead is a few seconds for 10,000 test cases. |
|
Ah, ok. That's not too bad! |
|
Mind updating the changelog? Then I'll merge 🙂 Do note that we've merged a breaking change to master though, so this won't be released for a bit |
Stack was evaluated early, so we need to fix it when changing the message
sokra
force-pushed
the
bugfix/memory-leak-error
branch
from
ea9e307 to
57b557a
Compare
|
Thanks! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
5 participants
see also https://crbug.com/v8/7142
Summary
Error object keep references to all "context"s in the stack. This will lead to memory leaks if the Error objects are stored somewhere persistent. This is an "optimization" of v8, since constructing the
stackproperty as string can be expensive.Accessing the
stackproperty will force stringifying of these internal objects and discards references, which allows scopes to be garbage-collected.A similar fix was made here: mochajs/mocha#3119
These memory leaks are usually very small since very few people store large objects outside of
it(). In the webpack test suite theit()is called from the bundle after compiling. Here the bundle and the Compilation is in the stack. This causes memory leaks.I believe but can't prove yet, that the inability of running too many test suites at once (in webpack) is also caused by this problem. It hangs without error. Let's see...
Test plan
Unsure how to test this.
I tested it manually via memory snapshots in the devtools.
I could construct a test suite that allocates much memory and will lead to memory errors without this fix, but this would probably slow down the CI... Tell me if you want this.