35336 2010-02-24 03:19:18 -0800 Optimize Font::normalizeSpaces() 2010-02-24 22:56:48 -0800 1 1 1 Unclassified WebKit Text 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 kling webkit-unassigned benjamin commit-queue kenneth sam oldest_to_newest 193286 0 kling 2010-02-24 03:19:18 -0800 This function is called quite a lot and is trivial to optimize. 193287 1 49373 kling 2010-02-24 03:20:45 -0800 Created attachment 49373 Proposed patch 193301 2 49373 kenneth 2010-02-24 04:28:23 -0800 Comment on attachment 49373 Proposed patch The changelog should explain what change you made and not just say "optimized Font::normalizeSpace()". Apart from that LGTM 193305 3 sam 2010-02-24 04:46:46 -0800 Is there any measured speedup on benchmarks or microbenchmarks? 193308 4 kling 2010-02-24 05:10:04 -0800 (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 193309 5 49379 kling 2010-02-24 05:11:00 -0800 Created attachment 49379 Same patch with fleshed-out ChangeLog 193322 6 sam 2010-02-24 05:54:09 -0800 (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. 193323 7 kenneth 2010-02-24 05:56:44 -0800 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? 193326 8 kling 2010-02-24 06:04:06 -0800 (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. 193327 9 sam 2010-02-24 06:08:11 -0800 Do you see an improvement in speed with the patch (in ms please)? 193368 10 49379 darin 2010-02-24 08:21:42 -0800 Comment on attachment 49379 Same patch with fleshed-out ChangeLog Does this cause measurable speedup? 193378 11 benjamin 2010-02-24 08:43:12 -0800 (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. 193642 12 49379 commit-queue 2010-02-24 22:56:43 -0800 Comment on attachment 49379 Same patch with fleshed-out ChangeLog Clearing flags on attachment: 49379 Committed r55224: <http://trac.webkit.org/changeset/55224> 193643 13 commit-queue 2010-02-24 22:56:48 -0800 All reviewed patches have been landed. Closing bug. 49373 2010-02-24 03:20:45 -0800 2010-02-24 05:11:00 -0800 Proposed patch bug-35336.diff text/plain 1271 kling ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MmMzOTc5YS4uOTFkYzNjOSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAxMC0wMi0yNCAgQW5kcmVhcyBLbGlu ZyAgPGFuZHJlYXMua2xpbmdAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIE9wdGltaXplIEZvbnQ6Om5vcm1hbGl6ZVNwYWNlcygpCisK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM1MzM2CisK KyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9Gb250LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OkZvbnQ6Om5vcm1hbGl6ZVNwYWNlcyk6CisKIDIwMTAtMDItMjMgIEdlb2ZmIEdhcmVuICA8Z2dh cmVuQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBPbGl2ZXIgSHVudC4KZGlmZiAt LWdpdCBhL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udC5jcHAgYi9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL0ZvbnQuY3BwCmluZGV4IDdkMGU1YTkuLjNkM2ZmZTMgMTAwNjQ0Ci0tLSBh L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udC5jcHAKKysrIGIvV2ViQ29yZS9wbGF0Zm9y bS9ncmFwaGljcy9Gb250LmNwcApAQCAtMjY3LDEyICsyNjcsMTMgQEAgRm9udFNlbGVjdG9yKiBG b250Ojpmb250U2VsZWN0b3IoKSBjb25zdAogCiBTdHJpbmcgRm9udDo6bm9ybWFsaXplU3BhY2Vz KGNvbnN0IFN0cmluZyYgc3RyaW5nKQogeworICAgIGNvbnN0IFVDaGFyKiBjaGFyYWN0ZXJzID0g c3RyaW5nLmNoYXJhY3RlcnMoKTsKICAgICB1bnNpZ25lZCBsZW5ndGggPSBzdHJpbmcubGVuZ3Ro KCk7CiAgICAgVmVjdG9yPFVDaGFyLCAyNTY+IGJ1ZmZlcihsZW5ndGgpOwogICAgIGJvb2wgZGlk UmVwbGFjZW1lbnQgPSBmYWxzZTsKIAogICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgPCBsZW5n dGg7ICsraSkgewotICAgICAgICBVQ2hhciBvcmlnaW5hbENoYXJhY3RlciA9IHN0cmluZ1tpXTsK KyAgICAgICAgVUNoYXIgb3JpZ2luYWxDaGFyYWN0ZXIgPSBjaGFyYWN0ZXJzW2ldOwogICAgICAg ICBidWZmZXJbaV0gPSBub3JtYWxpemVTcGFjZXMob3JpZ2luYWxDaGFyYWN0ZXIpOwogICAgICAg ICBpZiAoYnVmZmVyW2ldICE9IG9yaWdpbmFsQ2hhcmFjdGVyKQogICAgICAgICAgICAgZGlkUmVw bGFjZW1lbnQgPSB0cnVlOwo= 49379 2010-02-24 05:11:00 -0800 2010-02-24 22:56:43 -0800 Same patch with fleshed-out ChangeLog bug-35336-2.diff text/plain 1361 kling ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YTRlMzgyYi4uY2NlODZhNyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMC0wMi0yNCAgQW5kcmVhcyBLbGlu ZyAgPGFuZHJlYXMua2xpbmdAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIE9wdGltaXplZCBGb250Ojpub3JtYWxpemVTcGFjZXMoKSBi eSBjYWNoaW5nIHRoZSBTdHJpbmc6OmNoYXJhY3RlcnMoKQorICAgICAgICBpbnN0ZWFkIG9mIHJl cGVhdGVkbHkgY2FsbGluZyBvcGVyYXRvcltdCisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTM1MzM2CisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGlj cy9Gb250LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZvbnQ6Om5vcm1hbGl6ZVNwYWNlcyk6CisK IDIwMTAtMDItMjQgIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJl dmlld2VkIGJ5IEd1c3Rhdm8gTm9yb25oYS4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGxhdGZvcm0v Z3JhcGhpY3MvRm9udC5jcHAgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0ZvbnQuY3BwCmlu ZGV4IDdkMGU1YTkuLjNkM2ZmZTMgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvRm9udC5jcHAKKysrIGIvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9Gb250LmNwcApAQCAt MjY3LDEyICsyNjcsMTMgQEAgRm9udFNlbGVjdG9yKiBGb250Ojpmb250U2VsZWN0b3IoKSBjb25z dAogCiBTdHJpbmcgRm9udDo6bm9ybWFsaXplU3BhY2VzKGNvbnN0IFN0cmluZyYgc3RyaW5nKQog eworICAgIGNvbnN0IFVDaGFyKiBjaGFyYWN0ZXJzID0gc3RyaW5nLmNoYXJhY3RlcnMoKTsKICAg ICB1bnNpZ25lZCBsZW5ndGggPSBzdHJpbmcubGVuZ3RoKCk7CiAgICAgVmVjdG9yPFVDaGFyLCAy NTY+IGJ1ZmZlcihsZW5ndGgpOwogICAgIGJvb2wgZGlkUmVwbGFjZW1lbnQgPSBmYWxzZTsKIAog ICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgPCBsZW5ndGg7ICsraSkgewotICAgICAgICBVQ2hh ciBvcmlnaW5hbENoYXJhY3RlciA9IHN0cmluZ1tpXTsKKyAgICAgICAgVUNoYXIgb3JpZ2luYWxD aGFyYWN0ZXIgPSBjaGFyYWN0ZXJzW2ldOwogICAgICAgICBidWZmZXJbaV0gPSBub3JtYWxpemVT cGFjZXMob3JpZ2luYWxDaGFyYWN0ZXIpOwogICAgICAgICBpZiAoYnVmZmVyW2ldICE9IG9yaWdp bmFsQ2hhcmFjdGVyKQogICAgICAgICAgICAgZGlkUmVwbGFjZW1lbnQgPSB0cnVlOwo=