[webkit-dev] FontPlatformData, FontCache and HashMap

[Stephan Assmus]

On 2010-02-18 at 18:43:08 [+0100], Stephan Assmus <superstippi at gmx.de> wrote:
> Hi all,
> 
> currently, I am investigating some weirdness with regards to
> FontPlatformData and the FontCache which I see in my debugging output on
> the Haiku port. I am not even 100% sure that what I see is a weirdness, but
> at least I don't understand it and I was wondering if anyone could help me
> shed some light into this.
> 
> So I understand these things:
> 
> 1) SimpleFontData has a member m_platformData of type FontPlatformData,
> which is always instantiated by passing it a const reference of another
> FontPlatformData.
> 
> 2) The FontCache maintains SimplaFontData instances in a HashMap, and looks
> these up and compares them by using FontPlatformData objects as keys into
> the map.
> 
> 3) FontCache has a static "empty" FontPlatformData instance in
> FontDataCacheKeyTraits:
> 
>     static const FontPlatformData& emptyValue()
>     {
>         DEFINE_STATIC_LOCAL(FontPlatformData, key, (0.f, false, false));
>         return key;
>     }
> 
> Don't yet know how this comes into play, but possibly it's of relevance.
> 
> 
> My analysis is a bit awkward, since I cannot use a source level debugger on
> Haiku, and I won't bore you with the embarrasing details of why. So I added
> printf() statements in all my constructors of FontPlatformData, and printed
> among other things the memory address of the FontPlatformData instance
> (this). Also, I am printing "this" in a number of other places, operator=()
> for example. The output I am getting is a little weird, since it shows that
> operator=() is called on objects, for which no constructor has been called.
> Basically, I've narrowed it down to SimpleFontData*
> FontCache::getCachedFontData(const FontPlatformData* platformData) in
> FontCache.cpp. In this method, a new SimpleFontData instance is to be
> created and inserted into the HashMap. I will show you the output from the
> program start. It printed which FontPlatformData instances have been
> created up to "Kill Thread" (I let it drop into the debugger from the first
> invokation of FontPlatformData::operator=()):
> 
> The first font is created:
> 
> getCachedFontPlatformData(0)
> createFontPlatformData(const FontDescription&)
>   0x18018660->FontPlatformData(const FontDescription(16.0, Myriad Pro))
>     0x18019bf0->FontPlatformDataPrivate()
>   created result: 0x18018660
> getCachedFontPlatformData() - done: 0x18018660
> 
> Next, getCachedFontData() is called to insert a new SimpleFontData for this
> FontDescription:
> 
> getCachedFontData(0x18018660)
> 
> Next comes the output from line 289 of FontCache.cpp:
> pair<SimpleFontData*, unsigned> newValue(new SimpleFontData(*platformData),
> 1);
> A new SimpleFontData object is created with it's m_fontData member at
> address 0x2f4c6a8:
> 
>   0x2f4c6a8->FontPlatformData(const FontPlatformData& 0x18018660)
>     0x18019bf0->addRef()
>   new: 0x2f4c680
> 
> Next comes the call to HashMap::set(), which results in this output:
> 
>   0x18018970->FontPlatformData(0.0, 0, 0)
> 
> This is probably the key created from HashMap::inlineAdd(). And next,
> HashMap::set() invokes operator=() on the key (line 250) which gives this
> output:
> 
>   0x2ec8594 ((nil), 0.0, 0, 0) ->operator=(0x18018660, 0x18019bf0)
> 
> nil is a pointer to FontPlatformDataPrivate, which is ok. But 0x2ec8594
> should be the pointer to the FontPlatformData instance created in
> inlineAdd().
> 
> How can I be doing something wrong there? Nothing happens in my constructor
> for FontPlatformData() when the empty key values are used. Maybe the usage
> of the HashMap is broken in FontCache, since it would seem that such a
> low-level utility class as HashMap would not likely contain any bugs.

Ok, I've finally figured it out. FontCache.cpp defines the following for use 
in the HashMap:

struct FontDataCacheKeyTraits : WTF::GenericHashTraits<FontPlatformData> {
    static const bool emptyValueIsZero = true;
    static const bool needsDestruction = true;
    static const FontPlatformData& emptyValue()
    {
        DEFINE_STATIC_LOCAL(FontPlatformData, key, (0.f, false, false));
        return key;
    }
    static void constructDeletedValue(FontPlatformData& slot)
    {
        new (&slot) FontPlatformData(HashTableDeletedValue);
    }
    static bool isDeletedValue(const FontPlatformData& value)
    {
        return value.isHashTableDeletedValue();
    }
};

The "emptyValueIsZero" means the HashTable implementation will not call 
constructors for new slots. IMHO, this is a dangerous assumption, since the 
porters may not realize this and that this actually works is completely 
implementation specific.

Best regards,
-Stephan