stream: improve stream creation performance by mscdex · Pull Request #19401 · nodejs/node · GitHub
Skip to content

stream: improve stream creation performance#19401

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:perf-stream-Writable-creation
Closed

stream: improve stream creation performance#19401
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:perf-stream-Writable-creation

Conversation

@mscdex

@mscdex mscdex commented Mar 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor 
                                                 confidence improvement accuracy (*)   (**)  (***)
 streams/creation.js kind='duplex' n=50000000          ***     16.72 %       ±0.83% ±1.11% ±1.45%
 streams/creation.js kind='readable' n=50000000        ***     17.56 %       ±0.91% ±1.21% ±1.58%
 streams/creation.js kind='transform' n=10000000       ***     12.23 %       ±1.29% ±1.72% ±2.27%
 streams/creation.js kind='writable' n=50000000        ***     18.11 %       ±0.86% ±1.14% ±1.48%

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@mscdex mscdex added stream Issues and PRs related to the stream subsystem. performance Issues and PRs related to the performance of Node.js. labels Mar 17, 2018
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Mar 17, 2018
@jasnell jasnell requested a review from mcollina March 17, 2018 16:48
mcollina
mcollina approved these changes Mar 17, 2018
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Nice technique, I've tried to speed this up once and failed.

Do you think we could something similar for Readable?

@mscdex

mscdex commented Mar 18, 2018

Copy link
Copy Markdown
Contributor Author

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2018
@lpinca

lpinca commented Mar 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Tiny nit: subsystem in commit title should be "stream" or at least that seem more common than "streams"

@mscdex mscdex added the wip Issues and PRs that are still a work in progress. label Mar 18, 2018
@mscdex mscdex force-pushed the perf-stream-Writable-creation branch from af767c4 to 6ba37f5 Compare March 21, 2018 08:57
@mscdex mscdex removed the wip Issues and PRs that are still a work in progress. label Mar 21, 2018
@mscdex

mscdex commented Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Added improvement for stream.Readable.

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

@mscdex mscdex changed the title streams: improve Writable creation performance stream: improve stream creation performance Mar 21, 2018
@mcollina

mcollina commented Mar 21, 2018

Copy link
Copy Markdown
Member

This is superb. LGTM.

I expect the advantage to be consistent also in previous version with turbofan, so we should think about measuring and backporting to 8 as well.

@lpinca

lpinca commented Mar 21, 2018

Copy link
Copy Markdown
Member

@mcollina you mean v6.x not v8.x right?

@mcollina

mcollina commented Mar 21, 2018

Copy link
Copy Markdown
Member

I mean, we should look to backport it to 8.x. I wouldn't backport it to 6.x

@addaleax

addaleax commented Mar 23, 2018

Copy link
Copy Markdown
Member

@addaleax addaleax closed this Mar 23, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2018
addaleax pushed a commit that referenced this pull request Mar 23, 2018
@mscdex @addaleax
stream: improve stream creation performance
b41ed29 
PR-URL: #19401
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@mscdex mscdex deleted the perf-stream-Writable-creation branch March 23, 2018 18:42
targos pushed a commit that referenced this pull request Mar 27, 2018
@mscdex @targos
stream: improve stream creation performance
f13cc32 
PR-URL: #19401
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
targos added a commit that referenced this pull request Mar 27, 2018
@targos
2018-03-27, Version 9.10.0 (Current)
0a18bf1 
Notable changes:

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    #19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    #17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    #19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    #19520
* stream:
  - Improve stream creation performance (Brian White)
    #19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@targos @MylesBorins
2018-03-28, Version 9.10.0 (Current)
80310e9 
This is a security release. All Node.js users should consult the
security release summary at:

https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/

for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

* CVE-2018-7158
* CVE-2018-7159
* CVE-2018-7160

Notable changes:

* Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that
  are known to impact Node.js.
* **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**:
  A malicious website could use a DNS rebinding attack to trick a web
  browser to bypass same-origin-policy checks and allow HTTP
  connections to localhost or to hosts on the local network,
  potentially to an open inspector port as a debugger, therefore
  gaining full code execution access. The inspector now only allows
  connections that have a browser `Host` value of `localhost` or
  `localhost6`.
* **Fix for `'path'` module regular expression denial of service
  (CVE-2018-7158)**: A regular expression used for parsing POSIX an
  Windows paths could be used to cause a denial of service if an
  attacker were able to have a specially crafted path string passed
  through one of the impacted `'path'` module functions.
* **Reject spaces in HTTP `Content-Length` header values
  (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside
  `Content-Length` header values. Such values now lead to rejected
  connections in the same way as non-numeric values.
* **Update root certificates**: 5 additional root certificates have
  been added to the Node.js binary and 30 have been removed.

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    #19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    #17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    #19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    #19520
* stream:
  - Improve stream creation performance (Brian White)
    #19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs

PR-URL: https://github.com/nodejs-private/node-private/pull/111
Pranay180420 pushed a commit to Pranay180420/Node.jsforme that referenced this pull request Feb 17, 2025
@targos @MylesBorins
2018-03-28, Version 9.10.0 (Current)
c3c9984 
This is a security release. All Node.js users should consult the
security release summary at:

https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/

for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

* CVE-2018-7158
* CVE-2018-7159
* CVE-2018-7160

Notable changes:

* Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that
  are known to impact Node.js.
* **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**:
  A malicious website could use a DNS rebinding attack to trick a web
  browser to bypass same-origin-policy checks and allow HTTP
  connections to localhost or to hosts on the local network,
  potentially to an open inspector port as a debugger, therefore
  gaining full code execution access. The inspector now only allows
  connections that have a browser `Host` value of `localhost` or
  `localhost6`.
* **Fix for `'path'` module regular expression denial of service
  (CVE-2018-7158)**: A regular expression used for parsing POSIX an
  Windows paths could be used to cause a denial of service if an
  attacker were able to have a specially crafted path string passed
  through one of the impacted `'path'` module functions.
* **Reject spaces in HTTP `Content-Length` header values
  (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside
  `Content-Length` header values. Such values now lead to rejected
  connections in the same way as non-numeric values.
* **Update root certificates**: 5 additional root certificates have
  been added to the Node.js binary and 30 have been removed.

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    nodejs/node#19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    nodejs/node#17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    nodejs/node#19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    nodejs/node#19520
* stream:
  - Improve stream creation performance (Brian White)
    nodejs/node#19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs

PR-URL: https://github.com/nodejs-private/node-private/pull/111
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
@lpinca lpinca lpinca approved these changes
@JungMinu JungMinu JungMinu approved these changes
+1 more reviewer
@TimothyGu TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@mscdex @lpinca @mcollina @addaleax @jasnell @TimothyGu @JungMinu @nodejs-github-bot