GeezerVison

Copy link

Copy Markdown

Member

bricss reacted with thumbs up emoji

ronag requested a review from addaleax June 19, 2020 22:03

ronag force-pushed the fs-pool branch 2 times, most recently from cf19ba7 to b58a7bb Compare June 19, 2020 22:04

addaleax approved these changes Jun 19, 2020

View reviewed changes

ronag force-pushed the fs-pool branch from b58a7bb to 4300f74 Compare June 20, 2020 08:18

bnoordhuis approved these changes Jun 20, 2020

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag force-pushed the fs-pool branch from b961333 to 2bc17a5 Compare June 20, 2020 08:41

ronag requested a review from jasnell June 20, 2020 08:41

19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='asc'           ***    -11.00 %       ±3.01%  ±4.01%  ±5.22%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='buf'           ***    -10.15 %       ±3.46%  ±4.61%  ±6.01%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='utf'           ***     -6.08 %       ±2.53%  ±3.37%  ±4.40%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='asc'                 1.28 %       ±4.14%  ±5.52%  ±7.20%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='buf'          *     14.52 %      ±13.09% ±17.42% ±22.67%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='utf'                17.52 %      ±20.33% ±27.07% ±35.28%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='asc'                   -0.39 %       ±2.94%  ±3.91%  ±5.10%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='buf'             *     -4.47 %       ±3.36%  ±4.47%  ±5.83%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='utf'                   -1.49 %       ±2.29%  ±3.05%  ±3.97%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='asc'                  -1.46 %       ±8.08% ±10.75% ±13.99%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='buf'                  -0.84 %       ±7.46%  ±9.94% ±12.96%
19:58:22  fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='utf'                  -0.54 %       ±6.16%  ±8.19%

rvagg reviewed Jun 23, 2020

View reviewed changes

The performance benefit of using a custom pool are negligable.
Furthermore, it causes problems with Workers and transferrable.
Rather than further adding complexity for compat with Workers,
just remove the pooling logic.

Refs: #33880 (comment)
Fixes: #31733

PR-URL: #33981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

The performance benefit of using a custom pool are negligable.
Furthermore, it causes problems with Workers and transferrable.
Rather than further adding complexity for compat with Workers,
just remove the pooling logic.

Refs: nodejs#33880 (comment)
Fixes: nodejs#31733

PR-URL: nodejs#33981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

The performance benefit of using a custom pool are negligable.
Furthermore, it causes problems with Workers and transferrable.
Rather than further adding complexity for compat with Workers,
just remove the pooling logic.

Refs: #33880 (comment)
Fixes: #31733

PR-URL: #33981
Backport-PR-URL: #38397
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

The `test-crypto-authenticated-stream` test was moved out of
`test/known_issues` and now lives in `test/parallel`

PR-URL: #47454
Refs: #33981
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

The `test-crypto-authenticated-stream` test was moved out of
`test/known_issues` and now lives in `test/parallel`

PR-URL: #47454
Refs: #33981
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

The `test-crypto-authenticated-stream` test was moved out of
`test/known_issues` and now lives in `test/parallel`

PR-URL: #47454
Refs: #33981
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

The `test-crypto-authenticated-stream` test was moved out of
`test/known_issues` and now lives in `test/parallel`

PR-URL: nodejs#47454
Refs: nodejs#33981
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>