postMessage(): transfer allocation costs along with value.

postMessage(): transfer allocation costs along with value.

A MessageEvent's data value will in some cases hold on to significant
amounts of off-heap memory, so we take care of registering that
external allocation with v8, so that it can be taken into consideration
by the GC triggering logic.

However, when posting a message to another context, we must arrange for
its total 'external allocation' to be associated with the target context.
Including the sizes of any transferables (array buffers), so balance the
books more accurately by also transferring the external allocation cost
of those transferables.

R=
BUG=698981
Review-Url: https://codereview.chromium.org/2734173002
Cr-Commit-Position: refs/heads/master@{#456009}
Committed: https://chromium.googlesource.com/chromium/src/+/c7711215a15658e8932087b6a9676859a89f7609

[sof, 2017-03-07 16:39:53]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 16:41:00]

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

[sof, 2017-03-07 17:23:30]

Description was changed from

==========
postMessage(): transfer allocation costs along with value.

A serialized message carries some allocation overhead, so when posting
it to another context, arrange for its external allocation cost (wrt v8)
to be fully accounted for in its target context. Balance the books by
discounting the cost it had already registered in the context it was
sent from.

R=
BUG=
==========

to

==========
postMessage(): transfer allocation costs along with value.

A serialized message carries some allocation overhead, so when posting
it to another context, arrange for its external allocation cost (wrt v8)
to be fully accounted for in its target context. Balance the books by
discounting the cost it had already registered in the context it was
sent from.

R=
BUG=698981
==========

[sof, 2017-03-07 17:31:07]

Description was changed from

==========
postMessage(): transfer allocation costs along with value.

A serialized message carries some allocation overhead, so when posting
it to another context, arrange for its external allocation cost (wrt v8)
to be fully accounted for in its target context. Balance the books by
discounting the cost it had already registered in the context it was
sent from.

R=
BUG=698981
==========

to

==========
postMessage(): transfer allocation costs along with value.

A MessageEvent's data value will in some cases hold on to significant
amounts of off-heap memory, so we take care of registering that
external allocation with v8, so that it can be taken into consideration
by the GC triggering logic.

However, when posting a message to another context, we must arrange for
its total 'external allocation' to be associated with the target context.
Including the sizes of any transferables (array buffers), so balance the
books more accurately by also transferring the external allocation cost
of those transferables.

R=
BUG=698981
==========

[sof, 2017-03-07 17:34:58]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[sof, 2017-03-07 17:36:13]

sigbjornf@opera.com changed reviewers:
+ jbroman@chromium.org, oilpan-reviews@chromium.org

[sof, 2017-03-07 17:36:13]

please take a look.

[commit-bot: I haz the power, 2017-03-07 17:36:41]

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

[jbroman, 2017-03-07 18:00:04]

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:448: if
(m_externallyAllocatedMemory == 1)
Use of this magic value is confusing. I'd suggest just adding a boolean member
rather being clever.

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:144: void
unregisterMemoryAllocatedByCurrentScriptContext();
Can you clarify why this is needed now? It looks like the MessageEvent (which
calls registerMemoryAllocatedWithCurrentScriptContext) isn't created until it's
received by the second thread, so would there ever be external memory registered
at the time this is called?

[haraken, 2017-03-07 18:16:05]

The allocation accounting is getting more complex. Would it be possible to add a
DCHECK to verify that the add/subtract accounting is correctly balanced?

[commit-bot: I haz the power, 2017-03-07 19:13:49]

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

[commit-bot: I haz the power, 2017-03-07 19:13:50]

Dry run: This issue passed the CQ dry run.

[sof, 2017-03-07 19:14:03]

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:448: if
(m_externallyAllocatedMemory == 1)
On 2017/03/07 18:00:04, jbroman wrote:
> Use of this magic value is confusing. I'd suggest just adding a boolean member
> rather being clever.

It's not particularly clever; I can switch to an int64_t though (and avoid
spending a word to encode one bit of info on 64-bit arches.)

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:144: void
unregisterMemoryAllocatedByCurrentScriptContext();
On 2017/03/07 18:00:04, jbroman wrote:
> Can you clarify why this is needed now? It looks like the MessageEvent (which
> calls registerMemoryAllocatedWithCurrentScriptContext) isn't created until
it's
> received by the second thread, so would there ever be external memory
registered
> at the time this is called?

 - transferables (arraybuffers) will be accounted wrt v8 when created.
 - when serialized and passed along via postMessage(), we neuter the "source"
   side of the transferables, passing along the array buffer contents.
 - the allocation cost still resides with the "source" and not the "target".
 - ..yet the "source" cannot access the buffer contents, the "target" can.
 - to address that unbalanced accounting, unregister*() discharges the
   external allocation cost from "source". Cost includes both that of
   the serialized payload (if registered) and the transferable buffers.
 - register*() will now _additionally_ also account for the external
   allocation cost of the transferables buffers when the "target" dispatches
   the incoming message from a postMessage(). It will however only do so
   if the allocation cost for these have been discharged via a
   corresponding unregister*(). This is, like before, done upon
   MessageEvent construction.

