Add helper classes to for managing shared buffers.

Add helper classes to for managing shared buffers.

First of a series of patches for Motown.

FifoAllocator - does the bookkeeping to implement heap semantics on an
imaginary buffer assuming normal streaming behavior (first allocated,
first released).

MappedSharedBuffer - owns a shared buffer, maps and unmaps it and does
offset/pointer conversions.

SharedMediaBufferAllocator - derived from MappedSharedBuffer, adds
heap semantics using FifoAllocator and thread safety.

BUG=none
R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/9553aed2f2d15ac06050a6f1ec70c21670fe073b

[dalesat, 2015-11-19 19:07:25]

dalesat@chromium.org changed reviewers:
+ jamesr@chromium.org, jeffbrown@google.com, johngro@google.com

[dalesat, 2015-11-19 19:07:26]

Please take a look. Thanks!

[johngro, 2015-12-05 00:45:55]

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/fifo_allocator.cc (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:13: // front_ and free_ need to
be set to nullptr before calling reset.
MOJO_DCHECK(size < kNullOffset);

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:18: assert(front_ == back_);
its probably better to use MOJO_DCHECK here instead of assert.  The main
advantages are..

+ when the asserts drop out of the code in a release build, MOJO_DCHECK will
keep the compiler from complaining about any variables which would be computed
but unused if not for the DCHECK.  There are a few instances of this in the
file, and they currently break a release build.
+ MOJO_DCHECK will go to the logging infrastructure instead of dumping to
stdout.  As time goes on, this will become more and more important.
+ Any other standard context we want to dump when we fail an assert can be added
to DCHECK over time, and may or may not be present in the <cassert>
implementation of assert.

here and below.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:46: auto result =
active_->offset;
Generally, "auto" is supposed to be used when it reduces the clutter of long
type names and when the type being inferred is obvious from the context.  The
classic example would be an iterator pattern like...

std::set<MyTemplatedClass<horrible, template, argument, list>>::iterator iter =
my_set_.begin();

vs.

auto iter = my_set_.begin();

In this case of "auto" for "uint64_t"... I can figure out that this is needs to
be a uint64_t either by opening the header file and finding out what type
active_ is, then finding the type of active_->offset from that; or by looking at
how result is used later on in this function and assuming that the compiler will
enforce the type safety (and not perform and implicit casts).  Still, I feel
that it would help readability to use the explicit type instead of saying "auto"
here.

OTOH - If the goal here is to maintain flexibility (so that the type of an
offset can be changed without updating all of the code), then adding a using to
the class is probably the better thing to do.  Something like

using MyOffsetType = uint64_t;

or

typedef uint64_t MyOffsetType;

if you want a more old-school style.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:49: // active_ was the back
region and the front region isn't allocted. Make
s/allocted/allocated

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:64: auto allocated =
get_free(true, size, active_->offset);
s/auto/Region*

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:73: auto released =
s/auto/bool

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:83: Region* end) {
MOJO_DCHECK(begin);

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:132: remove(active_);
put_free(active_);

In general, is there ever a time where we would want to remove a node from the
[front_, back_] list and not return it to the free pool?  Perhaps remove should
just automatically add to the free pool.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:195: assert(size + offset <=
size_);
in theory, size + offset could wrap and pass this check when it shouldn't. 
suggest

MOJO_DCHECK(size <= size_);
MOJO_DCHECK(offset <= (size_ - size));

instead

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/fifo_allocator.h (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.h:60: static const uint64_t
kNullOffset = 0xffffffffffffffff;
std::numeric_limits<uint64_t>::max()

You will need to #include <limits> at the top

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/mapped_shared_buffer.cc (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/mapped_shared_buffer.cc:5: #include <cassert>
same comment as in the previous file, MOJO_DCHECK instead of assert

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/mapped_shared_buffer.cc:72: // TODO: We should
create a read-only duplicate of the handle.
TODO(dalesat)

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/mapped_shared_buffer.h (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/mapped_shared_buffer.h:72: // TODO: SharedBuffer
should support taking an existing handle.
TODO(dalesat):

[dalesat, 2015-12-07 18:30:20]

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/fifo_allocator.cc (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:13: // front_ and free_ need to
be set to nullptr before calling reset.
On 2015/12/05 00:45:55, johngro wrote:
> MOJO_DCHECK(size < kNullOffset);

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:18: assert(front_ == back_);
On 2015/12/05 00:45:54, johngro wrote:
> its probably better to use MOJO_DCHECK here instead of assert.  The main
> advantages are..
> 
> + when the asserts drop out of the code in a release build, MOJO_DCHECK will
> keep the compiler from complaining about any variables which would be computed
> but unused if not for the DCHECK.  There are a few instances of this in the
> file, and they currently break a release build.
> + MOJO_DCHECK will go to the logging infrastructure instead of dumping to
> stdout.  As time goes on, this will become more and more important.
> + Any other standard context we want to dump when we fail an assert can be
added
> to DCHECK over time, and may or may not be present in the <cassert>
> implementation of assert.
> 
> here and below.

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:46: auto result =
active_->offset;
On 2015/12/05 00:45:54, johngro wrote:
> Generally, "auto" is supposed to be used when it reduces the clutter of long
> type names and when the type being inferred is obvious from the context.  The
> classic example would be an iterator pattern like...
> 
> std::set<MyTemplatedClass<horrible, template, argument, list>>::iterator iter
=
> my_set_.begin();
> 
> vs.
> 
> auto iter = my_set_.begin();
> 
> In this case of "auto" for "uint64_t"... I can figure out that this is needs
to
> be a uint64_t either by opening the header file and finding out what type
> active_ is, then finding the type of active_->offset from that; or by looking
at
> how result is used later on in this function and assuming that the compiler
will
> enforce the type safety (and not perform and implicit casts).  Still, I feel
> that it would help readability to use the explicit type instead of saying
"auto"
> here.
> 
> OTOH - If the goal here is to maintain flexibility (so that the type of an
> offset can be changed without updating all of the code), then adding a using
to
> the class is probably the better thing to do.  Something like
> 
> using MyOffsetType = uint64_t;
> 
> or
> 
> typedef uint64_t MyOffsetType;
> 
> if you want a more old-school style.

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:49: // active_ was the back
region and the front region isn't allocted. Make
On 2015/12/05 00:45:55, johngro wrote:
> s/allocted/allocated

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:64: auto allocated =
get_free(true, size, active_->offset);
On 2015/12/05 00:45:54, johngro wrote:
> s/auto/Region*

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:73: auto released =
On 2015/12/05 00:45:54, johngro wrote:
> s/auto/bool

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:83: Region* end) {
On 2015/12/05 00:45:55, johngro wrote:
> MOJO_DCHECK(begin);

Can be nullptr if end is nullptr.
Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:132: remove(active_);
On 2015/12/05 00:45:55, johngro wrote:
> put_free(active_);
> 
> In general, is there ever a time where we would want to remove a node from the
> [front_, back_] list and not return it to the free pool?  Perhaps remove
should
> just automatically add to the free pool.

Done. I'd rather not put_free in remove, because this is inconsistent with
remove semantics in other containers.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:195: assert(size + offset <=
size_);
On 2015/12/05 00:45:54, johngro wrote:
> in theory, size + offset could wrap and pass this check when it shouldn't. 
> suggest
> 
> MOJO_DCHECK(size <= size_);
> MOJO_DCHECK(offset <= (size_ - size));
> 
> instead

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/fifo_allocator.h (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.h:60: static const uint64_t
kNullOffset = 0xffffffffffffffff;
On 2015/12/05 00:45:55, johngro wrote:
> std::numeric_limits<uint64_t>::max()
> 
> You will need to #include <limits> at the top

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/mapped_shared_buffer.cc (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/mapped_shared_buffer.cc:5: #include <cassert>
On 2015/12/05 00:45:55, johngro wrote:
> same comment as in the previous file, MOJO_DCHECK instead of assert

Done.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/mapped_shared_buffer.cc:72: // TODO: We should
create a read-only duplicate of the handle.
On 2015/12/05 00:45:55, johngro wrote:
> TODO(dalesat)

Removed. Also added a check so this works when InitFromHandle is used.

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/mapped_shared_buffer.h (right):

https://codereview.chromium.org/1460693004/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/mapped_shared_buffer.h:72: // TODO: SharedBuffer
should support taking an existing handle.
On 2015/12/05 00:45:55, johngro wrote:
> TODO(dalesat):

Removed.

[johngro, 2015-12-10 18:38:08]

+1

I'd still like James or Jeff to make a quick pass, but LGTM in general.

https://codereview.chromium.org/1460693004/diff/80001/mojo/services/media/com...
File mojo/services/media/common/cpp/fifo_allocator.cc (right):

https://codereview.chromium.org/1460693004/diff/80001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:134: put_free(active_);
If we are going to start using intrusive containers as a pattern in a lot of our
code, we should consider factoring the container code out so that it can be
re-used multiple places.  I recognize that it is a common pattern with intrusive
containers to remove an element from one container and place it into another, so
you don't want to force people to return to the free pool and re-allocate.  That
said, since we are not using naked pointers here it is easy to make a mistake
and start to slowly leak memory.  We may want to look for ways to make the free
of an element more of the default behavior, and force the developer to take
extra steps in order to remove and retain the element.  Perhaps the deleting the
element is the only way to destroy it (which would implicitly remove it from the
container), and we could have another operation to transfer from one container
to the other.  This op would then become
active_container_->transfer(active_element_, free_list_).

This is just something to think about, I am not requesting any action here.

[dalesat, 2015-12-10 22:34:39]

https://codereview.chromium.org/1460693004/diff/80001/mojo/services/media/com...
File mojo/services/media/common/cpp/fifo_allocator.cc (right):

https://codereview.chromium.org/1460693004/diff/80001/mojo/services/media/com...
mojo/services/media/common/cpp/fifo_allocator.cc:134: put_free(active_);
On 2015/12/10 18:38:08, johngro wrote:
> If we are going to start using intrusive containers as a pattern in a lot of
our
> code, we should consider factoring the container code out so that it can be
> re-used multiple places.  I recognize that it is a common pattern with
intrusive
> containers to remove an element from one container and place it into another,
so
> you don't want to force people to return to the free pool and re-allocate. 
That
> said, since we are not using naked pointers here it is easy to make a mistake
> and start to slowly leak memory.  We may want to look for ways to make the
free
> of an element more of the default behavior, and force the developer to take
> extra steps in order to remove and retain the element.  Perhaps the deleting
the
> element is the only way to destroy it (which would implicitly remove it from
the
> container), and we could have another operation to transfer from one container
> to the other.  This op would then become
> active_container_->transfer(active_element_, free_list_).
> 
> This is just something to think about, I am not requesting any action here.

Understood.

I didn't want to create a reusable intrusive container. This class only
uses/implements a small fraction of what you we would need from a reusable
container. Regions aren't exposed outside this class, so Region management isn't
a concern that callers need to share.

[dalesat, 2016-01-06 21:44:10]

Description was changed from

==========
Add helper classes to for managing shared buffers.

First of a series of patches for Motown.

FifoAllocator - does the bookkeeping to implement heap semantics on an
imaginary buffer assuming normal streaming behavior (first allocated,
first released).

MappedSharedBuffer - owns a shared buffer, maps and unmaps it and does
offset/pointer conversions.

SharedMediaBufferAllocator - derived from MappedSharedBuffer, adds
heap semantics using FifoAllocator and thread safety.

BUG=none
==========

to

==========
Add helper classes to for managing shared buffers.

First of a series of patches for Motown.

FifoAllocator - does the bookkeeping to implement heap semantics on an
imaginary buffer assuming normal streaming behavior (first allocated,
first released).

MappedSharedBuffer - owns a shared buffer, maps and unmaps it and does
offset/pointer conversions.

SharedMediaBufferAllocator - derived from MappedSharedBuffer, adds
heap semantics using FifoAllocator and thread safety.

BUG=none
R=johngro@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/9553aed2f2d15ac06050a6f1ec7...
==========

[dalesat, 2016-01-06 21:44:11]

Committed patchset #9 (id:160001) manually as
9553aed2f2d15ac06050a6f1ec70c21670fe073b (presubmit successful).