Bug 58063 - CTLine objects should outlive their CTRuns
Summary: CTLine objects should outlive their CTRuns
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-07 11:02 PDT by Ned Holbrook
Modified: 2011-04-10 09:11 PDT (History)
2 users (show)

See Also:


Attachments
Proposed changes. (2.08 KB, patch)
2011-04-07 16:44 PDT, Ned Holbrook
no flags Details | Formatted Diff | Diff
Remove m_coreTextRun. (4.38 KB, patch)
2011-04-07 17:50 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 2011-04-07 11:02:18 PDT
CoreText may need to perform some extra work if a CTLine object is released prior to its CTRuns, but this extra work can be avoided by either retaining CTLines longer than their CTRuns (in ComplexTextController), or by not retaining the CTRuns (in ComplexTextRun).
Comment 1 Ned Holbrook 2011-04-07 11:04:40 PDT
The latter approach is less desirable since it would eliminate the possibility of using the CTRunGet*Ptr() accessors, so I'll work up some numbers for the former.
Comment 2 Ned Holbrook 2011-04-07 16:44:28 PDT
Created attachment 88732 [details]
Proposed changes.

Numbers are a bit variable on my machine, but I'm seeing an improvement of around 2% for repeated optimizeLegibility test runs on Latin text—barely above the level of noise but certainly not a slowdown.
Comment 3 mitz 2011-04-07 17:07:14 PDT
Comment on attachment 88732 [details]
Proposed changes.

If the CTLine stays alive, then maybe ComplexTextRun::m_coreTextRun no longer needs to be a RetainPtr?
Comment 4 Ned Holbrook 2011-04-07 17:09:14 PDT
(In reply to comment #3)
> (From update of attachment 88732 [details])
> If the CTLine stays alive, then maybe ComplexTextRun::m_coreTextRun no longer needs to be a RetainPtr?

Good idea! Should it become a different smart pointer or will a bare CTRunRef suffice?
Comment 5 mitz 2011-04-07 17:15:51 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 88732 [details] [details])
> > If the CTLine stays alive, then maybe ComplexTextRun::m_coreTextRun no longer needs to be a RetainPtr?
> 
> Good idea! Should it become a different smart pointer or will a bare CTRunRef suffice?

Bare CTRunRef. A comment right above the member variable definition explaining why it’s ok to use a bare CTRunRef would be good.
Comment 6 Ned Holbrook 2011-04-07 17:31:18 PDT
(In reply to comment #5)
> Bare CTRunRef. A comment right above the member variable definition explaining why it’s ok to use a bare CTRunRef would be good.

Theoretically it could be removed entirely since it's only used in the constructor.
Comment 7 mitz 2011-04-07 17:33:39 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Bare CTRunRef. A comment right above the member variable definition explaining why it’s ok to use a bare CTRunRef would be good.
> 
> Theoretically it could be removed entirely since it's only used in the constructor.

Let’s remove it in practice, then.
Comment 8 Ned Holbrook 2011-04-07 17:50:00 PDT
Created attachment 88743 [details]
Remove m_coreTextRun.
Comment 9 WebKit Commit Bot 2011-04-07 21:38:56 PDT
Comment on attachment 88743 [details]
Remove m_coreTextRun.

Clearing flags on attachment: 88743

Committed r83251: <http://trac.webkit.org/changeset/83251>
Comment 10 WebKit Commit Bot 2011-04-07 21:39:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 mitz 2011-04-10 09:11:55 PDT
<rdar://problem/9250245>