The upshot is that for a testcase like seen on the associated bug, v8 is now
able to schedule GCs better for the "target" worker context, preventing OOMs for
some workloads.

[jbroman, 2017-03-07 19:29:05]

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:448: if
(m_externallyAllocatedMemory == 1)
On 2017/03/07 at 19:14:03, sof wrote:
> On 2017/03/07 18:00:04, jbroman wrote:
> > Use of this magic value is confusing. I'd suggest just adding a boolean
member
> > rather being clever.
> 
> It's not particularly clever; I can switch to an int64_t though (and avoid
spending a word to encode one bit of info on 64-bit arches.)

(u)int64_t + boolean in a bitfield would do the trick, though to be honest I'd
be surprised if we had enough SerializedScriptValues floating around for the
single word to contribute significantly to memory consumption.

As-is, I'm having to track what 1 means, and why 1 is added to size sometimes,
and so on. It's not immediately apparent to me. (Since dataLengthInBytes isn't
going to change, I believe all we _really_ need to store is two bits: "is my
buffer accounted for", and "are my transferables accounted for".)

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:144: void
unregisterMemoryAllocatedByCurrentScriptContext();
On 2017/03/07 at 19:14:03, sof wrote:
> On 2017/03/07 18:00:04, jbroman wrote:
> > Can you clarify why this is needed now? It looks like the MessageEvent
(which
> > calls registerMemoryAllocatedWithCurrentScriptContext) isn't created until
it's
> > received by the second thread, so would there ever be external memory
registered
> > at the time this is called?
> 
>  - transferables (arraybuffers) will be accounted wrt v8 when created.
>  - when serialized and passed along via postMessage(), we neuter the "source"
>    side of the transferables, passing along the array buffer contents.
>  - the allocation cost still resides with the "source" and not the "target".
>  - ..yet the "source" cannot access the buffer contents, the "target" can.

Ah, it wasn't obvious to me that this was intended to balance
WTF::ArrayBufferContents::adjustAmountOfExternalAllocatedMemory.

Might it be reasonable to add a comment, or possibly even delegate the call to
v8::Isolate::AdjustAmountOfExternalAllocatedMemory to a method on
ArrayBufferContents, so that it's clear that the adjustments are intended to
balance the ones done there (which is fairly distant from here)?

>  - to address that unbalanced accounting, unregister*() discharges the
>    external allocation cost from "source". Cost includes both that of
>    the serialized payload (if registered) and the transferable buffers.
>  - register*() will now _additionally_ also account for the external
>    allocation cost of the transferables buffers when the "target" dispatches
>    the incoming message from a postMessage(). It will however only do so
>    if the allocation cost for these have been discharged via a
>    corresponding unregister*(). This is, like before, done upon
>    MessageEvent construction.
> 
> The upshot is that for a testcase like seen on the associated bug, v8 is now
able to schedule GCs better for the "target" worker context, preventing OOMs for
some workloads.

Yeah, agreed that something along these lines will fix the bug.

[sof, 2017-03-07 19:30:12]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[sof, 2017-03-07 19:30:20]

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:448: if
(m_externallyAllocatedMemory == 1)
On 2017/03/07 19:14:03, sof wrote:
> On 2017/03/07 18:00:04, jbroman wrote:
> > Use of this magic value is confusing. I'd suggest just adding a boolean
member
> > rather being clever.
> 
> It's not particularly clever; I can switch to an int64_t though (and avoid
> spending a word to encode one bit of info on 64-bit arches.)

Done.

[commit-bot: I haz the power, 2017-03-07 19:30:58]

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, 2017-03-07 21:13:50]

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

[commit-bot: I haz the power, 2017-03-07 21:13:51]

Dry run: This issue passed the CQ dry run.

[sof, 2017-03-08 07:07:08]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[sof, 2017-03-08 07:07:17]

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

