Bug 207244 – REGRESSION(r254534): 1-3% regression on PLT

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

REGRESSION(r254534): 1-3% regression on PLT

[Myles C. Maxfield]

Created attachment 389757 [details]
Patch

[Myles C. Maxfield]

Created attachment 389758 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/58821709>

[Simon Fraser (smfr)]

Comment on attachment 389758 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        This is a partial revert of the regressing patch.

Please clarify that it's a full behavior revert, and a partial code revert.

> Source/WTF/wtf/PlatformHave.h:466
>  #define HAVE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE 1

Maybe just #define to 0

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:48
> +        paragraphStyle = adoptCF(CTParagraphStyleCreate(nullptr, 0));

Here adoptCF makes a temporary RetainPtr but it's then destroyed, so paragraphStyle is left with a pointer to a deleted object.

[Myles C. Maxfield]

Created attachment 389760 [details]
Patch

[Darin Adler]

Comment on attachment 389760 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:467
>  #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> -#define HAVE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE 1
> +#define HAVE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE 0
>  #endif

This doesn’t seem quite right. We have changed our behavior by deciding not to use this because it’s not fast enough. That doesn’t mean we don’t have it.

Claiming we don’t have it seems like a confusing shortcut. I’d rather see a change at the two call sites even if it’s about commenting out the HAVE and replacing it with a 0 and a comment explaining why. Or you could do something cute and change HAVE to USE at those call sites, knowing that USE(CTFONTTRANSFORMGLYPHSWITHLANGUAGE) will be false.

> Source/WebCore/platform/graphics/FontCascade.cpp:-432
> -    Vector<GlyphBufferGlyph, 16> glyphs;
> -    Vector<GlyphBufferAdvance, 16> advances;

Removing these unused vectors seems like an unrelated change. Not sure why it’s in this patch. Might be a tiny speedup, but seems to slightly muddy the waters about the roll-out.

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:51
> -    auto paragraphStyle = adoptCF(CTParagraphStyleCreate(nullptr, 0));
> -    CTParagraphStyleSetCompositionLanguage(paragraphStyle.get(), kCTCompositionLanguageNone);
> -    CFDictionarySetValue(attributesDictionary.get(), kCTParagraphStyleAttributeName, paragraphStyle.get());
> +    static CTParagraphStyleRef paragraphStyle = nullptr;
> +    if (!paragraphStyle) {
> +        paragraphStyle = CTParagraphStyleCreate(nullptr, 0);
> +        CTParagraphStyleSetCompositionLanguage(paragraphStyle, kCTCompositionLanguageNone);
> +    }
> +    CFDictionarySetValue(attributesDictionary.get(), kCTParagraphStyleAttributeName, paragraphStyle);

Two thoughts.

First, it seems unlikely that this small optimization is part of the revert. Seems like a good idea, but strange to mix it in with the the revert. Same thought as above about muddying the waters.

Second, if there *is* an issue with performance in this function, I suggest that we rewrite it to use CFDictionaryCreate instead of CFDictionaryCreateMutable. It would not be hard at all to assemble the keys and values in two fixed size arrays and then call CFDictionaryCreate at the end. That should be more efficient than a sequence of calls to CFDictionarySetValue, and also save a bit of memory. And also, if it’s called a lot on the same arguments maybe we should cache one or more recently created dictionaries to avoid re-creating them ever time; could use a TinyLRUCache or something like that.

[Myles C. Maxfield]

Created attachment 389915 [details]
Patch

[Myles C. Maxfield]

Created attachment 389916 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 389916 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        This patch changes the preprocessor guards to only use the function on those OSes.

to *not* use

[Myles C. Maxfield]

Created attachment 389919 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 389919 [details]
Patch

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

> Source/WebCore/page/cocoa/MemoryReleaseCocoa.mm:59
> +    LocaleMac::releaseMemory();

Is LocaleMac really LocaleCocoa?

[Darin Adler]

Comment on attachment 389919 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:467
> -#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> -#define HAVE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE 1
> +#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101600)
> +#define HAVE_ACCEPTABLE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE 1
>  #endif

Are you intentionally turning HAVE_ACCEPTABLE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE on for all iOS family platforms?

Also, I think we should just move this to PlatformUse.h and make it USE(CTFONTTRANSFORMGLYPHSWITHLANGUAGE). Not necessary in this patch, but would be better.

>> Source/WebCore/page/cocoa/MemoryReleaseCocoa.mm:59
>> +    LocaleMac::releaseMemory();
> 
> Is LocaleMac really LocaleCocoa?

It is. Worth renaming soon. Doesn’t seem necessary to do it in this patch.

> Source/WebCore/platform/text/mac/LocaleMac.h:70
> +    static String canonicalLanguageIdentifierFromString(const AtomString&);

Why not have the result also be an AtomString? Could save a little memory for identical strings, and since we’re keeping them in a map, the extra AtomString table lookup cost would be spent only the first time.

[Myles C. Maxfield]

