[webkit-reviews] review denied: [Bug 219283] Make FontCache and FontCascadeCache thread-safe : [Attachment 423029] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chris Lord <clord at igalia.com>'s
request for review:
Bug 219283: Make FontCache and FontCascadeCache thread-safe
https://bugs.webkit.org/show_bug.cgi?id=219283

Attachment 423029: Patch

https://bugs.webkit.org/attachment.cgi?id=423029&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 423029
  --> https://bugs.webkit.org/attachment.cgi?id=423029
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423029&action=review

I’m not sure this has the thread safety right. It’s good to do the locking more
consistently but I don’t understand the thread safety implications of using
AtomString in the internals here.

I need to understand more of the solid foundations of how this is thread-safe.
I’d assume that more cross-thread copying would be needed to turn strings into
a form that can be safely used across multiple threads guarded by a lock. If
someone passes in a string it can only do ref/deref on one thread. The
cross-thread copy could be used inside a lock on multiple threads.

None of this helps much with AtomString.

"More locking" is clear, but the rest of the story still seems not quite done.

> Source/WebCore/ChangeLog:9
> +	   Make the FontCache lock not iOS-specific and use it in more places.

I think one thing we should be clear on is that the iOS FontCache locking was
*only* to make it safe enough to use across the web thread and the main thread
(both return true for isMainThread) in certain ways.

These changes now aim to make this safe to use from multiple threads, including
non-web, non-main threads on iOS.

So this is not just about generalizing the iOS locking as if iOS was "ahead"
and other platforms are catching up, it’s about adding a thorough locking
strategy, which happens to allow us to remove the partial locking strategy that
was only useful on iOS.

I know you wrote *better* change log text in the bug, but I wanted to clarify
this situation and suggest even-better framing for the change.

> Source/WebCore/platform/graphics/FontCache.cpp:49
> -
>  

Remove one more blank line please.

> Source/WebCore/platform/graphics/FontCache.cpp:142
> +const String* FontCache::alternateFamilyName(const AtomString& familyName)

We should return just String or perhaps Optional<ASCIILiteral>. If it is
String, there’s no need to work so hard to avoid reference count churn since
we’re going to take a reference to the string anyway in the function that calls
this function. Since String has a convenient null value, we don’t need to use a
pointer or Optional just to add a null value. ASCIILiteral does need the
Optional, and might be a good solution depending on how this actually is used
by callers.

Changing the return type will clean up this family of functions a bit.

> Source/WebCore/platform/graphics/FontCache.cpp:144
> +    static NeverDestroyed<const String> arial("Arial",
String::ConstructFromLiteral);

These global variables are only OK because these strings are only used
internally, never returned from any function, and all use is guarded by the
font lock. It’s frustrating that there is nothing that helps make this clear.
If anyone called this function outside the lock we’d have a super-subtle bug!

Not 100% sure that it’s an important optimization to use ConstructFromLiteral
instead of just using the "Arial"_s syntax to make an ASCIILiteral, also unsure
how important the global variable optimization is.

If we use ASCIILiteral all those questions just go away and the need for global
variables goes away too.

> Source/WebCore/platform/graphics/FontCache.cpp:153
> +    const String* platformSpecificAlternate =
platformAlternateFamilyName(familyName);
> +    if (platformSpecificAlternate)
>	   return platformSpecificAlternate;

Lets scope this local inside the if statement in cases like this:

    if (auto alternate = platformAlternateFamilyName(familyName);
!alternate.isNull())
	return alternate;

> Source/WebCore/platform/graphics/FontCache.cpp:220
> +	       const String* alternateName = alternateFamilyName(familyName);
> +	       if (alternateName) {

Please scope this in the if statement:

    if (auto& alternateName = alternateFamilyName(familyName);
!alternateName.isNull()) {

> Source/WebCore/platform/graphics/FontCache.cpp:221
> +		   FontPlatformData* fontPlatformDataForAlternateName =
getCachedFontPlatformData(fontDescription, *alternateName, fontFaceFeatures,
fontFaceCapabilities, true);

I do not think it is thread-safe to turn a String into an AtomString here. I
don’t understand why it’s OK for us to pass alternateName here. What prevents
the global AtomString table from causing multiple threads to ref/deref the same
object at the same time? Once we make an AtomString, unrelated code on another
thread can also get that same string, and it’s then no longer safe to ref/deref
it on more than one thread.

Now that I realize that alternateName is being passed to an AtomString
constructor, I think it’s going to be expensive to keep looking up the names in
the AtomString function every time this is called. Also, it seems the
alternateFamilyName functions can just return ASCIILiteral if they are going to
have to be turned into an AtomString every time anyway. Using a String that is
not the AtomString has no performance advantage of value; just costs extra
reference count churn for no good reason.

Or since this function *takes* an AtomString it seems like this is
main-thread-only code. If it is, then why all the changes to the
alternateFamilyName functions? That seems unrelated to the new locking and
should be done in a separate patch with its own rationale.

Also, maybe use auto on this line.

> Source/WebCore/platform/graphics/FontCache.h:293
> +    static const String* alternateFamilyName(const AtomString&);
> +    static const String* platformAlternateFamilyName(const AtomString&);

These should just return String; no reason to use a pointer for this. (See
comments above.)

> Source/WebCore/platform/graphics/FontCascadeDescription.cpp:107
> +    if (family1.isEmpty() || family2.isEmpty())
> +	   return family1.isEmpty() && family2.isEmpty();

Why is this change needed? ASCIICaseInsensitiveHash::equal should already
handle empty strings correctly. Is this about null strings perhaps?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1431
> +const String* FontCache::platformAlternateFamilyName(const AtomString&
familyName)

Same comments as above apply.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:695
> +const String* FontCache::platformAlternateFamilyName(const AtomString&
familyName)

Same thing again.