stream_base: use MallocedBuffer abstraction for buffers

[codyhazelwood]

Drop Free and std::unique_ptr in favor of Node's MallocedBuffer
for char[] buffer memory mangement.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[addaleax]

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

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/1​77​88

[refack]

Resume: https://ci.nodejs.org/job/node-test-commit/22257/

[refack]

IMHO in this case std::unique_ptr is a better abstraction then MallocedBuffer.
As I see it, since MallocedBuffer does not use a std::unique_ptr, it's best for RAII of memory that is taken from an outside API, and when size should be tracked as well.

What makes this distinction noticeable to me is the continued use of data_size instead of storage.size

[refack]

Suggesting a possible tweak, something like:

Template<typename T>
struct Free {
  void operator()(unique_ptr<T, Deleter>::pointer ptr) const { free(ptr); }
};
Template<typename T>
std::unique_ptr<T, Free<T>> make_malloced_unique(size_t size) {
  return std::unique_ptr<T, Free<T>>(Malloc<T>(size));
}

[addaleax]

@refack We have MallocedBuffer exactly for this use case – a buffer that’s allocated by malloc().

As I see it, since MallocedBuffer does not use a std::unique_ptr, it's best for RAII of memory that is taken from an outside API, and when size should be tracked as well.

Why would it be better? It’s just as good as std::unique_ptr to declare ownership. Where the memory comes from is irrelevant.

What makes this distinction noticeable to me is the continued use of data_size instead of storage.size

Yes, because they can be different. data_size is not necessarily the actual length of the malloc’ed memory.

[refack]

@refack We have MallocedBuffer exactly for this use case – a buffer that’s allocated by malloc().

As I see it, since MallocedBuffer does not use a std::unique_ptr, it's best for RAII of memory that is taken from an outside API, and when size should be tracked as well.

Why would it be better? It’s just as good as std::unique_ptr to declare ownership. Where the memory comes from is irrelevant.

@addaleax:

1. MallocedBuffer is a new utility and is node specific, so it's semantics are not clear to me, so I assume to others as well. It's maturity is unknown (Ref: src: fix bug in MallocedBuffer #23434)
2. std::unique_ptr is free of runtime cost (except for the dtor), while MallocedBuffer is unknown at minimum carries the runtime cost of construction, and the memory cost of MallocedBuffer::size.

P.S. and probably most important to me - readability. std::unique_ptr standardized and well documented. MallocedBuffer is only documented by it's code

[codyhazelwood]

Not sure what changes I need to make here? If @refack’s review stands, then I should probably close the PR. However, as someone new to the project and not the most experienced C++ developer, I find the MallocedBuffer implementation to be much more readable, and I think I reduces the chances of someone like me introducing a memory leak.

Dismissing in favor of status quo

[refack]

Not sure what changes I need to make here? If @refack’s review stands, then I should probably close the PR. However, as someone new to the project and not the most experienced C++ developer, I find the MallocedBuffer implementation to be much more readable, and I think I reduces the chances of someone like me introducing a memory leak.

Hello @codyhazelwood and thank you for the contribution.
The discussion whether MallocedBuffer is the right abstraction for this case can continue in other PRs. So I dismissed my review.
As a side note std::unique_ptr does RAII, so there was no change in memory semantics in this case.

P.S. CI is as green as it's going to get for the time being.

[BridgeAR]

Landed in 810c0d9 🎉

@codyhazelwood congratulations on your commit to Node.js!