RESOLVED FIXED 23674
Speed up some things based on profiling the page load test
https://bugs.webkit.org/show_bug.cgi?id=23674
Summary Speed up some things based on profiling the page load test
Darin Adler
Reported 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.
Attachments
patch (84.38 KB, patch)
2009-02-01 09:51 PST, Darin Adler
no flags
patch (84.38 KB, patch)
2009-02-01 10:15 PST, Darin Adler
oliver: review-
patch — this time without TextResourceDecoder or reserveInitialCapacity changes as requested (60.57 KB, patch)
2009-02-01 19:03 PST, Darin Adler
hyatt: review+
Darin Adler
Comment 1 2009-02-01 09:51:48 PST
Darin Adler
Comment 2 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.
David Levin
Comment 3 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.
Oliver Hunt
Comment 4 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
Darin Adler
Comment 5 2009-02-01 17:43:48 PST
(In reply to comment #4) > * reserveInitialCapacity addition/use > * TextDecoder changes > * All the inlining changes Seems fine.
Darin Adler
Comment 6 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.
Darin Adler
Comment 7 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.
Darin Adler
Comment 8 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.
Dave Hyatt
Comment 9 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.
Darin Adler
Comment 10 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.
Darin Adler
Comment 11 2009-02-02 09:32:06 PST
trac.webkit.org/changeset/40475
Note You need to log in before you can comment on or make changes to this bug.