Bug 23674 – Speed up some things based on profiling the page load test

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

The Safari team has a page load test that we use to measure performance. It's not open source because it contains materials that are copyrighted and we don't have the right to publish.

I ran it recently and profiled it and found some things I could speed up simply.

[Darin Adler]

Created attachment 27231 [details]
patch

[Darin Adler]

Created attachment 27233 [details]
patch

First version was missing parentheses in one place and thus didn't compile.

[David Levin]

I did a quick look out of curiosity.

Two small comments:

In WebCore/loader/TextResourceDecoder.cpp 

 if (dataStart[0] == '@'
	            && dataStart[1] == 'c'
    	            && dataStart[2] == 'h'
Indent appears off for the && lines.

----
In StringImpl::createStrippingNullCharacters

    int foundNull = 0;
    for (unsigned i = 0; !foundNull && i < length; i++) {
        int c = characters[i]; // more efficient than using UChar here (at least on Intel Mac OS)
        foundNull |= !c;
    }

It could be "foundNull = !c;"  (no bitwise or)
No sure if this will matter but since it mentioned as hot and things like using int instead of UChar were considered, I thought I'd point it out.

[Oliver Hunt]

Comment on attachment 27233 [details]
patch

I am concerned that this patch seems to have a large amount of seemingly unrelated and non-local changes, i'm especially concerned about the inclusion of text decoder changes..  After some consideration i'd like this patch separated out into a few parts
* reserveInitialCapacity addition/use
* TextDecoder changes
* All the inlining changes

[Darin Adler]

(In reply to comment #4)
> * reserveInitialCapacity addition/use
> * TextDecoder changes
> * All the inlining changes

Seems fine.

[Darin Adler]

Created attachment 27242 [details]
patch — this time without TextResourceDecoder or reserveInitialCapacity changes as requested

The reserveInitialCapacity changes are now bug 23676.

The TextResourceDecoder changes are now bug 23677.

[Darin Adler]

(In reply to comment #3)
> In StringImpl::createStrippingNullCharacters
> 
>     int foundNull = 0;
>     for (unsigned i = 0; !foundNull && i < length; i++) {
>         int c = characters[i]; // more efficient than using UChar here (at
> least on Intel Mac OS)
>         foundNull |= !c;
>     }
> 
> It could be "foundNull = !c;"  (no bitwise or)

Good point. I just moved the code, didn't change it, but it seems obviously less efficient to do the |= there.

[Darin Adler]

Ollie, please let me know if you'd like this broken into even more pieces. It's quite a bit of work, but I'd be happy to do it.

I have a bunch more of these ideas that I'd like to tackle once these are landed.

[Dave Hyatt]

Comment on attachment 27242 [details]
patch — this time without TextResourceDecoder or reserveInitialCapacity changes as requested

r=me, although I don't really like "firstLineStyleSlowCase" as a function name, but I can't think of anything better.

[Darin Adler]

(In reply to comment #9)
> I don't really like "firstLineStyleSlowCase" as a function name,
> but I can't think of anything better.

In an earlier version I called it something like computeFirstLineStyle.

[Darin Adler]

trac.webkit.org/changeset/40475

[Darin Adler]

http://trac.webkit.org/changeset/40475

Regressions fixed in:
http://trac.webkit.org/changeset/40484
http://trac.webkit.org/changeset/40499