stream: remove useage of _readableState/_writableState.highWaterMark by calvinmetcalf · Pull Request #12860 · nodejs/node

calvinmetcalf · 2017-05-05T14:18:47Z

Replace with .readHWM or .writeHWM getters, part of #445 mega
issue.

@nodejs/streams

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, net, http

Fishrock123 · 2017-05-05T15:31:45Z

Should duplex have both getters?

Also, the read / write prefixes may be confused with get / set... maybe use readableHWM and writableHWM?

In addition, should these be documented?

mscdex · 2017-05-05T15:48:43Z

What kind of impact does this have performance-wise?

calvinmetcalf · 2017-05-06T06:33:37Z

Duplex inherits from readable so it doesn't need to be redefined

…

On Fri, May 5, 2017, 5:49 PM Brian White ***@***.***> wrote: What kind of impact performance-wise does this have? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12860 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE4n1orQSyLYP4PrCiLkMnPffx1OVvDks5r20UFgaJpZM4NR-q6> .

mscdex · 2017-05-06T06:39:59Z

@calvinmetcalf I'm guessing that was supposed to be directed at @Fishrock123 instead of me :-)

calvinmetcalf · 2017-05-08T12:04:51Z

@mscdex yes sorry, also yeah we'll have to bench mark them all, and if they are noticeably slower we can check if methods are faster

jasnell · 2017-06-13T20:18:38Z

lib/_stream_duplex.js

nit: this could just be get() { ... }

jasnell · 2017-06-13T20:19:47Z

lib/_stream_duplex.js

not really a fan of the abbreviation on this. Would prefer spelling it out e.g. writeHighWaterMark

BridgeAR · 2017-09-22T23:19:20Z

Is this something that should stay open / is still still pursued? @calvinmetcalf @mcollina

mcollina · 2017-09-23T06:27:05Z

@BridgeAR this is part of #445. Yes, it should stay open.

BridgeAR

LGTM % comments.

BridgeAR · 2017-10-02T12:15:20Z

lib/_stream_duplex.js

Those should be enumerable.

jasnell · 2017-10-29T17:42:19Z

This is not going to make 9.0.0, removing from the milestone

mcollina · 2017-11-14T15:58:05Z

Updated and added docs. @jasnell please check again.

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

mcollina · 2017-11-14T18:38:17Z

This is going to suffer from the same problems of #12857 (comment). We need to make the properties non-enumerable.

jasnell

LGTM

Replace with .readableHighWaterMark or .writableHighWaterMark getters. Ref: nodejs#445.

mcollina · 2017-11-15T09:58:01Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1074/
CI: https://ci.nodejs.org/job/node-test-pull-request/11447/

mcollina · 2017-11-15T11:50:28Z

CI failures are unrelated, CITGM is ok.

mcollina · 2017-11-15T11:53:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11452/ (just for being safe)

mcollina · 2017-11-15T13:31:11Z

Landed as 157df5a.

Replaced _readableState.highWaterMark with a .readableHighWaterMark getter and _writableState.highWaterMark with a .writableHighWaterMark getter. The getters are non-enumerable because they break some prototype manipulation that happen in the ecosystem. Ref: #445. PR-URL: #12860 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

mscdex · 2017-11-15T14:33:31Z

