Bug 35336 – Optimize Font::normalizeSpaces()

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Andreas Kling]

This function is called quite a lot and is trivial to optimize.

[Andreas Kling]

Created attachment 49373 [details]
Proposed patch

[Kenneth Rohde Christiansen]

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

[Sam Weinig]

Is there any measured speedup on benchmarks or microbenchmarks?

[Andreas Kling]

(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

[Andreas Kling]

Created attachment 49379 [details]
Same patch with fleshed-out ChangeLog

[Sam Weinig]

(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.

[Kenneth Rohde Christiansen]

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?

[Andreas Kling]

(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.

[Sam Weinig]

Do you see an improvement in speed with the patch (in ms please)?

[Darin Adler]

Comment on attachment 49379 [details]
Same patch with fleshed-out ChangeLog

Does this cause measurable speedup?

[Benjamin Poulain]

(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.

[WebKit Commit Bot]

Comment on attachment 49379 [details]
Same patch with fleshed-out ChangeLog

Clearing flags on attachment: 49379

Committed r55224: <http://trac.webkit.org/changeset/55224>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.