Bug 161936 – [Cocoa] Improve performance of complex text codepath

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] Improve performance of complex text codepath

[Myles C. Maxfield]

Created attachment 288749 [details]
Patch

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

Created attachment 288845 [details]
Patch

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

Committed r206466: <http://trac.webkit.org/changeset/206466>