Bug 23674 - Speed up some things based on profiling the page load test
Summary: Speed up some things based on profiling the page load test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-01 09:50 PST by Darin Adler
Modified: 2009-02-02 16:09 PST (History)
3 users (show)

See Also:


Attachments
patch (84.38 KB, patch)
2009-02-01 09:51 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch (84.38 KB, patch)
2009-02-01 10:15 PST, Darin Adler
oliver: review-
Details | Formatted Diff | Diff
patch — this time without TextResourceDecoder or reserveInitialCapacity changes as requested (60.57 KB, patch)
2009-02-01 19:03 PST, Darin Adler
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-02-01 09:50:27 PST
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.
Comment 1 Darin Adler 2009-02-01 09:51:48 PST
Created attachment 27231 [details]
patch
Comment 2 Darin Adler 2009-02-01 10:15:06 PST
Created attachment 27233 [details]
patch

First version was missing parentheses in one place and thus didn't compile.
Comment 3 David Levin 2009-02-01 11:46:05 PST
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.
Comment 4 Oliver Hunt 2009-02-01 12:27:33 PST
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
Comment 5 Darin Adler 2009-02-01 17:43:48 PST
(In reply to comment #4)
> * reserveInitialCapacity addition/use
> * TextDecoder changes
> * All the inlining changes

Seems fine.
Comment 6 Darin Adler 2009-02-01 19:03:31 PST
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.
Comment 7 Darin Adler 2009-02-01 19:06:30 PST
(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.
Comment 8 Darin Adler 2009-02-01 19:20:52 PST
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.
Comment 9 Dave Hyatt 2009-02-01 20:10:31 PST
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.
Comment 10 Darin Adler 2009-02-02 09:13:59 PST
(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.
Comment 11 Darin Adler 2009-02-02 09:32:06 PST
trac.webkit.org/changeset/40475