V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it into a String.

V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it into a String.

This causes v8::ValueSerializer to use the buffer partition for its allocations, then
holds the result in a unique_ptr within the SerializedScriptValue. Removal of the String
alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In particular,
large performance issues with default malloc on Android are blocking this feature from
shipping at present.

BUG=148757
Committed: https://crrev.com/4a94f7a4a9055e7b919c4c3ef9604324c75a70f5
Cr-Commit-Position: refs/heads/master@{#432232}

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

Description was changed from

==========
WIP: Use PartitionAlloc to back V8SVS allocations, and skip the copy.

BUG=148757
==========

to

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then holds the result in a unique_ptr within the SerializedScriptValue. Removal
of the String alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular, large performance issues with default malloc on Android are blocking
this feature from shipping at present.

BUG=148757
==========

[jbroman, 2016-11-11 02:32:11]

Description was changed from

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then holds the result in a unique_ptr within the SerializedScriptValue. Removal
of the String alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular, large performance issues with default malloc on Android are blocking
this feature from shipping at present.

BUG=148757
==========

to

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

1234567889012345678890123456788901234567889012345678890123456788901234567889012345678890

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then
holds the result in a unique_ptr within the SerializedScriptValue. Removal of
the String
alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular,
large performance issues with default malloc on Android are blocking this
feature from
shipping at present.

BUG=148757
==========

[jbroman, 2016-11-11 02:32:18]

Description was changed from

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

1234567889012345678890123456788901234567889012345678890123456788901234567889012345678890

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then
holds the result in a unique_ptr within the SerializedScriptValue. Removal of
the String
alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular,
large performance issues with default malloc on Android are blocking this
feature from
shipping at present.

BUG=148757
==========

to

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then
holds the result in a unique_ptr within the SerializedScriptValue. Removal of
the String
alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular,
large performance issues with default malloc on Android are blocking this
feature from
shipping at present.

BUG=148757
==========

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

jbroman@chromium.org changed reviewers:
+ haraken@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 exposes PartitionAlloc (specifically, the buffer partition) to
v8::ValueSerializer, and then takes ownership of the resulting buffer.

V8 side: https://codereview.chromium.org/2492943002/

[haraken, 2016-11-11 03:50:01]

LGTM

> PartitionAlloc has much more consistent cross-platform performance. In
particular,
> large performance issues with default malloc on Android are blocking this
feature from
> shipping at present.

Not related to this CL, would you elaborate on your findings? This is something
project-trim@ might want to discuss in more details.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:141: size_t
stringSizeBytes = (m_dataBufferSize + 1) & ~1;

Shall we use CheckedNumeric<>?

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:155: size_t
wireSizeBytes = (m_dataBufferSize + 1) & ~1;

Ditto.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:138:
m_data.ensure16Bit();

Shall we rename m_data to m_dataString?

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:196: // or
|m_dataBuffer| is (if it is stored in a raw buffer allocation). In the

Can we add a DCHECK to verify that |m_data| and |m_dataBuffer| are not used at
the same time?

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:198: // even
in length.

What do you mean by "will not necessarily be even in length"? If the data is
stored in m_dataBuffer, m_data.length() should be 0.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/allocator/Partitions.h (right):

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/allocator/Partitions.h:96: return
partitionReallocGeneric(bufferPartition(), p, n, typeName);

Can we add a unit test to PartitionAllocTest?

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

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

[commit-bot: I haz the power, 2016-11-11 15:51:48]

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 16:07:52]

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

[commit-bot: I haz the power, 2016-11-11 16:07:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jbroman, 2016-11-11 16:14:58]

Sent a note.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:141: size_t
stringSizeBytes = (m_dataBufferSize + 1) & ~1;
On 2016/11/11 at 03:50:01, haraken wrote:
> Shall we use CheckedNumeric<>?

I can if you'd like, but the addition could only overflow if m_dataBufferSize
were already SIZE_MAX (which, on our supported platforms, implies that the data
buffer fills the entire virtual address space already, which is impossible), so
I don't see how this could overflow. If you would still like this, I'll switch.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:138:
m_data.ensure16Bit();
On 2016/11/11 at 03:50:01, haraken wrote:
> Shall we rename m_data to m_dataString?

Done.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:196: // or
|m_dataBuffer| is (if it is stored in a raw buffer allocation). In the
On 2016/11/11 at 03:50:01, haraken wrote:
> Can we add a DCHECK to verify that |m_data| and |m_dataBuffer| are not used at
the same time?

I've added one to the data() getter; I'm not sure where else you would like one.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:198: // even
in length.
On 2016/11/11 at 03:50:01, haraken wrote:
> What do you mean by "will not necessarily be even in length"? If the data is
stored in m_dataBuffer, m_data.length() should be 0.

