What about SlowBuffer?

[ChALkeR]

SlowBuffer (and its documentation) has several issues atm:

1. The documentation states that it's a class and lists only one method of creating a SlowBuffer: new SlowBuffer(size). On the other hand, all the tests, benchmarks, and even the code samples in the documentation exclude the new keyword and just call SlowBuffer(size): link. That has to be clarified in the docs.
2. SlowBuffer(size) is unsafe, pretty much like Buffer(size) was. It doesn't have accidential call problem, though — there is no SlowBuffer(value). But it tells users that they should zero-fill the buffer themselves, which isn't very nice (the reasons were already discussed).
3. What is the real reason for it to be a separate class nowdays, when Buffer instances are created without the constructor functions, but using a class method?
4. (Not SlowBuffer), btw, Buffer(something) documentation still includes the new keyword: link. It's deprecated though, and can perhaps be left like that.
5. Buffer.from('abc') also returns a pooled buffer afaik. There is an unpooled counterpart to Buffer.allocUnsafe(size), but not to all the other Buffer creation methods. Would anyone want those?

So, perhaps we should soft-deprecate SlowBuffer altogether and provide an API as Buffer methods, that would conform with Buffer.from() and Buffer.alloc()?

I'm not yet sure how that would look like — it could be some options to the current methods, scary-looking names like Buffer.allocSlow/Buffer.allocSlowUnsafe or even something like Buffer.Slow.alloc/Buffer.Slow.allocUnsafe.

[ChALkeR]

So, the proposal is to replace SlowBuffer with Buffer class methods (in some way), deprecate SlowBuffer, and provide unpooled counterparts to all the other Buffer creation methods, not just to Buffer.allocUnsafe().

This would be a semver-major change (due to the soft deprecation), and it should probably be paired with those Buffer changes that were already landed in #4682.

/cc @nodejs/ctc

[jasnell]

Buffer.alloc already does an unpooled zero filled allocation. So we're covered there. All that is needed is an unpooled non-zeroed allocation which is what SlowBuffer provides.

[ChALkeR]

@jasnell

Buffer.alloc already does an unpooled zero filled allocation.

But it's not documented and atm it's just an implementation detail (i.e. theoretically might change).

Edit: ah, the documentation states:

Buffer instances returned by Buffer.allocUnsafe(size) may be allocated off a shared internal memory pool if the size is less than or equal to half Buffer.poolSize.

So yes, that looks like documented, though it might be better to add a clear notice that Buffer.alloc() is never pooled.

[trevnorris]

The documentation states that it's a class and lists only one method of creating a SlowBuffer

It used to be a class. Now it simply returns an unpooled Buffer.

and even the code samples in the documentation exclude the new keyword and just call SlowBuffer(size): link. That has to be clarified in the docs.

It's the exact same with Buffer. They are both simply wrappers around Uint8Array.

But it tells users that they should zero-fill the buffer themselves, which isn't very nice (the reasons were already discussed).

The primary use case (and pretty much the only use case) for SlowBuffer is to allocate memory when it immediately needs to be filled by the user. e.g. when needing to more permanently store a slice of a larger Buffer.

What is the real reason for it to be a separate class nowdays, when Buffer instances are created without the constructor functions, but using a class method?

The reason it existed it because in a prior implementation Buffer took slices of SlowBuffer. When I reimplemented Buffer the first time this was no longer needed. Though it was, and currently is, the only way to get a non-pooled uninitialized chunk of memory. So instead of writing a new API I decided to keep around and slightly repurpose the old one (which as far as the user was concerned still did the same thing).

scary-looking names

Sorry but making sure the API is "scary-looking" to make sure it's not misused by developers seems a little strange. Forcing the user to require() a new function seems like a more explicit way to prevent accidental usage. You probably don't think users should use this API. I don't want to pollute Buffer with more methods. I'm fine with changing the name, but forcing the require() it a more solid option if we want to prevent accidental misuse.

[ChALkeR]

@jasnell

Buffer.alloc already does an unpooled zero filled allocation. So we're covered there. All that is needed is an unpooled non-zeroed allocation which is what SlowBuffer provides.

Is there a reason for it to be a separate method (and documented as a class)?

Also, the documentation is misleading then: it suggests to use SlowBuffer(size) and fill it with zeroes, when the same result could be achived with just calling Buffer.alloc(size) now.

[ChALkeR]

@trevnorris

scary-looking names

Sorry but making sure the API is "scary-looking" to make sure it's not misused by developers seems a little strange.

That's not exactly what I meant there. Imo Buffer.allocSlowUnsafe is just too long and has too many adjectives in it, this is why I called it «scary-looking».

[trevnorris]

Imo Buffer.allocSlowUnsafe is just too long and has too many adjectives in it, this is why I called it «scary-looking».

Hah. Sorry for the misinterpretation. It's late here. I'll go to bed now.

[trevnorris]

Is there a reason for it to be a separate method (and documented as a class)?

Prior to my first Buffer rewrite SlowBuffer did in fact return a different object. instanceof Buffer wouldn't have worked on it. So that's still around for legacy reasons.

[jasnell]

How about we keep SlowBuffer as is, doc deprecate it, and add an optional
Boolean to allocUnsafe that will ensure unpooled allocation if true. Nice,
simple, no scary new methods, and existing code doesn't break.
On Mar 19, 2016 1:06 AM, "Trevor Norris" notifications@github.com wrote:

Is there a reason for it to be a separate method (and documented as a
class)?

Prior to my first Buffer rewrite SlowBuffer did in fact return a different
object. instanceof Buffer wouldn't have worked on it. So that's still
around for legacy reasons.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5799 (comment)

[ChALkeR]

@jasnell That would solve it =).

[trevnorris]

Sounds like a reasonable solution. For how long the Buffer API has been around, I don't see any forward conflicts by introducing a second argument to allocUnsafe. I would also like the documentation to be clarified. Much of the current wording was chosen for me on the initial rewrite, and I've not taken the time to rewrite it since.

Though I believe the API is necessary, it also has a narrow use case. Which is if someone wants to retain a Buffer slice for an extended period of time. The purpose is to prevent retaining excess memory. For example:

const SlowBuffer = require('buffer').SlowBuffer;
const retainData = [];

net.createServer((c) => {
  c.on('data', (chunk) => {
    const first = chunk.indexOf('abc');
    const last = chunk.lastIndexOf('xyz');
    if (first >= last)
      return;
    const size = last - first;
    const sb = SlowBuffer(size);
    chunk.copy(sb, 0, first, size);
    // Doing retainData.push(chunk.slice(start, end)) will retain
    // the entire packet. This can lead to unnecessary memory
    // growth over time.
    retainData.push(sb);
  });
});

Despite this narrow use case, it would also not enough to create a method that achieves the same. There are also circumstances where multiple Buffers need to be copied into the same new allocation. Granted this can be achieved by doing:

Buffer.concat([ buf1.slice(start, end), buf2.slice(start, end), ...]);

but this comes with unnecessary overhead.

[jasnell]

I'll work up the new PR adding the additional parameter and the docs-only deprecation for SlowBuffer