Stop using a hash as key in the FontCascadeCache. Instead, use HashTraits and use FontCascadeCacheKey as key.
Created attachment 424540 [details] Patch
Comment on attachment 424540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424540&action=review > Source/WebCore/platform/graphics/FontCache.cpp:462 > + auto addResult = m_fontCascadeCache.add(key, nullptr); This is going to require a change to Chris Lord’s patch to make the font cache work from non-main threads. We’ll have to isolate the strings in the key after adding it. > Source/WebCore/platform/graphics/FontCache.cpp:584 > + // FIXME: Should hash the key and the family name characters rather than making a hash out of other hashes. Also should use Hasher instead of IntegerHasher. And, yes, I know that’s how we got into this mess in the first place.
Created attachment 424559 [details] Patch
Created attachment 424565 [details] Patch
Comment on attachment 424565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424565&action=review > Source/WebCore/platform/graphics/FontCache.cpp:406 > +bool FontCascadeCacheKey::operator==(const FontCascadeCacheKey& other) const This does’t need to be a member function. Could just rename from keysMatch to operator== and leave it as a non-member function. > Source/WebCore/platform/graphics/FontCache.cpp:590 > + for (unsigned i = 0; i < key.families.size(); ++i) { > + auto& family = key.families[i]; How about a modern for loop? for (auto& family : key.families) {
(In reply to Darin Adler from comment #5) > Comment on attachment 424565 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424565&action=review > > > Source/WebCore/platform/graphics/FontCache.cpp:406 > > +bool FontCascadeCacheKey::operator==(const FontCascadeCacheKey& other) const > > This does’t need to be a member function. Could just rename from keysMatch > to operator== and leave it as a non-member function. > > > Source/WebCore/platform/graphics/FontCache.cpp:590 > > + for (unsigned i = 0; i < key.families.size(); ++i) { > > + auto& family = key.families[i]; > > How about a modern for loop? > > for (auto& family : key.families) { But having the index is convenient to look up the family in the "other" object.
Created attachment 424579 [details] Patch
Comment on attachment 424565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424565&action=review >>> Source/WebCore/platform/graphics/FontCache.cpp:406 >>> +bool FontCascadeCacheKey::operator==(const FontCascadeCacheKey& other) const >> >> This does’t need to be a member function. Could just rename from keysMatch to operator== and leave it as a non-member function. > > But having the index is convenient to look up the family in the "other" object. In the == function, but I was talking about the hash function.
Comment on attachment 424579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424579&action=review > Source/WebCore/platform/graphics/FontCache.cpp:590 > + for (unsigned i = 0; i < key.families.size(); ++i) { > + auto& family = key.families[i]; Unlike the == function, this can use the modern for loop. > Source/WebCore/platform/graphics/FontCache.h:199 > +namespace WTF { We don’t need to put this in the WTF namespace. It doesn’t have to the the default hash for FontCascadeCacheKey. It can just be the hash we pass to the HashMap template. > Source/WebCore/platform/graphics/FontCache.h:219 > +typedef HashMap<FontCascadeCacheKey, std::unique_ptr<FontCascadeCacheEntry>> FontCascadeCache; How about using instead of typedef since we have to touch this line?
Created attachment 424593 [details] Patch
*** This bug has been marked as a duplicate of bug 223858 ***