Bug 18666 – StringImpl::createStrippingNullCharacters has broken optimization

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mike Belshe]

I believe a bug was introduced in Change #29470.

The culprit is:
   foundNull |= ~c;      // This is a one's complement

it should be:
   foundNull |= !c;      // if c is 0, then foundNull is true.

The effect is that the optimization in this function doesn't work, and we usually take two passes through the entire string.

I went ahead and wrote a test to optimize further.  My main concern was the copying which is done byte by byte.  I wrote a quick benchmark and tested the conversion of a 10byte string, a 1000 byte string, and a 10000 byte string.  I optimized it, and ran in a loop of 100,000 iterations.  

Results:
                   10B         1000B            10000B 
CURRENT            95ms        1888ms           20409ms
FIXED VERSION      79ms        1051ms           10007ms
FIXED + MEMCPY     75ms         801ms            9125ms

So, simply fixing the bug doubles the performance of this routine for medium and large strings.  Switching to the new algorithm (which I'm attaching) gets another 10%.  Overall, this is negligible on page load performance, however.

[Mike Belshe]

Created attachment 20739 [details]
Patch to StringImpl::createStrippingNullCharacters

[Eric Seidel (no email)]

This looks good to me.  Darin shoudl at least see this change go by.  But the change is r=me.  I can land it with ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 20739 [details]
Patch to StringImpl::createStrippingNullCharacters

This looks good.  Generally changes should have ChangeLogs, using prepare-ChangeLog.  I'll add one and land this.

http://webkit.org/coding/contributing.html  -- talks about the process

[Darin Adler]

Comment on attachment 20739 [details]
Patch to StringImpl::createStrippingNullCharacters

Looks fine to me.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/text/StringImpl.cpp
Committed r32350