Bug 18666

Summary: StringImpl::createStrippingNullCharacters has broken optimization
Product: WebKit Reporter: Mike Belshe <mbelshe>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, mjs
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to StringImpl::createStrippingNullCharacters eric: review+

Mike Belshe
Reported 2008-04-21 13:07:02 PDT
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.
Attachments
Patch to StringImpl::createStrippingNullCharacters (1.40 KB, patch)
2008-04-21 13:07 PDT, Mike Belshe 		eric: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mike Belshe
Comment 1 2008-04-21 13:07:41 PDT
Created attachment 20739 [details] Patch to StringImpl::createStrippingNullCharacters
Eric Seidel (no email)
Comment 2 2008-04-21 13:16:29 PDT
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 3 2008-04-21 13:18:34 PDT
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 4 2008-04-21 13:25:16 PDT
Comment on attachment 20739 [details] Patch to StringImpl::createStrippingNullCharacters Looks fine to me.
Eric Seidel (no email)
Comment 5 2008-04-21 15:17:02 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/platform/text/StringImpl.cpp Committed r32350
Note You need to log in before you can comment on or make changes to this bug.