Add a ref-counted data holder to ArrayBufferContents

Source/wtf/ArrayBufferContents.cpp

Index: Source/wtf/ArrayBufferContents.cpp
diff --git a/Source/wtf/ArrayBufferContents.cpp b/Source/wtf/ArrayBufferContents.cpp
index b318ebec94c2fa5c69ae37faf6406e2df2de4f7d..98f596ce559ec3f5fb004298a1a940f0e6894144 100644
--- a/Source/wtf/ArrayBufferContents.cpp
+++ b/Source/wtf/ArrayBufferContents.cpp
@@ -37,67 +37,59 @@ namespace WTF {
 AdjustAmountOfExternalAllocatedMemoryFunction ArrayBufferContents::s_adjustAmountOfExternalAllocatedMemoryFunction;
 
 ArrayBufferContents::ArrayBufferContents()
-    : m_data(0)
-    , m_sizeInBytes(0) { }
+    : m_holder(adoptRef(new DataHolder())) { }
 
-ArrayBufferContents::ArrayBufferContents(unsigned numElements, unsigned elementByteSize, ArrayBufferContents::InitializationPolicy policy)
-    : m_data(0)
-    , m_sizeInBytes(0)
+ArrayBufferContents::ArrayBufferContents(unsigned numElements, unsigned elementByteSize, SharingType isShared, ArrayBufferContents::InitializationPolicy policy)
+    : m_holder(adoptRef(new DataHolder()))
 {
     // Do not allow 32-bit overflow of the total size.
+    unsigned totalSize = numElements * elementByteSize;
     if (numElements) {
-        unsigned totalSize = numElements * elementByteSize;
         if (totalSize / numElements != elementByteSize) {
-            m_data = 0;
             return;
         }
     }
-    allocateMemory(numElements * elementByteSize, policy, m_data);
-    m_sizeInBytes = numElements * elementByteSize;
+
+    m_holder->allocateNew(totalSize, isShared, policy);
 }
 
-ArrayBufferContents::ArrayBufferContents(void* data, unsigned sizeInBytes)
-    : m_data(data)
-    , m_sizeInBytes(sizeInBytes)
+ArrayBufferContents::ArrayBufferContents(
+    void* data, unsigned sizeInBytes, SharingType isShared)
+    : m_holder(adoptRef(new DataHolder()))
 {
-    if (!m_data) {
-        ASSERT(!m_sizeInBytes);
-        m_sizeInBytes = 0;
-        // Allow null data if size is 0 bytes, make sure m_data is valid pointer.
+    if (!data) {

[haraken, 2015/06/19 12:05:11]

Avoid using if(!...) {} else {} pattern.

[binji, 2015/06/19 18:14:57]

Done.

+        ASSERT(!sizeInBytes);
+        sizeInBytes = 0;
+        // Allow null data if size is 0 bytes, make sure data is valid pointer.
         // (partitionAllocGeneric guarantees valid pointer for size 0)

[haraken, 2015/06/19 12:05:11]

Nit: partitionAllocGeneric => PartitionAlloc (ArrayBuffer uses a dedicated
partition for ArrayBuffers. It's different from partitionAllocGeneric.)

[binji, 2015/06/19 18:14:57]

Done.

-        allocateMemory(0, ZeroInitialize, m_data);
+        m_holder->allocateNew(sizeInBytes, isShared, ZeroInitialize);
+    } else {
+        m_holder->adopt(data, sizeInBytes, isShared);

[haraken, 2015/06/19 12:05:11]

Help me understand: Why isn't this allocateNew? If isShared is NotShared, I
guess we need to call allocateNew.

[binji, 2015/06/19 18:14:57]

The previous behavior was to assign data to m_data without allocation if data is
not NULL. This keeps that behavior.

     }
 }
 
 ArrayBufferContents::~ArrayBufferContents()
 {
-    freeMemory(m_data, m_sizeInBytes);
     clear();
 }
 
 void ArrayBufferContents::clear()
 {
-    m_data = 0;
-    m_sizeInBytes = 0;
+    m_holder.clear();

[haraken, 2015/06/19 12:05:12]

I think it's better to introduce DataHolder::clear. We can call
m_holder->clear() here. (See the below comment.)

[binji, 2015/06/19 18:14:57]

I think that has the wrong behavior if this ArrayBufferContents is shared. I
would not expect clear to clear the data from another buffer.

I did a compile check, and this function is only used in two places:
ArrayBuffer::transfer and ArrayBufferContents::transfer. In both cases it is
meant to be equivalent to "neuter". I've changed the name for clarity.

 }
 
 void ArrayBufferContents::transfer(ArrayBufferContents& other)
 {
-    ASSERT(!other.m_data);
-    other.m_data = m_data;
-    other.m_sizeInBytes = m_sizeInBytes;
-    clear();
+    ASSERT(!other.m_holder->data());
+    other.m_holder = m_holder;
+    if (!m_holder->isShared())

[haraken, 2015/06/19 12:05:12]

It would be better to just call clear() here and let DataHolder::clear() decide
what to do there (i.e., not clear the data is the data is a shared one).

Question: Why don't we clear the data if the data is a shared one?

[binji, 2015/06/19 18:14:57]

AFAICT, we never want to clear() once we have shared contents, because we never
know what the other owner(s) want to do with the data. So I don't think adding
DataHolder::clear makes sense.
A shared ArrayBuffer needs to share its contents with another ArrayBuffer via
the ref-counted DataHolder. So if it cleared the data (or neutered itself,
removing its reference to the data), it wouldn't be shared anymore.

+        clear();
 }
 
 void ArrayBufferContents::copyTo(ArrayBufferContents& other)
 {
-    ASSERT(!other.m_sizeInBytes);
-    other.freeMemory(other.m_data, other.m_sizeInBytes);
-    allocateMemory(m_sizeInBytes, DontInitialize, other.m_data);
-    if (!other.m_data)
-        return;
-    memcpy(other.m_data, m_data, m_sizeInBytes);
-    other.m_sizeInBytes = m_sizeInBytes;
+    ASSERT(!m_holder->isShared() && !other.m_holder->isShared());

[haraken, 2015/06/19 12:05:12]

Is this a TODO? Or are you not planning to support copyTo for shared buffers?

[binji, 2015/06/19 18:14:57]

AFAICT, this is only used by ArrayBuffer::transfer when the ArrayBuffer being
copied from cannot be neutered. That can never happen with SharedArrayBuffers,
so this function should never be called by SharedArrayBuffers. I suppose I can
write the functionality for it, but it would be untested.

+    m_holder->copyMemoryTo(*other.m_holder);
 }
 
 void ArrayBufferContents::allocateMemory(size_t size, InitializationPolicy policy, void*& data)
@@ -116,4 +108,47 @@ void ArrayBufferContents::freeMemory(void* data, size_t size)
         s_adjustAmountOfExternalAllocatedMemoryFunction(-static_cast<int>(size));
 }
 
+ArrayBufferContents::DataHolder::DataHolder()
+    : m_data(nullptr)
+    , m_sizeInBytes(0)
+    , m_isShared(NotShared) { }
+
+ArrayBufferContents::DataHolder::~DataHolder()

[haraken, 2015/06/19 12:05:12]

You can do this in DataHolder::clear().

+{
+    ArrayBufferContents::freeMemory(m_data, m_sizeInBytes);
+
+    m_data = nullptr;
+    m_sizeInBytes = 0;
+    m_isShared = NotShared;
+}
+
+void ArrayBufferContents::DataHolder::allocateNew(unsigned sizeInBytes, SharingType isShared, InitializationPolicy policy)
+{
+    ASSERT(!m_data);
+    void* data = nullptr;
+    allocateMemory(sizeInBytes, policy, data);
+    m_data = data;
+    m_sizeInBytes = sizeInBytes;
+    m_isShared = isShared;
+}
+
+void ArrayBufferContents::DataHolder::adopt(void* data, unsigned sizeInBytes, SharingType isShared)

[haraken, 2015/06/19 12:05:12]

Question: What is the purpose of adopt?

[binji, 2015/06/19 18:14:57]

To take over the passed in |data| pointer, without allocating new memory.

+{
+    ASSERT(!m_data);
+    m_data = data;
+    m_sizeInBytes = sizeInBytes;
+    m_isShared = isShared;
+}
+
+void ArrayBufferContents::DataHolder::copyMemoryTo(DataHolder& other)
+{
+    ASSERT(!other.m_sizeInBytes);
+    ArrayBufferContents::freeMemory(other.m_data, other.m_sizeInBytes);
+    ArrayBufferContents::allocateMemory(m_sizeInBytes, DontInitialize, other.m_data);
+    if (!other.m_data)
+        return;
+    memcpy(other.m_data, m_data, m_sizeInBytes);
+    other.m_sizeInBytes = m_sizeInBytes;
+}
+
 } // namespace WTF