This function is called quite a lot and is trivial to optimize.
Created attachment 49373 [details] Proposed patch
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
Is there any measured speedup on benchmarks or microbenchmarks?
(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
Created attachment 49379 [details] Same patch with fleshed-out ChangeLog
(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.
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?
(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.
Do you see an improvement in speed with the patch (in ms please)?
Comment on attachment 49379 [details] Same patch with fleshed-out ChangeLog Does this cause measurable speedup?
(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 on attachment 49379 [details] Same patch with fleshed-out ChangeLog Clearing flags on attachment: 49379 Committed r55224: <http://trac.webkit.org/changeset/55224>
All reviewed patches have been landed. Closing bug.