ValueSerializer: Replace use of std::vector with a delegate-allocated buffer.

ValueSerializer: Replace use of std::vector with a delegate-allocated buffer.

BUG=chromium:148757
Committed: https://crrev.com/94142ed8c91d2110bba85d4820bd098da3f76fea
Cr-Commit-Position: refs/heads/master@{#40943}

[jbroman, 2016-11-10 22:26:43]

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

[commit-bot: I haz the power, 2016-11-10 22:26:49]

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

[jbroman, 2016-11-10 22:43:08]

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

[commit-bot: I haz the power, 2016-11-10 22:43:16]

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-11-10 23:11:02]

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

[commit-bot: I haz the power, 2016-11-10 23:11:02]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-11-10 23:16:41]

Description was changed from

==========
ValueSerialiezr: Replace use of std::vector with a delegate-allocated buffer.

BUG=chromium:148757
==========

to

==========
ValueSerializer: Replace use of std::vector with a delegate-allocated buffer.

BUG=chromium:148757
==========

[jbroman, 2016-11-10 23:26:23]

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

[commit-bot: I haz the power, 2016-11-10 23:26:32]

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-11-10 23:29:02]

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

[commit-bot: I haz the power, 2016-11-10 23:29:03]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12320)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17569)

[jbroman, 2016-11-10 23:39:43]

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

[commit-bot: I haz the power, 2016-11-10 23:39:54]

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-11-11 00:07:17]

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

[commit-bot: I haz the power, 2016-11-11 00:07:18]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-11-11 01:03:34]

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

[commit-bot: I haz the power, 2016-11-11 01:03:43]

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-11-11 01:33:17]

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

[commit-bot: I haz the power, 2016-11-11 01:33:18]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-11-11 02:36:29]

jbroman@chromium.org changed reviewers:
+ cbruni@chromium.org

[jbroman, 2016-11-11 02:36:30]

It turns out that Android malloc is rather slow for certain cases; Blink's
PartitionAlloc is more consistent across platforms and tuned for typical needs.

This provides hooks for realloc+free, and gathers the memory management code
from various uses of vector to one place, ReserveRawBytes. A simple
doubling-plus-constant growth strategy is used, and the raw buffer is ultimately
released to the embedder (which is responsible for freeing it).

Blink side: https://codereview.chromium.org/2494823002

[Camillo Bruni, 2016-11-11 10:19:26]

lgtm

[jbroman, 2016-11-11 15:51:10]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-11-11 15:51:12]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-11-11 15:52:29]

jbroman@chromium.org changed reviewers:
+ danno@chromium.org

[jbroman, 2016-11-11 15:52:30]

+danno for include/OWNERS (jochen is travelling)

[jbroman, 2016-11-11 17:07:42]

jbroman@chromium.org changed reviewers:
+ jochen@chromium.org

[jbroman, 2016-11-11 17:07:43]

(or jochen for include/OWNERS)

[DmitrySkiba, 2016-11-11 17:21:22]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2016-11-11 17:21:23]

I wonder if we could write a custom allocator for PA, and use it in
std::vector<>. I.e. signature of ReleaseBuffer() would change to
std::vector<uint8_t, partition_allocator<uint8_t>>, and that would basically be
it.

We could go even further and introduce

template <class T>
using pa_vector = std::vector<T, partition_allocator<T>>;

with that ReleaseBuffer() signature becomes just 'pa_vector<uint8_t>
ReleaseBuffer()'.

Writing allocator is easy if you want to just intercept allocate/deallocate, for
example see unshimmed_allocator here:
https://codereview.chromium.org/2440393002/patch/100001/110004

[jbroman, 2016-11-11 17:29:38]

On 2016/11/11 at 17:21:23, dskiba wrote:
> I wonder if we could write a custom allocator for PA, and use it in
std::vector<>. I.e. signature of ReleaseBuffer() would change to
std::vector<uint8_t, partition_allocator<uint8_t>>, and that would basically be
it.
> 
> We could go even further and introduce
> 
> template <class T>
> using pa_vector = std::vector<T, partition_allocator<T>>;
> 
> with that ReleaseBuffer() signature becomes just 'pa_vector<uint8_t>
ReleaseBuffer()'.
> 
> Writing allocator is easy if you want to just intercept allocate/deallocate,
for example see unshimmed_allocator here:
https://codereview.chromium.org/2440393002/patch/100001/110004

This type would have to be shared between Blink and V8 (which makes it
complicated to figure out where it would live, especially since the allocator
implementation is not in V8), and it would either have to not rely on any local
state, of an allocator object would have to be provided. I looked at this, but
it seemed much more complicated than doing this, especially since the needs of
ValueSerializer are narrower than the full range of uses for std::vector. Plus,
it makes it easier to take advantage of PartitionAlloc being able to provide the
actual size of allocations, and its ability to sometimes realloc in place.

[jbroman, 2016-11-11 17:30:41]

Oh, and lest I forget: it also makes it easier to skip the zero-filling that
std::vector loves to do even when it's unnecessary (and using vector::insert, as
this did previously, is surprisingly slow, because it doesn't seem to compile
down to memcpy when that would be the efficient thing to do).

[DmitrySkiba, 2016-11-11 17:53:44]

On 2016/11/11 17:30:41, jbroman wrote:
> Oh, and lest I forget: it also makes it easier to skip the zero-filling that
> std::vector loves to do even when it's unnecessary (and using vector::insert,
as
> this did previously, is surprisingly slow, because it doesn't seem to compile
> down to memcpy when that would be the efficient thing to do).

I see. Thanks for the explanation!

[jochen (gone - plz use gerrit), 2016-11-12 21:03:06]

lgtm

[jbroman, 2016-11-12 21:03:45]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-11-12 21:03:55]

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-11-12 21:49:39]

Committed patchset #6 (id:100001)

[haraken, 2016-11-14 02:07:40]

LGTM

[commit-bot: I haz the power, 2016-11-17 22:31:31]

Description was changed from

==========
ValueSerializer: Replace use of std::vector with a delegate-allocated buffer.

BUG=chromium:148757
==========

to

==========
ValueSerializer: Replace use of std::vector with a delegate-allocated buffer.

BUG=chromium:148757

Committed: https://crrev.com/94142ed8c91d2110bba85d4820bd098da3f76fea
Cr-Commit-Position: refs/heads/master@{#40943}
==========

[commit-bot: I haz the power, 2016-11-17 22:31:32]

Patchset 6 (id:??) landed as
https://crrev.com/94142ed8c91d2110bba85d4820bd098da3f76fea
Cr-Commit-Position: refs/heads/master@{#40943}