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

streams: refactor getHighWaterMark in state.js #20415

Closed
wants to merge 3 commits into from
Closed

streams: refactor getHighWaterMark in state.js #20415

wants to merge 3 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 30, 2018

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

@danbev
streams: refactor getHighWaterMark in state.js
4d49e5b 
This commit aims to reduce some code duplication in state.js
@danbev
Copy link
Contributor Author

danbev commented Apr 30, 2018

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

apapirovski
apapirovski approved these changes Apr 30, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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

lib/internal/streams/state.js Outdated
if (typeof hwm !== 'number' || !(hwm >= 0))
throw new ERR_INVALID_OPT_VALUE(duplexKey, hwm);
return Math.floor(hwm);
if (typeof hwm !== 'number' || !(hwm >= 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@lpinca lpinca Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

It wasn't changed yet, sorry.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lpinca lpinca Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

lib/internal/streams/state.js Outdated
return Math.floor(hwm);
if (typeof hwm !== 'number' || !(hwm >= 0)) {
const name = isDuplex ? duplexKey : 'highWaterMark';
throw new ERR_INVALID_OPT_VALUE(`options.${name}`, hwm);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

lpinca commented Apr 30, 2018

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

BridgeAR
BridgeAR reviewed Apr 30, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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.

lib/internal/streams/state.js Outdated
if (typeof hwm !== 'number' || !(hwm >= 0))
throw new ERR_INVALID_OPT_VALUE(duplexKey, hwm);
return Math.floor(hwm);
if (typeof hwm !== 'number' || !(hwm >= 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@BridgeAR BridgeAR added the stream Issues and PRs related to the stream subsystem. label Apr 30, 2018
@danbev
Copy link
Contributor Author

danbev commented May 2, 2018

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

@apapirovski
Copy link
Member

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

danbev commented May 17, 2018

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

danbev commented May 17, 2018

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

Landed in 9b24be1 🎉

@BridgeAR BridgeAR closed this May 18, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2018
@danbev @BridgeAR
stream: refactor getHighWaterMark in state.js
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>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@danbev @MylesBorins
stream: refactor getHighWaterMark in state.js
ed5f253 
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>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@lpinca lpinca lpinca approved these changes

@apapirovski apapirovski apapirovski approved these changes

@trivikr trivikr trivikr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
6 participants
@danbev @lpinca @apapirovski @BridgeAR @jasnell @trivikr