postMessage(): transfer allocation costs along with value.

third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp

Index: third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
index a75fca0dd2f36a3bb7e437581a0bf871b6ab9e4d..206ecfb24d1371abadd354a9490eb8f4df54f328 100644
--- a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
@@ -125,7 +125,7 @@ SerializedScriptValue::~SerializedScriptValue() {
   // If the allocated memory was not registered before, then this class is
   // likely used in a context other than Worker's onmessage environment and the
   // presence of current v8 context is not guaranteed. Avoid calling v8 then.
-  if (m_externallyAllocatedMemory) {
+  if (m_externallyAllocatedMemory > 0) {
     ASSERT(v8::Isolate::GetCurrent());
     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
         -m_externallyAllocatedMemory);
@@ -444,13 +444,46 @@ SerializedScriptValue::transferArrayBufferContents(
   return contents;
 }
 
+void SerializedScriptValue::unregisterMemoryAllocatedByCurrentScriptContext() {
+  if (m_externallyAllocatedMemory < 0)
+    return;
+  if (m_externallyAllocatedMemory) {
+    v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
+        -(m_externallyAllocatedMemory - 1));
+    m_externallyAllocatedMemory = 0;
+  }
+  // TODO: if other transferables start accounting for their external
+  // allocations with V8, extend this with corresponding cases.
+  if (m_arrayBufferContentsArray) {
+    for (size_t i = 0; i < m_arrayBufferContentsArray->size(); ++i) {
+      WTF::ArrayBufferContents& buffer = m_arrayBufferContentsArray->at(i);
+      buffer.adjustExternalAllocatedMemoryUponContextTransfer(
+          WTF::ArrayBufferContents::Leave);
+    }
+    // Assign a negative size to mark the value as needing re-registration
+    // of transferable allocation costs by its target context in
+    // registerMemoryAllocatedWithCurrentScriptContext().
+    m_externallyAllocatedMemory = -1;

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

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

[sof, 2017/03/08 09:40:19]

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.

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

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).

+  }
+}
+
 void SerializedScriptValue::registerMemoryAllocatedWithCurrentScriptContext() {
-  if (m_externallyAllocatedMemory)
+  if (m_externallyAllocatedMemory > 0)
     return;
 
-  m_externallyAllocatedMemory = static_cast<intptr_t>(dataLengthInBytes());
+  bool registerTransferables = m_externallyAllocatedMemory < 0;
+  m_externallyAllocatedMemory = static_cast<int64_t>(dataLengthInBytes());
+  DCHECK_GE(m_externallyAllocatedMemory, 0);
   v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
       m_externallyAllocatedMemory);
+  if (registerTransferables) {
+    DCHECK(m_arrayBufferContentsArray);
+    for (size_t i = 0; i < m_arrayBufferContentsArray->size(); ++i) {
+      WTF::ArrayBufferContents& buffer = m_arrayBufferContentsArray->at(i);
+      buffer.adjustExternalAllocatedMemoryUponContextTransfer(
+          WTF::ArrayBufferContents::Enter);
+    }
+  }
 }
 
 }  // namespace blink