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: buffer-list encapsulation #28974

Closed
wants to merge 1 commit into from

Conversation

@ronag
Copy link
Contributor

commented Aug 5, 2019

Only use public API on buffer list to improve encapsulation and make it easier to experiment with alternative implementations.

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 ronag force-pushed the nxtedition:stream-buffer-list branch 6 times, most recently from b33fec8 to 324b965 Aug 5, 2019

@nodejs-github-bot

This comment has been minimized.

while (p !== null) {
content += decoder.write(p.data);
p = p.next;
for (const data of this._readableState.buffer) {

This comment has been minimized.

Copy link
@lpinca

lpinca Aug 5, 2019

Member

Is this as fast as the original loop?

This comment has been minimized.

Copy link
@ronag

ronag Aug 5, 2019

Author Contributor

I don't know... trying to figure out the whole benchmark thing at the moment

This comment has been minimized.

Copy link
@ronag

ronag Aug 5, 2019

Author Contributor

streams/readable-readall.js n=200 0.67 % ±3.32% ±4.42% ±5.77%

@ronag ronag force-pushed the nxtedition:stream-buffer-list branch 3 times, most recently from 7497bf0 to 2a729ce Aug 5, 2019

@ronag ronag referenced this pull request Aug 6, 2019
2 of 2 tasks complete
@jasnell
jasnell approved these changes Aug 7, 2019
@lpinca
lpinca approved these changes Aug 7, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@mcollina
Copy link
Member

left a comment

Would you mind adding a unit test of the iterator in test-stream-buffer-list.js?

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

test updated

@ronag ronag force-pushed the nxtedition:stream-buffer-list branch from 2a729ce to 957f403 Aug 9, 2019

@mcollina
Copy link
Member

left a comment

lgtm

@nodejs-github-bot

This comment has been minimized.

@ronag ronag force-pushed the nxtedition:stream-buffer-list branch from 957f403 to 4885e6e Aug 9, 2019

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

needed a master rebase + no-unused-vars

@mcollina
Copy link
Member

left a comment

LGTM

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Aug 9, 2019
Trott added a commit to Trott/io.js that referenced this pull request Aug 9, 2019
stream: encapsulate buffer-list
PR-URL: nodejs#28974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Landed in c15b496

@Trott Trott closed this Aug 9, 2019

targos added a commit that referenced this pull request Aug 19, 2019
stream: encapsulate buffer-list
PR-URL: #28974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
stream: encapsulate buffer-list
PR-URL: #28974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos referenced this pull request Aug 19, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: encapsulate buffer-list
PR-URL: nodejs#28974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: encapsulate buffer-list
PR-URL: nodejs#28974
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@dave-kennedy

This comment has been minimized.

Copy link

commented Sep 14, 2019

Is this intended to be used in place of something like bl?

@mcollina

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

Not really, BufferList is not really documented API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.