Skip to content

net: remove redundant code from _writeGeneric()#18429

Closed
lpinca wants to merge 1 commit intonodejs:masterfrom
lpinca:remove/redundant-code
Closed

net: remove redundant code from _writeGeneric()#18429
lpinca wants to merge 1 commit intonodejs:masterfrom
lpinca:remove/redundant-code

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

mcollina commented Jan 29, 2018

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

mcollina commented Jan 30, 2018

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018

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

@lpinca
Copy link
Member Author

lpinca commented Mar 20, 2018 •
edited
Loading

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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.

8 participants

@lpinca @mcollina @MylesBorins @jasnell @addaleax @cjihrig @BridgeAR @nodejs-github-bot