[webkit-reviews] review denied: [Bug 30186] Optimization: Many StringImpl transformations are no-ops and should just return 'this' : [Attachment 40918] patch 2

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jens Alfke <snej at chromium.org>'s
request for review:
Bug 30186: Optimization: Many StringImpl transformations are no-ops and should
just return 'this'
https://bugs.webkit.org/show_bug.cgi?id=30186

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

------- Additional Comments from David Levin <levin at chromium.org>
Thanks.  This looks good.

Just a little clean and a potential bug to address.

> Index: WebCore/ChangeLog
> +2009-10-08  Jens Alfke  <snej at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
Please add 
    bug title
    bug link
    blank line
here.

> +	   Optimized StringImpl methods lower(), stripWhiteSpace() and
simplifyWhiteSpace() to
> +	   detect no-ops and return this instead of creating a new instance.
> +	   Empirical testing shows that the majority of calls to these methods
are no-ops, making
> +	   this worthwhile even if (in the case of lower()) the non-no-op case
is slightly slowed.
> +	   Upper() is very rarely a no-op, so it wasn't worthwhile to optimize
it.


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

>  PassRefPtr<StringImpl> StringImpl::lower()

> +    UChar* data;
> +    RefPtr<StringImpl> newImpl = createUninitialized(m_length, data);
> +
> +    if (!(ored & ~0x7F)) {
> +	   // Do a faster loop for the case where all the characters are ASCII.

> +	   for (int i = 0; i < length; i++) {
> +	       UChar c = m_data[i];
> +	       data[i] = toASCIILower(c);
> +	   }
> +	   return newImpl;

return newImpl.release();

Since you changed to a RefPtr (at my request).
  


> @@ -346,6 +363,9 @@ PassRefPtr<StringImpl> StringImpl::simpl
>      if (outc > 0 && to[outc - 1] == ' ')
>	   outc--;
>      
> +    if (static_cast<unsigned>(outc) == m_length)
> +	   return this;
> +    

It looks like outc may equal m_length, but a newline may have been converted to
a space. In this case, "this" is not equivalent to "data".