Closed Bug 921858 Opened 6 years ago Closed 6 years ago

Text rendering -50% slowdown since Firefox23 if default font is using "Meiryo" , "IPAexGothic" or "HiraKakuProN-W3"

Categories

(Core :: Graphics: Text, defect, P2)

23 Branch
x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- wontfix
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: alice0775, Assigned: jtd)

References

()

Details

(Keywords: perf, regression, Whiteboard: [c= p= s=2014.04.11 u=])

Attachments

(6 files, 11 obsolete files)

13.36 KB, application/pdf
Details
4.80 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
49.08 KB, text/plain
Details
27.84 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
14.64 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
1.18 MB, patch
jfkthame
: review+
Details | Diff | Splinter Review
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/8f805d3ef377
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130929030202

Rendering speed regression about 50% slow since Firefox23.

Steps To Reproduce:
1. Start Firefox with maximized window if necessary
2. Open URL http://www.craftymind.com/factory/guimark2/HTML5TextTest.html
3. Start test

Actual Results:
Regression appears in Firefox23. And the rendering speed is about 50% slow.

25fps
http://hg.mozilla.org/releases/mozilla-release/rev/e55e45536133
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 ID:20130618035212

12fps
http://hg.mozilla.org/releases/mozilla-release/rev/a55c55edf302
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 ID:20130814063812

12fps
http://hg.mozilla.org/releases/mozilla-release/rev/7c3b0732e765
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 ID:20130910160258

12fps
http://hg.mozilla.org/releases/mozilla-beta/rev/5939fc06a3cd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20130926170421

11fps
http://hg.mozilla.org/releases/mozilla-aurora/rev/af05b2a644b8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130928004001

11fps
http://hg.mozilla.org/mozilla-central/rev/8f805d3ef377
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130929030202


Regression window:
Good:
http://hg.mozilla.org/mozilla-central/rev/b9d56a1e0a61
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130411 Firefox/23.0 ID:20130411081408
Bad:
http://hg.mozilla.org/mozilla-central/rev/7b8ed29c6bc0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130411 Firefox/23.0 ID:20130411122104
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b9d56a1e0a61&tochange=7b8ed29c6bc0

