doc: buffer.from will return internal pool buffer by mritunjayz · Pull Request #24312 · nodejs/node · GitHub

Checklist

#22139
Buffer.from will also use internal Buffer pool if it's size is less then Buffer.poolSize.

documentation is changed or added
commit message follows commit guidelines

@ChALkeR can you please review this ? 😊

@ChALkeR I removed length condition from documentation.

@nodejs/buffer @nodejs/documentation

This is not always correct. It may return 0, and any subsequent call will probably increase it.
So it doesn't always print 344.

This could be misleading. Changing Buffer.poolSize does have immediate effect on which buffers would be pooled, but it doesn't immediately change the size of the current pool, it changes the size of the next pool.
It does, however, take immediate effect for determining whether should the buffer be allocated from the pool or not.

@mritunjaygoutam12 Sorry for delay on this, that was mostly due to my personal issues :-/.
I am available and will respond more actively on this from now on.

Overall, I don't think that the code example is very useful -- it's the confusing part atm, as it seems for me. Perhaps we can get this to land faster without the code example at all.

As for the remaining part -- it specifically mentions Buffer.from(array) for some reason, when in fact many other buffer operations are pooled.

E.g. these are not covered:

> Buffer.from('sdf').offset
280
> Buffer.concat([Buffer.from('xxx'), Buffer.from('yyy')]).offset
304
> Buffer.from(Buffer.from('sdf')).offset
320
> Buffer.from(new String('test')).offset
328

The problem of the current documentation in master is that this section:

node/doc/api/buffer.md

Lines 100 to 103 in 17c6b1d

    
           `Buffer` instances returned by [`Buffer.allocUnsafe()`] *may* be allocated off 
        
           a shared internal memory pool if `size` is less than or equal to half 
        
           [`Buffer.poolSize`]. Instances returned by [`Buffer.allocUnsafeSlow()`] *never* 
        
           use the shared internal memory pool.

makes it seem that only .allocUnsafe is pooled, when in fact mostly everything is pooled.

Same for:

node/doc/api/buffer.md

Lines 597 to 609 in 17c6b1d

    
           Note that the `Buffer` module pre-allocates an internal `Buffer` instance of 
        
           size [`Buffer.poolSize`] that is used as a pool for the fast allocation of new 
        
           `Buffer` instances created using [`Buffer.allocUnsafe()`] and the deprecated 
        
           `new Buffer(size)` constructor only when `size` is less than or equal to 
        
           `Buffer.poolSize >> 1` (floor of [`Buffer.poolSize`] divided by two). 
        
           Use of this pre-allocated internal memory pool is a key difference between 
        
           calling `Buffer.alloc(size, fill)` vs. `Buffer.allocUnsafe(size).fill(fill)`. 
        
           Specifically, `Buffer.alloc(size, fill)` will *never* use the internal `Buffer` 
        
           pool, while `Buffer.allocUnsafe(size).fill(fill)` *will* use the internal 
        
           `Buffer` pool if `size` is less than or equal to half [`Buffer.poolSize`]. The 
        
           difference is subtle but can be important when an application requires the 
        
           additional performance that [`Buffer.allocUnsafe()`] provides.

Perhaps some of the rewording should touch those?

Also, /cc @nodejs/tsc @nodejs/buffer @nodejs/security-wg -- what was the reason for making Buffer.alloc non-pooled, but Buffer.from pooled? Buffer.alloc could also benefit from pre-allocating zero-filled buffers, can't it? If providing pooled buffers is an issue, we shouldn't do that for everything. If it's not, perhaps we should just pool Buffer.alloc in a semver-major, so users won't be punished for using it instead of Buffer.allocUnsafe().fill()?

Some nits for consistency with other examples.

Suggested change

      
            // prints: 344
          
            // Prints: 344

Suggested change

      
            // prints: 8192
          
            // Prints: 8192

Suggested change

      
            // prints: 0
          
            // Prints: 0

@mritunjayz - can you address the review comments?

@mritunjayz, are you still interested in working on the PR ? If not, let me know; I can pick this up and progress.

@HarshithaKP yes, you can. Thanks

This can be closed as the intention is fulfilled in #32703.

doc: Buffer.from will return internal pool buffer

ce878bb

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Nov 11, 2018

mritunjayz changed the title doc: Buffer.from will return internal pool buffer doc: buffer.from will return internal pool buffer Nov 11, 2018

doc: buffer.from willreturn pooled buffer

0c3566b

Trott reviewed Nov 16, 2018

View reviewed changes

Comment thread doc/api/buffer.md

Trott reviewed Nov 16, 2018

View reviewed changes

Comment thread doc/api/buffer.md Outdated

Trott reviewed Nov 16, 2018

View reviewed changes

Comment thread doc/api/buffer.md Outdated

ChALkeR self-requested a review November 16, 2018 19:58

ChALkeR reviewed Nov 16, 2018

View reviewed changes

Comment thread doc/api/buffer.md

length condition removed

7b7c4b4

Trott reviewed Nov 24, 2018

View reviewed changes

Comment thread doc/api/buffer.md Outdated

buffer.from() tiny doc nit change

d1f3b51

ChALkeR reviewed Mar 2, 2019

View reviewed changes

ChALkeR self-requested a review March 2, 2019 02:43

vsemozhetbyt reviewed Mar 2, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

HarshithaKP mentioned this pull request Apr 7, 2020

doc: document buffer.from returns internal pool buffer #32703

Closed

4 tasks

Trott closed this Apr 14, 2020

+              const nodeBuffers = Buffer.from([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
+              console.log(nodeBuffers.offset);
+              // prints: 344

+              // prints: 8192
+              // But you can change it
+              Buffer.poolSize = 5;

+              // Default Buffer.poolsize is 8192
+              console.log(Buffer.poolSize);
+              // prints: 8192

+              const buf = Buffer.from([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
+              console.log(buf.offset);
+              // prints: 0

doc: buffer.from will return internal pool buffer by mritunjayz · Pull Request #24312 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: buffer.from will return internal pool buffer - #24312

doc: buffer.from will return internal pool buffer#24312mritunjayz wants to merge 4 commits intonodejs:masternodejs/node:masterfrom mritunjayz:buffer-docmritunjayz/node:buffer-docCopy head branch name to clipboard

Conversation

mritunjayz commented Nov 11, 2018

Checklist

Uh oh!

mritunjayz commented Nov 12, 2018

Uh oh!

mritunjayz commented Nov 14, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mritunjayz commented Nov 19, 2018

Uh oh!

Uh oh!

mritunjayz commented Nov 25, 2018

Uh oh!

Trott commented Nov 28, 2018

Uh oh!

ChALkeR Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

ChALkeR Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Dec 28, 2019

Uh oh!

HarshithaKP commented Mar 31, 2020

Uh oh!

mritunjayz commented Apr 3, 2020

Uh oh!

HarshithaKP commented Apr 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

doc: buffer.from will return internal pool buffer#24312
mritunjayz wants to merge 4 commits into
nodejs:masterfrom
mritunjayz:buffer-doc

ChALkeR commented Mar 2, 2019 •
edited
Loading