https://codereview.chromium.org/2734173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:144: void
unregisterMemoryAllocatedByCurrentScriptContext();
On 2017/03/07 19:29:05, jbroman wrote:
> On 2017/03/07 at 19:14:03, sof wrote:
> > On 2017/03/07 18:00:04, jbroman wrote:
> > > Can you clarify why this is needed now? It looks like the MessageEvent
> (which
> > > calls registerMemoryAllocatedWithCurrentScriptContext) isn't created until
> it's
> > > received by the second thread, so would there ever be external memory
> registered
> > > at the time this is called?
> > 
> >  - transferables (arraybuffers) will be accounted wrt v8 when created.
> >  - when serialized and passed along via postMessage(), we neuter the
"source"
> >    side of the transferables, passing along the array buffer contents.
> >  - the allocation cost still resides with the "source" and not the "target".
> >  - ..yet the "source" cannot access the buffer contents, the "target" can.
> 
> Ah, it wasn't obvious to me that this was intended to balance
> WTF::ArrayBufferContents::adjustAmountOfExternalAllocatedMemory.
> 
> Might it be reasonable to add a comment, or possibly even delegate the call to
> v8::Isolate::AdjustAmountOfExternalAllocatedMemory to a method on
> ArrayBufferContents, so that it's clear that the adjustments are intended to
> balance the ones done there (which is fairly distant from here)?
> 

Added a method on ArrayBufferContents to handle the +/- of costs when
entering&leaving a context.

[commit-bot: I haz the power, 2017-03-08 07:07:33]

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

[sof, 2017-03-08 08:53:06]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 08:53:31]

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

[haraken, 2017-03-08 09:15:21]

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

[haraken, 2017-03-08 09:15:22]

LGTM

https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:466:
m_externallyAllocatedMemory = -1;

I'd prefer preparing a dedicated boolean flag rather than hacking on
m_externallyAllocatedMemory.

[sof, 2017-03-08 09:40:20]

https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:466:
m_externallyAllocatedMemory = -1;
On 2017/03/08 09:15:22, haraken wrote:
> 
> I'd prefer preparing a dedicated boolean flag rather than hacking on
> m_externallyAllocatedMemory.

Given that all allocations are within base::kGenericMaxDirectMapped's range in
Blink, I don't think we need to pad out this object further for this one
ephemeral bit. ymmv.

[haraken, 2017-03-08 09:41:39]

On 2017/03/08 09:40:20, sof wrote:
>
https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> (right):
> 
>
https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:466:
> m_externallyAllocatedMemory = -1;
> On 2017/03/08 09:15:22, haraken wrote:
> > 
> > I'd prefer preparing a dedicated boolean flag rather than hacking on
> > m_externallyAllocatedMemory.
> 
> Given that all allocations are within base::kGenericMaxDirectMapped's range in
> Blink, I don't think we need to pad out this object further for this one
> ephemeral bit. ymmv.

ah, sounds reasonable.

[sof, 2017-03-08 10:04:01]

On 2017/03/08 09:41:39, haraken wrote:
> On 2017/03/08 09:40:20, sof wrote:
> >
>
https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:466:
> > m_externallyAllocatedMemory = -1;
> > On 2017/03/08 09:15:22, haraken wrote:
> > > 
> > > I'd prefer preparing a dedicated boolean flag rather than hacking on
> > > m_externallyAllocatedMemory.
> > 
> > Given that all allocations are within base::kGenericMaxDirectMapped's range
in
> > Blink, I don't think we need to pad out this object further for this one
> > ephemeral bit. ymmv.
> 
> ah, sounds reasonable.

I'll wait for jbroman's feedback. My arm could be twisted on representation, but
it would hurt less if it wasn't :)

[commit-bot: I haz the power, 2017-03-08 10:59:16]

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

[commit-bot: I haz the power, 2017-03-08 10:59:17]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-08 16:56:46]

Apologies for twisting your arm, but I still don't understand the need to use
negative values this way. I've clarified (or I hope I have) what I'm suggesting.

https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2734173002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:466:
m_externallyAllocatedMemory = -1;
On 2017/03/08 at 09:40:19, sof wrote:
> On 2017/03/08 09:15:22, haraken wrote:
> > 
> > I'd prefer preparing a dedicated boolean flag rather than hacking on
> > m_externallyAllocatedMemory.
> 
> Given that all allocations are within base::kGenericMaxDirectMapped's range in
Blink, I don't think we need to pad out this object further for this one
ephemeral bit. ymmv.

