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

dgram: refactor dgram to module.exports #11696

Conversation

claudiorodriguez
Copy link
Contributor

Refactor dgram module to use the more efficient
module.exports = {} pattern.

See #11611

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

dgram

Refactor dgram module to use the more efficient
module.exports = {} pattern.
@claudiorodriguez claudiorodriguez added the dgram Issues and PRs related to the dgram subsystem / UDP. label Mar 5, 2017
@ronkorving
Copy link
Contributor

ronkorving commented Mar 6, 2017

What's more efficient about this? I can see the benchmark, but are you saying every time we access one of the exported properties we take a perf hit? Even if so, this is so not on any hot code path, I see little point in doing this. Even if we did change this, we would have to change all built-in modules to follow that pattern. Has anyone discussed doing that yet?

@TimothyGu
Copy link
Member

@ronkorving, even though no mass migration has happened yet, patches like 2298bc4, 62e9609, 531de63, 804bb22, b610a4d have made it an established pattern. It is more efficient because it ensures that V8 will use "fast" properties as opposed to hashtable-based "slow" properties.

@TimothyGu
Copy link
Member

@ronkorving
Copy link
Contributor

@TimothyGu Thank you, I wasn't aware this was already ongoing. LGTM 👍

@joyeecheung
Copy link
Member

@TimothyGu I think those are already fast properties without being exported this way? See #11430

@fhinkel
Copy link
Member

fhinkel commented Mar 9, 2017

Thanks! Landed in a9e64a8.

@fhinkel fhinkel closed this Mar 9, 2017
fhinkel pushed a commit that referenced this pull request Mar 9, 2017
Refactor dgram module to use the more efficient
module.exports = {} pattern.

PR-URL: #11696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
Refactor dgram module to use the more efficient
module.exports = {} pattern.

PR-URL: nodejs#11696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Refactor dgram module to use the more efficient
module.exports = {} pattern.

PR-URL: nodejs#11696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
@MylesBorins
Copy link
Contributor

should we backport?

@MylesBorins
Copy link
Contributor

I've labelled this as don't land. I believe that these optimizations were made with turbofan in mind.

@jasnell
Copy link
Member

jasnell commented May 15, 2017

-1 on backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants