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

stream: improve stream creation performance #19401

Closed
wants to merge 1 commit into from
Closed

stream: improve stream creation performance #19401

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Mar 17, 2018 •
edited
Loading 

                                                 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
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.

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

Do you think we could something similar for Readable?

@mscdex
Copy link
Contributor Author

mscdex commented Mar 18, 2018

Do you think we could something similar for Readable?

Not the exact same thing since Readable never does instanceof Stream.Duplex more than once, but I do have something else that may help...

@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
Copy link
Member

lpinca commented Mar 18, 2018 •
edited
Loading

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
Copy link
Contributor Author

mscdex commented Mar 21, 2018

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
Copy link
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
Copy link
Member

lpinca commented Mar 21, 2018

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

@mcollina
Copy link
Member

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

@addaleax
Copy link
Member

Landed in b41ed29

@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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JungMinu JungMinu JungMinu approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

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