Fix a leak in coverage #5289

rickhanlonii · 2018-01-12T00:03:09Z

Summary

This PR fixes a memory leak in coverage reporting

Details

From the reported issue, there's a minimal example that shows any test ran with coverage will report a leak when using --detectLeaks. This time I assumed the tool was correct and went directly to heapdumps:

Dump Before (without coverage)

Dump After (with coverage)

As you can see, tests with coverage are leaving behind the Window object.

That convinced me that the issue was with coverage and not with --detectLeaks reporting. From there I basically looked over coverage related areas and started commenting them out to see if the tests would pass (in the biz this is known and plug and chug)

Eventually, I found that commenting out the enclosed line passed the tests and realized that we were keeping around references to the whole test result so that we could process the coverage for each file after they all run. The fix is to make a copy of the coverage results.

Test plan

Considering this test repo:

jest --detectLeaks module.spec.js from master will pass
jest --detectLeaks module.spec.js --coverage from master will fail:

On my branch this test will pass:

For the curious, here's the heapdump after the fix:

No more Window 💥

rickhanlonii · 2018-01-12T00:05:15Z

packages/jest-runtime/src/index.js

-  getAllCoverageInfo() {
-    return this._environment.global.__coverage__;
+  getAllCoverageInfoCopy() {
+    return JSON.parse(JSON.stringify(this._environment.global.__coverage__));


This is probably the worst way to do this but I figured I would push and ask if there's a preferred way to deep copy objects in this project. I tried deepCyclicCopy from jest-utils but that did not work

For the curious, this is what the object looks like: https://gist.github.com/rickhanlonii/337bb5310f827cef35ec2d329edbfc3b

@rickhanlonii What was the issue with deepCyclicCopy? It's true it's too basic to copy RegExp, Dates and such, but being JSON copy-able, it should work out of the box 😔

Checking at the code, what can be happening is that we create the copied object using the prototype of the original one, so the instanceof operator still works for the copied one (this becomes especially important when creating copies of objects like process).

But, in turn, it can be preventing the memory to release, which I guess it's why it did not work for you.

Yeah that makes sense, that is what's happening

SimenB

Thanks for once again digging into this!

This is failing flow, run yarn flow locally to see it (based on your second commit, I guess the typing is incorrect).

Can you add an entry to the changelog?

/cc @mjesun for the question about deepCyclicCopy

mjesun · 2018-01-12T13:31:07Z

@SimenB what is the question regarding deepCyclicCopy? :D can't see anything on the thread

mjesun · 2018-01-12T13:32:05Z

NVM, just saw the comment (collapsed after a change).

rickhanlonii · 2018-01-12T15:23:46Z

Thanks @SimenB, updated!

If deepCyclicCopy is the preferred way to deep copy I can debug why that isn't working, I just didn't want to spend too much time on it before confirming cc @mjesun

codecov-io · 2018-01-12T15:28:19Z

Codecov Report

Merging #5289 into master will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #5289      +/-   ##
==========================================
+ Coverage   61.22%   61.25%   +0.02%     
==========================================
  Files         205      205              
  Lines        6894     6893       -1     
  Branches        4        3       -1     
==========================================
+ Hits         4221     4222       +1     
+ Misses       2672     2670       -2     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-runner/src/run_test.js	`2.22% <0%> (ø)`	⬆️
packages/jest-runtime/src/index.js	`73.62% <100%> (ø)`	⬆️
packages/jest-util/src/index.js	`100% <100%> (+10.52%)`	⬆️
packages/jest-util/src/create_process_object.js	`100% <100%> (ø)`	⬆️
packages/jest-util/src/deep_cyclic_copy.js	`96.15% <100%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c799a02...704d1aa. Read the comment docs.

rickhanlonii · 2018-01-12T19:07:11Z

@SimenB @mjesun I'm coming up short on tracking down why deepCyclicCopy doesn't work (note that deepcopy also fails). Is there a strong objection to using JSON.parse(JSON.stringify(obj))?

cpojer · 2018-01-12T22:16:27Z

Could you make a gist with the data structures that are being cloned again? The comment from earlier got lost. I'd like to take a look first.

rickhanlonii · 2018-01-12T22:36:44Z

@cpojer here's the gist, the Flow type is defined here

rickhanlonii · 2018-01-12T22:44:33Z

@mjesun is right, the issue with deepCyclicCopy is that the prototype is copied here

I could add a copyPrototype flag to the function (default to true)?

mjesun · 2018-01-12T23:27:21Z

@rickhanlonii Yes, that sounds good to me. To be honest, now that I look at the implementation, we're behaving differently when cloning an Array VS cloning an Object:

Arrays always get instantiated using the constructor from the main context, since we initialize the copy with [].
Objects always get instantiated using their original constructor, no matter what they are.

I would add the flag as you suggested, then correct the Array implementation so when the flag is set to true, we also use Object.create + Object.getPrototypeOf (which is the expected behavior!). The only nit is that I would call the flag keepPrototype, though; since it technically does not get copied, only reused.

I also want to say I'm actually amazed that you found this leak. This is going to hugely improve large test collections, especially the ones relying on jsdom, since the global environment is pretty big for these ones.

rickhanlonii · 2018-01-13T03:28:34Z

Thanks @mjesun!

Updated with the change to use deepCyclicCopy

I didn't update for Object.create(Object.getPrototypeOf(array)) because it threw a type error and I'm pretty sure all arrays have the prototype value of Array.prototype? Also, it changes the semantics of the array with respect to Array.isArray:

mjesun · 2018-01-13T14:50:33Z

Good catch on the Array.isArray behavior, which is, by the way, extremely confusing, because:

Array.isArray called on a subclass of Array returns true (e.g. class Foo extends Array {}, then Array.isArray(new Foo()) returns true), and
Array.isArray(Array.prototype) returns true, and
the accepted polyfill for isArray (({}).toString.call(array) === '[object Array]') would also return true 😅

To avoid the issue, I would suggest making the new Array instance the following way:

const newArray = options.keepPrototype
  ? new (Object.getPrototypeOf(array).constructor)(array.length)
  : [];

This will create a new Array instance using its original constructor, if keepPrototype is set to true.

All arrays have their prototype equal to Array.prototype, but this is only true when being on the same context (which is the eternal problem with Arrays generated inside <IFRAME>s, which then they get evaluated with instanceof Array on the parent frame).

rickhanlonii · 2018-01-13T18:50:08Z

Great point about the iFrame @mjesun

Updated to that version and it works great. For testing, I used array-like objects and mocked Array.isArray. Take a look and let me know if you would recommend a different approach

I also ran the before and after on React to get an idea of the memory improvement here, it's hard to compare but it seems like a pretty big improvement (note that I ran this three times to rule out normal variations, this should be a fair sample)

rickhanlonii · 2018-01-13T19:25:32Z

To anyone following along, here's a good explination of what we're talking about for extending arrays

mjesun · 2018-01-13T20:06:13Z

LGTM! Considering the green check from @SimenB I'll merge it. Thanks a lot!

thymikee · 2018-01-13T20:14:38Z

packages/jest-util/src/__tests__/deep_cyclic_copy.test.js

+  spy.mockRestore();
+});
+
+it('does not keep the prototype of arrays when keepPrototype = false', () => {


This test is a duplicate of above

One asserts the default behavior and the other with the flag explicitly passed in

Do you think that's too much?

I mean its content is literally a copy 😄

Oh wow good catch -- the second test should have:

const copy = deepCyclicCopy(sourceArray, {keepPrototype: false});

thymikee · 2018-01-13T20:15:51Z

packages/jest-util/src/deep_cyclic_copy.js

  const length = array.length;

  cycles.set(array, newArray);

  for (let i = 0; i < length; i++) {
-    newArray[i] = deepCyclicCopy(array[i], EMPTY, cycles);
+    delete options.blacklist;


We should be good moving this out of the loop

Woah, this is a bug -- it deletes the blacklist on the first key iteration so only the first key can be blacklisted

cpojer · 2018-01-14T11:53:36Z

Awesome!

github-actions · 2021-05-12T23:07:20Z

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Fix a leak in coverage

fd40599

facebook-github-bot added the CLA Signed ✔️ label Jan 12, 2018

rickhanlonii commented Jan 12, 2018

View reviewed changes

Fix tests

dfe1cc7

SimenB requested changes Jan 12, 2018

View reviewed changes

rickhanlonii added 2 commits January 12, 2018 10:18

Return undefined instead of null

40a1c86

Update changelog

e803b79

Merge branch 'master' into rh-fix-coverage-leak

423d579

Update to use deepCyclicCopy

01128f0

SimenB approved these changes Jan 13, 2018

View reviewed changes

Respect keepPrototype option for arrays

704d1aa

mjesun merged commit 6d2394f into jestjs:master Jan 13, 2018

thymikee reviewed Jan 13, 2018

View reviewed changes

rickhanlonii deleted the rh-fix-coverage-leak branch January 13, 2018 21:07

renovate-tests mentioned this pull request Feb 6, 2018

Update dependency jest to v22 - autoclosed renovate-tests/changelog-md#3

Closed

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a leak in coverage #5289

Fix a leak in coverage #5289

rickhanlonii commented Jan 12, 2018

rickhanlonii Jan 12, 2018

rickhanlonii Jan 12, 2018

mjesun Jan 12, 2018

mjesun Jan 12, 2018

rickhanlonii Jan 12, 2018

SimenB left a comment

mjesun commented Jan 12, 2018

mjesun commented Jan 12, 2018

rickhanlonii commented Jan 12, 2018

codecov-io commented Jan 12, 2018 •

edited

Loading

rickhanlonii commented Jan 12, 2018 •

edited

Loading

cpojer commented Jan 12, 2018

rickhanlonii commented Jan 12, 2018

rickhanlonii commented Jan 12, 2018

mjesun commented Jan 12, 2018 •

edited

Loading

rickhanlonii commented Jan 13, 2018 •

edited

Loading

mjesun commented Jan 13, 2018 •

edited

Loading

rickhanlonii commented Jan 13, 2018 •

edited

Loading

rickhanlonii commented Jan 13, 2018

mjesun commented Jan 13, 2018

thymikee Jan 13, 2018

rickhanlonii Jan 13, 2018

thymikee Jan 13, 2018

rickhanlonii Jan 13, 2018

thymikee Jan 13, 2018

rickhanlonii Jan 13, 2018

cpojer commented Jan 14, 2018

github-actions bot commented May 12, 2021

Fix a leak in coverage #5289

Fix a leak in coverage #5289

Conversation

rickhanlonii commented Jan 12, 2018

Summary

Details

Dump Before (without coverage)

Dump After (with coverage)

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

mjesun commented Jan 12, 2018

mjesun commented Jan 12, 2018

rickhanlonii commented Jan 12, 2018

codecov-io commented Jan 12, 2018 • edited Loading

Codecov Report

rickhanlonii commented Jan 12, 2018 • edited Loading

cpojer commented Jan 12, 2018

rickhanlonii commented Jan 12, 2018

rickhanlonii commented Jan 12, 2018

mjesun commented Jan 12, 2018 • edited Loading

rickhanlonii commented Jan 13, 2018 • edited Loading

mjesun commented Jan 13, 2018 • edited Loading

rickhanlonii commented Jan 13, 2018 • edited Loading

rickhanlonii commented Jan 13, 2018

mjesun commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 14, 2018

github-actions bot commented May 12, 2021

codecov-io commented Jan 12, 2018 •

edited

Loading

rickhanlonii commented Jan 12, 2018 •

edited

Loading

mjesun commented Jan 12, 2018 •

edited

Loading

rickhanlonii commented Jan 13, 2018 •

edited

Loading

mjesun commented Jan 13, 2018 •

edited

Loading

rickhanlonii commented Jan 13, 2018 •

edited

Loading