andywer / leakage

🐛 Memory leak testing for node.
1.58k stars 52 forks source link

Document best practices: console.log #18

Closed mjhea0 closed 7 years ago

mjhea0 commented 7 years ago

Thoughts on adding a few best practices around garbage collection. Adding a console.log for instance...

describe.only('test()', () => {
  it('does not leak when doing stuff', () => {
    iterate(6, () => { const test = new Person(); console.log(test); });
  });
});

function Person() {
  this.age = 1;
  this.name = 'mike';
}

vs

describe.only('test()', () => {
  it('does not leak when doing stuff', () => {
    iterate(6, () => { const test = new Person();  });
  });
});

function Person() {
  this.age = 1;
  this.name = 'mike';
}
andywer commented 7 years ago

Hey @mjhea0.

Adding "best practices" to the documentation is a good point! Concerning your example above... I haven't yet tried the code, but the console.log(test) should not introduce a leak or does it?

mjhea0 commented 7 years ago

it prevents garbage collection. i don't think you can consider it a leak, but it does throw an error

andywer commented 7 years ago

I don't quite get why, though, to be honest. Why should console.log() keep a reference to the object after printing it? 🤔

But good to know. Thanks! Any other best practice proposals? 😉

mjhea0 commented 7 years ago

http://stackoverflow.com/a/28859493/1799408

I imagine the same thing happens in node since it uses v8.

andywer commented 7 years ago

Thanks for the link!

Even an explicit delete test after console.log(test) does not seem to help... (My bad. delete does not mark the object for GC in JS at all; it's really just for removing props from an object)

mjhea0 commented 7 years ago

firefox does the same. must be v8 and spidermonkey just keeping a reference for debugging purposes. another reason to git rid of those nasty console.logs!

mjhea0 commented 7 years ago

Probably a good idea to add in the docs that leakage tests should be kept separate from your normal test suite. They took like triple amount of the time to run!

andywer commented 7 years ago

Good point! Got to open another issue and make a list...

andywer commented 7 years ago

Closing this as there is now #19. Let's collect ideas there :)