Bug 113243 – Enable measure-once optimization on Chromium-Mac

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Support for -webkit-font-feature-settings: 'kern' (seemingly only enabled on CrMac) was preventing us from enabling the great performance win of only measuring text once during prefs-widths calculation instead of again for line layout. Catching this case specifically fixes the problem and gives this straggler platform the 10% perf win!

[Levi Weintraub]

Created attachment 194933 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 194933 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:785
> +static inline bool hasKerningFeatureSetting(const Font& font)

We must have other code which depends on being able to get this out of the font, no?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:787
> +    FontFeatureSettings* settings = font.fontDescription().featureSettings();

So this is http://dev.w3.org/csswg/css3-fonts/#propdef-font-feature-settings, correct?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:792
> +    for (unsigned i = 0; i < numFeatures; ++i) {

Shouldn't this loop just be on FontFeatureSettings::hasKerning?  Then it can know the magic of that this is a 4-char string, and do the AtomicString constant all nicely, etc.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:793
> +        if (settings->at(i).tag() == "kern")

I assume tag() is an AtomicString?  We should probably be using a DEFINE_STATIC_LOCAL(AtomicString, kernAtom, "kern") to make this compare faster.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:828
>      bool kerningIsEnabled = font.typesettingFeatures() & Kerning;

So previously we were always ignoring kerning, but in order to partake of this optimization we need this code to be kerning aware, correct?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:830
> +    kerningIsEnabled = kerningIsEnabled || hasKerningFeatureSetting(font);

So this then teaches this code to be aware of both types of kerning.  One which comes from optimize-legibility, and one which comes from font-feature-settings?  Is that correct?  Previously this code only understood optimize-legibility, thus turning on this optimization introduced a new dependency on thsi coe which was not font-feature-settings-aware?  correct?

[Levi Weintraub]

Comment on attachment 194933 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:785
>> +static inline bool hasKerningFeatureSetting(const Font& font)
> 
> We must have other code which depends on being able to get this out of the font, no?

Sadly no... we pipe this directly into HarfBuzz. See: HarfBuzzShaper::setFontFeatures()

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:787
>> +    FontFeatureSettings* settings = font.fontDescription().featureSettings();
> 
> So this is http://dev.w3.org/csswg/css3-fonts/#propdef-font-feature-settings, correct?

Yup.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:792
>> +    for (unsigned i = 0; i < numFeatures; ++i) {
> 
> Shouldn't this loop just be on FontFeatureSettings::hasKerning?  Then it can know the magic of that this is a 4-char string, and do the AtomicString constant all nicely, etc.

Yeah. Right now FontFeatureSettings has effectively no functionality to do lookups, but this one clearly needs to be in there.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:793
>> +        if (settings->at(i).tag() == "kern")
> 
> I assume tag() is an AtomicString?  We should probably be using a DEFINE_STATIC_LOCAL(AtomicString, kernAtom, "kern") to make this compare faster.

Thank you! :)

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:828
>>      bool kerningIsEnabled = font.typesettingFeatures() & Kerning;
> 
> So previously we were always ignoring kerning, but in order to partake of this optimization we need this code to be kerning aware, correct?

Before the optimization, we always measured the text here and didn't have a problem. We still end up measuring here when there's kerning, but we were missing the fact that kerning was enabled for exactly the reasons you mention below.

[Levi Weintraub]

Created attachment 195347 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195347 [details]
Patch

Attachment 195347 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17345107

New failing tests:
fast/text/shaping/shaping-selection-rect.html

[WebKit Review Bot]

Created attachment 195360 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Levi Weintraub]

So after more testing, it turns out this occurs whenever there are any font-feature-settings defined, which isn't too surprising given it uses complex text features. The surprising thing is that it only effects Mac. For now, I'm teaching this bit of code that this doesn't use the simple font path, but I imagine we'll find more places where the lack of plumbing this info throughout the rendering code may get us in trouble.

[Eric Seidel (no email)]

Comment on attachment 195347 [details]
Patch

LGTM.

[Levi Weintraub]

Created attachment 195632 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 195632 [details]
Patch for landing

Clearing flags on attachment: 195632

Committed r147156: <http://trac.webkit.org/changeset/147156>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.