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

Debugging: name every function #8913

Closed
Raynos opened this issue Oct 3, 2016 · 56 comments
Closed

Debugging: name every function #8913

Raynos opened this issue Oct 3, 2016 · 56 comments
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem.

Comments

@Raynos
Copy link
Contributor

Raynos commented Oct 3, 2016

  • Version: 4.4.7
  • Platform: linux
  • Subsystem: http

There are too many anonymous functions in the source code which makes heap debugging frustrating

This once('response') listener ( https://github.com/nodejs/node/blob/master/lib/_http_client.js#L235-L237 ) is anonymous.

When I try to debug why I am leaking response listeners in a heap snapshot

image

I see that the listener in the once closure is function () {} which gives me no information. I strongly suspect that it's the abort listener but i have no evidence for it.

There are many, many, many anonymous functions in node core, there should be zero.

@Raynos
Copy link
Contributor Author

Raynos commented Oct 3, 2016

This applies to 4.x & master. I did not check 6.x, I assume 6.x and master are the same.

@mscdex mscdex added http Issues or PRs related to the http subsystem. good first issue Issues that are suitable for first-time contributors. labels Oct 3, 2016
@Fishrock123
Copy link
Contributor

We can't name arrow functions though. Since those are done for perf reasons you'd still need to live with them... :/

@fl0w
Copy link

fl0w commented Oct 6, 2016

@Fishrock123 out of curiosity (pardon if this venue isn't suited for inquiry); pre-assigning the arrow function before usage does retain the naming while --inspect:ing - is what you're referring to not to do due to performance hits?

@Fishrock123
Copy link
Contributor

@fl0w you mean assigning to a variable means you get the variable name? You can't assign names to arrow functions like to regular functions though.

@addaleax
Copy link
Member

addaleax commented Oct 6, 2016

const a = () => 42;
a.name; // returns 'a'

works since V8 5.1, so that should be possible.

@fl0w
Copy link

fl0w commented Oct 6, 2016

@Fishrock123 @addaleax answered before me - I was wondering if that was an overhead you were referring to in your original comment.

koajs/koa#805 (comment) for pictures

@Raynos
Copy link
Contributor Author

Raynos commented Oct 6, 2016

@Fishrock123

Do you have a link to a demonstration that arrows are more performant. I suspect named function declarations would be more performant then arrow functions.

@bnoordhuis
Copy link
Member

V8 doesn't have to worry about super and new.target when emitting code for arrow functions.

It's a minor thing and probably not much of an issue once the function makes it to the optimizing tier but it means that core should lean towards arrow functions, all other things being equal.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 7, 2016

@Raynos anywhere where bind/this passing was/is necessary.

Other than that, they are only marginally different like @bnoordhuis said. The best rule of thumb is use them where you need the lexical this and regular functions elsewhere. (Which is exactly what we do.)

@Raynos
Copy link
Contributor Author

Raynos commented Oct 7, 2016

Thanks for input @Fishrock123 @bnoordhuis Agreed that bind is bad.

I wrote a quick benchmark ( https://gist.github.com/Raynos/93d275463a90306b4b0779fed308550c ).
I ran the benchmark with node 6.4.0 & v8 5.0.71 and performance of both arrows and closures is identical.

Having a named function declaration;

var self = this;
function someName() {
  self.wat()
}

Instead of () => self.wat() will still improve heap debugging.

I suspect using a technique that improves heap introspection is more valuable then saving a few lines of code. Especially if the allocation and calling performance of both is the same.

@bnoordhuis
Copy link
Member

In your benchmark the arrow/non-arrow functions are optimized almost right from the start but that's not very representative of long tail code (code that gets called periodically but not frequently enough to get optimized.)

var self = this is something I definitely recommend against. () => this.wat() captures just the lexical this but the function in your example can over-capture the enclosing lexical scope due to how V8 implements closures. If you had a var big = Buffer(1 << 28) in there, it could stay alive as long as the function.

It's not a theoretical concern either. Over the years I've fixed several memory leaks in core that were the result of over-capturing.

@solebox
Copy link
Contributor

solebox commented Oct 8, 2016

can i give it a go?
im new to node but not js and this looks like a good way for me to learn node's innards :)

Imgur

@addaleax
Copy link
Member

addaleax commented Oct 8, 2016

@soleboxy I’d suggest picking a file in lib/, doing it, opening a PR and seeing how that goes. A single pull request for all functions across all JS files would basically be a recipe for conflicts. If you’re unsure about anything, feel free to ask here or in #node-dev on Freenode! :)

@solebox
Copy link
Contributor

solebox commented Oct 8, 2016

k thanks 👍

@Raynos
Copy link
Contributor Author

Raynos commented Oct 9, 2016

@bnoordhuis Over capturing closure variables that leads to leak is a great point. I'm convinced arrow functions are worth using for that alone. Thanks for explaining the details.

@tylerbrazier
Copy link
Contributor

I can take lib/console.js for this

@addaleax
Copy link
Member

@trivikr I think a lot of the PRs currently being opened don’t actually improve the debugging situation – when a function is stored on a property on some object, the engine infers the name from it properly anyway…

@Trott
Copy link
Member

Trott commented Jul 11, 2018

I checked off punycode.js because it's vendored in and not something we should patch in our source for this. If someone wants to open a PR upstream for it, the repository is https://github.com/bestiejs/punycode.js.

@BridgeAR
Copy link
Member

I agree with @addaleax and I think our situation improved significantly since this issue was opened. Not only have we named almost all functions throughout the code but the compiler will now infer the the names very well by now.

Therefore I am going to close this as resolved. If someone disagrees, please feel free to reopen. However, even if this is closed, it should of course not keep someone from opening a PR that provides names to functions that can not be inferred.

trivikr pushed a commit that referenced this issue Jul 18, 2018
Refs: #8913

PR-URL: #21755
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
trivikr pushed a commit that referenced this issue Jul 18, 2018
Refs: #8913

PR-URL: #21754
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jul 18, 2018
PR-URL: nodejs#21753
Refs: nodejs#8913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jul 18, 2018
PR-URL: nodejs#21756
Refs: nodejs#8913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this issue Jul 19, 2018
Refs: #8913

PR-URL: #21755
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue Jul 19, 2018
Refs: #8913

PR-URL: #21754
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
targos pushed a commit that referenced this issue Jul 19, 2018
PR-URL: #21753
Refs: #8913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue Jul 19, 2018
PR-URL: #21756
Refs: #8913
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Trott pushed a commit to mlrv/node that referenced this issue Aug 1, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

Refs: nodejs#8913
maclover7 pushed a commit that referenced this issue Aug 4, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

PR-URL: #21412
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Refs: #8913
targos pushed a commit that referenced this issue Aug 6, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

PR-URL: #21412
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Refs: #8913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests