Add a SkRWBuffer reserve mechanism

Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2385803002

Committed: https://skia.googlesource.com/skia/+/5776508126534db2af97d560588e7046e745df65

[f(malita), 2016-09-30 17:02:10]

Description was changed from

==========
Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
==========

to

==========
Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2385803002
==========

[f(malita), 2016-09-30 17:04:16]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 17:04:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 17:22:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 17:22:40]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-09-30 17:35:48]

lgtm

[f(malita), 2016-09-30 17:57:35]

Prolly need Mike too, for public header.

[Dmitry Skiba, 2016-09-30 19:08:01]

dskiba@google.com changed reviewers:
+ dskiba@google.com

[Dmitry Skiba, 2016-09-30 19:08:02]

This API it not very convenient, caller has to calculate reserve on every call
because it has way of knowing if given append(data, size, reserve) will actually
reserve anything. So depending on what the state of the buffer this may or may
not reserve:

buffer->append(ptr, size, 1000);
for (...) {
  buffer->append(ptr, size);
}

Maybe let's implement proper reserve() method? It doesn't seem that hard, we
just need one new member 'SkBufferBlock*  fReserved' to hold list of
preallocated blocks, which append() would check before allocating new block.

[f(malita), 2016-09-30 19:34:37]

Description was changed from

==========
Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2385803002
==========

to

==========
Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2385803002
==========

[f(malita), 2016-09-30 19:34:37]

fmalita@chromium.org changed reviewers:
+ mtklein@google.com

[f(malita), 2016-09-30 19:35:22]

On 2016/09/30 19:08:02, Dmitry Skiba wrote:
> This API it not very convenient, caller has to calculate reserve on every call
> because it has way of knowing if given append(data, size, reserve) will
actually
> reserve anything. So depending on what the state of the buffer this may or may
> not reserve:
> 
> buffer->append(ptr, size, 1000);
> for (...) {
>   buffer->append(ptr, size);
> }
> 
> Maybe let's implement proper reserve() method? It doesn't seem that hard, we
> just need one new member 'SkBufferBlock*  fReserved' to hold list of
> preallocated blocks, which append() would check before allocating new block.

The current API purposefully makes no guarantees re. what we do with the reserve
count - it truly is just a hint.  Adding a dedicated API may imply stronger
guarantees, which I'd prefer to avoid.

I am also looking at tweaking the prealloc size internally such that the total
block size falls onto allocator-friendly values (power of 2), to minimize the
overhead.  So even if the client requests e.g. a 17000 reserve, we have the
liberty to prealloc a 16K chunk -- and that's perfectly fine with the current
setup where the client will simply request an adjusted reserve on the next call
(which in turn will get trimmed to the next lower pow2, etc).

We could do what you suggest, but then we'd have to perform all this work
upfront, on the initial reserve call, and then manage a list of preallocated
blocks.  I see that as unneeded complexity, because we already have an external
control loop and recomputing the reserve is trivial.

[reed1, 2016-09-30 19:40:09]

lgtm

[Dmitry Skiba, 2016-09-30 20:00:25]

On 2016/09/30 19:35:22, f(malita) wrote:
> On 2016/09/30 19:08:02, Dmitry Skiba wrote:
> > This API it not very convenient, caller has to calculate reserve on every
call
> > because it has way of knowing if given append(data, size, reserve) will
> actually
> > reserve anything. So depending on what the state of the buffer this may or
may
> > not reserve:
> > 
> > buffer->append(ptr, size, 1000);
> > for (...) {
> >   buffer->append(ptr, size);
> > }
> > 
> > Maybe let's implement proper reserve() method? It doesn't seem that hard, we
> > just need one new member 'SkBufferBlock*  fReserved' to hold list of
> > preallocated blocks, which append() would check before allocating new block.
> 
> The current API purposefully makes no guarantees re. what we do with the
reserve
> count - it truly is just a hint.  Adding a dedicated API may imply stronger
> guarantees, which I'd prefer to avoid.
> 
> I am also looking at tweaking the prealloc size internally such that the total
> block size falls onto allocator-friendly values (power of 2), to minimize the
> overhead.  So even if the client requests e.g. a 17000 reserve, we have the
> liberty to prealloc a 16K chunk -- and that's perfectly fine with the current
> setup where the client will simply request an adjusted reserve on the next
call
> (which in turn will get trimmed to the next lower pow2, etc).
> 
> We could do what you suggest, but then we'd have to perform all this work
> upfront, on the initial reserve call, and then manage a list of preallocated
> blocks.  I see that as unneeded complexity, because we already have an
external
> control loop and recomputing the reserve is trivial.

But that control loop is just a single usage by a single (albeit big) API
consumer? Is it OK that we're changing public API for one specific use case?

[f(malita), 2016-09-30 20:16:17]

On 2016/09/30 20:00:25, Dmitry Skiba wrote:
> But that control loop is just a single usage by a single (albeit big) API
> consumer? Is it OK that we're changing public API for one specific use case?

AFAIK, SkRWBuffer & friends were implemented/exist to support Chromium's
deferred decoders :)

I concede that the API is awkward, and all things being equal I would prefer to
go the route you suggest.  But it works well for the current client, and
minimizes complexity -- so I think it's the fastest way to move forward ATM.

We can always convert to the more complex impl later if it makes sense.

[f(malita), 2016-09-30 20:17:02]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-09-30 20:17:02]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/2385803002/#ps20001
(title: "missed the initial allocation")

[commit-bot: I haz the power, 2016-09-30 20:17:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 20:34:21]

Description was changed from

==========
Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2385803002
==========

to

==========
Add a SkRWBuffer reserve mechanism

Currently, Chromium stores segmented data in a SharedBuffer and appends
to SkRWBuffer one segment at a time:

        const char* segment = 0;
        for (size_t length = data->getSomeData(segment, m_rwBuffer->size());
            length; length = data->getSomeData(segment, m_rwBuffer->size())) {
            m_rwBuffer->append(segment, length, remaining);
        }

This can yield a bunch of just-above-4k allocations => wasted RAM due to
internal fragmentation.

Ideally, we'd want a SkRWBuffer::reserve(size_t bytes) API, but the
current internals don't support that trivially.

Alternatively, the caller can pass a reserve hint at append() time.

BUG=chromium:651698
R=scroggo@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2385803002

Committed:
https://skia.googlesource.com/skia/+/5776508126534db2af97d560588e7046e745df65
==========

[commit-bot: I haz the power, 2016-09-30 20:34:22]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/5776508126534db2af97d560588e7046e745df65