src: refacor `MallocedBuffer` to it's usage scope #23641

refack · 2018-10-13T17:02:17Z

As discussed in #23543 (review).
MallocedBuffer is an unknown quantity as compared to std::unique_ptr.

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)
- It's ambiguous if MallocedBuffer::size is in units of T or number of byte
std::unique_ptr is free of runtime cost (except for its dtor), while MallocedBuffer at minimum carries the runtime cost of construction, and the memory cost of MallocedBuffer::size.
And probably most important to me - readability. std::unique_ptr is standardized and well documented. MallocedBuffer is only documented by its code.
While refactoring is seems like it has no unit test coverage.

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

Checklist

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

Original commit message: [api] Remove deprecated wasm methods These methods were deprecated in 7.0, now we can remove them. [email protected] Bug: v8:7868 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I60badb378a055152bdd27aed67d11ddf74fce174 Reviewed-on: https://chromium-review.googlesource.com/1209283 Reviewed-by: Adam Klein <[email protected]> Commit-Queue: Clemens Hammacher <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#55695} Refs: v8/v8@b0af309 PR-URL: nodejs#23415 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs#23455 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Covering the case when fs-read get invalid argument for file handle PR-URL: nodejs#23601 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Minor cleanup in the lifetime for the platform worker initialization synchronization barrier. PR-URL: nodejs#23419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

When using `assert.strictEqual`, the first argument must be the actual value and the second argument must be the expected value. PR-URL: nodejs#23448 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23449 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Have actual first, expected second. PR-URL: nodejs#23450 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Reverse the argument for assertion. The first argument should be the actual value and the second value should be the expected value. When there is an AssertionError, the expected and actual value will be labeled correctly. PR-URL: nodejs#23451 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Catch statement defines err variable that is never used, so it is safe to remove that. PR-URL: nodejs#23452 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

Converts RangeError assertions to use common.expectsError and includes an assertion for the error code. PR-URL: nodejs#23454 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23456 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23457 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23458 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23459 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23461 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23463 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#23465 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

nodejs-github-bot · 2018-10-13T17:02:18Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1230/pipeline

joyeecheung · 2018-10-13T17:15:41Z

I am not sure about arguing that std::unique_ptr as a standardized type is more readable than MallocedBuffer. The scope of MallocedBuffer is very well explained in the one-line comment:

// Simple RAII wrapper for contiguous data that uses malloc()/free().

Whereas with std::unique_ptr you don't get any answer about

Is it wrapping a pointer that needs to be freed or deleted? (You may get the answer by looking at the template parameter, but first you need to see the parameter explicitly spelled somewhere, which means you need to spell std::unique_ptr<something, i_use_free>)
Is it pointing to a contiguous chunk of memory, or an end of some other data structure?

Ref: nodejs#23543 (review) Ref: nodejs#23434

addaleax · 2018-10-13T17:28:48Z

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.

As @joyeecheung said, the semantics are defined right next to the class.

It's maturity is unknown (Ref: #23434)

I don’t think that bug lets you make conclusions about “maturity” – this is not a full-featured memory manager, this is a small utility class.

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.

There’s no reason to believe that there is any difference in runtime cost. The memory cost of extra 8 bytes is negligible, and is actually useful a lot of the time.

I don’t think comparing to std::unique_ptr makes sense in the first place.

This structure always represents a continuous array of data. It’s closest STL equivalent is std::vector, not std::unique_ptr. Sometimes, we need to use malloc(), though (because we can actually handle allocation failures, because we convert it to a Buffer, etc.), and writing custom STL allocators is painful and adds a lot more complexity than just using MallocedBuffer().

addaleax

-1, see above.

refack · 2018-10-13T17:30:44Z

I am not sure about arguing that std::unique_ptr as a standardized type is more readable than MallocedBuffer. The scope of MallocedBuffer is very well explained in the one-line comment:

I am not sure about arguing that std::unique_ptr as a standardized type is more readable than MallocedBuffer. The scope of MallocedBuffer is very well explained in the one-line comment:

// Simple RAII wrapper for contiguous data that uses malloc()/free().

Whereas with std::unique_ptr you don't get any answer about

