Skip to content

doc: document Buffer.concat may use internal pool - #35541

Closed
puzpuzpuz wants to merge 1 commit into
nodejs:masterfrom
puzpuzpuz:documentation/improve-buffer-pool-doc
Closed

doc: document Buffer.concat may use internal pool#35541
puzpuzpuz wants to merge 1 commit into
nodejs:masterfrom
puzpuzpuz:documentation/improve-buffer-pool-doc

Conversation

@puzpuzpuz

@puzpuzpuzpuzpuzpuz commented Oct 7, 2020

Copy link
Copy Markdown
Member

Refs: #32703

The documentation was silent about the fact that Buffer.concat() uses Buffer.allocaUnsafe() and, thus, may return pooled buffers. This PR fixes that.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Oct 7, 2020
@puzpuzpuzpuzpuzpuz added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 13, 2020
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2020
@nodejs-github-bot

nodejs-github-bot commented Oct 13, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Oct 13, 2020•
edited
Loading

Copy link
Copy Markdown
Member

@addaleax isn't this behavior problematic for transferList? i.e. we should maybe try to only use pool if at least one of the input buffers also use the pool?

@addaleax

addaleax commented Oct 13, 2020

Copy link
Copy Markdown
Member

@ronag This should be fine after #32759, right?

@ronag

ronag commented Oct 13, 2020

Copy link
Copy Markdown
Member

@ronag This should be fine after #32759, right?

Yes and no. Should be fine in regards to the problem I was worried about. What's unfortunate is that there is no way to not use the pool when concatenating if I know that might cause problems or reduced performance (e.g. transferList). But maybe that is just a nit.

@puzpuzpuz

puzpuzpuz commented Oct 13, 2020

Copy link
Copy Markdown
MemberAuthor

What's unfortunate is that there is no way to not use the pool when concatenating if I know that might cause problems or reduced performance (e.g. transferList). But maybe that is just a nit.

Users may decide to implement their own concat method which doesn't use the pool. Alternatively, we could introduce Buffer.concatSlow() method to make things more explicit. But that means larger API surface.

As for this very PR, it simply describes what we have now.

@ronag

ronag commented Oct 13, 2020

Copy link
Copy Markdown
Member

As for this very PR, it simply describes what we have now.

Yes, sorry for going off-topic.

@nodejs-github-bot

nodejs-github-bot commented Oct 13, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Oct 14, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Oct 14, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Oct 14, 2020

Copy link
Copy Markdown
Collaborator

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

puzpuzpuz added a commit that referenced this pull request Oct 14, 2020
@puzpuzpuz
doc: document Buffer.concat may use internal pool
ef1645e
PR-URL: #35541
Refs: #32703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@puzpuzpuz

puzpuzpuz commented Oct 14, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in ef1645e

@puzpuzpuz
puzpuzpuz deleted the documentation/improve-buffer-pool-doc branch October 14, 2020 17:48
MylesBorins pushed a commit that referenced this pull request Oct 14, 2020
@puzpuzpuz@MylesBorins
doc: document Buffer.concat may use internal pool
0c87776
PR-URL: #35541
Refs: #32703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@MylesBorinsMylesBorins mentioned this pull request Oct 14, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@puzpuzpuz@MylesBorins
doc: document Buffer.concat may use internal pool
54a29f1
PR-URL: #35541
Refs: #32703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@puzpuzpuz@MylesBorins
doc: document Buffer.concat may use internal pool
df52814
PR-URL: #35541
Refs: #32703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@puzpuzpuz@joesepi
doc: document Buffer.concat may use internal pool
8d03adb
PR-URL: nodejs#35541
Refs: nodejs#32703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@ronagronagronag approved these changes
@HarshithaKPHarshithaKPHarshithaKP approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues and PRs related to the buffer subsystem.docIssues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@puzpuzpuz@nodejs-github-bot@ronag@addaleax@Trott@lpinca@HarshithaKP