Bug 43436

Summary: Simple diffs for ~5% complex layout speedup
Product: WebKit Reporter: Ned Holbrook <ned>
Component: TextAssignee: Ned Holbrook <ned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, darin, mitz
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Proposed changes.
none
Changes per review. none

Description Ned Holbrook 2010-08-03 11:16:38 PDT
Attached please find suggested changes that, using a simple test case of my own devising, improve by 4-5% the time to render a Roman string of 69 words using "text-rendering: optimizeLegibility". These changes have no effect on "run-webkit-tests --complex-text fast/text".
Comment 1 Ned Holbrook 2010-08-03 11:24:25 PDT
Created attachment 63357 [details]
Proposed changes.
Comment 2 mitz 2010-08-03 11:31:13 PDT
<rdar://problem/8237336>
Comment 3 Darin Adler 2010-08-03 11:56:24 PDT
Comment on attachment 63357 [details]
Proposed changes.

This looks good.

> -        RetainPtr<CFMutableDataRef> m_coreTextIndicesData;
> +        Vector<CFIndex, 64> m_coreTextIndicesVector;

Maybe m_coreTextIndicesStorage would be a better name? Or m_coreTextIndicesBuffer? The name should communicate the fact that we should access this through the m_coreTextIndices pointer and not directly.

> +    m_coreTextIndicesVector.reserveCapacity(m_stringLength);

Since this is always used on a new vector, you could use reserveInitialCapacity for slightly better performance.

r=me without additional changes, but you might want to do one of the improvements above. If you decide not to do either improvement, please let a committer know so they can set commit-queue+ on the bug.
Comment 4 Ned Holbrook 2010-08-03 12:02:54 PDT
(In reply to comment #3)
> Maybe m_coreTextIndicesStorage would be a better name? Or m_coreTextIndicesBuffer? The name should communicate the fact that we should access this through the m_coreTextIndices pointer and not directly.

Since the naming of the new member variable matches that of the existing m_glyphsVector and m_advancesVector, I think I'll leave this one as is.
 
> Since this is always used on a new vector, you could use reserveInitialCapacity for slightly better performance.

Sounds like a plan!
Comment 5 Ned Holbrook 2010-08-03 12:03:40 PDT
Created attachment 63364 [details]
Changes per review.
Comment 6 Eric Seidel (no email) 2010-08-04 10:22:45 PDT
Comment on attachment 63357 [details]
Proposed changes.

Cleared Darin Adler's review+ from obsolete attachment 63357 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 7 WebKit Commit Bot 2010-08-05 04:54:59 PDT
Comment on attachment 63364 [details]
Changes per review.

Clearing flags on attachment: 63364

Committed r64734: <http://trac.webkit.org/changeset/64734>
Comment 8 WebKit Commit Bot 2010-08-05 04:55:03 PDT
All reviewed patches have been landed.  Closing bug.