Bug 211739 – [iOS] Rebuild MobileGestalt cache if needed

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Rebuild the MobileGestalt cache in the UI process if the MobileGestalt cache is stale. This should be done before starting a new WebContent process. If the cache is not rebuilt, every MobileGestalt query in the WebContent process will fail, since access to the daemon is denied.

[Per Arne Vollan]

Created attachment 399047 [details]
Patch

[Darin Adler]

Comment on attachment 399047 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/ios/MobileGestaltSPI.h:84
> +bool _MGCacheValid();

Need to include <MobileGestalt/MobileGestaltPrivate.h> in this header.

[Per Arne Vollan]

(In reply to Darin Adler from comment #2)
> Comment on attachment 399047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399047&action=review
> 
> > Source/WebCore/PAL/pal/spi/ios/MobileGestaltSPI.h:84
> > +bool _MGCacheValid();
> 
> Need to include <MobileGestalt/MobileGestaltPrivate.h> in this header.

I added the include, but it seems the file does not exist in the SDK.

Thanks for reviewing!

[Darin Adler]

(In reply to Per Arne Vollan from comment #3)
> I added the include, but it seems the file does not exist in the SDK.

I guess despite its name it’s an "internal" header not a "private" SPI one.

[Per Arne Vollan]

(In reply to Darin Adler from comment #4)
> (In reply to Per Arne Vollan from comment #3)
> > I added the include, but it seems the file does not exist in the SDK.
> 
> I guess despite its name it’s an "internal" header not a "private" SPI one.

Ah, I see!

[Geoffrey Garen]

Comment on attachment 399047 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:123
> +SOFT_LINK_LIBRARY(libMobileGestalt)

How does the call to _MGCacheValid() work if the library is soft-linked? Don't we need to hard link to make the call?

(What's our goal in soft linking here?)

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 399047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399047&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:123
> > +SOFT_LINK_LIBRARY(libMobileGestalt)
> 
> How does the call to _MGCacheValid() work if the library is soft-linked?
> Don't we need to hard link to make the call?
> 
> (What's our goal in soft linking here?)

Ah, that's a good point. _MGCacheValid() should be soft-linked as well, so that embedding apps don't need to hard link to MobileGestalt. I will update the patch.

Thanks for reviewing!

[Geoffrey Garen]

But we're going to call it right away, right? What's the point of soft linking?

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #8)
> But we're going to call it right away, right? What's the point of soft
> linking?

Yes, so in that sense, soft-linking is not strictly needed, although I had to add soft-linking to fix a link error where the MobileGestaltHelperProxy class was undefined.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #9)
> (In reply to Geoffrey Garen from comment #8)
> > But we're going to call it right away, right? What's the point of soft
> > linking?
> 
> Yes, so in that sense, soft-linking is not strictly needed, although I had
> to add soft-linking to fix a link error where the MobileGestaltHelperProxy
> class was undefined.

For performance reasons, maybe it makes sense to not soft-link _MGCacheValid, but only MobileGestaltHelperProxy? Then the library will only be dynamically loaded if the cache is not valid.

[Geoffrey Garen]

> For performance reasons, maybe it makes sense to not soft-link
> _MGCacheValid, but only MobileGestaltHelperProxy? Then the library will only
> be dynamically loaded if the cache is not valid.

That sounds reasonable, if they're separate.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #11)
> > For performance reasons, maybe it makes sense to not soft-link
> > _MGCacheValid, but only MobileGestaltHelperProxy? Then the library will only
> > be dynamically loaded if the cache is not valid.
> 
> That sounds reasonable, if they're separate.

Ok, sounds good! Keeping patch as-is, then.

[Darin Adler]

(In reply to Per Arne Vollan from comment #10)
> For performance reasons, maybe it makes sense to not soft-link
> _MGCacheValid, but only MobileGestaltHelperProxy? Then the library will only
> be dynamically loaded if the cache is not valid.

Wait, what? That’s not how I understand linking.

If we link to a framework, there’s no point in also "soft linking" the framework.

If we "soft link" to a framework, we can’t just call a function in the framework. The build of our framework will fail..

What am I missing?

[Per Arne Vollan]

(In reply to Darin Adler from comment #13)
> (In reply to Per Arne Vollan from comment #10)
> > For performance reasons, maybe it makes sense to not soft-link
> > _MGCacheValid, but only MobileGestaltHelperProxy? Then the library will only
> > be dynamically loaded if the cache is not valid.
> 
> Wait, what? That’s not how I understand linking.
> 
> If we link to a framework, there’s no point in also "soft linking" the
> framework.
> 

In this case, even if we already link to libMobileGestalt, we have to soft-link the class MobileGestaltHelperProxy, since it is not exported by the dynamic library libMobileGestalt.

> If we "soft link" to a framework, we can’t just call a function in the
> framework. The build of our framework will fail..
> 
> What am I missing?

You are right. Both symbols _MGCacheValid and MobileGestaltHelperProxy are in the same dynamic library libMobileGestalt, so it probably does not make much sense to only soft-link MobileGestaltHelperProxy. If the dynamic library has not been loaded by the time _MGCacheValid is called, it has to be loaded then, regardless of linking method. So there is probably no performance benefit in keeping _MGCacheValid hard-linked. The class MobileGestaltHelperProxy has to be soft-linked, since it is not exported by the library. I will update the patch to soft-link both symbols.

[Per Arne Vollan]

Created attachment 399129 [details]
Patch

[Darin Adler]

Comment on attachment 399129 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:125
> +SOFT_LINK_LIBRARY(libMobileGestalt)
> +SOFT_LINK_CLASS(libMobileGestalt, MobileGestaltHelperProxy);
> +SOFT_LINK(libMobileGestalt, _MGCacheValid, bool, (), ());

I just noticed that both the WebCore and WebKitLegacy frameworks already link to MobileGestalt (WK_MOBILE_GESTALT_LDFLAGS). So even after all this conversation, I am not sure that it’s super-valuable to do soft linking instead for the WebKit framework.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:129
> +@interface MobileGestaltHelperProxy : NSObject
> +- (BOOL) proxyRebuildCache;
> +@end

Best practice is to put this in MobileGestaltSPI.h header in PAL instead of here.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:494
> +        auto proxy = adoptNS([allocMobileGestaltHelperProxyInstance() init]);
> +        [proxy proxyRebuildCache];

Could even consider doing this as a one-liner.

[Geoffrey Garen]

> If the dynamic
> library has not been loaded by the time _MGCacheValid is called, it has to
> be loaded then, regardless of linking method. So there is probably no
> performance benefit in keeping _MGCacheValid hard-linked.

In general, the performance benefit of hard linking (and weak linking), as compared to soft linking, is that the linkage can be computed ahead of time, so you don't pay a linkage cost at application startup.

If we soft link libMobileGestalt, we will pay the cost of running the linker (dlopen + dlsym + other linker overhead) the first time the app makes a WKWebView.

> The class
> MobileGestaltHelperProxy has to be soft-linked, since it is not exported by
> the library. I will update the patch to soft-link both symbols.

Can you clarify "not exported"? Does that just mean not declared in a header? Generally, if an SPI symbol is not declared in a header, we declare it ourselves and then invoke it.

[Per Arne Vollan]

(In reply to Darin Adler from comment #16)
> Comment on attachment 399129 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399129&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:125
> > +SOFT_LINK_LIBRARY(libMobileGestalt)
> > +SOFT_LINK_CLASS(libMobileGestalt, MobileGestaltHelperProxy);
> > +SOFT_LINK(libMobileGestalt, _MGCacheValid, bool, (), ());
> 
> I just noticed that both the WebCore and WebKitLegacy frameworks already
> link to MobileGestalt (WK_MOBILE_GESTALT_LDFLAGS). So even after all this
> conversation, I am not sure that it’s super-valuable to do soft linking
> instead for the WebKit framework.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:129
> > +@interface MobileGestaltHelperProxy : NSObject
> > +- (BOOL) proxyRebuildCache;
> > +@end
> 
> Best practice is to put this in MobileGestaltSPI.h header in PAL instead of
> here.
> 

Good point! Will do.

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:494
> > +        auto proxy = adoptNS([allocMobileGestaltHelperProxyInstance() init]);
> > +        [proxy proxyRebuildCache];
> 
> Could even consider doing this as a one-liner.

Will fix.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #17)
> > If the dynamic
> > library has not been loaded by the time _MGCacheValid is called, it has to
> > be loaded then, regardless of linking method. So there is probably no
> > performance benefit in keeping _MGCacheValid hard-linked.
> 
> In general, the performance benefit of hard linking (and weak linking), as
> compared to soft linking, is that the linkage can be computed ahead of time,
> so you don't pay a linkage cost at application startup.
> 
> If we soft link libMobileGestalt, we will pay the cost of running the linker
> (dlopen + dlsym + other linker overhead) the first time the app makes a
> WKWebView.
> 

Do you think not soft-linking _MGCacheValid would be faster than soft-linking in this case, even if the dynamic library is already loaded? If so, there might still be a performance benefit of not soft-linking _MGCacheValid, but only MobileGestaltHelperProxy.

Would you prefer to not soft link _MGCacheValid (just MobileGestaltHelperProxy)?

> > The class
> > MobileGestaltHelperProxy has to be soft-linked, since it is not exported by
> > the library. I will update the patch to soft-link both symbols.
> 
> Can you clarify "not exported"? Does that just mean not declared in a
> header? Generally, if an SPI symbol is not declared in a header, we declare
> it ourselves and then invoke it.

By 'not exported', I mean that the symbol is not visible to apps which are hard-linking the library. In Xcode, I think that .tbd files are used by the linker to determine which symbols are available in a dylib, but I could be mistaken. The symbol MobileGestaltHelperProxy is not present in the MobileGestalt .tbd file, which means it has to be soft-linked, while _MGCacheValid is present in the .tbd file, and does not need to be soft-linked.

[Darin Adler]

(In reply to Per Arne Vollan from comment #19)
> Do you think not soft-linking _MGCacheValid would be faster than
> soft-linking in this case, even if the dynamic library is already loaded?

Yes. Normal linking is always faster than soft linking.

I finally understand now what's going on. Dynamically getting a non-exported internal class like MobileGestaltHelperProxy isn’t really "soft linking" and I think it might be better to not use the soft linking macros. Just use NSClassFromString, objc_lookUpClass, or objc_getClass directly.

[Per Arne Vollan]

(In reply to Darin Adler from comment #20)
> (In reply to Per Arne Vollan from comment #19)
> > Do you think not soft-linking _MGCacheValid would be faster than
> > soft-linking in this case, even if the dynamic library is already loaded?
> 
> Yes. Normal linking is always faster than soft linking.
> 
> I finally understand now what's going on. Dynamically getting a non-exported
> internal class like MobileGestaltHelperProxy isn’t really "soft linking" and
> I think it might be better to not use the soft linking macros. Just use
> NSClassFromString, objc_lookUpClass, or objc_getClass directly.

Ah, I see! Will change.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 399163 [details]
Patch

[EWS]

Committed r261584: <https://trac.webkit.org/changeset/261584>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399163 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63158277>