Skip to content

Fix memory leak when creating debug instances#740

Merged
Qix- merged 3 commits into
masterfrom
no-leak
Sep 19, 2020
Merged

Fix memory leak when creating debug instances#740
Qix- merged 3 commits into
masterfrom
no-leak

Conversation

@Qix-

@Qix- Qix- commented Jan 12, 2020

Copy link
Copy Markdown
Member

Closes #678 .

One possible solution to the memory leak and needless .destroy() method.

Will leave open for a while to allow people to test.

@Qix- Qix- added bug This issue identifies a malfunction change-minor This proposes or provides a change that requires a minor release labels Jan 12, 2020
@coveralls

coveralls commented Jan 12, 2020

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.6%) to 89.338% when pulling a59b635 on no-leak into 22e13fe on master.

@ibc

ibc commented Jan 12, 2020

Copy link
Copy Markdown
Contributor

It works fine :)

Will you release this in v4?

@Qix-

Qix- commented Jan 12, 2020

Copy link
Copy Markdown
Member Author

Yes, this would be a minor release if pushed.

@ibc

ibc commented Jan 12, 2020

Copy link
Copy Markdown
Contributor

"If pushed"? :)

What does it depend on? I've already changed my projects 3 times today to conform to this issue :)

@Qix-

Qix- commented Jan 12, 2020

Copy link
Copy Markdown
Member Author

@ibc

image

Making sure I didn't make a mistake and break millions of users. Don't expect this to be released in the coming week. For comparison, this is roughly 10x more than React.

You can replace your package.json entry with "debug": "visionmedia/debug#no-leak" if you'd like to test.

@ibc

ibc commented Jan 12, 2020

Copy link
Copy Markdown
Contributor

Sure, thanks.

@iFwu

iFwu commented Feb 21, 2020

Copy link
Copy Markdown

I have encountered this problem in the production environment. So I sincerely hope this can be merged ASAP.

@ibc

ibc commented Apr 5, 2020

Copy link
Copy Markdown
Contributor

Which is the state of this PR? is it expected to be merged soon? I already tested it and works fine, I understand it's not trivial to move this into production release but, what is the current state or plan?

@Qix-

Qix- commented Apr 6, 2020

Copy link
Copy Markdown
Member Author

Which is the state of this PR?

image

@ibc

ibc commented Apr 6, 2020

Copy link
Copy Markdown
Contributor

Thanks, somehow I already see it's still open :)
I just mean, is there any ETA or plan to merge it?

@ericf89

ericf89 commented Jun 4, 2020

Copy link
Copy Markdown

Just another follow up that the no-leak branch solved some memory issues I was seeing with a project.

@jlalmes

jlalmes commented Sep 19, 2020

Copy link
Copy Markdown

Another follow up - This branch has just fixed the memory leak that's been haunting me for the past 6 months

@Qix- Qix- merged commit e2d3bc9 into master Sep 19, 2020
@Qix- Qix- deleted the no-leak branch September 19, 2020 08:35
@Qix-

Qix- commented Sep 19, 2020

Copy link
Copy Markdown
Member Author

Published as 4.3.0 under tag beta (therefore it's not going to be automatically pulled unless you explicitly ask for it). Please let me know that it still works for you.

@ab-pm

ab-pm commented Oct 30, 2020

Copy link
Copy Markdown

Fixing memory leaks is of course important, but I wonder whether this merge is a performance regression. Has it been tested?
Disabled debug instances previously just checked a single boolean property and then returned early, executing an object lookup and a single branching. Now, they are calling a getter, which in turn calls createDebug.enabled, which loops through all the enabled and disabled namespaces and runs a regex test on them. This seems much costlier, or am I missing something?

@Qix-

Qix- commented Oct 30, 2020

Copy link
Copy Markdown
Member Author

@ab-pm You're exactly right, good eye. It's a well known issue that I've been thinking about since I started maintaining this library. v5 is going to be a rewrite that will improve this a bit.

debug has always been a performance suck, though. Even if the debug namespace is disabled, the parameters are still evaluated going into it. In trivial cases this is fine, but I know for a fact many people are passing complicated expressions to debug(). Further, Javascript engines do not have dead code removal advanced enough to be able to determine that pure expressions passed to a no-op function can be elided to save on cycles.

As for whether or not it's a "performance regression", by this logic anything that adds an extra instruction to the overall code path is a "performance regression". The trade-off here is that debug was not correct and leaked memory.

The real fix here would be to overhaul the enabled namespaces system altogether, which would be a major breaking change and is thorougly discussed elsewhere on the issue tracker. As I mentioned before, it's slated for v5, whenever I get around to that.

@ab-pm

ab-pm commented Oct 30, 2020

Copy link
Copy Markdown

@qix Well it's not just "an extra instruction", but quite many of them, on what might be a hot code path :-) But if you've consciously made the trade-off to favour memory safety, I'm totally fine with this - that's all I wanted to know, thanks!

@cmotsn

cmotsn commented Jan 13, 2023

Copy link
Copy Markdown

Would a backport of this memory fix leak to 3.x.x version be possible? Some packages do not want to upgrade to debug:4.x in order to not lose compatibility with earlier-but-apparently-still-used versions of Node.js...

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

Labels

bug This issue identifies a malfunction change-minor This proposes or provides a change that requires a minor release

Development

Successfully merging this pull request may close these issues.

memory leak when instance is created inside a function.

9 participants