Add a ref-counted data holder to ArrayBufferContents

Source/wtf/ArrayBufferContents.cpp

 /*
  * Copyright (C) 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 19 matching lines @@
 #include "wtf/Assertions.h"
 #include "wtf/PartitionAlloc.h"
 #include "wtf/Partitions.h"
 #include <string.h>
 
 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)
 {
     if (s_adjustAmountOfExternalAllocatedMemoryFunction)
         s_adjustAmountOfExternalAllocatedMemoryFunction(static_cast<int>(size));
     data = partitionAllocGenericFlags(WTF::Partitions::bufferPartition(), PartitionAllocReturnNull, size);
     if (policy == ZeroInitialize && data)

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

Not related to this CL, but I think we should just crash here if PartitionAlloc
failed at allocating the data. Continuing the execution won't make much sense
and will just lead to crashes in other places (which are hard to investigate).

         memset(data, '\0', size);
 }
 
 void ArrayBufferContents::freeMemory(void* data, size_t size)
 {
     partitionFreeGeneric(WTF::Partitions::bufferPartition(), data);
     if (s_adjustAmountOfExternalAllocatedMemoryFunction)
         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