Bug 35336

Summary: Optimize Font::normalizeSpaces()
Product: WebKit Reporter: Andreas Kling <kling>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, kenneth, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Proposed patch
kenneth: review-
Same patch with fleshed-out ChangeLog none

Description Andreas Kling 2010-02-24 03:19:18 PST
This function is called quite a lot and is trivial to optimize.
Comment 1 Andreas Kling 2010-02-24 03:20:45 PST
Created attachment 49373 [details]
Proposed patch
Comment 2 Kenneth Rohde Christiansen 2010-02-24 04:28:23 PST
Comment on attachment 49373 [details]
Proposed patch

The changelog should explain what change you made and not just say "optimized Font::normalizeSpace()".

Apart from that LGTM
Comment 3 Sam Weinig 2010-02-24 04:46:46 PST
Is there any measured speedup on benchmarks or microbenchmarks?
Comment 4 Andreas Kling 2010-02-24 05:10:04 PST
(In reply to comment #3)
> Is there any measured speedup on benchmarks or microbenchmarks?

Tested with the NASM manual (!), I get these insn fetch counts with callgrind:

Before: 18492923
After: 16805832
Comment 5 Andreas Kling 2010-02-24 05:11:00 PST
Created attachment 49379 [details]
Same patch with fleshed-out ChangeLog
Comment 6 Sam Weinig 2010-02-24 05:54:09 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Is there any measured speedup on benchmarks or microbenchmarks?
> 
> Tested with the NASM manual (!), I get these insn fetch counts with callgrind:
> 
> Before: 18492923
> After: 16805832

That's not really what I meant.  I meant are there any benchmarks (HTML content) where this improves page loading or rending time.  I am not even sure what you are testing with the callgrind data.
Comment 7 Kenneth Rohde Christiansen 2010-02-24 05:56:44 PST
I believe he was testing some of our reference sites (slashdot.org etc), and this one came up as one of the top ones in the profiling. That should have been a bit more clear in the bug report.

Could you please comment Andreas?
Comment 8 Andreas Kling 2010-02-24 06:04:06 PST
(In reply to comment #7)
> I believe he was testing some of our reference sites (slashdot.org etc), and
> this one came up as one of the top ones in the profiling. That should have been
> a bit more clear in the bug report.

This is correct, my apologies for being excessively terse.

I was basically profiling some reference pages with callgrind and looking at the top 5 (or so) functions for page loading, rendering and scrolling.

This particular function showed up on the page loading profiles, so I took a quick look at it and saw an obvious optimization.
Comment 9 Sam Weinig 2010-02-24 06:08:11 PST
Do you see an improvement in speed with the patch (in ms please)?
Comment 10 Darin Adler 2010-02-24 08:21:42 PST
Comment on attachment 49379 [details]
Same patch with fleshed-out ChangeLog

Does this cause measurable speedup?
Comment 11 Benjamin Poulain 2010-02-24 08:43:12 PST
(In reply to comment #10)
> (From update of attachment 49379 [details])
> Does this cause measurable speedup?

Extract from test_Loading on the N900:
        benchmark: http://cs.nyu.edu/courses/fall02/V22.0201-001/nasm_doc_html/nasmdocb.html
                mean:     1043 msecs +/- 4 msecs, +/- 0,383509 %
                avg:      1042 msecs
                        1050, 1045, 1038, 1040, 1039, 1043, 1046, 1042, 1036
        benchmark: http://cs.nyu.edu/courses/fall02/V22.0201-001/nasm_doc_html/nasmdocb.html
                mean:     1030 msecs +/- 3 msecs, +/- 0,291262 %
                avg:      1030 msecs
                        1035, 1036, 1025, 1026, 1030, 1032, 1032, 1026, 1029

Given the variance, it is a <1% improvement.
Comment 12 WebKit Commit Bot 2010-02-24 22:56:43 PST
Comment on attachment 49379 [details]
Same patch with fleshed-out ChangeLog

Clearing flags on attachment: 49379

Committed r55224: <http://trac.webkit.org/changeset/55224>
Comment 13 WebKit Commit Bot 2010-02-24 22:56:48 PST
All reviewed patches have been landed.  Closing bug.