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.

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

stream: increase MAX_HWM #29938

Closed
wants to merge 7 commits into from
Closed

Conversation

@ronag
Copy link
Member

ronag commented Oct 11, 2019

MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 11, 2019

Copy link
Member

mcollina left a comment

Can you add some description/comments to the test? It would be more explicit to add some assertions as well.

@ronag ronag force-pushed the nxtedition:fix-readable-max-hwm branch from 1d5d383 to 310821f Oct 12, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 12, 2019

Added simple description. Not sure what assertion would be relevant here?

Copy link
Member

addaleax left a comment

Can you also add a test for file streams specifically, along the lines of the reproduction in the original issue?

@ronag ronag force-pushed the nxtedition:fix-readable-max-hwm branch 2 times, most recently from bc62925 to 0d228cb Oct 12, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 13, 2019

@Trott Trott added the author ready label Oct 13, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott Trott removed the author ready label Oct 14, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 14, 2019

Test failures look like this:

Mismatched noop function calls. Expected exactly 1, actual 0.
    at Object.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:337:10)
    at Object.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-stream-readable-read-max-hwm.js:29:20)
    at Module._compile (internal/modules/cjs/loader.js:958:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:994:10)
    at Module.load (internal/modules/cjs/loader.js:813:32)
    at Function.Module._load (internal/modules/cjs/loader.js:725:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1046:10)
    at internal/main/run_main_module.js:17:11
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 15, 2019

@Trott: Picking another bufferSize triggered another type of failure. I've partly fixed it but not entirely.

So though this fixes the referred issue it doesn't fix every issue. I will need to spend more time on this.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 15, 2019

@mcollina @addaleax I think fixing this is basically going to end up with almost identical behaviour as not having a MAX_HWM at all... I'm not sure what the rationale for MAX_HWM was.

I need a little guidance on what we would prefer here:

  • Leave as is.
  • Partly fix (current state of this PR)
  • Make and document size purely as a hint (which is what it kind of is now due to MAX_HWM).
  • Add an error if read(size) exceeds MAX_HWM and try to increase the MAX_HWM value to a value high enough that it doesn't break most of existing usage.
  • Temporarily increase highWaterMark to requested size and then bring it back down again once length has reached the required length to fulfil one read of that size.
@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Oct 19, 2019

I would recommend a scavenge in history in why there is a MAX_HWM at all.
It was added in 9208c89 to avoid pathological failures with synchronous calls to _read after push(). Those are not there anymore since Node 10, so MAX_HWM can be removed.

It would be good to do a git bisect to find out which commit broke this (likely is one of mine), so we would know more.

@ronag ronag force-pushed the nxtedition:fix-readable-max-hwm branch from 03a686d to d147179 Oct 20, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 20, 2019

@mcollina: Thanks. Didn't even consider removing it. Updated PR.

@ronag ronag force-pushed the nxtedition:fix-readable-max-hwm branch 3 times, most recently from b8288e5 to 3a63e31 Oct 20, 2019
@ronag ronag requested a review from Fishrock123 Oct 20, 2019
@ronag ronag force-pushed the nxtedition:fix-readable-max-hwm branch from 4f0a6dd to ba28187 Oct 20, 2019
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Oct 20, 2019

@Trott: This is no longer WIP

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Nov 1, 2019

@Trott: I think this is ready?

@Trott Trott added the author ready label Nov 3, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Nov 11, 2019

@Trott ping

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 12, 2019

Landed in f8c069f.

@Trott Trott closed this Nov 12, 2019
Trott added a commit to Trott/io.js that referenced this pull request Nov 12, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: nodejs#29933

PR-URL: nodejs#29938
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Nov 13, 2019

@BridgeAR this should probably be backported to node 12?

MylesBorins added a commit that referenced this pull request Nov 17, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
@targos targos removed the lts-watch-v12.x label Dec 1, 2019
targos added a commit that referenced this pull request Dec 1, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins added a commit that referenced this pull request Dec 17, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
BethGriggs added a commit that referenced this pull request Jan 7, 2020
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.