We are spending a lot of time loading fonts when loading weather.com: ~4.2% of WebProcess's cpu time in FontCache::getCachedFrontData(). In particular, we are spending a lot of time doing font auto-activation because we don't have the Open Sans fonts installed and weather.com is trying to load those.
<rdar://problem/18545368>
Created attachment 239348 [details] Patch
Created attachment 239354 [details] Patch
Comment on attachment 239354 [details] Patch This is great! Looks good to me.
Comment on attachment 239354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review Code looks right to me -- but how serious is this auto-activation at a different size problem? Should we keep a set of sets, where the value in the first set is a set of sizes? Also, I be this would be faster if you used HashSet<RetainPtr<NSString>> instead of NSMutableSet. Actually, I think you should make that change before committing, since this patch is about performance. > Source/WebCore/platform/mac/WebFontCache.mm:303 > + // FIXME: Font auto-activation also takes a font size argument so in theory, this could break Should be "...font size argument. So, in theory,...".
Is it possible to do this for iOS too? I'm trying to get rid of WebFontCache altogether.
Comment on attachment 239354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review >> Source/WebCore/platform/mac/WebFontCache.mm:303 >> + // FIXME: Font auto-activation also takes a font size argument so in theory, this could break > > Should be "...font size argument. So, in theory,...". Font files themselves don't contain particular sizes. The size argument is regarding the return value of fontWithName. I recommend you remove this entire comment.
I know Enrica already r+'ed this, but I think it should be r-'ed. Please investigate doing this in a non-mac-specific way.
(In reply to comment #5) > (From update of attachment 239354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review > > Code looks right to me -- but how serious is this auto-activation at a different size problem? Should we keep a set of sets, where the value in the first set is a set of sizes? I discussed this over with Gavin. It seemed unlikely to happen in the real world to us. Considering the performance implication, this is much better than completely disabling the font auto-activation. I will file a bug against NSFont so that we could add an API telling us if auto-activation failed because of the size only. Most of the time, however, it will fail simply because SpotLight couldn't find a matching font file. Note that having a cache that would take use [Family, size] as key (as you suggest) would not help much performance wise. The reason we end up doing auto-activation 250 times for only 10 font families is because the page is requesting the font in various sizes (and italics, weight, but mostly sizes).
(In reply to comment #8) > I know Enrica already r+'ed this, but I think it should be r-'ed. Please investigate doing this in a non-mac-specific way. What do you mean? auto-activation *is* mac-specific.
(In reply to comment #7) > (From update of attachment 239354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review > > >> Source/WebCore/platform/mac/WebFontCache.mm:303 > >> + // FIXME: Font auto-activation also takes a font size argument so in theory, this could break > > > > Should be "...font size argument. So, in theory,...". > > Font files themselves don't contain particular sizes. The size argument is regarding the return value of fontWithName. I recommend you remove this entire comment. Oh OK, this is good to know, thanks. It is safer than I thought then, good.
After discussing with Miles, it seems feasible to address the issue at higher level and cache if we already asked for a given font family in the past. Apparently, the fact that the size is different for each request does not matter. However, I need to ask for the font again if other font traits differ (e.g. italics).
(In reply to comment #9) > (In reply to comment #5) > > (From update of attachment 239354 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review > > > > Code looks right to me -- but how serious is this auto-activation at a different size problem? Should we keep a set of sets, where the value in the first set is a set of sizes? > > I discussed this over with Gavin. It seemed unlikely to happen in the real world to us. Considering the performance implication, this is much better than completely disabling the font auto-activation. I will file a bug against NSFont so that we could add an API telling us if auto-activation failed because of the size only. Most of the time, however, it will fail simply because SpotLight couldn't find a matching font file. > > Note that having a cache that would take use [Family, size] as key (as you suggest) would not help much performance wise. The reason we end up doing auto-activation 250 times for only 10 font families is because the page is requesting the font in various sizes (and italics, weight, but mostly sizes). By the way, we eventually want to move font selection to 100% CoreText functions and move away from NSFont completely (thereby allowing us to unify the mac + iOS font caches). Please see https://bugs.webkit.org/show_bug.cgi?id=134752 and https://bugs.webkit.org/show_bug.cgi?id=132159
Comment on attachment 239354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review > Source/WebCore/platform/mac/WebFontCache.mm:283 > + static NSMutableSet *set = [[NSMutableSet alloc] init]; Is this code main thread only? Please add an assertion to check for that.
(In reply to comment #14) > (From update of attachment 239354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review > > > Source/WebCore/platform/mac/WebFontCache.mm:283 > > + static NSMutableSet *set = [[NSMutableSet alloc] init]; > > Is this code main thread only? Please add an assertion to check for that. Ok, I will. The patch will likely change a lot, I am trying to handle the caching at FontCache.cpp level instead.
Ok, so I implemented this at higher level (FontCache.cpp) by basically asking the platform for the font only if this did not ask for that Family/weight/italics combination before (notice that size is not in there). However, this causes 60 font auto-activations to happens (because of the different weight / italics combinations) instead of 10 with my lower-level patch (and 250 without any change). Font auto activation only takes the font family as argument so implementing this at higher level is suboptimal. This also adds complexity to FontCache.cpp. After discussing this over with Myles, I think I will refine my earlier patch instead (use a HashSet as suggested and add a assertion for threading).
Created attachment 239450 [details] Patch
Here is a new iteration of the patch. I am setting the review flag again because the changes are non-trivial even though the approach stays the same. The following changes were made: - Use a HashSet<AtomicString> instead of an NSSet of NSStrings as suggested by Geoffrey. To avoid having to convert NSStrings into AtomicStrings (especially considering the string is initially an AtomicString), I moved the cache up to FontCacheMac.mm, where the font family is still an AtomicString, as suggested by Myles. - I added a fontWithFamily() overload to WebFontCache taking an additional |shouldAutoActivateFontIfNeeded| argument as the cache is now at the call site. I kept the old API around in case other clients are relying on it. - I added an assertion making sure the new cache is always used from the same thread, as suggested by Alexey.
Comment on attachment 239450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239450&action=review > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:104 > + if (knownFamilies.get().size() == maxCacheSize) I’d also assert that size is <= maxCacheSize.
Created attachment 239457 [details] Patch
Comment on attachment 239457 [details] Patch Clearing flags on attachment: 239457 Committed r174456: <http://trac.webkit.org/changeset/174456>
All reviewed patches have been landed. Closing bug.
This broke Mavericks build: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/builds/11204/steps/compile-webkit/logs/errors
(In reply to comment #23) > This broke Mavericks build: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/builds/11204/steps/compile-webkit/logs/errors Fixed in https://trac.webkit.org/r174457, thanks.