[webkit-reviews] review denied: [Bug 24739] Combine StringImpl struct and data allocations : [Attachment 28821] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Mar 21 07:40:13 PDT 2009


Darin Adler <darin at apple.com> has denied Mike Belshe <mike at belshe.com>'s
request for review:
Bug 24739: Combine StringImpl struct and data allocations
https://bugs.webkit.org/show_bug.cgi?id=24739

Attachment 28821: patch
https://bugs.webkit.org/attachment.cgi?id=28821&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   Rework StringImpl::create methods to try to allocate
> +	   a single buffer rahter than allocating both the StringImpl class
> +	   and a separate data buffer.

I like this idea! Seems like a good approach that will save us quite a bit of
memory.

> +    // Allocate a single buffer large enough to contain the StringImpl
> +    // struct as well as the data which it contains.  This removes one 
> +    // heap allocation from this call.
> +    int size = sizeof(StringImpl) + (length * sizeof(UChar));
> +    char* buffer = static_cast<char*>(fastMalloc(size));
> +    UChar* data = reinterpret_cast<UChar*>(buffer + sizeof(StringImpl));
> +    memcpy(data, characters, length * sizeof(UChar));
> +    StringImpl* rv = new (buffer) StringImpl(data, length, AdoptBuffer());
> +    rv->m_internalBuffer = true;
> +    return adoptRef(rv);

If we’ve created a StringImpl this way, then we can’t just delete it with a
“delete” call. Instead we need to separately call the destructor and then call
fastFree. As you can see in https://bugs.webkit.org/show_bug.cgi?id=22834 and
http://trac.webkit.org/changeset/41877 if we mismatch fastMalloc and delete we
will end up causing trouble for valgrind.

But it’s relatively easy to fix. We can always use fastMalloc and placement new
to allocate StringImpl, even when the buffer is not internal, and then we can
implement our own deletion that uses fastFree instead of delete. The way to
implement our own deletion is to inherit from RefCountedBase instead of from
RefCounted<StringImpl> and then write our own deref function:

    void deref() {
	if (derefBase()) {
	    this->~StringImpl();
	    fastFree(this);
	}
    }

Also, WebKit code would normally use a name like "string" rather than "rv" for
the newly created StringImpl.

> +    // In some cases, we allocate the StringImpl struct and its data
> +    // within a single heap buffer.	In this case, the m_data pointer
> +    // is an "internal buffer", and does not need to be deallocated.
> +    bool m_internalBuffer;

m_internalBuffer is a good name for a buffer, but not a good name for a boolean
telling you the type of buffer. m_bufferIsInternal or m_isInternalBuffer would
be a better name for a boolean. But I don't think we need the boolean. You can
write it like this:

    if (m_data != reinterpret_cast<UChar*>(this + 1))
	deleteUCharVector(m_data);

I think it’s better for the future to not have a boolean for this, even though
at the moment we can afford it. Not having to set that boolean will streamline
the creation code a bit too.

I’m going to say review- because of the easily-fixable fastMalloc/delete
mismatch.


More information about the webkit-reviews mailing list