Bug 45589

Summary:

Inline UTF8SequenceLength

Product:

WebKit

Reporter:

Patrick R. Gansterer <paroga>

Component:

Web Template Framework

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, darin, eric, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Changes used for testing	paroga: commit-queue-
Patch	none

Patrick R. Gansterer

Reported 2010-09-11 08:52:12 PDT

see patch

Attachments
Patch (2.03 KB, patch) 2010-09-11 08:57 PDT, Patrick R. Gansterer	no flags	Details Formatted Diff Diff
Patch (2.06 KB, patch) 2010-09-18 01:28 PDT, Patrick R. Gansterer	no flags	Details Formatted Diff Diff
Changes used for testing (957 bytes, patch) 2010-09-22 07:49 PDT, Patrick R. Gansterer	paroga: commit-queue-	Details Formatted Diff Diff
Patch (1.16 KB, patch) 2010-09-22 09:14 PDT, Patrick R. Gansterer	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Patrick R. Gansterer

Comment 1 2010-09-11 08:57:42 PDT

Created attachment 67299 [details] Patch

Eric Seidel (no email)

Comment 2 2010-09-14 23:53:48 PDT

Comment on attachment 67299 [details] Patch Please update your ChangeLog to explain "why".

Patrick R. Gansterer

Comment 3 2010-09-15 03:30:30 PDT

(In reply to comment #2) > (From update of attachment 67299 [details]) > Please update your ChangeLog to explain "why". Doesn't "inline" already explain it all?

Patrick R. Gansterer

Comment 4 2010-09-17 15:25:58 PDT

Comment on attachment 67299 [details] Patch (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 67299 [details] [details]) > > Please update your ChangeLog to explain "why". > Doesn't "inline" already explain it all? There is no real "why". It's only a cleanup.

Darin Adler

Comment 5 2010-09-17 17:04:48 PDT

Does it make the code faster? Smaller? Easier to read?

Patrick R. Gansterer

Comment 6 2010-09-18 01:28:32 PDT

Created attachment 68003 [details] Patch Added a "to improve performance" in the ChangeLog.

Darin Adler

Comment 7 2010-09-18 16:17:29 PDT

Comment on attachment 68003 [details] Patch OK. So does this improve performance?

Patrick R. Gansterer

Comment 8 2010-09-22 07:49:34 PDT

Created attachment 68371 [details] Changes used for testing > OK. So does this improve performance? I've done a quick performance test with this patch. I've got ~8% speed improvement on WinCE (time dropped from 2385 to 2203).

Darin Adler

Comment 9 2010-09-22 08:18:00 PDT

(In reply to comment #8) > I've done a quick performance test with this patch. > I've got ~8% speed improvement on WinCE (time dropped from 2385 to 2203). Sorry to keep pushing on this, but did you try just calling the inline version as-is rather than merging them? I’d expect an even-better speed improvement if you just changed the call site in convertUTF8ToUTF16 and didn’t make the other changes.

Patrick R. Gansterer

Comment 10 2010-09-22 09:14:11 PDT

Created attachment 68378 [details] Patch (In reply to comment #9) > Sorry to keep pushing on this, but did you try just calling the inline version as-is rather than merging them? I’d expect an even-better speed improvement if you just changed the call site in convertUTF8ToUTF16 and didn’t make the other changes. I don't know why, but you're right! It's faster with the newest patch.

Darin Adler

Comment 11 2010-09-22 09:45:33 PDT

(In reply to comment #10) > (In reply to comment #9) > > Sorry to keep pushing on this, but did you try just calling the inline version as-is rather than merging them? I’d expect an even-better speed improvement if you just changed the call site in convertUTF8ToUTF16 and didn’t make the other changes. > I don't know why, but you're right! It's faster with the newest patch. I believe I wrote the original function, and I followed the principle of inlining the hot part of the code and keeping the less-hot part out of line. This often makes things faster than just inlining everything. Seeing that your original patch reversed that decision, I asked you to measure. Thanks for the extra effort.

WebKit Commit Bot

Comment 12 2010-09-22 10:29:05 PDT

Comment on attachment 68378 [details] Patch Clearing flags on attachment: 68378 Committed r68055: <http://trac.webkit.org/changeset/68055>

WebKit Commit Bot

Comment 13 2010-09-22 10:29:10 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 14 2010-09-22 10:57:09 PDT

http://trac.webkit.org/changeset/68055 might have broken Qt Windows 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/68056 http://trac.webkit.org/changeset/68055

Note You need to log in before you can comment on or make changes to this bug.