Bug 113243

Summary: Enable measure-once optimization on Chromium-Mac
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: bashi, dglazkov, eric, esprehn+autocc, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 100794    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64
none
Patch for landing none

Description Levi Weintraub 2013-03-25 15:21:39 PDT
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!
Comment 1 Levi Weintraub 2013-03-25 15:26:27 PDT
Created attachment 194933 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-03-25 15:49:02 PDT
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?
Comment 3 Levi Weintraub 2013-03-25 15:55:47 PDT
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.
Comment 4 Levi Weintraub 2013-03-27 10:18:13 PDT
Created attachment 195347 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-27 11:08:02 PDT
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
Comment 6 WebKit Review Bot 2013-03-27 11:08:06 PDT
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
Comment 7 Levi Weintraub 2013-03-28 11:17:12 PDT
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.
Comment 8 Eric Seidel (no email) 2013-03-28 11:24:14 PDT
Comment on attachment 195347 [details]
Patch

LGTM.
Comment 9 Levi Weintraub 2013-03-28 13:26:44 PDT
Created attachment 195632 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-03-28 14:02:01 PDT
Comment on attachment 195632 [details]
Patch for landing

Clearing flags on attachment: 195632

Committed r147156: <http://trac.webkit.org/changeset/147156>
Comment 11 WebKit Review Bot 2013-03-28 14:02:05 PDT
All reviewed patches have been landed.  Closing bug.