If you are concerned about the size of this object, could you not do, with
essentially the same semantics but fewer magic constants by a bitfield instead
of the sign bit. Something along the lines of:

uint64_t m_externallyAllocatedMemory : 63;                            // zero by
default
uint64_t m_externallyAllocatedMemoryForTransferrablesRegistered : 1;  // true by
default, IIUC?

void SerializedScriptValue::unregisterMemoryAllocatedByCurrentScriptContext() {
  if (m_externallyAllocatedMemory) {
    v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
        -static_cast<intptr_t>(m_externallyAllocatedMemory));
    m_externallyAllocatedMemory = 0;
  }

  // TODO: if other transferables start accounting for their external
  // allocations with V8, extend this with corresponding cases.
  if (!m_externallyAllocatedMemoryForTransferrablesRegistered)
    return;
  if (m_arrayBufferContentsArray) {
    for (const auto& contents : *m_arrayBufferContentsArray) {
      contents.adjustExternalAllocatedMemoryUponContextTransfer(
          WTF::ArrayBufferContents::Leave);
    }
  }
  m_externallyAllocatedMemoryForTransferrablesRegistered = false;
}

void SerializedScriptValue::registerMemoryAllocatedWithCurrentScriptContext() {
  if (!m_externallyAllocatedMemory) {
    m_externallyAllocatedMemory = dataLengthInBytes();
    v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
        static_cast<intptr_t>(m_externallyAllocatedMemory));
  }

  // TODO: if other transferables start accounting for their external
  // allocations with V8, extend this with corresponding cases.
  if (m_externallyAllocatedMemoryForTransferrablesRegistered)
    return;

  if (m_arrayBufferContentsArray) {
    for (const auto& contents : *m_arrayBufferContentsArray) {
      contents.adjustExternalAllocatedMemoryUponContextTransfer(
          WTF::ArrayBufferContents::Enter);
    }
  }
  m_externallyAllocatedMemoryForTransferrablesRegistered = true;
}

To me, this is easier to reason about.

There's a case to be made that m_externallyAllocatedMemory could itself be
replaced by a boolean (since after serialization it's either equal to
dataLengthInBytes or it's zero, which is only two states).

[sof, 2017-03-08 18:41:55]

done.

[sof, 2017-03-08 18:47:32]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 18:48:11]

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, 2017-03-08 19:43:13]

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

[commit-bot: I haz the power, 2017-03-08 19:43:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2017-03-08 22:43:17]

lgtm

https://codereview.chromium.org/2734173002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2734173002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:460: for
(size_t i = 0; i < m_arrayBufferContentsArray->size(); ++i) {
super-nit: here and below, a range-based loop would be more concise:

for (const auto& buffer : *m_arrayBufferContentsArray) {
}

https://codereview.chromium.org/2734173002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:477:
int64_t diff = static_cast<int64_t>(m_externallyAllocatedMemory);
nit: intptr_t, since that's what AdjustAmountOfExternalAllocatedMemory takes and
nothing else is done with this?

[sof, 2017-03-09 06:24:33]

https://codereview.chromium.org/2734173002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2734173002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:460: for
(size_t i = 0; i < m_arrayBufferContentsArray->size(); ++i) {
On 2017/03/08 22:43:16, jbroman wrote:
> super-nit: here and below, a range-based loop would be more concise:
> 
> for (const auto& buffer : *m_arrayBufferContentsArray) {
> }

Switched idiom.

https://codereview.chromium.org/2734173002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:477:
int64_t diff = static_cast<int64_t>(m_externallyAllocatedMemory);
On 2017/03/08 22:43:16, jbroman wrote:
> nit: intptr_t, since that's what AdjustAmountOfExternalAllocatedMemory takes
and
> nothing else is done with this?

int64_t Isolate::AdjustAmountOfExternalAllocatedMemory(int64_t) is the signature
we're working with.

[sof, 2017-03-09 06:24:42]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-03-09 06:24:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2734173002/#ps140001
(title: "loop tweak")

[commit-bot: I haz the power, 2017-03-09 06:25:05]

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, 2017-03-09 07:23:52]

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

[commit-bot: I haz the power, 2017-03-09 07:23:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sof, 2017-03-09 07:55:18]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 07:55:40]

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

[sof, 2017-03-09 08:59:38]

Last trybot run (thankfully) flushed out a condition that hadn't been accounted
for -- we shouldn't discount the cost of a SerializedScriptValue if
postMessage() didn't end up transferring the ownership of that value.

Updated the CL with the extra check.

