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

lib: refactor some lib/internals #11406

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@jasnell
Member

jasnell commented Feb 15, 2017

Some light refactoring of internals/*.js

  • For linkedlist and freelist, this yields about a 5% perf improvement in general
  • Consistent use of the module.exports = {} pattern, with module.exports = {} at the end.

(do not squash commits)

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

lib/internal

Show outdated Hide outdated lib/internal/freelist.js
alloc() {
return this.list.length ? this.list.pop() :
this.constructor.apply(this, arguments);

This comment has been minimized.

@mscdex

mscdex Feb 15, 2017

Contributor

Unrelated to this PR, but perhaps this can be simplified since http is the only user of FreeList and we'd know how what arguments to pass on to the constructor. I'm not sure if adding new would help (allowing us to avoid an instanceof check in the constructor if possible) more than it might hurt (e.g. if stack traces would change with new constructor(..)).

@mscdex

mscdex Feb 15, 2017

Contributor

Unrelated to this PR, but perhaps this can be simplified since http is the only user of FreeList and we'd know how what arguments to pass on to the constructor. I'm not sure if adding new would help (allowing us to avoid an instanceof check in the constructor if possible) more than it might hurt (e.g. if stack traces would change with new constructor(..)).

This comment has been minimized.

@jasnell

jasnell Feb 15, 2017

Member

Worth playing around with. I would also note that I had attempted to replace this use of arguments with ...args, but found that in the case there are no arguments passed to the alloc(), it caused a whopping 48% slowdown. Definitely something we'll need to keep in mind.

@jasnell

jasnell Feb 15, 2017

Member

Worth playing around with. I would also note that I had attempted to replace this use of arguments with ...args, but found that in the case there are no arguments passed to the alloc(), it caused a whopping 48% slowdown. Definitely something we'll need to keep in mind.

Show outdated Hide outdated lib/internal/net.js
@@ -11,8 +9,12 @@ function isLegalPort(port) {
return +port === (+port >>> 0) && port <= 0xFFFF;
}

This comment has been minimized.

@mscdex

mscdex Feb 15, 2017

Contributor

Unintended?

@mscdex

mscdex Feb 15, 2017

Contributor

Unintended?

Show outdated Hide outdated lib/internal/freelist.js
class FreeList {
constructor(name, max, constructor) {
this.name = name;
this.constructor = constructor;

This comment has been minimized.

@addaleax

addaleax Feb 16, 2017

Member

Naming own properties of instances constructor is kind of confusing, maybe you could change that while you’re here?

@addaleax

addaleax Feb 16, 2017

Member

Naming own properties of instances constructor is kind of confusing, maybe you could change that while you’re here?

Show outdated Hide outdated lib/internal/socket_list.js
constructor(slave, key) {
super();
this.key = key;
this.slave = slave;

This comment has been minimized.

@addaleax

addaleax Feb 16, 2017

Member

Likewise, it would be awesome if we could change this during the refactor (here and in the other parts of the file this PR touches).

@addaleax

addaleax Feb 16, 2017

Member

Likewise, it would be awesome if we could change this during the refactor (here and in the other parts of the file this PR touches).

jasnell added some commits Feb 15, 2017

lib: more consistent use of module.exports = {} model
Switch to using the more efficient module.exports = {}
where possible.
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 16, 2017

Member

@addaleax @mscdex ... updated!

Member

jasnell commented Feb 16, 2017

@addaleax @mscdex ... updated!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 16, 2017

Member

semver-major because an error message is changed.

Member

jasnell commented Feb 16, 2017

semver-major because an error message is changed.

@targos

targos approved these changes Feb 18, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Feb 22, 2017

lib: refactor internal/linkedlist
PR-URL: #11406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell added a commit that referenced this pull request Feb 22, 2017

lib: refactor internal/freelist
PR-URL: #11406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell added a commit that referenced this pull request Feb 22, 2017

lib: refactor internal/socket_list
PR-URL: #11406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell added a commit that referenced this pull request Feb 22, 2017

lib: more consistent use of module.exports = {} model
Switch to using the more efficient module.exports = {}
where possible.

PR-URL: #11406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Feb 22, 2017

Landed in d61a511...62e9609

@jasnell jasnell closed this Feb 22, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

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