Is it wrapping a pointer that needs to be freed or deleted? (You may get the answer by looking at the template parameter, but first you need to see the parameter explicitly spelled somewhere, which means you need to spell std::unique_ptr<something, i_use_free>)

Is it pointing to a contiguous chunk of memory, or an end of some other data structure?

It might be my perspective, but https://en.cppreference.com/w/cpp/memory/unique_ptr seems to answer these quastion, and leave much less ambiguity.

refack · 2018-10-13T17:35:42Z

I don’t think comparing to std::unique_ptr makes sense in the first place.

This structure always represents a continuous array of data. It’s closest STL equivalent is std::vector, not std::unique_ptr. Sometimes, we need to use malloc(), though (because we can actually handle allocation failures, because we convert it to a Buffer, etc.), and writing custom STL allocators is painful and adds a lot more complexity than just using MallocedBuffer().

As I see it (again this might point to ambiguity) MallocedBuffer is a: Simple RAII wrapper for contiguous data that uses malloc()/free(). So the analogy to std::unique_ptr is not too far fetched (and is exemplified in #23543).

I think that the main difference between this and std::vector is that vector by design hides memory management, while MallocedBuffer put it front and center.

addaleax · 2018-10-13T17:36:51Z

I think that the main difference between this and std::vector is that vector by design hides memory management, while MallocedBuffer put it front and center.

Yes. So let’s use it when it matters that we use a specific memory allocator.

refack · 2018-10-13T17:41:21Z

I think that the main difference between this and std::vector is that vector by design hides memory management, while MallocedBuffer put it front and center.

Yes. So let’s use it when it matters that we use a specific memory allocator.

I agree that it makes sense to abstract that, I just disagree on the implementation. IMHO we should minimize our own code. Something like:

template<typename T>
struct Free {
  void operator()(T* ptr) const { free(ptr); }
};

// Specialization of std::unique_ptr that used Malloc<t>
template<typename T>
using malloced_unique_ptr = std::unique_ptr<T, Free<T>>;

template<typename T>
malloced_unique_ptr make_malloced_unique(size_t number_of_t) {
  return malloced_unique_ptr(Malloc<T>(number_of_t));
}

addaleax · 2018-10-13T17:44:24Z

@refack I think the main downside to that is that it does not provide size explicitly.

I just disagree on the implementation.

This PR seems to do a lot more than changing the implementation.

refack · 2018-10-13T17:54:14Z

@refack I think the main downside to that is that it does not provide size explicitly.

As I see it we have two use cases:

A "buffer" where we want to keep track of its size, store or keep track in a wider scope. There MallocedBuffer is probably a better fit.
An RAII wrapper for Malloc, where size is tracked independently, and probably has a release() in scope. That's where IMHO MallocedBuffer is not the best abstraction.

refack · 2018-10-13T18:05:09Z

This PR seems to do a lot more than changing the implementation.

Ok, so an alternative suggested at #23642

joyeecheung · 2018-10-13T18:46:07Z

It might be my perspective, but https://en.cppreference.com/w/cpp/memory/unique_ptr seems to answer these quastion, and leave much less ambiguity.

@refack That is a long document which just explains std::unique_ptr, can you point out how it answer those questions?

Maybe I wasn't being clear: std::unique_ptr may be able to provide the means to answer questions about the nature of the underlying pointer through template parameters, but that doesn't mean it's readable and easy to use, and the answers ultimately come from the users, not the std::unique_ptr type itself, whereas MallocedBuffer only does one job and the name iis expressive enough.

(Also, I think it's even possible to implement MallocedBuffer on top of unique_ptr and I think there is still value doing it that instead of just using unique_ptr)

EDIT: oh, yeah there is a (sort-of) implementation #23641 (comment) already

Ref: nodejs#23641 Ref: nodejs#23543 (review) Ref: nodejs#23434

refack · 2018-10-20T21:22:12Z

Abandoned

Ref: nodejs#23641 Ref: nodejs#23543 (review) Ref: nodejs#23434

addaleax and others added 17 commits October 12, 2018 21:07

repl: remove unused variable e from try catch

f0c5c96

PR-URL: nodejs#23449 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

test: fix assert.strictEqual argument order

a239270

PR-URL: nodejs#23457 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

test: swap assert.strictEqual args to actual, expected

767d4da

PR-URL: nodejs#23459 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

test: fix incorrect ordering of args in assert.strictEqual()

657ee7b

PR-URL: nodejs#23461 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

test: correct assert test

1c73990

PR-URL: nodejs#23463 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

test: remove unused e variable in catch statement

8691d8f

PR-URL: nodejs#23465 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

refack requested review from addaleax, tniessen and joyeecheung October 13, 2018 17:02

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 13, 2018

refack self-assigned this Oct 13, 2018

src: refacor MallocedBuffer to it's usage scope

d16f098

Ref: nodejs#23543 (review) Ref: nodejs#23434

refack force-pushed the refactor-MallocedBuffer branch from 20ea06d to d16f098 Compare October 13, 2018 17:26

addaleax requested changes Oct 13, 2018

View reviewed changes

src: malloced_unique_ptr & make_malloced_unique

5837d36

refack mentioned this pull request Oct 13, 2018

src: malloced_unique_ptr & make_malloced_unique #23642

Closed

3 tasks

sam-github force-pushed the master branch from 882e022 to 9ed4646 Compare October 13, 2018 18:22

refack added a commit to refack/node that referenced this pull request Oct 13, 2018

src: malloced_unique_ptr & make_malloced_unique

bca838b

Ref: nodejs#23641 Ref: nodejs#23543 (review) Ref: nodejs#23434

refack mentioned this pull request Oct 13, 2018

src: malloced_unique_ptr & make_malloced_unique #23649

Closed

3 tasks

refack closed this Oct 20, 2018

refack deleted the refactor-MallocedBuffer branch October 20, 2018 21:22

refack removed their assignment Oct 20, 2018

refack added a commit to refack/node that referenced this pull request Oct 20, 2018

src: malloced_unique_ptr & make_malloced_unique

5bd49eb

Ref: nodejs#23641 Ref: nodejs#23543 (review) Ref: nodejs#23434

refack added the stalled Issues and PRs that are stalled. label Oct 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: refacor `MallocedBuffer` to it's usage scope #23641

src: refacor `MallocedBuffer` to it's usage scope #23641

refack commented Oct 13, 2018 •

edited

Loading

nodejs-github-bot commented Oct 13, 2018

joyeecheung commented Oct 13, 2018 •

edited

Loading

addaleax commented Oct 13, 2018

addaleax left a comment

refack commented Oct 13, 2018

refack commented Oct 13, 2018 •

edited

Loading

addaleax commented Oct 13, 2018

refack commented Oct 13, 2018 •

edited

Loading

addaleax commented Oct 13, 2018

refack commented Oct 13, 2018

refack commented Oct 13, 2018

joyeecheung commented Oct 13, 2018 •

edited

Loading

refack commented Oct 20, 2018

src: refacor MallocedBuffer to it's usage scope #23641

src: refacor MallocedBuffer to it's usage scope #23641

Conversation

refack commented Oct 13, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 13, 2018

joyeecheung commented Oct 13, 2018 • edited Loading

addaleax commented Oct 13, 2018

addaleax left a comment

Choose a reason for hiding this comment

refack commented Oct 13, 2018

refack commented Oct 13, 2018 • edited Loading

addaleax commented Oct 13, 2018

refack commented Oct 13, 2018 • edited Loading

addaleax commented Oct 13, 2018

refack commented Oct 13, 2018

refack commented Oct 13, 2018

joyeecheung commented Oct 13, 2018 • edited Loading

refack commented Oct 20, 2018

src: refacor `MallocedBuffer` to it's usage scope #23641

src: refacor `MallocedBuffer` to it's usage scope #23641

refack commented Oct 13, 2018 •

edited

Loading

joyeecheung commented Oct 13, 2018 •

edited

Loading

refack commented Oct 13, 2018 •

edited

Loading

refack commented Oct 13, 2018 •

edited

Loading

joyeecheung commented Oct 13, 2018 •

edited

Loading