[webkit-reviews] review denied: [Bug 29500] Reduce memory usage of WebCore::StringImpl : [Attachment 39786] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jens Alfke <snej at chromium.org>'s
request for review:
Bug 29500: Reduce memory usage of WebCore::StringImpl
https://bugs.webkit.org/show_bug.cgi?id=29500

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

------- Additional Comments from David Levin <levin at chromium.org>
StringImpl(const UChar*, unsigned length, unsigned hash); copies the buffer.
StringImpl(const UChar*, unsigned length); doesn't.
It is very surprising that the buffer is either adopted or copied depending on
whether I pass in the hash value for the string or not.

net: I recommend that you bring back the AdoptBuffer parameter for the
constructor that does adopt the buffer.


> Index: WebCore/ChangeLog
> +	   - Remove unnecessary m_bufferIsInternal member (saves 4 bytes).

Great stuff!

> +	   - Use 16-bit flag member if not using JSC (saves 2 bytes, or 6 in
64-bit).

This doesn't seem useful for several reasons:
1. StringImpl has a pointer in it and ends with a m_buffer[], so the m_flags
member be have padding after it for alignment reasons, so there is no space
savings here.
2. Although I've not yet had a chance to do so :(, this field will also be used
for sharing the buffers across threads which will also be useful to platforms
that don't use jsc. Not having this would put those platforms at a perf
disadvantage when they have to allocate and copy strings for these scenarios.
3. This part of the change makes the code more complicated to understand and
maintain (This alone wouldn't be a reason to avoid this but is the final straw
after 1 and 2.)

oth, I think "unsigned" defaults to 32 bits under 64-bit builds. (Unless the
compiler uses an ILP64 model and I'm pretty sure xcode's gcc does LP64 and
Win64 uses LLP64 which both treat int's as 32 bits.) 

If it is 32 bits, then moving the two unsigned field (m_length, m_hash) next
two each other could save space for that 64bit platforms.


> Index: WebCore/platform/text/StringImpl.cpp

> -
> +
Please don't do misc trailing whitespace changes.
    
>  PassRefPtr<StringImpl> StringImpl::createUninitialized(unsigned length,
UChar*& data)
>  {
>      if (!length) {
> @@ -1006,11 +982,10 @@ PassRefPtr<StringImpl> StringImpl::creat

It seems that the only thing that needs to be done is this:

> -    StringImpl* string = new (buffer) StringImpl(data, length,
AdoptBuffer());
> -    string->m_bufferIsInternal = true;
> +    string = new (buffer) StringImpl(data, length);

The rest just changes 3 lines of code into 3 lines of code (and I find the
previous 3 lines of code easier to read, so this is pretty subjective).
Net: Please do the minimal change here.


>  PassRefPtr<StringImpl> StringImpl::createWithTerminatingNullCharacter(const
StringImpl& string)
>  {
> -    return adoptRef(new StringImpl(string, WithTerminatingNullCharacter()));

> +    // Use createUninitialized instead of 'new StringImpl' so that the
string and its buffer
> +    // get allocated in a single malloc block.
> +    UChar* data;
> +    int length = string.m_length;
> +    PassRefPtr<StringImpl> paddedString = createUninitialized(length + 1,
data);

Use RefPtr within functions not PassRefPtr (and then do release when returning
it).
Seems like terminatedString would be more appropriate than paddedString.

>  PassRefPtr<StringImpl> StringImpl::copy()
>  {
> -    // Using the constructor directly to make sure that per-thread empty
string instance isn't returned.
> -    return adoptRef(new StringImpl(m_data, m_length));
> +    // Special-case empty string to make sure that per-thread empty string
instance isn't returned.
> +    if (!m_length)
> +	   return adoptRef(new StringImpl());
> +    else
> +	   return create(m_data, m_length);

Don't add an else when it isn't needed (if there is a return right before it). 


(very optional) Since I suspect that m_length > 0 more often than it is 0, I
would reverse the conditions here (but this probably won't be noticeable at all
given the allocations that will happen).

    if (m_length)
	return create(m_data, m_length);
    return adoptRef(new StringImpl());



> Index: WebCore/platform/text/StringImpl.h

> -
> +    
WebKit specifically does not care about trailing whitespace so please don't
remove it.  It only adds to the size of your patch and the number of diffs.


> +    // We usually allocate the StringImpl struct and its data

usually? seems like it could change.
Why not just state that this flag "Indicates when the StringImpl and m_data
were created with a single allocation."

> +    // within a single heap buffer. In this case, the m_data pointer
> +    // is an "internal buffer", and does not need to be deallocated.
> +    bool bufferIsInternal() { return m_data == &m_buffer[0]; }


> +    inline bool isFlagSet(StringImplFlags flag) const;
> +    inline void setFlag(StringImplFlags flag);

"flag" param name doesn't add any information so please remove it. (In fact I
suspect this change will go away complete when the if USE(JSC) stuff is
removed.


> +    const UChar m_buffer[0]; // m_data often points here (past the end of
the object.)

"often" seems subjective/relative and may easily change.


> +inline bool StringImpl::isFlagSet(StringImplFlags flag) const
> +{
> +    return (m_flags & (1 << flag)) != 0;

Don't do comparisons to 0.

btw, check-webkit-style would have caught a few of the things I mentioning
here. (This should be one of them.) 

Of course this code should go away...