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.
Created attachment 399047 [details] Patch
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.
(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!
(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.
(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!
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?)
(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!
But we're going to call it right away, right? What's the point of soft linking?
(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.
(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.
> 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.
(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.
(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?
(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.
Created attachment 399129 [details] Patch
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.
> 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.
(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!
(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.
(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.
(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!
Created attachment 399163 [details] Patch
Committed r261584: <https://trac.webkit.org/changeset/261584> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399163 [details].
<rdar://problem/63158277>