[commit-bot: I haz the power, 2017-03-09 09:46:39]

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

[commit-bot: I haz the power, 2017-03-09 09:46:40]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-09 21:48:02]

lgtm

[sof, 2017-03-09 22:00:38]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-03-09 22:00:39]

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/2734173002/#ps160001
(title: "check for successful ownership transfer before unregistering")

[commit-bot: I haz the power, 2017-03-09 22:01:26]

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, 2017-03-09 22:07:59]

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

[commit-bot: I haz the power, 2017-03-09 22:08:01]

Failed to apply patch for
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:179
  error: third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:
patch does not apply

Patch:       third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
Index: third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
index
ceb9d63e64389f54d7ef4cec4ed2bf23c86c38de..49d31136d9c8e968e0799e8e49d3431972c138de
100644
--- a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
+++ b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h
@@ -128,13 +128,21 @@ class CORE_EXPORT SerializedScriptValue
       const ImageBitmapArray&,
       ExceptionState&);
 
-  // Informs the V8 about external memory allocated and owned by this object.
+  // Informs V8 about external memory allocated and owned by this object.
   // Large values should contribute to GC counters to eventually trigger a GC,
   // otherwise flood of postMessage() can cause OOM.
   // Ok to invoke multiple times (only adds memory once).
   // The memory registration is revoked automatically in destructor.
   void registerMemoryAllocatedWithCurrentScriptContext();
 
+  // Upon passing a serialized value from one context to another (via a
+  // postMessage()), the allocation amounts it has registered with the
+  // 'origining' context must be discharged, as the 'target' context will
assume
+  // ownership of value. This method takes care of the first part of the
+  // external allocation bookkeeping, the above registration method the other
+  // half.
+  void unregisterMemoryAllocatedByCurrentScriptContext();
+
   const uint8_t* data() const { return m_dataBuffer.get(); }
   size_t dataLengthInBytes() const { return m_dataBufferSize; }
 
@@ -179,7 +187,8 @@ class CORE_EXPORT SerializedScriptValue
   std::unique_ptr<ArrayBufferContentsArray> m_arrayBufferContentsArray;
   std::unique_ptr<ImageBitmapContentsArray> m_imageBitmapContentsArray;
   BlobDataHandleMap m_blobDataHandles;
-  intptr_t m_externallyAllocatedMemory;
+  size_t m_externallyAllocatedMemory;
+  bool m_adjustTransferableExternalAllocationOnContextTransfer;
 };
 
 }  // namespace blink

[sof, 2017-03-09 22:18:16]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-03-09 22:18:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2734173002/#ps180001
(title: "rebased upto r455878")

[commit-bot: I haz the power, 2017-03-09 22:19:02]

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, 2017-03-09 23:47:01]

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

[commit-bot: I haz the power, 2017-03-09 23:47:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2017-03-10 05:49:42]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2017-03-10 05:49:56]

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, 2017-03-10 06:29:41]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489124982472040,
"parent_rev": "c995cd504967cbdead7960097409835d6efde089", "commit_rev":
"c7711215a15658e8932087b6a9676859a89f7609"}

[commit-bot: I haz the power, 2017-03-10 06:30:57]

Description was changed from

==========
postMessage(): transfer allocation costs along with value.

A MessageEvent's data value will in some cases hold on to significant
amounts of off-heap memory, so we take care of registering that
external allocation with v8, so that it can be taken into consideration
by the GC triggering logic.

However, when posting a message to another context, we must arrange for
its total 'external allocation' to be associated with the target context.
Including the sizes of any transferables (array buffers), so balance the
books more accurately by also transferring the external allocation cost
of those transferables.

R=
BUG=698981
==========

to

==========
postMessage(): transfer allocation costs along with value.

A MessageEvent's data value will in some cases hold on to significant
amounts of off-heap memory, so we take care of registering that
external allocation with v8, so that it can be taken into consideration
by the GC triggering logic.

However, when posting a message to another context, we must arrange for
its total 'external allocation' to be associated with the target context.
Including the sizes of any transferables (array buffers), so balance the
books more accurately by also transferring the external allocation cost
of those transferables.

R=
BUG=698981

Review-Url: https://codereview.chromium.org/2734173002
Cr-Commit-Position: refs/heads/master@{#456009}
Committed:
https://chromium.googlesource.com/chromium/src/+/c7711215a15658e8932087b6a967...
==========

[commit-bot: I haz the power, 2017-03-10 06:30:58]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/c7711215a15658e8932087b6a967...