Bug 43436 - Simple diffs for ~5% complex layout speedup
Summary: Simple diffs for ~5% complex layout speedup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Enhancement
Assignee: Ned Holbrook
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-08-03 11:16 PDT by Ned Holbrook
Modified: 2010-08-05 04:55 PDT (History)
3 users (show)

See Also:


Attachments
Proposed changes. (6.46 KB, patch)
2010-08-03 11:24 PDT, Ned Holbrook
no flags Details | Formatted Diff | Diff
Changes per review. (6.46 KB, patch)
2010-08-03 12:03 PDT, Ned Holbrook
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.