Regression window(cached m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ca09bff8d6f4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130411 Firefox/23.0 ID:20130411062607
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b16ed870d536
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130411 Firefox/23.0 ID:20130411075308
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ca09bff8d6f4&tochange=b16ed870d536

Suspected: Bug 851379
No longer blocks: 851379
Bug 761442 seems the more likely cause in that range.
In local build,
Last Good: 8a0073ae1a45
First Bad: 703e4668b5c8

Regressed by:
703e4668b5c8	John Daggett — Bug 761442 - treat substitution and positioning lookups involving <space> differently. r=jkew
I'm not seeing the same results as Alice, I do see some regression in the results but nothing like a 50% slowdown:

ThinkPad T510, Windows 7

FF22: 45.08/44.93/44.89  avg: 44.97  
FF23: 44.12/44.42/44.09  avg: 44.21 (-1.7%)
FF24: 42.97/43.82/43.44  avg: 43.41 (-3.6%)

Steps to test:

1. Start browser, navigate to URL
2. After 30 seconds, click on run test
3. After 30 seconds, click on run test
4. After 30 seconds, click on run test

I installed the en_us builds for Firefox 22/23/24 and I'm running with DirectWrite.

Alice, could you put in which localization you're using? ja?  And on the 'about:support' page, what is the value of 'DirectWrite Enabled'?

This test is somewhat poorly designed, it uses two fonts but only one passes the sanitizer so for 4 out of the 5 text blocks, no font is specified.  This means the test will depend upon the default font used for each block.  That doesn't explain the differences across Firefox versions but it means cross-browser comparisons are somewhat variable.
Summary: Rendering speed regression 50% slow since Firefox23 → Text rendering -50% slowdown since Firefox23
(In reply to John Daggett (:jtd) from comment #3)
> 
> Alice, could you put in which localization you're using? ja?  

en-US Nightly build

>  And on the 'about:support' page, what is the value of 'DirectWrite Enabled'?
> 

DirectWrite Enabled: true (6.1.7601.18126)
(In reply to John Daggett (:jtd) from comment #3)

> This test is somewhat poorly designed, it uses two fonts but only one passes
> the sanitizer so for 4 out of the 5 text blocks, no font is specified.  This
> means the test will depend upon the default font used for each block. 

You are right.
It is depend on font.
Maybe TrueType font is required for Japanese text and some Latin font. ("MS PGothic" is not enough to reproduce the problem).

* Change default font to "Meiryo"(メイリオ) in Options > Content.
OR
* Install "IPAex" font from http://ipafont.ipa.go.jp/#en .
  And change default font to "IPAexGothic"(IPAexゴシック) in Options > Content.
Summary: Text rendering -50% slowdown since Firefox23 → Text rendering -50% slowdown since Firefox23 if default font is using "Meiryo" , "IPAexGothic" or "HiraKakuProN-W3"
If default font set to "IPAexGothic"(IPAexゴシック),
the performance regression also happens on Ubuntu12.04 32bit.

http://hg.mozilla.org/mozilla-central/rev/8f805d3ef377
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130929030202
OS: Windows 7 → All
I think we need to concentrate on whether the regression occurred with "normal" platform fonts.  IPAexGothic is "exotic" in the sense that it's a *huge* font and we probably don't need to be worried so much about text performance in this case.  It would certainly be good to understand why it's slower but if it's only this font it will naturally be lower priority than if the same behavior happens with default fonts.
(In reply to John Daggett (:jtd) from comment #7)
> I think we need to concentrate on whether the regression occurred with
> "normal" platform fonts.  

I do not understand this. 
メイリオ is very popular font in Windows Vista, 7, 8, 8.1 and  and "ヒラギノ角ゴ Pro W3" in MAC OSX.

Because, web page can be specified these font in their css.
Ex.
http://www.seiren-udoku.com/tsrBasicStyle.css?v20121207  --- specified メイリオ
http://images.apple.com/jp/global/styles/base.css   --- specified "ヒラギノ角ゴ Pro W3" and メイリオ

So, many website was affected.
I think the potential performance loss exists surely in real world.
In the case of IPAexGothic, at least, the problem is that the font includes features such as 'hwid' and 'fwid', which provide substitutions for the default space glyph. These features are present in all its script systems, including 'DFLT', 'latn', 'cyrl', and 'grek' as well as the more obvious 'hani' and 'kana'.

(For a moment, I thought this seemed odd, but on reflection it does make sense, otherwise the 'hwid' and 'fwid' features would not have the expected effect on Latin (Greek, Cyrillic) text runs.)

This means that when we examine the font for shaping rules that involve <space>, we always find that it may be involved, even when we're dealing with Latin text. As a result, we end up bypassing the word cache and reshaping all the text, all the time.

A possible fix would be to make the shaping-may-involve-space test specific to the features actually being applied, rather than just a per-script check. But this could increase the cost of the test/decision whether to use the cache in all cases, so we'd need to be careful how this is handled.
(In reply to Alice0775 White from comment #8)
> (In reply to John Daggett (:jtd) from comment #7)
> > I think we need to concentrate on whether the regression occurred with
> > "normal" platform fonts.  
> 
> I do not understand this. 
> メイリオ is very popular font in Windows Vista, 7, 8, 8.1 and  and "ヒラギノ角ゴ Pro
> W3" in MAC OSX.

Sorry for not being clearer.  My statement was simply to say that we *not* use IPAexMincho as the basis for any fix because it's rarely used and it's somewhat of an outlier in terms of it's design.  Slowdowns involving Meiryo and Hiragino is what we should focus on.
Given that Japanese content generally lacks spaces, I think it would make sense to simply do a check for spaces in the textrun before taking the non-cached shaping path.  This seems to eliminate the regression in my tests.
Attachment #829935 - Flags: review?(jfkthame)
The testcase mentioned in the URL field contains mostly Latin and Cyrillic text, which includes plenty of spaces, and so it'll still be kept out of the word cache if the default font has GSUB features that touch <space>. Only one of the <div>s has Japanese text that would be affected by this patch; is that text alone responsible for the entire regression that was reported?

Moreover, AFAICT the Japanese <div> does in fact contain a few space characters. Do we split it into multiple textruns in some other way?

So I don't understand why this would help significantly here. Is something else going on that I've overlooked? If this patch can eliminate the regression, I'd like us to understand why!
You're right, it doesn't completely fix the regression for the testcase here, since using a default font of Meiryo will lead to slower Latin text.  But for pages with lots of Japanese on them (i.e. the norm for Japanese users), the patch pretty much makes up the ground lost due to the patch from bug 761442.

Here's my Japanese text perf page:
http://people.mozilla.org/~jdaggett/tests/textsizeperf-ja.html

Steps:

1. Create a new profile, specify Meiryo as the default (or Hiragino Kaku Gothic Pro for OSX)
2. Run tests

Using this profile with the original testcase:

FF22: 46.8fps
FF23: 21.3fps (-54.5%)

Japanese text perf page above:

FF22: 0.922kb/ms 14.7items/ms
FF24: 0.758kb/ms 12.5items/ms  (-17.8% / -15.0%)

I can do a direct comparison with tryserver builds because of the non-PGO nature of these builds, but comparing two optimized tryserver builds with and without the patch, I see a 15% improvement in the basic text test for Japanese and and overall 15% improvement for the original testcase.
How about this: why don't we land the patch since it's a simple, clean optimization but leave the bug open to address the lower-level GSUB feature optimization issue?  Japanese users will at least see the benefit of the performance improvement without having to wait for a more complete fix.
It's still not clear to me why this will necessarily be a win - or even harmless - in all cases. For a font that has GSUB/space features, it leads to an additional pass over the text that's to be shaped; in most cases, this will return true pretty quickly, but in the case of a long run (e.g. of Japanese) that has no spaces, it scans the entire text before returning false. And for what? This tells us we are allowed to use the word cache after all; but a long run of Japanese with no spaces will exceed the word-length threshold and not get cached anyway.

So where does the perf win come from in your tests? Unless I'm missing something, it seems like for large amounts of Japanese text without spaces, we'll have done -more- work, only to wind up using the same (non-caching) shaping code path in the end.
Attached file Book1.pdf
test results with textsizeperf-ja.html of Comment 13.

Layout performance is gradually slow every version up :(

Now, Nightly28 is 40% slower than Firefox22.
Based on bug 967292, comment 7, I think we need to take a closer look at this because it looks like the default font on FFOS is affected by this problem.

I don't think we want to try and line up features applied with which features contain lookups with spaces, that's too costly. But I think a simple improvement would be to distinguish default substitution features from non-default substitution features and use the logic below:

  if (default features with space lookups for script)
    bypass word cache
  else if (non-default features with space lookups for script && font features set)
    bypass word cache
  else
    use word cache

The given font features might not intersect with the set of non-default features wit space lookups but using this I think should be enough to avoid the bypass case 99% of the time.
Blocks: 967292
Priority: -- → P2
Testcase demonstrating the word cache bypass problem for the latest version of FiraSans:

  http://people.mozilla.org/~jdaggett/firasanstest/

(test uses FiraSans 2.001 from https://github.com/mozilla-b2g/moztt)

With textperf logging enabled (see bug 967292, comment 17):

chars: 18329 content-textruns: 168 chrome-textruns: 0 max-textrun-len: 1007 
word-cache-lookups: 0 word-cache-hit-ratio: 0.000 
word-cache-space: 168 word-cache-long: 0 

Result: all content textruns bypass the word cache

Expected result: all content textruns use the word cache (since no feature with space lookups is enabled)
(In reply to John Daggett (:jtd) from comment #17)
> Based on bug 967292, comment 7, I think we need to take a closer look at
> this because it looks like the default font on FFOS is affected by this
> problem.
> 
> I don't think we want to try and line up features applied with which
> features contain lookups with spaces, that's too costly. But I think a
> simple improvement would be to distinguish default substitution features
> from non-default substitution features 

Don't forget that the list of "default substitution features" depends on the script - it's not constant across shapers. I suppose you could use the union of all "default" features from all scripts.

>and use the logic below:
> 
>   if (default features with space lookups for script)
>     bypass word cache
>   else if (non-default features with space lookups for script && font
> features set)
>     bypass word cache
>   else
>     use word cache
> 
> The given font features might not intersect with the set of non-default
> features wit space lookups but using this I think should be enough to avoid
> the bypass case 99% of the time.
Uses the logic described in comment 17.  Need to do some fine tuning here and figure out what the complete set of default features should be.

With this patch, all the textruns in comment 18 that bypassed the word cache now use the word cache.
Tryserver builds:

tryserver build without patch
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdaggett@mozilla.com-4d942b15b440/

tryserver build with patch
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdaggett@mozilla.com-bbbf64a01b5e/

On my Windows 7 machine, running the original testcase linked in the URL, the frame rate goes from 12fps without the patch to 24fps with (using a profile with the default font set to Meiryo).
This adds a set of fonts for testing space-containing lookups used in both default and non-default features.  This is to test some the various permutations that are possible, given that distinguishing between default and non-default features adds more complexity to the GSUB analysis.
Attachment #8378849 - Flags: review?(jfkthame)
Distinguish default substitution features from non-default substitution features and use the logic in comment 17 to avoid bypassing the word cache except when really necessary.  This will avoid bypassing the word cache due to the space-containing lookups for the non-default 'hwid' feature in Hiragino and Meiryo families and in the Fira families used on FFOS.
Attachment #8376078 - Attachment is obsolete: true
Attachment #8378975 - Flags: review?(jfkthame)
John.  Another way you can simplify this is to use hb_ot_shape_plan_collect_lookups().  That will give you a per-script&language list only though, so if you want per-script you need to do more work, so perhaps what you have is adequate.
(In reply to Behdad Esfahbod from comment #24)
> John.  Another way you can simplify this is to use
> hb_ot_shape_plan_collect_lookups().  That will give you a
> per-script&language list only though, so if you want per-script you need to
> do more work, so perhaps what you have is adequate.

Thanks Behdad.  Existing code is already sifting through lookups on a per-script basis, the patch here just extends that to distinguish between default vs. non-default features so that we can avoid bypassing the word cache when the font only has space-containing lookups in non-default features.
tryserver build with patch
https://tbpl.mozilla.org/?tree=Try&rev=9756d6be4bbc

tryserver build without patch (for perf comparison)
https://tbpl.mozilla.org/?tree=Try&rev=494fde0776b0
Revised version of the original patch checking for spaces in the
textrun before bypassing the word cache. Modified to only test for
spaces when the run length is less than what would fit in word cache,
to avoid doing this for long spaceless runs of CJK text that would
simply fall through to the non-word-cache path anyways.

Testcase:

1. Create a new profile, set serif/sans-serif fonts to Calibri
2. Run with text perf logging using the new profile

  NSPR_LOG_MODULES=textperf:5

3. Open URL for Ben Franklin wiki page

Result: checking for spaces before bypassing the word cache results in
increased use of the word cache for fonts with space-containing
lookups in default features.

Comparison of word cache bypass frequency without and with patch:
(textperf-loaddone) time-ms: 9449 reflow: 17 chars: 323473 [http://en.wikipedia.org/wiki/Benjamin_Franklin]
content-textruns: 12930 chrome-textruns: 4 max-textrun-len: 1413 
word-cache-lookups: 7381 word-cache-hit-ratio: 0.956 word-cache-space: 5575 

(textperf-loaddone) time-ms: 9419 reflow: 17 chars: 323473 [http://en.wikipedia.org/wiki/Benjamin_Franklin] 
content-textruns: 12930 chrome-textruns: 4 max-textrun-len: 1413 
word-cache-lookups: 8915 word-cache-hit-ratio: 0.905 word-cache-space: 3875
Attachment #829935 - Attachment is obsolete: true
Attachment #829935 - Flags: review?(jfkthame)
Attachment #8379482 - Flags: review?(jfkthame)
Comment on attachment 8378975 [details] [diff] [review]
patch, distinguish default and non-default substitution features

Review of attachment 8378975 [details] [diff] [review]:
-----------------------------------------------------------------

Some initial comments below... I want to think a bit more about this, but feel free to update the patch and/or respond to comments as you think appropriate at this stage.

::: gfx/thebes/gfxFont.cpp
@@ +1962,5 @@
>  }
>  
>  static void
>  CollectLookupsByLanguage(hb_face_t *aFace, hb_tag_t aTableTag,
> +                         const nsTHashtable<nsUint32HashKey>& aExcludeFeatures,

I'd really like to find some different terminology to use here (and all the related places), in place of "excluded features", which I think is misleading. They're not being "excluded": their lookups are collected just as much as the "non-excluded" one - just into a different set. I think it would help the future understandability of the code to find better names.

This is used in two ways: for GSUB, we're dividing features into "may be used automatically by the shaper" (a specific list) and "only used if explicitly enabled by the user" (the remainder). And for GPOS, the division is simply into "kern" and "the rest".

So in both cases, we have a specific list of features we care about treating separately, and then a bucket for everything else. Maybe this param could be "aSpecificFeatures", and the two sets would be "aSpecificFeatureLookups" and "aOtherFeatureLookups". Or something like that.

WDYT?

@@ +2010,5 @@
> +                               hb_tag_t aScriptTag, uint32_t aScriptIndex,
> +                               uint16_t aGlyph,
> +                               const nsTHashtable<nsUint32HashKey>&
> +                                   aDefaultFeatures,
> +                               bool& hasDefaultFeatureWithGlyph)

Argument should have an 'a' prefix

@@ +2138,5 @@
>  
>  nsDataHashtable<nsUint32HashKey, int32_t> *gfxFont::sScriptTagToCode = nullptr;
> +nsTHashtable<nsUint32HashKey>             *gfxFont::sDefaultFeatures = nullptr;
> +
> +#define HAS_SUBSTITUTION(v, s) ((v[s >> 5] & (1 << (s & 0x1f))) != 0)

A static inline function would be preferable to a macro.

@@ +2140,5 @@
> +nsTHashtable<nsUint32HashKey>             *gfxFont::sDefaultFeatures = nullptr;
> +
> +#define HAS_SUBSTITUTION(v, s) ((v[s >> 5] & (1 << (s & 0x1f))) != 0)
> +
> +// union of all default positioning features across scripts

s/positioning/substitution/.

The problem with this is the risk that it may get out of sync with what's actually implemented by the shapers. Indeed, it is already inaccurate, in that the shaper may automatically apply 'frac', 'numr' and 'dnom' features; this was fairly recently added to harfbuzz, so as to support examples like

data:text/html,<div style="font:36px Source Sans Pro">foo 12⁄345 67⁄89 bar

which now renders automatically-formatted fractions.

@@ +2197,1 @@
>      uint32_t spaceGlyph = GetSpaceGlyph();

Bail out if space glyph is missing (zero)?

@@ +2216,5 @@
> +            uint32_t numDefaultFeatures = ArrayLength(defaultFeatures);
> +            sDefaultFeatures = new nsTHashtable<nsUint32HashKey>(numDefaultFeatures);
> +            for (uint32_t i = 0; i < numDefaultFeatures; i++) {
> +                sDefaultFeatures->PutEntry(defaultFeatures[i]);
> +            }

I wonder whether it's actually worth building a hashtable for this, instead of just using the array directly. There are about 40-odd features in that list, which means we could binary-search the array in 6 iterations. The benefit of a hashtable lookup over that must be pretty marginal, and it's not part of the hottest inner loop. And for GPOS, this means we end up creating a hashtable to hold and look up a single tag, which seems like overkill.

@@ +2231,5 @@
> +            for (uint32_t i = 0; i < len; i++) {
> +                bool isDefaultFeature = false;
> +                int32_t s;
> +                if (!HasLookupRuleWithGlyphByScript(face, HB_OT_TAG_GSUB,
> +                                                    scriptTags[i], i,

This looks wrong; shouldn't that param be |offset + i|?
Comment on attachment 8379482 [details] [diff] [review]
patch, v2, check for spaces before doing non-cached shaping

Review of attachment 8379482 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +3680,5 @@
>  inline static bool IsChar8Bit(uint8_t /*aCh*/) { return true; }
>  inline static bool IsChar8Bit(char16_t aCh) { return aCh < 0x100; }
>  
>  template<typename T>
> +inline static bool HasSpaces(const T *aString, uint32_t aStart, uint32_t aLen)

I'd be inclined to drop the aStart parameter here, making this just

  HasSpaces(const T *aString, uint32_t aLen)

and have the caller pass its aString + aRunLength.

@@ +3683,5 @@
>  template<typename T>
> +inline static bool HasSpaces(const T *aString, uint32_t aStart, uint32_t aLen)
> +{
> +    const T *ch = aString + aStart;
> +    for (ch = aString + aStart; ch < aString + (aStart + aLen); ch++) {

No need to initialize ch twice!

@@ +3689,5 @@
> +            return true;
> +        }
> +    }
> +    return false;
> +}

In the 8-bit case, this could be implemented as

  return memchr(aString, 0x20, aLen) != nullptr;

which is a built-in on gcc (and many other compilers); we should probably use that, as it's likely to be tuned for best performance.

@@ +3725,4 @@
>      uint32_t wordCacheCharLimit =
>          gfxPlatform::GetPlatform()->WordCacheCharLimit();
>  
> +    if (BypassShapedWordCache(aRunScript)) {

With the introduction of the additional conditions, I think we need to rename this - it's no longer actually making the decision of whether to bypass, only providing one of the inputs into that decision. Maybe something like SpaceMayParticipateInShaping(aRunScript)?

@@ +3726,5 @@
>          gfxPlatform::GetPlatform()->WordCacheCharLimit();
>  
> +    if (BypassShapedWordCache(aRunScript)) {
> +        if (aRunLength > wordCacheCharLimit ||
> +            HasSpaces(aString, aRunStart, aRunLength)) {

I think we need a comment here to explain the logic: when <space> may participate in shaping, we want to use the cache only if (a) the run is short enough to be cached in its entirety AND (b) it does not actually contain any spaces. (I.e., I think it's easier to understand when expressed in terms of "the conditions where we can use the cache", although the code actually tests for the opposite - the conditions where we need to bypass it.)
(In reply to Jonathan Kew (:jfkthame) from comment #28)
> 
> The problem with this is the risk that it may get out of sync with what's
> actually implemented by the shapers. Indeed, it is already inaccurate, in
> that the shaper may automatically apply 'frac', 'numr' and 'dnom' features;
> this was fairly recently added to harfbuzz,

Right.  That's why I suggested using hb_shape_plan_collect_lookups() instead.
Depends on: 975915
(In reply to Jonathan Kew (:jfkthame) from comment #28)
> The problem with this is the risk that it may get out of sync with what's
> actually implemented by the shapers. Indeed, it is already inaccurate, in
> that the shaper may automatically apply 'frac', 'numr' and 'dnom' features;
> this was fairly recently added to harfbuzz, so as to support examples like
> 
> data:text/html,<div style="font:36px Source Sans Pro">foo 12⁄345 67⁄89 bar
> 
> which now renders automatically-formatted fractions.

I don't think we (gecko + harfbuzz) should be contextually applying off-by-default features like this.  Filed that issue as bug 975915.  Making "frac" a default feature will mean word cache bypass for many fonts, including Fira Sans, the B2G system font.  I don't think this feature is beneficial enough to warrant the perf hit required to make that work.

You're right here, the list of default features needs to be kept in sync with what takes place in the shapers. But I think this shouldn't be hard (modulo the specific automatic fractions feature).  It only needs to be a superset of default features, not a precise list for a given script, and I think since that superset is roughly dictated by OpenType specs and practice that it shouldn't vary a lot.
(In reply to Behdad Esfahbod from comment #30)
> (In reply to Jonathan Kew (:jfkthame) from comment #28)
> > 
> > The problem with this is the risk that it may get out of sync with what's
> > actually implemented by the shapers. Indeed, it is already inaccurate, in
> > that the shaper may automatically apply 'frac', 'numr' and 'dnom' features;
> > this was fairly recently added to harfbuzz,
> 
> Right.  That's why I suggested using hb_shape_plan_collect_lookups() instead.

That API doesn't appear to provide any way to distinguish script/features. We'll need both for sniffing specific features used in fallback situations (e.g. small caps fallback, superscript/subscript fallback).

But I think the larger issue is that it should be possible to define a superset of default features that is consistent across shapers. I think the enabling of frac by default is a mistake, as per comment 31 (and bug 975915).
Updated based on review comments. I didn't revise the use of a hashtable and the issue of what to do about frac/numr/dnom remains.
Attachment #8378975 - Attachment is obsolete: true
Attachment #8378975 - Flags: review?(jfkthame)
Attachment #8381181 - Flags: review?(jfkthame)
Updated based on review comments.
Attachment #8379482 - Attachment is obsolete: true
Attachment #8379482 - Flags: review?(jfkthame)
Attachment #8381182 - Flags: review?(jfkthame)
Attachment #8381182 - Attachment description: fix-jpn-font-slowdown-v3.patch → patch, v3, check for spaces before doing non-cached shaping
Always a good idea to include the test in the appropriate reftest.list. :P
Attachment #8378849 - Attachment is obsolete: true
Attachment #8378849 - Flags: review?(jfkthame)
Attachment #8381183 - Flags: review?(jfkthame)
(In reply to John Daggett (:jtd) from comment #33)
> Created attachment 8381181 [details] [diff] [review]
> patch, v2, distinguish default and non-default substitution features
> 
> Updated based on review comments. I didn't revise the use of a hashtable and
> the issue of what to do about frac/numr/dnom remains.

As noted in bug 975915 comment 4, I don't think the presence of <space> in Fira's <frac> feature actually means we need to avoid the cache; we can ignore <frac> for this purpose, even though it is (correctly! though selectively) applied by default, because the automatic use of <frac> does not encompass spaces.

Nevertheless, this is an illustration of the problem we're up against here: that we're making a separate level of Gecko code intimately dependent on knowing the implementation details of the shaping process, with all the maintenance headaches that implies. But it may still be our best option, as long as we understand what we're doing.
(In reply to Jonathan Kew (:jfkthame) from comment #36)

> Nevertheless, this is an illustration of the problem we're up against here:
> that we're making a separate level of Gecko code intimately dependent on
> knowing the implementation details of the shaping process, with all the
> maintenance headaches that implies. But it may still be our best option, as
> long as we understand what we're doing.

For now this seems to be the best you can do.  When we have a better solution in HarfBuzz we can talk about it.
Comment on attachment 8381181 [details] [diff] [review]
patch, v2, distinguish default and non-default substitution features

I was curious how much extra cost there is for the more complex feature check here (compared to what we currently do), so I added timing to gfxFont::CheckForFeaturesInvolvingSpace() and tested on my Peak device.

For the various faces of Fira Sans, current trunk's implementation takes around 7-8ms (I saw a range of 6.6-8.9). With the patch here, each face now takes around 14ms (range 12.9-18.2). So we're adding roughly 7ms per Fira face to the startup time of each process.

This means that for a typical Gaia app that uses around 4 of the Fira faces (e.g. Light, Medium, Bold, and an Italic), this will add 30ms to app launch time on a GP Peak - presumably rather more on the lowest-end devices. Are we OK with that, or do we need to re-think this?
(In reply to Jonathan Kew (:jfkthame) from comment #38)
> Comment on attachment 8381181 [details] [diff] [review]
> patch, v2, distinguish default and non-default substitution features
> 
> This means that for a typical Gaia app that uses around 4 of the Fira faces
> (e.g. Light, Medium, Bold, and an Italic), this will add 30ms to app launch
> time on a GP Peak - presumably rather more on the lowest-end devices. Are we
> OK with that, or do we need to re-think this?

Since we ship Fira Sans, it would probably make sense to explicitly maintain a list of fonts that we can skip the GSUB/GPOS analysis for unless font features are explicitly in use.  Since fonts rarely include space-containing lookups in default features, I think this would be fairly easy to maintain.

How much of the time is just the time to load the GSUB/GPOS tables? We won't avoid that part by skipping the space lookup analysis.
(In reply to John Daggett (:jtd) from comment #39)
> How much of the time is just the time to load the GSUB/GPOS tables? We won't
> avoid that part by skipping the space lookup analysis.

Right, we need to load those tables at some point before we can use the font. But the existing CheckForFeaturesInvolvingSpace() was already doing that, so the *increase* measured here is strictly the result of doing more complex analysis once those tables are loaded.
My understanding is that this patch will fix bug 967292 based on https://bugzilla.mozilla.org/show_bug.cgi?id=967292#c29. So nominating for 1.4 since this one block a blocker of bug 942750 which is already 1.4+'ed.
blocking-b2g: --- → 1.4?
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #41)
> My understanding is that this patch will fix bug 967292 based on
> https://bugzilla.mozilla.org/show_bug.cgi?id=967292#c29. So nominating for
> 1.4 since this one block a blocker of bug 942750 which is already 1.4+'ed.

This should help slightly, as it will make textrun creation a little cheaper, but according to the comments in bug 967292 on the simpler patch proposed there - which has an equivalent effect as far as general textrun perf is concerned - it won't make a noticeable difference overall. (See bug 967292 comment 30.)

Note also that this patch risks regressing app startup time (see comment 38 here), which may be at least as big a concern as the marginal text-perf improvement it will offer. (The "50% slowdown" of this bug's summary reflects a particularly bad case, it's not at all typical of what you'll see in Gaia apps.)
Lets work on this in 1.5 given the risk it poses to slowing down.
blocking-b2g: 1.4? → 1.5?
(In reply to Jonathan Kew (:jfkthame) from comment #38)
> Comment on attachment 8381181 [details] [diff] [review]
> patch, v2, distinguish default and non-default substitution features
> 
> I was curious how much extra cost there is for the more complex feature
> check here (compared to what we currently do), so I added timing to
> gfxFont::CheckForFeaturesInvolvingSpace() and tested on my Peak device.
> 
> For the various faces of Fira Sans, current trunk's implementation takes
> around 7-8ms (I saw a range of 6.6-8.9). With the patch here, each face now
> takes around 14ms (range 12.9-18.2). So we're adding roughly 7ms per Fira
> face to the startup time of each process.
> 
> This means that for a typical Gaia app that uses around 4 of the Fira faces
> (e.g. Light, Medium, Bold, and an Italic), this will add 30ms to app launch
> time on a GP Peak - presumably rather more on the lowest-end devices. Are we
> OK with that, or do we need to re-think this?

Is there a way to trigger this check from JavaScript ? Applications are usually loaded into a process which is pre-allocated. This pre-allocated process, contains a pre-loaded tab that is doing various thing in order to save some load time (see http://mxr.mozilla.org/mozilla-central/source/dom/ipc/preload.js). If there is a way to trigger this check programatically from JS then this additional load time could be part of the preloading work, and won't be noticeable for users.
(In reply to Jonathan Kew (:jfkthame) from comment #42)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #41)
> > My understanding is that this patch will fix bug 967292 based on
> > https://bugzilla.mozilla.org/show_bug.cgi?id=967292#c29. So nominating for
> > 1.4 since this one block a blocker of bug 942750 which is already 1.4+'ed.
> 
> This should help slightly, as it will make textrun creation a little
> cheaper, but according to the comments in bug 967292 on the simpler patch
> proposed there - which has an equivalent effect as far as general textrun
> perf is concerned - it won't make a noticeable difference overall. (See bug
> 967292 comment 30.)

I wonder if the results there are affected by the switch from JS to emulate position: sticky to really use position: sticky. I'm trying to get rid of it in sms (bug 977680) and the call log (bug 978067). It would be nice if BenWa can re-profile without position: sticky.

If the perf improvment is nice (it seems to help for me, without any profile, just visually) and the load time impact could be move to the pre-allocated process, then I think we should reconsider it for 1.4. This + text-rendering: optimizeSpeed seems to make it harder to checkerboard some apps for me.

BenWa do you have some cycle to profile sms without position: sticky with and without this patch. As well as adding |* { text-rendering: optimizeSpeed; }| in b2g/chrome/content/content.css ?
Flags: needinfo?(bgirard)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #45)
> BenWa do you have some cycle to profile sms without position: sticky with
> and without this patch. As well as adding |* { text-rendering:
> optimizeSpeed; }| in b2g/chrome/content/content.css ?

I've spent a *lot* of effort making the profiler easy to use for everyone in the hope to get more people looking at performance. My goal was to make it easy for people to get their performance numbers while they were developing their own changes since they are the best fit for understanding the expected, and *unexpected*, impacts of their changes.

For now I'd rather respectfully decline and offer assistance to anyone trying to profile or understand the results.

Also if we're expecting these changes to affect one or two function like nsDisplayText then it's probably better to add manual timestamps to that function to validate the fix.
Flags: needinfo?(bgirard)
Explicitly whitelist common B2G fonts (Fira Sans, Fira Mono) that lack feature lookups containing spaces for default features. This will avoid the startup regression pointed out in comment 38 and should actually improve the startup time slightly in common usage.

The patch also dumps space feature info and timing info to the fontinit log so that we can assess possible problem fonts as issues arise.

I think this resolves the main outstanding issue here and with this we can push forward and land the patches here.
Attachment #8384461 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #42)
> Note also that this patch risks regressing app startup time (see comment 38
> here), which may be at least as big a concern as the marginal text-perf
> improvement it will offer. (The "50% slowdown" of this bug's summary
> reflects a particularly bad case, it's not at all typical of what you'll see
> in Gaia apps.)

With the latest patch, I don't think this is true.  This will assure that we use the word cache in the majority of cases across platforms, including B2G.
tryserver build with all patches
https://tbpl.mozilla.org/?tree=Try&rev=fda917f09c9b
Includes times to call CheckForFeaturesInvolvingSpace.
Comment on attachment 8384461 [details] [diff] [review]
patch, use whitelist pref to avoid checking common B2G fonts

Review of attachment 8384461 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh. I guess we can do this, although each time we add another of these font-specific hacks, I'm sure another kitten somewhere dies....

However, as written it does have one critical flaw - see below.

::: modules/libpref/src/init/all.js
@@ +1488,5 @@
> +#if MOZ_B2G
> +// for fonts that ship with B2G and are known to not include space lookups for
> +// non-default font features, avoid the check unless features are explicitly enabled
> +// use NSPR_LOG_MODULES=fontinit:5 to dump out details of space lookups
> +pref("font.whitelist.skip_space_lookup_check", "Fira Sans,Fira Mono");

This is wrong; the font families we ship are named Fira Sans OT and Fira Mono OT. (See the font.name.* prefs in this file.)

Also, the comment could use a bit of punctuation; looks like it's really two separate comments.
Comment on attachment 8381182 [details] [diff] [review]
patch, v3, check for spaces before doing non-cached shaping

Review of attachment 8381182 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.h
@@ +1796,5 @@
>  
>      // whether font contains substitution lookups containing spaces
>      bool HasSubstitutionRulesWithSpaceLookups(int32_t aRunScript);
>  
> +    // do spaces participate in shaping rules? if so, can't used word cache

Maybe more accurate to say "If so, we may need to bypass the word cache." (After all, we still use it if the string is short and space-free.)
Attachment #8381182 - Flags: review?(jfkthame) → review+
Comment on attachment 8381181 [details] [diff] [review]
patch, v2, distinguish default and non-default substitution features

Review of attachment 8381181 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +2140,4 @@
>  }
>  
>  nsDataHashtable<nsUint32HashKey, int32_t> *gfxFont::sScriptTagToCode = nullptr;
> +nsTHashtable<nsUint32HashKey>             *gfxFont::sDefaultFeatures = nullptr;

I guess we're OK with not cleaning up these hashtables at shutdown? Given that they don't contain "interesting" objects, it's only the singleton hashtables themselves that we're leaking - and that's already the case for the script-tag one, so <shrug>...

@@ +2147,5 @@
> +    return (aBitVector[aBit >> 5] & (1 << (aBit & 0x1f))) != 0;
> +}
> +
> +// union of all default substitution features across scripts
> +static const hb_tag_t defaultFeatures[] = {

With the whitelist to address the issue of the b2g fonts, we should add frac/numr/dnom here given that the shaper -may- apply these by default. It won't (by default) apply them across spaces, but in principle a font might have contextual lookups that depend on seeing the adjacent spaces.

(The same applies to many of the other default features that in practice will rarely, if ever, actually care about spaces - but any attempt on our side to pre-judge what font designers might do is risky.)

@@ +2209,5 @@
>      if (hb_ot_layout_has_substitution(face)) {
>  
>          // set up the script ==> code hashtable if needed
>          if (!sScriptTagToCode) {
>              sScriptTagToCode = new nsDataHashtable<nsUint32HashKey, int32_t>(MOZ_NUM_SCRIPT_CODES);

This looks very long - might be nice to wrap it, while you're here?

@@ +2222,5 @@
>                  }
>              }
> +
> +            uint32_t numDefaultFeatures = ArrayLength(defaultFeatures);
> +            sDefaultFeatures = new nsTHashtable<nsUint32HashKey>(numDefaultFeatures);

Also somewhat over 80 chars, I think.
Comment on attachment 8381183 [details] [diff] [review]
patch, reftest for various types of fonts with space-containing lookups

Review of attachment 8381183 [details] [diff] [review]:
-----------------------------------------------------------------

I admit I haven't gone through all this carefully yet, but here are a few thoughts from looking at one of the TTX dumps; I assume this is essentially the same in all the fonts.

::: layout/reftests/fonts/spacelookups/spacelookup-defscr-deflang-deffeat.ttx
@@ +270,5 @@
> +      <map code="0x77" name="w"/><!-- LATIN SMALL LETTER W -->
> +      <map code="0x78" name="x"/><!-- LATIN SMALL LETTER X -->
> +      <map code="0x79" name="y"/><!-- LATIN SMALL LETTER Y -->
> +      <map code="0x7a" name="z"/><!-- LATIN SMALL LETTER Z -->
> +      <map code="0xff00" name=".notdef"/><!-- ???? -->

What's 0xff00 doing here? That seems peculiar.

@@ +603,5 @@
> +
> +    <TTGlyph name="uni0000"/><!-- contains no outline data -->
> +    <TTGlyph name="nonmarkingreturn"/><!-- contains no outline data -->
> +    <TTGlyph name="space"/><!-- contains no outline data -->
> +    <TTGlyph name="hyphen"/><!-- contains no outline data -->

I think it'd be helpful to include simple outlines for the ASCII characters used here, instead of leaving them all blank; it will make it much easier to see what's happening if we ever look at testcases where the features are -not- being applied.

@@ +642,5 @@
> +    <TTGlyph name="y"/><!-- contains no outline data -->
> +    <TTGlyph name="z"/><!-- contains no outline data -->
> +
> +    <TTGlyph name="firefox" xMin="0" yMin="-75" xMax="799" yMax="689">
> +      <contour>

Are these glyphs the result of auto-tracing some scans, or what? The outline data is horribly rough, which makes the files unnecessarily bloated.

How about:

(a) ensuring each glyph being used as a feature "target" is unique (currently, there are duplicates, which makes it more difficult to be sure which feature is actually being applied);

(b) using symbols from an existing (free) font with clean, simple outlines;

(c) encoding the feature-target symbols at PUA codepoints (in addition to accessing them via GSUB rules), so that it's easy to reftest by using (feature-free) PUA text in the reference case.
Updated based on review comments.
Attachment #8384461 - Attachment is obsolete: true
Attachment #8384461 - Flags: review?(jfkthame)
Attachment #8386492 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #54)

> ::: layout/reftests/fonts/spacelookups/spacelookup-defscr-deflang-deffeat.ttx
> @@ +270,5 @@
> > +      <map code="0x77" name="w"/><!-- LATIN SMALL LETTER W -->
> > +      <map code="0x78" name="x"/><!-- LATIN SMALL LETTER X -->
> > +      <map code="0x79" name="y"/><!-- LATIN SMALL LETTER Y -->
> > +      <map code="0x7a" name="z"/><!-- LATIN SMALL LETTER Z -->
> > +      <map code="0xff00" name=".notdef"/><!-- ???? -->
> 
> What's 0xff00 doing here? That seems peculiar.

> @@ +642,5 @@
> > +    <TTGlyph name="y"/><!-- contains no outline data -->
> > +    <TTGlyph name="z"/><!-- contains no outline data -->
> > +
> > +    <TTGlyph name="firefox" xMin="0" yMin="-75" xMax="799" yMax="689">
> > +      <contour>
> 
> Are these glyphs the result of auto-tracing some scans, or what? The outline
> data is horribly rough, which makes the files unnecessarily bloated.

Er, um, okay. I simply took the font you made and checked in
(LigatureSymbolsWithSpaces.woff) and swizzled it around. These
irregularities are present in the original font. :P  

Unless you have a better option, I'll take FontAwesome as a base and rework that.

> How about:
> 
> (a) ensuring each glyph being used as a feature "target" is unique
> (currently, there are duplicates, which makes it more difficult to be sure
> which feature is actually being applied);
> 
> (b) using symbols from an existing (free) font with clean, simple outlines;
> 
> (c) encoding the feature-target symbols at PUA codepoints (in addition to
> accessing them via GSUB rules), so that it's easy to reftest by using
> (feature-free) PUA text in the reference case.

Ok, sounds reasonable.
Updated based on review comments.

Switched to using Junction (League of Moveable Type) for the base ASCII glyphs, combined with a number of icon glyphs from Font Awesome.

- real glyphs for ASCII codepoints
- individual icon glyphs for all the default ligature glyphs (e.g. default-script, default-lang, etc.)
- PUA mappings for all of the icon glyphs

The reftest has been updated to use the PUA mappings.
Attachment #8381183 - Attachment is obsolete: true
Attachment #8381183 - Flags: review?(jfkthame)
Attachment #8386550 - Flags: review?(jfkthame)
Updated based on review comments (except as noted below).

> With the whitelist to address the issue of the b2g fonts, we should add
> frac/numr/dnom here given that the shaper -may- apply these by default. It
> won't (by default) apply them across spaces, but in principle a font might
> have contextual lookups that depend on seeing the adjacent spaces.

Doing this would actually defeat the main purpose of the fixes here!
The fonts listed in the original report (Meiryo, Hiragino families)
all support the "frac" feature. So making "frac" a default feature
would, in effect, disable the use of the word cache for strings
containing spaces. I would also disable the use of the word cache for
any font supporting the "frac" feature and I expect the usage of this
to only increase in the future.

The list of default features should be a superset of possible features
that may be on by default in a shaper. The fact that harfbuzz has code
that enables 'frac' by default is unfortunate and I don't think it's
really such a great idea (see discussion on bug 975915).  But based on
bug 975915, comment 4, I don't think this is an issue since the
enabling of the "frac" feature to enable automatic fractions when a
fraction-slash is used in the string doesn't depend on lookups that
involve spaces.  I think we should assume that rather than taking the
perf hit for these commonly used fonts.
Attachment #8381181 - Attachment is obsolete: true
Attachment #8381181 - Flags: review?(jfkthame)
Attachment #8386571 - Flags: review?(jfkthame)
(In reply to John Daggett (:jtd) from comment #58)
> Created attachment 8386571 [details] [diff] [review]
> patch, v2a, distinguish default and non-default substitution features
> 
> Updated based on review comments (except as noted below).
> 
> > With the whitelist to address the issue of the b2g fonts, we should add
> > frac/numr/dnom here given that the shaper -may- apply these by default. It
> > won't (by default) apply them across spaces, but in principle a font might
> > have contextual lookups that depend on seeing the adjacent spaces.
> 
> Doing this would actually defeat the main purpose of the fixes here!
> The fonts listed in the original report (Meiryo, Hiragino families)
> all support the "frac" feature. So making "frac" a default feature
> would, in effect, disable the use of the word cache for strings
> containing spaces. I would also disable the use of the word cache for
> any font supporting the "frac" feature and I expect the usage of this
> to only increase in the future.

This is only true if the 'frac' feature in those fonts involves the <space> glyph, surely. Does it? I don't think that's normally the case - Fira is quite unusual in this regard.
(In reply to John Daggett (:jtd) from comment #56)

> > Are these glyphs the result of auto-tracing some scans, or what? The outline
> > data is horribly rough, which makes the files unnecessarily bloated.
> 
> Er, um, okay. I simply took the font you made and checked in
> (LigatureSymbolsWithSpaces.woff) and swizzled it around. These
> irregularities are present in the original font. :P  

Ewww, yuck. (In my defence, I didn't actually create that! It was pulled from an example on the web, see bug 761442 comment 28.)

> Unless you have a better option, I'll take FontAwesome as a base and rework
> that.

That sounds fine, thanks.
(In reply to Jonathan Kew (:jfkthame) from comment #59)
> This is only true if the 'frac' feature in those fonts involves the <space>
> glyph, surely. Does it? I don't think that's normally the case - Fira is
> quite unusual in this regard.

I double-checked and you're right, for Meiryo and the Hiragino
families the frac/dnom/numr lookups don't have spaces in them.
However, several standard Adobe fonts, Minion Pro, Myriad Pro, Adobe
Garamond Pro and Source Sans Pro, *do* have lookups for these features
that include spaces.  So I don't think assuming this is rare is a safe
assumption.

In most cases, I think implementations should "do the right thing" and
figure out how to make performance optimal without sacrificing quality.
But in this case, I think the standard expectation of authors is that
fraction formatting involves explicit markup.  I don't think we should
sacrifice performance here for a general class of fonts out of concern
that the automatic handling of fraction-slash's *might* not work in
all cases.
(In reply to John Daggett (:jtd) from comment #61)
> 
> But in this case, I think the standard expectation of authors is that
> fraction formatting involves explicit markup.

You keep repeating this.  You are wrong.  Author expectations might currently be that, but only because that's what status quo has been.  In the same vain, people in India also expect that Indic rendering be broken and buggy...

What you say is correct for ASCII slash.  But Unicode FRACTION SLASH should Just Work.

>  I don't think we should
> sacrifice performance here for a general class of fonts out of concern
> that the automatic handling of fraction-slash's *might* not work in
> all cases.
(In reply to Behdad Esfahbod from comment #62)
> > But in this case, I think the standard expectation of authors is that
> > fraction formatting involves explicit markup.
> 
> You keep repeating this.  You are wrong.  Author expectations might
> currently be that, but only because that's what status quo has been.  In the
> same vain, people in India also expect that Indic rendering be broken and
> buggy...
> 
> What you say is correct for ASCII slash.  But Unicode FRACTION SLASH should
> Just Work.

Behdad, you're overstating your case here. Displaying Indic text
incorrectly is wrong but automatically applying the frac/dnom/numr
features for strings containing spaces is a much subtler issue I
think. Author expectations are important here. And for a browser,
delicate handling of text strings is always balanced with the need to
layout out text quickly and efficiently.

Speaking with other layout engine developers (Eric Muller at Adobe), I
don't think this issue is as black-and-white as you make it out to be.
His opinion was that making explicit markup a requirement is not
unreasonable here.  But that's really a discussion for bug 975915.

The issue for this bug is whether we treat frac/dnom/numr features as
default features and avoid using the word cache when the feature
lookups for these features contain spaces. The question is whether the
special case FRACTION SLASH handling should "just work" for
string-font combinations *where the presence of spaces affects
layout*. You're arguing an absolute "yes" to this and I'm saying I
think that's out of balance with performance needs and author
expectations, both for FRACTION SLASH handling and for automatic
fractions in general.

Jonathan's comment from bug 975915, comment 4:

> Fortunately, we don't actually need to avoid the word cache for
> this, because of the Unicode spec for how Fraction Slash behaves:
> the effect does not include or reach across spaces. The Fira 'frac'
> feature may also affect spaces (intended to improve examples such as
> "1 ²⁄₃" by reducing the space), but to gain *that* benefit authors
> will need to explicitly apply it. So for purposes of word caching, I
> think we can legitimately ignore the frac feature, even though
> harfbuzz may apply it.

To me this suggests that for the specific implementation issue here,
in which situations should the word cache not be used, we should *not*
consider frac/dnom/numr a default feature.
(In reply to John Daggett (:jtd) from comment #63)
> (In reply to Behdad Esfahbod from comment #62)
> > > But in this case, I think the standard expectation of authors is that
> > > fraction formatting involves explicit markup.
> > 
> > You keep repeating this.  You are wrong.  Author expectations might
> > currently be that, but only because that's what status quo has been.  In the
> > same vain, people in India also expect that Indic rendering be broken and
> > buggy...
> > 
> > What you say is correct for ASCII slash.  But Unicode FRACTION SLASH should
> > Just Work.
> 
> Behdad, you're overstating your case here. Displaying Indic text
> incorrectly is wrong but automatically applying the frac/dnom/numr
> features for strings containing spaces is a much subtler issue I
> think.

Not really. The expectation that (for example) the presence of virama results in the substitution of conjoining forms in Indic scripts, and that a sequence of <digit> <fraction-slash> <digit> results in rendering as a fraction are both behaviors that are an inherent property of the Unicode characters involved.

The fact that fraction rendering is achieved by applying certain OpenType features is an implementation detail; it could also have been implemented (more cumbersomely) by adjusting font sizes and positioning at some other level. But given the existence of the OT features, that is by far the simplest way for us to implement it - just like that's the way we implement Indic-script support.

And the fact that we care whether the text string contains spaces is, again, purely an implementation detail on the Gecko side; it has no bearing on what is the correct behavior for these Unicode characters or OpenType features.


> Jonathan's comment from bug 975915, comment 4:
> 
> > Fortunately, we don't actually need to avoid the word cache for
> > this, because of the Unicode spec for how Fraction Slash behaves:
> > the effect does not include or reach across spaces. The Fira 'frac'
> > feature may also affect spaces (intended to improve examples such as
> > "1 ²⁄₃" by reducing the space), but to gain *that* benefit authors
> > will need to explicitly apply it. So for purposes of word caching, I
> > think we can legitimately ignore the frac feature, even though
> > harfbuzz may apply it.
> 
> To me this suggests that for the specific implementation issue here,
> in which situations should the word cache not be used, we should *not*
> consider frac/dnom/numr a default feature.

Yes, in view of the fact that the use of these features to implement fraction-slash support can never (per the Unicode rules) result in applying the features across <space> characters, I'm willing to exclude them from the "default" list here, even though that's strictly speaking inaccurate.

The only time this would affect the result, AFAICS, is if the numr/dnom/frac lookups for a <digits><fraction><digits> sequence involve a contextual look-ahead or -behind rule that wants to see the adjacent <space>, e.g. to use alternate numerator/denominator glyphs for the first/last digits of the fraction. This seems sufficiently far-fetched that I'm prepared to allow it to fail, at least for now; if we encounter real-world fonts that depend on something like this (which seems unlikely), perhaps we'll have to reconsider.
Comment on attachment 8386492 [details] [diff] [review]
patch, v2, use whitelist pref to avoid checking common B2G fonts

Review of attachment 8386492 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFT2FontList.cpp
@@ +1363,5 @@
> +    uint32_t numFonts = skiplist.Length();
> +    for (uint32_t i = 0; i < numFonts; i++) {
> +        nsAutoString keyName;
> +        keyName = skiplist[i];
> +        ToLowerCase(keyName);

Why not simply do
  ToLowerCase(skiplist[i])
and use that as the entry key? The string copy looks redundant.

::: gfx/thebes/gfxFont.cpp
@@ +83,5 @@
> +#define LOG_FONTINIT(args) PR_LOG(gfxPlatform::GetLog(eGfxLog_fontinit), \
> +                               PR_LOG_DEBUG, args)
> +#define LOG_FONTINIT_ENABLED() PR_LOG_TEST( \
> +                                   gfxPlatform::GetLog(eGfxLog_fontinit), \
> +                                   PR_LOG_DEBUG)

Please tidy up the indentation here.

::: modules/libpref/src/init/all.js
@@ +1498,5 @@
>  // Some CJK fonts have bad underline offset, their CJK character glyphs are overlapped (or adjoined)  to its underline.
>  // These fonts are ignored the underline offset, instead of it, the underline is lowered to bottom of its em descent.
>  pref("font.blacklist.underline_offset", "FangSong,Gulim,GulimChe,MingLiU,MingLiU-ExtB,MingLiU_HKSCS,MingLiU-HKSCS-ExtB,MS Gothic,MS Mincho,MS PGothic,MS PMincho,MS UI Gothic,PMingLiU,PMingLiU-ExtB,SimHei,SimSun,SimSun-ExtB,Hei,Kai,Apple LiGothic,Apple LiSung,Osaka");
>  
> +#if MOZ_B2G

Convention in this file is to use #ifdef, except for more complex conditions.

Also, in general we seem to use #ifdef MOZ_WIDGET_GONK here. I don't know if MOZ_B2G is defined when we process this, or whether it's equivalent. Please follow that unless you have a good reason to differ.
Comment on attachment 8386550 [details] [diff] [review]
patch, v3, reftest for various types of fonts with space-containing lookups

Review of attachment 8386550 [details] [diff] [review]:
-----------------------------------------------------------------

This looks basically fine, except it's rather bulky. If you strip the hinting from the fonts, that should more or less halve the size of everything here (as well as making the .ttx files more manageable to look at). You can easily do this at the TTX level by just removing the <prep>, <fpgm> and <cvt> tables, and all the truetype assembly content of the glyph instructions.
(In reply to John Daggett (:jtd) from comment #61)
> 
> However, several standard Adobe fonts, Minion Pro, Myriad Pro, Adobe
> Garamond Pro and Source Sans Pro, *do* have lookups for these features
> that include spaces.

What exactly do these fonts do with <space> in these features? Is it part of the input or part of the context? That will determine whether there's going to be any impact on behavior here.
Comment on attachment 8386571 [details] [diff] [review]
patch, v2a, distinguish default and non-default substitution features

Review of attachment 8386571 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not terribly happy that we're going to all this trouble to analyse the font, and -still- may not behave entirely correctly in all cases - not only the potential edge-case with fraction-slash support and spaces in the context, but also the difference in behavior between automatic kerning and explicitly-enabled kerning, and the fact that (IIRC) we don't check for spaces in any legacy 'kern' tables that we may apply, only in GPOS 'kern'. But I suppose this will do for now, until such time as something prompts us to revisit the corner cases.

::: gfx/thebes/gfxFont.h
@@ +1795,5 @@
>          return mFontEntry->TryGetSVGData(this);
>      }
>  
>  protected:
> +    friend class gfxPlatform; // for static singleton cleanup

I think it would seem cleaner to give gfxFont a public static Cleanup() or Shutdown() method, and call that from gfxPlatform::Shutdown(), like we do for various other classes.

::: gfx/thebes/gfxPlatform.cpp
@@ +503,5 @@
>  
>      gfxPrefs::DestroySingleton();
>  
> +    delete gfxFont::sScriptTagToCode;
> +    delete gfxFont::sDefaultFeatures;

How about gfxFont::DestroySingletons() ?
Attachment #8386571 - Flags: review?(jfkthame) → review+
Updated based on review comments. Removed hinting instructions along with prep, fpgm, and cvt tables. Font size reduced from 24k to 13k.
Attachment #8386550 - Attachment is obsolete: true
Attachment #8386550 - Flags: review?(jfkthame)
Attachment #8389024 - Flags: review?(jfkthame)
Updated based on review comments.
Attachment #8386492 - Attachment is obsolete: true
Attachment #8386492 - Flags: review?(jfkthame)
Attachment #8389050 - Flags: review?(jfkthame)
Comment on attachment 8389050 [details] [diff] [review]
patch, v3, use whitelist pref to avoid checking common B2G fonts

Review of attachment 8389050 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, modulo a suggested renaming for greater clarity - see below.

::: gfx/thebes/gfxFont.h
@@ +437,5 @@
>      bool             mHasSpaceFeaturesInitialized : 1;
>      bool             mHasSpaceFeatures : 1;
>      bool             mHasSpaceFeaturesKerning : 1;
>      bool             mHasSpaceFeaturesNonKerning : 1;
> +    bool             mSkipSpaceFeatureCheck : 1;

On re-reading this patch, I think you should make this flag name more explicit: name it mSkipDefaultFeatureSpaceCheck, or something along those lines. Otherwise it's a bit misleading when reading the code, as we -do- still do the check for space if non-default features are enabled.

::: modules/libpref/src/init/all.js
@@ +1503,5 @@
> +// Whitelist of fonts that ship with B2G that do not include space lookups in
> +// default features. This allows us to skip analyzing the GSUB/GPOS tables
> +// unless features are explicitly enabled.
> +// Use NSPR_LOG_MODULES=fontinit:5 to dump out details of space lookups
> +pref("font.whitelist.skip_space_lookup_check", "Fira Sans OT,Fira Mono OT");

And maybe adjust this name similarly, e.g. skip_default_features_space_check. Though I'm less concerned about this than the flag in the code, where the unclear name makes it harder to keep track of the full logic we're using.
Attachment #8389050 - Flags: review?(jfkthame) → review+
(In reply to John Daggett (:jtd) from comment #69)
> Created attachment 8389024 [details] [diff] [review]
> patch, v4, reftest for various types of fonts with space-containing lookups
> 
> Updated based on review comments. Removed hinting instructions along with
> prep, fpgm, and cvt tables. Font size reduced from 24k to 13k.

Thanks, that's nicer to look through.

So these tests are all very well as far as they go: they test that we can apply features across space - i.e. choose the non-word-caching path - when necessary. But what they fail to test, AFAICS, is that we *do* still use the word cache in cases where that is allowed. E.g. using the *-ndeffeat fonts, with no explicit features in the style, we *should* be using the word cache - but that's not tested.

I think you could test this, given how we handle 'kern', by adding kern pairs that involve space to the font, and checking that they're NOT applied in the cases where caching is allowed.
> I think you could test this, given how we handle 'kern', by adding kern
> pairs that involve space to the font, and checking that they're NOT applied
> in the cases where caching is allowed.

Ok, done. Added GPOS tables with kern features defined for some of the glyphs. Test relies on kerns being ignored due to word cache usage in the case of fonts with non-default features containing space lookups. For fonts with space lookups in default features or when features are applied, kerns should be applied.
Attachment #8389024 - Attachment is obsolete: true
Attachment #8389024 - Flags: review?(jfkthame)
Attachment #8389565 - Flags: review?(jfkthame)
BTW, we'll be seeing more special-behavior per Unicode characters as OpenType syncs up with Unicode.  See the 'stch' feature in Syriac shaper for example:

  http://www.microsoft.com/typography/OpenTypeDev/syriac/intro.htm
review ping

The only remaining piece here is the final review of the reftest. It would be good to get this landed for the B2G folks especially.
Comment on attachment 8389565 [details] [diff] [review]
patch, v5, reftest for various types of fonts with space-containing lookups

Review of attachment 8389565 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please re-check the metadata you're including in the testcases before landing, and update or remove as needed. (IMO, the more useful thing to have there would be a link back to this bug, so that it's easy to find later without trawling through hg blame.)

Also, it'd be nice to regenerate the .ttx files by dumping the .ttfs afresh, so as to fix up various comments that are no longer accurate due to manual editing of the table dumps. And while you're there, how about stripping the cmap subtable with '<cmap_format_0 platformID="1" platEncID="0" language="0">', which serves no useful purpose.

::: layout/reftests/font-features/spacelookups-wordcache-ref.html
@@ +5,5 @@
> +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +<link rel="author" title="John Daggett" href="mailto:jdaggett@mozilla.com"/>
> +<link rel="help" href="http://www.w3.org/TR/css-fonts-3/#default-features"/>
> +<link rel="help" href="http://www.w3.org/TR/css-fonts-3/#font-feature-settings"/>
> +<link rel="match" href="spacelookups-ref.html"/>

I don't think so?

::: layout/reftests/font-features/spacelookups-wordcache.html
@@ +5,5 @@
> +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +<link rel="author" title="John Daggett" href="mailto:jdaggett@mozilla.com"/>
> +<link rel="help" href="http://www.w3.org/TR/css-fonts-3/#default-features"/>
> +<link rel="help" href="http://www.w3.org/TR/css-fonts-3/#font-feature-settings"/>
> +<link rel="match" href="spacelookups-kern-ref.html"/>

It's not clear to me what value you're adding with the links etc here, but if you want to include them, they should at least be accurate :) ... this one looks like you've changed the filename of the reference, and failed to update the href here.

::: layout/reftests/fonts/spacelookups/spacelookup-defscr-deflang-deffeat.ttx
@@ +4912,5 @@
> +
> +  <GPOS>
> +    <Version value="1.0"/>
> +    <ScriptList>
> +      <!-- ScriptCount=2 -->

I only see one ScriptRecord. And there are a bunch more "stale" comments below.
Attachment #8389565 - Flags: review?(jfkthame) → review+
I see an average of 10 fps on FF 30.0a2 in comparison with 20 fps on FF 31.0a1, Win 7 x64, using the 'Meiryo' font.
Thoughts ?
Flags: needinfo?
That sounds like the expected improvement in FF31 compared to FF30. For a further comparison, what do you get with FF22?
Flags: needinfo?
FF 22 - Test Average: 37 fps
So there's still a big difference between FF22 and current trunk, apparently. John, any thoughts on what additional factors may be affecting this?
Flags: needinfo?(jdaggett)
Component: Layout: Text → Graphics: Text
Flags: needinfo?(jdaggett)
Depends on: 987408
Here are some numbers running the original test:

Running with Meiryo set as the default font on Windows 7.  Nightly
(including the patches on this bug) vs previous versions, using test
in URL link.

           test   ratio of FF22
  FF22     46.6
  FF23     21.3   0.46
  FF28     16.7   0.36
  Nightly  29.2   0.63

Pastebin containing textbench results for FF22 vs. Nightly 31a1
http://pastebin.mozilla.org/4688327

FF22 vs. Nightly 31a1, Win7 with Meiryo set as the default font:

basic_generic_standard_fonts_serif_simpletext_en      28.2ms[1.6%]  36.2ms[2.3%]  128.5% ▼
basic_generic_standard_fonts_sansserif_simpletext_en  30.3ms[0.9%]  37.2ms[1.9%]  122.9% ▼
basic_generic_standard_fonts_monospace_simpletext_en  22.7ms[2.0%]  33.1ms[1.9%]  145.6% ▼

Off the top of my head I'd guess that this is due to harfbuzz updates
but I'd really need to profile this to be able to drill down on this.
The Gecko profiler isn't giving me very clear results here.
Note: the patches landed contained a slipup, I renamed the pref but didn't adjust the code using the pref which resulted in the whitelist not being in place. Any backporting should include the correct prefname (as fixed by bug 987408).
Fabrice, does this need backporting to B2G 1.3?
Flags: needinfo?(fabrice)
Whiteboard: [c= p= s=2014.04.11 u=]
(In reply to John Daggett (:jtd) from comment #86)
> Fabrice, does this need backporting to B2G 1.3?

It's all about how risky this is. If it feels safe to you, I would take that on 1.3t
Flags: needinfo?(fabrice)
blocking-b2g: 2.0? → ---
Nom'ing for 1.4 based on partner testing in bug 986681 comment 35.  While it did not completely fix contact app checkerboarding, Tapas indicates it did improve the situation significantly.

Any uplift should include the follow-up pref fix in bug 987408.
blocking-b2g: --- → 1.4?
:jtd,

Please comment on risk of the patch. Help us with risk analysis.
Flags: needinfo?(jdaggett)
The patches here do a slower analysis of fonts used but there's a whitelist mechanism that should speed up the use of Fira Sans.  Assuming the 1.4 tree isn't far from Gecko trunk, I don't think it would be that hard to port it, nor do I think it would be that risky.
Flags: needinfo?(jdaggett)
blocking-b2g: 1.4? → 1.4+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #91)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/d140b15c4e1e
> https://hg.mozilla.org/releases/mozilla-aurora/rev/c963101ad96e
> https://hg.mozilla.org/releases/mozilla-aurora/rev/393fe4873574
> https://hg.mozilla.org/releases/mozilla-aurora/rev/73aa08de22e9

I think this missed the fix in bug 987408.  I nom'd that bug again to get the fix.  This won't have any effect until the pref is fixed unfortunately.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.