Bug 161936 - [Cocoa] Improve performance of complex text codepath
Summary: [Cocoa] Improve performance of complex text codepath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-13 16:55 PDT by Myles C. Maxfield
Modified: 2016-09-27 15:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.71 KB, patch)
2016-09-13 16:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2016-09-14 12:19 PDT, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2016-09-13 16:55:44 PDT
[Cocoa] Improve performance of complex text codepath
Comment 1 Myles C. Maxfield 2016-09-13 16:58:59 PDT
Created attachment 288749 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-09-14 11:30:24 PDT
Comment on attachment 288749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288749&action=review

> Source/WebCore/ChangeLog:8
> +        CoreText exposes a bit on the CTRunStatus which represents if the

s/if the/whether the/

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:791
> +                while (m_glyphOrigins.size() + 1 < m_adjustedBaseAdvances.size())
> +                    m_glyphOrigins.append(CGPointZero);

This seems like a really inefficient way to zero-fill an array.
Comment 3 Myles C. Maxfield 2016-09-14 12:19:08 PDT
Created attachment 288845 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-09-14 12:52:36 PDT
Comment on attachment 288845 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288845&action=review

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:795
> +                auto oldSize = m_glyphOrigins.size();
> +                m_glyphOrigins.grow(m_adjustedBaseAdvances.size());
> +                auto newSize = m_glyphOrigins.size();
> +                memset(m_glyphOrigins.data() + oldSize, 0, sizeof(CGPoint) * (newSize - oldSize));
> +                m_glyphOrigins[m_glyphOrigins.size() - 1] = origins[i];

You're still running this code for time through the "for (unsigned i = 0; i < glyphCount; i++) {" loop. Can you resized m_glyphOrigins up-front?

Also if we used a class that inherited from CGPoint and had a constructor, we wouldn't need to explicitly zero-fill.
Comment 5 Myles C. Maxfield 2016-09-14 14:41:28 PDT
(In reply to comment #4)
> Comment on attachment 288845 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288845&action=review
> 
> > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:795
> > +                auto oldSize = m_glyphOrigins.size();
> > +                m_glyphOrigins.grow(m_adjustedBaseAdvances.size());
> > +                auto newSize = m_glyphOrigins.size();
> > +                memset(m_glyphOrigins.data() + oldSize, 0, sizeof(CGPoint) * (newSize - oldSize));
> > +                m_glyphOrigins[m_glyphOrigins.size() - 1] = origins[i];
> 
> You're still running this code for time through the "for (unsigned i = 0; i
> < glyphCount; i++) {" loop. Can you resized m_glyphOrigins up-front?

The whole point of this patch is to lazily append to the vector. The vector gets populated piece by piece (this zero-filling doesn't overlap any previous zero-fill).

> 
> Also if we used a class that inherited from CGPoint and had a constructor,
> we wouldn't need to explicitly zero-fill.

True, but I don't think it's worth the added cognitive load.
Comment 6 Myles C. Maxfield 2016-09-27 15:01:20 PDT
Committed r206466: <http://trac.webkit.org/changeset/206466>