lib/_http_server.js

  state.outgoingData += delta;
  if (socket._paused &&
-      state.outgoingData < socket._writableState.highWaterMark) {
+      state.outgoingData < socket.writeHWM) {


This isn't correct.

interesting enough, our test suite passed.

I'll get it updated.

mscdex · 2017-11-15T14:33:38Z

lib/_http_server.js


 function socketOnDrain(socket, state) {
-  var needPause = state.outgoingData > socket._writableState.highWaterMark;
+  var needPause = state.outgoingData > socket.writeHWM;


mscdex · 2017-11-15T14:37:14Z

I honestly think this should have waited until we came up with a general decision about _readableState and _writableState.

mcollina · 2017-11-15T14:44:19Z

@mscdex that happened: nodejs/TSC#355

See: nodejs#12860 (review)

See: #12860 (review) PR-URL: #17050 Reviewed-By: Calvin Metcalf <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Replaced _readableState.highWaterMark with a .readableHighWaterMark getter and _writableState.highWaterMark with a .writableHighWaterMark getter. The getters are non-enumerable because they break some prototype manipulation that happen in the ecosystem. Ref: #445. PR-URL: #12860 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

See: #12860 (review) PR-URL: #17050 Reviewed-By: Calvin Metcalf <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: Coming Soon

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: #17631

gibfahn · 2018-01-15T23:05:45Z

Release team were +1 on backporting to v8.x.

Replaced _readableState.highWaterMark with a .readableHighWaterMark getter and _writableState.highWaterMark with a .writableHighWaterMark getter. The getters are non-enumerable because they break some prototype manipulation that happen in the ecosystem. Ref: #445. PR-URL: #12860 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

See: #12860 (review) PR-URL: #17050 Reviewed-By: Calvin Metcalf <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

calvinmetcalf added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels May 5, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 5, 2017

This was referenced May 5, 2017

stream: remove {writeableState/readableState}.length #12857

Closed

streams: remove all references to readableState/writableState from docs #12855

Closed

jasnell reviewed Jun 13, 2017

View reviewed changes

lib/_stream_duplex.js Outdated

Copy link

Member

jasnell Jun 13, 2017

Choose a reason for hiding this comment

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

nit: this could just be get() { ... }

jasnell requested changes Jun 13, 2017

View reviewed changes

lib/_stream_duplex.js Outdated

Copy link

Member

jasnell Jun 13, 2017

Choose a reason for hiding this comment

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

not really a fan of the abbreviation on this. Would prefer spelling it out e.g. writeHighWaterMark

mcollina modified the milestone: 9.0.0 Jul 20, 2017

BridgeAR approved these changes Oct 2, 2017

View reviewed changes

lib/_stream_duplex.js Outdated

Copy link

Member

BridgeAR Oct 2, 2017

Choose a reason for hiding this comment

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

Those should be enumerable.

jasnell removed this from the 9.0.0 milestone Oct 29, 2017

mcollina force-pushed the highwatermark-state-removal branch 2 times, most recently from 4bb31c0 to 7fdcbe0 Compare November 14, 2017 15:56

jasnell approved these changes Nov 14, 2017

View reviewed changes

stream: remove useage of _readableState/_writableState.highWaterMark

4bed9d7

Replace with .readableHighWaterMark or .writableHighWaterMark getters. Ref: nodejs#445.

mcollina force-pushed the highwatermark-state-removal branch from 7fdcbe0 to 4bed9d7 Compare November 15, 2017 09:57

mcollina added the dont-land-on-v4.x label Nov 15, 2017

mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 15, 2017

mcollina closed this Nov 15, 2017

mscdex reviewed Nov 15, 2017

View reviewed changes

mcollina added a commit to mcollina/node that referenced this pull request Nov 15, 2017

http, stream: writeHWM -> writableHighWaterMark

543dd1c

See: nodejs#12860 (review)

mcollina mentioned this pull request Nov 15, 2017

http, stream: writeHWM -> writableHighWaterMark #17050

Closed

2 tasks

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added the lts-watch-v8.x label Jan 15, 2018

Uh oh!

Conversation

calvinmetcalf commented May 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented May 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

calvinmetcalf commented May 6, 2017 via email

Uh oh!

mscdex commented May 6, 2017

Uh oh!

calvinmetcalf commented May 8, 2017

Uh oh!

jasnell Jun 13, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Jun 13, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 22, 2017

Uh oh!

mcollina commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 29, 2017

Uh oh!

mcollina commented Nov 14, 2017

Uh oh!

mcollina commented Nov 14, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Nov 15, 2017

Uh oh!

mcollina commented Nov 15, 2017

Uh oh!

mcollina commented Nov 15, 2017

Uh oh!

mcollina commented Nov 15, 2017

Uh oh!

mscdex Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex commented Nov 15, 2017

Uh oh!

mcollina commented Nov 15, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mscdex commented May 5, 2017 •

edited

Loading

mcollina commented Sep 23, 2017 •

edited

Loading