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.

Sign up for GitHub

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

Jump to bottom

net: remove redundant code from _writeGeneric() #18429

Closed
wants to merge 1 commit into from
Closed

net: remove redundant code from _writeGeneric() #18429

wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jan 29, 2018

The encoding is already handled by Writable.prototype.write().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

net

@lpinca
net: remove redundant code from _writeGeneric()
09ccdff 
The encoding is already handled by `Writable.prototype.write()`.
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jan 29, 2018
@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/12803/

mcollina
mcollina approved these changes Jan 29, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cjihrig
cjihrig approved these changes Jan 29, 2018
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We might want to run CITGM though, in case any popular modules are calling _writeGeneric() directly.

@lpinca
Copy link
Member Author

lpinca commented Jan 29, 2018

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1243/

@lpinca
Copy link
Member Author

lpinca commented Jan 30, 2018

I can't spot any noticeable difference from the previous CITGM run.

On a side note: I noticed that Socket.prototpe._writeGeneric() is a "hot" function profiling a basic net echo server (10k roundtrips of 64 B - binary data). Instantiating a WriteWrap is expensive but I'm not sure if it's that or if we can do anything about this.

@mcollina
Copy link
Member

On a side note: I noticed that Socket.prototpe._writeGeneric() is a "hot" function profiling a basic net echo server (10k roundtrips of 64 B - binary data). Instantiating a WriteWrap is expensive but I'm not sure if it's that or if we can do anything about this.

_writeGeneric is one of hottest functions in core. I think part of the problem is it being 'generic', as it's in the path for both _write and _writev flows. This function has to instantiate a WriteWrap.

@lpinca
Copy link
Member Author

lpinca commented Jan 30, 2018

Yes I think that instantiating WriteWrap is what makes it slow but I may be wrong.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
@lpinca
Copy link
Member Author

lpinca commented Feb 3, 2018

Landed in f8fda0d.

lpinca added a commit that referenced this pull request Feb 3, 2018
@lpinca
net: remove redundant code from _writeGeneric()
f8fda0d 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@lpinca lpinca closed this Feb 3, 2018
@lpinca lpinca deleted the remove/redundant-code branch February 3, 2018 15:22
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
d737972 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
8d80974 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
958f5ed 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins
Copy link
Member

@lpinca does this apply to 6.x or 8.x?

@lpinca
Copy link
Member Author

lpinca commented Mar 20, 2018 •
edited

Yes, it applies to both 6.x and 8.x. Let me know if it doesn't apply cleanly.

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
7c4adca 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
fd6cd46 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
5dade66 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
af3e5f2 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
269c2f3 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@lpinca @MylesBorins
net: remove redundant code from _writeGeneric()
35aaee1 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: #18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
Merged
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@lpinca @MayaLekova
net: remove redundant code from _writeGeneric()
8dbde54 
The encoding is already handled by `Writable.prototype.write()`.

PR-URL: nodejs#18429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@lpinca @mcollina @MylesBorins @jasnell @addaleax @cjihrig @BridgeAR @nodejs-github-bot