stream,zlib: performance improvements by mscdex · Pull Request #13322 · nodejs/node · GitHub

This PR brings various performance improvements to stream.Transform and zlib. A good chunk of the performance increases come from the inlining of stream.Transform state initialization during stream instantiation. The rest of the increases come from changes to zlib itself.

Here is an overview of the notable zlib-specific changes:

Skipped the more complex machinery used for exported constructors (to allow new and without) when calling the zlib.create*() methods
Improved consistency in zlib parameter value validations
Used a typed array for more efficiently returning results from the C++ layer
Avoided direct calls to assert() by putting them behind an if containing their conditional
Avoided redundant handle assertions
Removed checks for impossible situations (e.g. false-y or non-Buffer/TypedArray chunk values)
- objectMode/writableObjectMode and/or encoding are now explicitly overridden if set to non-defaults in options passed to constructors
Async-specific:
- Stream-specific:
  - Moved write() callback out of _transform() for better reuse by other instances and removed the sync-specific code
  - Stored write() callback at the C++ layer for faster invocation (avoiding a dynamic lookup on the handle's JS object for every write())
  - Avoided re-assigning the same chunk value when multiple write()s are needed for the same chunk
- Convenience functions:
  - Moved event handlers outside of zlibBuffer() helper function for better reusability
  - Switched to simpler 'data' event listening instead of stream.read()
  - Avoided Buffer.concat() for single Buffer case
Sync-specific:
- Avoided Buffer.concat() for single Buffer case
- Copied common and sync-specific parts from callback() previously shared by both sync and async functionality

Here are some results with the included benchmarks:

                                                            improvement confidence      p.value
streams/transform-creation.js n=1000000                       497.78 %        *** 9.135161e-41
zlib/creation.js n=1000000 options="false" type="Deflate"      18.57 %        *** 2.157176e-45
zlib/creation.js n=1000000 options="true" type="Deflate"       18.80 %        *** 4.619755e-44
zlib/deflate.js n=400000 inputLen=1024 method="createDeflate"   6.57 %        *** 3.938906e-17
zlib/deflate.js n=400000 inputLen=1024 method="deflateSync"    27.62 %        *** 1.717435e-37
zlib/deflate.js n=100000 inputLen=1024 method="deflate"        29.41 %        *** 4.915297e-27

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

Checklist

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

Affected core subsystem(s)

stream
zlib

Optional, but given the extent of the changes, it would be good to run a test coverage report and make sure this doesn't decrease coverage for lib/zlib.js etc. (and if it does, then hopefully add some tests to fully cover the new code).

LGTM for the streams parts.

would you mind adding a comment that this matches both null and undefined, and it is done on purpose?

Comments added.

As any changes in streams, a run through CITGM would be nice.

why not Object.assign ? still too slow?

Yes.

any numbers?

I don't have any handy, no. We could always flip it to be a whitelist in the future I suppose if we run into issues. Using a whitelist would allow us to just specify an object inline and avoid any expensive copying-related operations.

This needs a rebase.

Rebased.

can we try to use bind here? It might be slightly faster as we don't keep the context alive.

As discussed and checked with @mcollina, this can be really fast using bind, especially once this CL lands in V8. The CL should apply cleanly to 6.0, but also 5.9.

Here's a simple micro-benchmark:

function bar() {}

function A() { this.x = (x) => bar(x); }

function Ab() { this.x = bar.bind(undefined); }

function c() { return new A(); }

function cb() { return new Ab(); }

function l() { return {x: (x) => bar(x)}; }

function lb() { return {x: bar.bind(undefined)}; }

const N = 10000000;

function test(fn, n) {
	for (var i = 0; i < n; ++i) {
		fn.call(this);
	}
}

const FNS = [c, cb, l, lb];

for (const fn of FNS) {
	test(fn, 100);
}

for (const fn of FNS) {
	console.time(fn.name);
	test(fn, N);
	console.timeEnd(fn.name);
}

I think we should see if you can bring those improvements also to _readableState and _writableState, as allocating those are a hot path whenever using node streams.

I just copied the state object as-is without really paying attention. I will look into adding bind()...

I've now incorporated bind() and it doesn't seem to have negatively affected performance, so it's fine.

As discussed and checked with @mcollina, this can be really fast using bind, especially once this CL lands in V8. The CL should apply cleanly to 6.0, but also 5.9.

Here's a simple micro-benchmark:

function bar() {}

function A() { this.x = (x) => bar(x); }

function Ab() { this.x = bar.bind(undefined); }

function c() { return new A(); }

function cb() { return new Ab(); }

function l() { return {x: (x) => bar(x)}; }

function lb() { return {x: bar.bind(undefined)}; }

const N = 10000000;

function test(fn, n) {
	for (var i = 0; i < n; ++i) {
		fn.call(this);
	}
}

const FNS = [c, cb, l, lb];

for (const fn of FNS) {
	test(fn, 100);
}

for (const fn of FNS) {
	console.time(fn.name);
	test(fn, N);
	console.timeEnd(fn.name);
}

Let's let #13374 land first, then get this rebased and landed after that.

:-(

I understand the :-( but I'd like to get #13374 landed and pulled into a quick 8.x patch or minor release next week while letting this PR sit for a few weeks before pulling it in.

When the input to Function.prototype.bind is a known function, we can inline the allocation of the JSBoundFunction into TurboFan, which provides a 2x speed-up for several hot functions in Node streams (as discovered by Matteo Collina). One of example of this can be found in nodejs/node#13322, which can be optimized and made more readable using bind instead of closures. R=jarin@chromium.org Review-Url: https://codereview.chromium.org/2916063002 Cr-Commit-Position: refs/heads/master@{#45679}

Rebased.

Still LGTM

CITGM with READABLE_STREAM=disable: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/858/

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

Benchmark results with V8 5.9 in master:

                                                              improvement confidence      p.value
 streams/transform-creation.js n=1000000                        693.97 %        *** 2.539023e-41
 zlib/creation.js n=1000000 options="false" type="Deflate"       14.76 %        *** 1.049980e-22
 zlib/creation.js n=1000000 options="true" type="Deflate"        12.35 %        *** 2.782802e-20
 zlib/deflate.js n=400000 inputLen=1024 method="createDeflate"    3.35 %        *** 2.227584e-09
 zlib/deflate.js n=400000 inputLen=1024 method="deflateSync"     27.42 %        *** 3.447520e-43
 zlib/deflate.js n=100000 inputLen=1024 method="deflate"         28.95 %        *** 1.054469e-34

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

I’ve removed this from the 8.2.0 proposal and labelled it dont-land so we can wait for #14161 to be resolved.

So this should have likely been reverted as soon as we noticed issues in zlib. At this point there is a whole bunch of code that has landed on top of this PR in zlib.js and it is extremely hard to revert without a TON of conflicts

/cc @nodejs/tsc

@MylesBorins I have add4b0a reverted and all tests running successfully. Most of the conflicts were due to the new internal error stuff. I'll open a PR. Any idea if e5dc934 needs to be reverted too (or perhaps even instead)?

PR to revert add4b0a: #16280

add4b0a made the assumption that compressed data would never lead to an empty decompressed stream. Fix that by explicitly checking the number of read bytes. Fixes: nodejs#17041 Refs: nodejs#13322

add4b0a made the assumption that compressed data would never lead to an empty decompressed stream. Fix that by explicitly checking the number of read bytes. PR-URL: #17042 Fixes: #17041 Refs: #13322 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mscdex added performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem. zlib Issues and PRs related to the zlib subsystem. labels May 31, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. stream Issues and PRs related to the stream subsystem. zlib Issues and PRs related to the zlib subsystem. labels May 31, 2017

mscdex force-pushed the zlib-perf-improvements branch from d96b10e to e6540d1 Compare May 31, 2017 01:54

jasnell approved these changes May 31, 2017

View reviewed changes

mcollina approved these changes May 31, 2017

View reviewed changes

YurySolovyov reviewed May 31, 2017

View reviewed changes

mscdex force-pushed the zlib-perf-improvements branch from e6540d1 to c050ccb Compare May 31, 2017 23:32

mcollina reviewed Jun 1, 2017

View reviewed changes

bmeurer suggested changes Jun 1, 2017

View reviewed changes

mscdex force-pushed the zlib-perf-improvements branch 2 times, most recently from 7358c93 to 18ad2d7 Compare June 2, 2017 02:22

watilde mentioned this pull request Jun 2, 2017

Throws error in node v8.0.0 archiverjs/node-archiver#236

Closed

mscdex force-pushed the zlib-perf-improvements branch from 18ad2d7 to 153f870 Compare June 5, 2017 21:19

addaleax pushed a commit that referenced this pull request Jun 17, 2017

stream: improve Transform performance

ca4cdce

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax pushed a commit that referenced this pull request Jun 17, 2017

zlib: improve performance

13285bb

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

addaleax pushed a commit that referenced this pull request Jun 21, 2017

stream: improve Transform performance

fd4d384

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax pushed a commit that referenced this pull request Jun 21, 2017

zlib: improve performance

869cbec

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax pushed a commit that referenced this pull request Jun 24, 2017

stream: improve Transform performance

e7248a4

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax pushed a commit that referenced this pull request Jun 24, 2017

zlib: improve performance

749f029

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

rvagg pushed a commit that referenced this pull request Jun 29, 2017

stream: improve Transform performance

012f60d

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

rvagg pushed a commit that referenced this pull request Jun 29, 2017

zlib: improve performance

fea2133

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax mentioned this pull request Jul 10, 2017

node_zlib.cc assertion failure #14161

Closed

addaleax pushed a commit that referenced this pull request Jul 11, 2017

stream: improve Transform performance

6512fd7

PR-URL: #13322 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax added dont-land-on-v4.x labels Jul 11, 2017

gibfahn mentioned this pull request Sep 24, 2017

deps: update npm to 5.4.2 #15486

Closed

2 tasks

mcollina mentioned this pull request Sep 27, 2017

Closing zlib stream may throw uncaught exception #15625

Closed

MylesBorins mentioned this pull request Oct 17, 2017

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-10-18 nodejs/TSC#384

Closed

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

addaleax mentioned this pull request Nov 15, 2017

zlib: fix decompression of empty data streams #17042

Closed

3 tasks

lmm-git mentioned this pull request Nov 22, 2017

Node v9.x.x compatibility pngjs/pngjs#95

Closed

stream,zlib: performance improvements by mscdex · Pull Request #13322 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream,zlib: performance improvements - #13322

stream,zlib: performance improvements#13322mscdex wants to merge 2 commits intonodejs:masternodejs/node:masterfrom mscdex:zlib-perf-improvementsmscdex/io.js:zlib-perf-improvementsCopy head branch name to clipboard

Conversation

mscdex commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented May 31, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 31, 2017

Uh oh!

mscdex commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 2, 2017

Uh oh!

mscdex commented Jun 2, 2017

Uh oh!

jasnell commented Jun 2, 2017

Uh oh!

mscdex commented Jun 5, 2017

Uh oh!

jasnell commented Jun 5, 2017

Uh oh!

mcollina commented Jun 6, 2017

Uh oh!

mcollina commented Jun 6, 2017

Uh oh!

mscdex commented Jun 9, 2017

Uh oh!

addaleax commented Jul 11, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Trott commented Oct 18, 2017

Uh oh!

Trott commented Oct 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

stream,zlib: performance improvements#13322
mscdex wants to merge 2 commits into
nodejs:masterfrom
mscdex:zlib-perf-improvements

mscdex commented May 31, 2017 •
edited
Loading

mscdex Jun 2, 2017 •
edited
Loading