[webkit-reviews] review granted: [Bug 30261] Optimization: Reduce calls to StringImpl's upper / lower / isLower : [Attachment 41124] patch 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jens Alfke <snej at chromium.org>'s
request for review:
Bug 30261: Optimization: Reduce calls to StringImpl's upper / lower / isLower
https://bugs.webkit.org/show_bug.cgi?id=30261

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // Note: This is a hot function in the Dromaeo benchmark.
> +    StringImpl *myImpl = impl();

The * here needs to be next to StringImpl, not next to myImpl.

Informally, the style in WebKit code is to do it like this:

    StringImpl* impl = this->impl();

Rather than using a prefix like "my".

> -    for (int i = 0; i < length; i++) {
> -	   UChar c = m_data[i];
> -	   ored |= c;
> -	   noUpper = noUpper && !isASCIIUpper(c);
> +    for (const UChar* chp = m_data, *end = m_data + m_length; chp != end;
chp++) {
> +	   if (UNLIKELY(isASCIIUpper(*chp)))
> +	       noUpper = false;
> +	   ored |= *chp;
>      }

I suggest declaring the end pointer outside the loop to avoid the multiple
initialization in the for, which is something we almost never do.

We've found in the past that indexing is as efficient as pointer chasing, so
I'm surprised that this change was needed to speed things up.

r=me but you should consider both comments above. Some reviewers might review-
just based on the StringImpl* part ;-)