streams: refactor getHighWaterMark in state.js by danbev · Pull Request #20415 · nodejs/node

danbev · 2018-04-30T05:23:00Z

This commit aims to reduce some code duplication in state.js

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

This commit aims to reduce some code duplication in state.js

danbev · 2018-04-30T05:23:26Z

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

apapirovski

LGTM. (I would maybe prefer an early if return instead of the nested ternary but either's fine.)

apapirovski · 2018-04-30T05:33:44Z

lib/internal/streams/state.js

-      if (typeof hwm !== 'number' || !(hwm >= 0))
-        throw new ERR_INVALID_OPT_VALUE(duplexKey, hwm);
-      return Math.floor(hwm);
+    if (typeof hwm !== 'number' || !(hwm >= 0)) {


Could we also change hwm >= 0 to hwm < 0? That negation is just confusing, I think.

That has a different meaning though because it does not catch NaN anymore.

Good point. In that case maybe we should be checking for that explicitly? The fact that both me and @lpinca missed it means it is likely to get missed again in the future and then no one might correct it.

Well, hopefully we have tests for it ;-) Besides that, I guess it would be good to add a comment next to it.

~~It seems we don't as CI is green for this :)~~

It wasn't changed yet, sorry.

I think we have tests actually. @danbev hasn't made the change yet. Anyway, I'm fine with adding a comment. Just not a big fan of non-obvious checks like this.

Indeed, it would be semver-major but using Number.isInteger() seems the ideal solution.

Sorry about the delay on this. I'll take a closer look tomorrow (public holiday here today)

apapirovski · 2018-04-30T05:39:01Z

lib/internal/streams/state.js

-      return Math.floor(hwm);
+    if (typeof hwm !== 'number' || !(hwm >= 0)) {
+      const name = isDuplex ? duplexKey : 'highWaterMark';
+      throw new ERR_INVALID_OPT_VALUE(`options.${name}`, hwm);


I don't think the options. bit is necessary? IMO it's more readable without.

The only reason for making this change was to be consistent with #20284 but I'm happy to remove this.

The error constructors are different though. This one is more specific since it's not ERR_INVALID_ARG_TYPE but rather ERR_INVALID_OPT_VALUE. Meaning it already communicates that it's within the options object.

lpinca · 2018-04-30T06:26:11Z

Nit: subsystem in commit message should be "stream:" not "streams:"

BridgeAR

I am -0 on this one. If the code is not inligned by V8, it is more work than before and the readability is IMO reduced now.

BridgeAR · 2018-04-30T10:04:18Z

lib/internal/streams/state.js

-      if (typeof hwm !== 'number' || !(hwm >= 0))
-        throw new ERR_INVALID_OPT_VALUE(duplexKey, hwm);
-      return Math.floor(hwm);
+    if (typeof hwm !== 'number' || !(hwm >= 0)) {


That has a different meaning though because it does not catch NaN anymore.

danbev · 2018-05-02T06:24:52Z

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

apapirovski · 2018-05-17T09:40:32Z

@danbev Could we perhaps run the streams creation benchmark that was created recently? That might tell us whether this change is detrimental to performance. Seems like a good next step in deciding whether we land this or not.

danbev · 2018-05-17T10:39:27Z

Could we perhaps run the streams creation benchmark that was created recently? That might tell us whether this change is detrimental to performance. Seems like a good next step in deciding whether we land this or not.

Sounds good, I'll run benchmark/streams/creation.js and compare them and report back.

danbev · 2018-05-17T12:10:30Z

I ran the following benchmark:

$ node benchmark/compare.js --old ./node-master --new ./node-pr-20415 --filter creation.js streams > compare-pr-20415.csv
[00:26:25|% 100| 1/1 files | 60/60 runs | 4/4 configs]: Done
 $ cat compare-pr-20415.csv | Rscript benchmark/compare.R
                                                 confidence improvement accuracy (*)   (**)  (***)
 streams/creation.js kind='duplex' n=50000000             *      0.63 %       ±0.56% ±0.74% ±0.97%
 streams/creation.js kind='readable' n=50000000                 -0.29 %       ±0.40% ±0.53% ±0.69%
 streams/creation.js kind='transform' n=50000000                 0.14 %       ±0.56% ±0.74% ±0.97%
 streams/creation.js kind='writable' n=50000000                 -0.06 %       ±0.61% ±0.81% ±1.06%

Be aware that when doing many comparisions the risk of a false-positive
result increases. In this case there are 4 comparisions, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

BridgeAR · 2018-05-18T13:23:08Z

Landed in 9b24be1 🎉

This commit aims to reduce some code duplication in state.js PR-URL: nodejs#20415 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

This commit aims to reduce some code duplication in state.js PR-URL: #20415 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

streams: refactor getHighWaterMark in state.js

4d49e5b

This commit aims to reduce some code duplication in state.js

apapirovski approved these changes Apr 30, 2018

View reviewed changes

lpinca approved these changes Apr 30, 2018

View reviewed changes

BridgeAR reviewed Apr 30, 2018

View reviewed changes

BridgeAR added the stream Issues and PRs related to the stream subsystem. label Apr 30, 2018

danbev added 2 commits May 2, 2018 06:17

squash: remove options prefix

08bcceb

squash: update hwm check

66cfaa5

jasnell approved these changes May 17, 2018

View reviewed changes

trivikr approved these changes May 17, 2018

View reviewed changes

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

BridgeAR closed this May 18, 2018

addaleax mentioned this pull request May 22, 2018

v10.2.0 proposal #20724

Merged

Uh oh!

Conversation

danbev commented Apr 30, 2018

Checklist

Uh oh!

danbev commented Apr 30, 2018

Uh oh!

apapirovski 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!

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!

lpinca Apr 30, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

lpinca Apr 30, 2018 • 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!

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!

lpinca commented Apr 30, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented May 2, 2018

Uh oh!

apapirovski commented May 17, 2018

Uh oh!

danbev commented May 17, 2018

Uh oh!

danbev commented May 17, 2018

Uh oh!

BridgeAR commented May 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lpinca Apr 30, 2018 •

edited

Loading

lpinca Apr 30, 2018 •

edited

Loading