I mean that while the string version will always be a multiple of two bytes in
length (due to being backed by a 16-bit string), the raw buffer version may not
be. I've reworded the comment; let me know if it's clearer.

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/allocator/Partitions.h (right):

https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/allocator/Partitions.h:96: return
partitionReallocGeneric(bufferPartition(), p, n, typeName);
On 2016/11/11 at 03:50:01, haraken wrote:
> Can we add a unit test to PartitionAllocTest?

What kind of test would you want? We don't currently have unit tests for the
other shorthands (bufferMalloc, bufferFree, etc., which fill in one parameter of
the generic functions), and partitionReallocGeneric is covered by
PartitionAllocTest.GenericAlloc and PartitionAllocTest.Realloc.

[haraken, 2016-11-11 16:24:49]

On 2016/11/11 16:14:58, jbroman wrote:
> Sent a note.
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> (right):
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:141:
size_t
> stringSizeBytes = (m_dataBufferSize + 1) & ~1;
> On 2016/11/11 at 03:50:01, haraken wrote:
> > Shall we use CheckedNumeric<>?
> 
> I can if you'd like, but the addition could only overflow if m_dataBufferSize
> were already SIZE_MAX (which, on our supported platforms, implies that the
data
> buffer fills the entire virtual address space already, which is impossible),
so
> I don't see how this could overflow. If you would still like this, I'll
switch.
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
(right):
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:138:
> m_data.ensure16Bit();
> On 2016/11/11 at 03:50:01, haraken wrote:
> > Shall we rename m_data to m_dataString?
> 
> Done.
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:196: // or
> |m_dataBuffer| is (if it is stored in a raw buffer allocation). In the
> On 2016/11/11 at 03:50:01, haraken wrote:
> > Can we add a DCHECK to verify that |m_data| and |m_dataBuffer| are not used
at
> the same time?
> 
> I've added one to the data() getter; I'm not sure where else you would like
one.
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:198: //
even
> in length.
> On 2016/11/11 at 03:50:01, haraken wrote:
> > What do you mean by "will not necessarily be even in length"? If the data is
> stored in m_dataBuffer, m_data.length() should be 0.
> 
> I mean that while the string version will always be a multiple of two bytes in
> length (due to being backed by a 16-bit string), the raw buffer version may
not
> be. I've reworded the comment; let me know if it's clearer.
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/allocator/Partitions.h (right):
> 
>
https://codereview.chromium.org/2494823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/allocator/Partitions.h:96: return
> partitionReallocGeneric(bufferPartition(), p, n, typeName);
> On 2016/11/11 at 03:50:01, haraken wrote:
> > Can we add a unit test to PartitionAllocTest?
> 
> What kind of test would you want? We don't currently have unit tests for the
> other shorthands (bufferMalloc, bufferFree, etc., which fill in one parameter
of
> the generic functions), and partitionReallocGeneric is covered by
> PartitionAllocTest.GenericAlloc and PartitionAllocTest.Realloc.

All make sense. LGTM.

[jbroman, 2016-11-14 21:28:00]

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

[commit-bot: I haz the power, 2016-11-14 21:29:05]

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-15 02:05:52]

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

[commit-bot: I haz the power, 2016-11-15 02:05:53]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-11-15 19:12:16]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-11-15 19:12:17]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2494823002/#ps80001
(title: "initialize SerializedScriptValue::m_dataBufferSize")

[commit-bot: I haz the power, 2016-11-15 19:12:51]

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-15 19:22:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-15 19:34:57]

Description was changed from

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then
holds the result in a unique_ptr within the SerializedScriptValue. Removal of
the String
alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular,
large performance issues with default malloc on Android are blocking this
feature from
shipping at present.

BUG=148757
==========

to

==========
V8ScriptValueSerializer: Use PartitionAlloc for the buffer, and avoid copying it
into a String.

This causes v8::ValueSerializer to use the buffer partition for its allocations,
then
holds the result in a unique_ptr within the SerializedScriptValue. Removal of
the String
alternative is expected to follow eventually.

PartitionAlloc has much more consistent cross-platform performance. In
particular,
large performance issues with default malloc on Android are blocking this
feature from
shipping at present.

BUG=148757

Committed: https://crrev.com/4a94f7a4a9055e7b919c4c3ef9604324c75a70f5
Cr-Commit-Position: refs/heads/master@{#432232}
==========

[commit-bot: I haz the power, 2016-11-15 19:34:58]

Patchset 5 (id:??) landed as
https://crrev.com/4a94f7a4a9055e7b919c4c3ef9604324c75a70f5
Cr-Commit-Position: refs/heads/master@{#432232}