(In reply to Darin Adler from comment #12)
> Are you intentionally turning
> HAVE_ACCEPTABLE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE on for all iOS family
> platforms?

Yes. It should never have been off for those platforms.

It looks like either r254676 disabled it for those platforms, or I don't understand the interaction between PLATFORM(IOS_FAMILY) and __IPHONE_OS_VERSION_MIN_REQUIRED. (Probably the latter.)

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #13)
> (In reply to Darin Adler from comment #12)
> > Are you intentionally turning
> > HAVE_ACCEPTABLE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE on for all iOS family
> > platforms?
> 
> Yes. It should never have been off for those platforms.
> 
> It looks like either r254676 disabled it for those platforms, or I don't
> understand the interaction between PLATFORM(IOS_FAMILY) and
> __IPHONE_OS_VERSION_MIN_REQUIRED. (Probably the latter.)

After asking some experts, it turns out I don't understand the interaction between PLATFORM(IOS_FAMILY) and __IPHONE_OS_VERSION_MIN_REQUIRED.

[Myles C. Maxfield]

Created attachment 389979 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 389987 [details]
Patch for committing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389987 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html bug 207335 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 389987 [details]
Patch for committing

Clearing flags on attachment: 389987

Committed r255988: <https://trac.webkit.org/changeset/255988>

[Myles C. Maxfield]

Comment on attachment 389760 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCascade.cpp:-432
>> -    Vector<GlyphBufferAdvance, 16> advances;
> 
> Removing these unused vectors seems like an unrelated change. Not sure why it’s in this patch. Might be a tiny speedup, but seems to slightly muddy the waters about the roll-out.

https://bugs.webkit.org/show_bug.cgi?id=207374

>> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:51
>> +    CFDictionarySetValue(attributesDictionary.get(), kCTParagraphStyleAttributeName, paragraphStyle);
> 
> Two thoughts.
> 
> First, it seems unlikely that this small optimization is part of the revert. Seems like a good idea, but strange to mix it in with the the revert. Same thought as above about muddying the waters.
> 
> Second, if there *is* an issue with performance in this function, I suggest that we rewrite it to use CFDictionaryCreate instead of CFDictionaryCreateMutable. It would not be hard at all to assemble the keys and values in two fixed size arrays and then call CFDictionaryCreate at the end. That should be more efficient than a sequence of calls to CFDictionarySetValue, and also save a bit of memory. And also, if it’s called a lot on the same arguments maybe we should cache one or more recently created dictionaries to avoid re-creating them ever time; could use a TinyLRUCache or something like that.

https://bugs.webkit.org/show_bug.cgi?id=207374

[Myles C. Maxfield]

Comment on attachment 389919 [details]
Patch

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

>>> Source/WebCore/page/cocoa/MemoryReleaseCocoa.mm:59
>>> +    LocaleMac::releaseMemory();
>> 
>> Is LocaleMac really LocaleCocoa?
> 
> It is. Worth renaming soon. Doesn’t seem necessary to do it in this patch.

https://bugs.webkit.org/show_bug.cgi?id=207371

[Myles C. Maxfield]

Fixing this patch now.

[Myles C. Maxfield]

Reopening to attach new patch.

[Myles C. Maxfield]

Created attachment 390053 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 390053 [details]
Patch

Rejecting attachment 390053 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 390053, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/13318344

[Myles C. Maxfield]

Committed r256008: <https://trac.webkit.org/changeset/256008>

[Darin Adler]

(In reply to Myles C. Maxfield from comment #14)
> (In reply to Myles C. Maxfield from comment #13)
> > (In reply to Darin Adler from comment #12)
> > > Are you intentionally turning
> > > HAVE_ACCEPTABLE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE on for all iOS family
> > > platforms?
> > 
> > Yes. It should never have been off for those platforms.
> > 
> > It looks like either r254676 disabled it for those platforms, or I don't
> > understand the interaction between PLATFORM(IOS_FAMILY) and
> > __IPHONE_OS_VERSION_MIN_REQUIRED. (Probably the latter.)
> 
> After asking some experts, it turns out I don't understand the interaction
> between PLATFORM(IOS_FAMILY) and __IPHONE_OS_VERSION_MIN_REQUIRED.

Oh, I realize now that my comment should have been clearer.

I knew that it was probably always a mistake that this was off for watchOS and tvOS, and that eventually we would want to correct that mistake, and I was even thinking that it was probably you who would take care of it.

I’ve been discussing this with Jonathan Bedard while reviewing his patches, and he’s intentionally been preserving these kinds of omissions of tvOS and watchOS that we think are likely accidental. Our philosophy was to rewrite the conditional expressions with idioms that make it explicit and clear they are currently omitted.

What I was driving at was that I felt that if you were doing it now, intentionally, the change log ought to have mentioned it! But I used language that was too oblique.

I’m really happy with the actual patch you landed.

[Myles C. Maxfield]

(In reply to Darin Adler from comment #26)
> What I was driving at was that I felt that if you were doing it now,
> intentionally, the change log ought to have mentioned it!

Ah yes, I see. You're right, it should have. Probably too late now though. I'll keep this in mind for the future.