ARROW-2998: [C++] Add unique_ptr versions of Allocate[Resizable]Buffer

[jbapple-cloudera]

This could be improved in a couple of ways:

1. Remove duplication. I didn't do this yet because ther already is duplication in buffer.cc and I wanted some feedback before proceeding.

2. Add tests. I didn't do this yet because the testing of the existing shared_ptr AllocateBuffer functions is quite slim, so I wanted some feedback before proceeding.

[wesm]

Thanks for adding this. Adding some basic unit tests would be a good idea. These could be done with TYPED_TEST to avoid code duplication in the test suite if desired

[wesm]

This could be templated something like

template <typename BufferType, typename Container>
inline Status ReturnBufferSized(Container<BufferType>&& buffer, const int64_t size,
                                Container<Buffer>* out) {
  RETURN_NOT_OK(buffer->Resize(size));
  buffer->ZeroPadding();
  *out = std::move(buffer);
  return Status::OK();
}

This can also be used to address code duplication in AllocateResizableBuffer as it is now

[pitrou]

Hmm, why is this useful? This is basically duplicating the existing API. If we start applying this pattern everywhere, we'll end up maintaining two mostly similar APIs...

[wesm]

I don't think we should use unique_ptr<Buffer> on any internal Arrow code. Some library users may prefer to use unique_ptr to deal with explicit ownership transfers. Of course they can release() the pointer from shared_ptr<Buffer> into unique_ptr<Buffer> if so desired

[jbapple-cloudera]

@pitrou My rationale is that shared_ptr has runtime overhead and also obscures an important fact about the return(*) values - that only one reference to them exists.

@wesm I don't see a thread-safe way to get a pointer out of a shared_ptr unless we know that shared_ptr is unique. release is only on unique_ptr. While in this case, we do know the object is not aliased elsewhere, this feels a bit like a yak shave.

(*) the output parameters at the end of the param list, usually.

[pitrou]

The runtime overhead is only when copying a shared_ptr, and it's easy to workaround by taking the raw pointer if you ever need it in e.g. a tight loop. I'd be curious if there is a case where it's not possible to eliminate the overhead.

As for that fact that only one reference exists, it may not always be the case. For example, if you are asking for a 0-sized buffer, returning a shared 0-sized Buffer would be a valid optimization IMO.

[wesm]

I think there's a small amount of overhead when the pointer is dereferenced. Since memory allocation is the lowest level of the stack, I'm fine with having unique_ptr variants for the allocation functions. I don't expect we'll be using unique_ptr<Buffer> in any significant way ourselves in other parts of the Arrow libraries

[wesm]

Needs a rebase

[pitrou]

I think there's a small amount of overhead when the pointer is dereferenced

I doubt it's significantly different from unique_ptr's overhead...

[jbapple-cloudera]

@wesm in the libc++ that comes with gcc 5.4.0, there is no dereference overhead, but there is overhead in destruction, which must be thread-safe and so uses atomic operations.

From https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-shared_ptr:

Prefer a unique_ptr over a shared_ptr if there is never more than one owner at a time. shared_ptr is for shared ownership.

Note that pervasive use of shared_ptr has a cost (atomic operations on the shared_ptr's reference count have a measurable aggregate cost).

[wesm]

I'm familiar with the guidelines. What I'm saying is that we have a project here that is oriented largely around hierarchical shared memory references (e.g. to memory maps, POSIX shared memory, payloads coming over the wire), which explains our general preference for using shared_ptr<Buffer> as the fundamental unit of memory management.

[pitrou]

I think that APi consistency trumps micro-optimization here. If we discover a hot internal code path where the shared_ptr overhead is affecting performance, we can revisit this proposal (or simply work around the issue, which we already do internally at places).

[wesm]

I originally suggested to Jim to submit this patch in apache/parquet-cpp#432

[pitrou]

Ah, right, parquet-cpp is using different internal conventions :-/ How do you plan to deal with that if we merge the codebases together? Would we migrate parquet-cpp to the same conventions as the Arrow C++ codebase?

[wesm]

The use case there was a private buffer that would not be exported outside the scope of the Bloom filter. I think it is OK for components to use unique_ptr in such cases if the memory is definitely private.

In the case of a lot of the rest of Arrow, e.g. the columnar data structures, the memory could be shared or reused in many cases, so we need to use shared_ptr.

[pitrou]

It's ok, but is it worth adding to our API surface? You'd be saving a tiny bit of memory and a tiny bit of overhead.

[wesm]

I'm in favor in this limited case. We have plenty of other APIs that could return both kinds of pointers, e.g. https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/file.h#L48. I wouldn't be in favor of adding both variants of the functions in such cases.

I agree that there is the slippery slope possibility of trying to "be all things to all people", but memory allocation is about as close to the metal as we get. I would rather see people reuse these abstractions (particularly since we deal with jemalloc interop, padding/alignment, and other issues) rather than rolling their own.

[alendit]

Maybe we should use a similar pattern directly in AllocateResizableBuffer, i.e. allow the user to pass anything initializable with a uint8_t* as the out parameter.

[wesm]

Rebased. The flaked build (from a package manager timeout) should hopefully pass now.

+1 -- per above discussion I think we should be conservative going forward about adding too many duplicate unique_ptr/shared_ptr APIs

[pitrou]

Ok with me.

[codecov-io]

Codecov Report

Merging #2395 into master will increase coverage by 2.09%.
The diff coverage is 93.1%.

@@            Coverage Diff             @@
##           master    #2395      +/-   ##
==========================================
+ Coverage    84.8%   86.89%   +2.09%     
==========================================
  Files         296      237      -59     
  Lines       45641    42706    -2935     
==========================================
- Hits        38705    37110    -1595     
+ Misses       6891     5596    -1295     
+ Partials       45        0      -45

Impacted Files	Coverage Δ
cpp/src/arrow/buffer.h	95.69% <ø> (ø)	⬆️
cpp/src/arrow/buffer-test.cc	100% <100%> (ø)	⬆️
cpp/src/arrow/buffer.cc	93.81% <69.23%> (-4.02%)	⬇️
rust/src/list.rs
go/arrow/memory/memory.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/memory/checked_allocator.go
go/arrow/math/uint64_sse4_amd64.go
go/arrow/array/list.go
rust/src/builder.rs
... and 52 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aff1dca...b157ded. Read the comment docs.

[wesm]

thanks @jbapple-cloudera!