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

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.

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

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

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.

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.

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.

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.

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.

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

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.

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

Updated based on review comments.

Always a good idea to include the test in the appropriate reftest.list. :P

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

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

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

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

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.

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

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.

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

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.

(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() ?

Updated based on review comments. Removed hinting instructions along with prep, fpgm, and cvt tables. Font size reduced from 24k to 13k.

Updated based on review comments.

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.

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

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.

Pushed to trunk, with changes based on review comments https://hg.mozilla.org/integration/mozilla-inbound/rev/677211f8c2e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9e91c268d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/4faf43711dac https://hg.mozilla.org/integration/mozilla-inbound/rev/652e389a3592

https://hg.mozilla.org/mozilla-central/rev/677211f8c2e2 https://hg.mozilla.org/mozilla-central/rev/0c9e91c268d4 https://hg.mozilla.org/mozilla-central/rev/4faf43711dac https://hg.mozilla.org/mozilla-central/rev/652e389a3592

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 ?

That sounds like the expected improvement in FF31 compared to FF30. For a further comparison, what do you get with FF22?

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?

Bug 987408 seems to be a case where this isn't kicking in, on trunk.

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?

(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

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.

:jtd, Please comment on risk of the patch. Help us with risk analysis.

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.

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

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