Bug 218289

Created attachment 412533 [details]
Patch

Comment on attachment 412533 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:4
> +        [Cocoa] Remove soft linking of Contacts.framework
> +        https://bugs.webkit.org/show_bug.cgi?id=218289

Nice!

> Source/WebKit/ChangeLog:9
> +        Since Contacts.framework does not depend on WebKit and soft linking is
> +        discouraged, WebKit should link Contacts.framework normally.

This is a complex topic that few people understand, so I think a slightly more detailed rationale is warranted in this ChangeLog.

Soft linking has a performance cost because it uses a function call (dlopen) at runtime, and that requires running the linker at runtime, and running the linker has a cost.

Weak linking avoids this performance cost because weak linking declares your linkage at build time, which enables running the linker at build time or update time or install time, avoiding the runtime cost.

Created attachment 412538 [details]
Patch for landing.

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 412533 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412533&action=review
> 
> r=me

Thank you for the review!
 
> > Source/WebKit/ChangeLog:4
> > +        [Cocoa] Remove soft linking of Contacts.framework
> > +        https://bugs.webkit.org/show_bug.cgi?id=218289
> 
> Nice!
> 
> > Source/WebKit/ChangeLog:9
> > +        Since Contacts.framework does not depend on WebKit and soft linking is
> > +        discouraged, WebKit should link Contacts.framework normally.
> 
> This is a complex topic that few people understand, so I think a slightly
> more detailed rationale is warranted in this ChangeLog.
> 
> Soft linking has a performance cost because it uses a function call (dlopen)
> at runtime, and that requires running the linker at runtime, and running the
> linker has a cost.
> 
> Weak linking avoids this performance cost because weak linking declares your
> linkage at build time, which enables running the linker at build time or
> update time or install time, avoiding the runtime cost.

I've added a similar description to the new Changelog :)

Committed r269109: <https://trac.webkit.org/changeset/269109>

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

<rdar://problem/70770410>

Comment on attachment 412538 [details]
Patch for landing.

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

> Source/WebKit/ChangeLog:19
> +        Since Contacts.framework does not depend on WebKit and soft linking is
> +        discouraged, WebKit should link Contacts.framework normally.
> +
> +        Soft linking requires running the linker at runtime, leading to an
> +        unwanted performance cost. By normally linking (on iOS) and weak linking
> +        (on macOS), the linkage is declared at build time, which enables running
> +        the linker at build/update/install time and avoiding the runtime cost.
> +
> +        * Configurations/WebKit.xcconfig:
> +
> +        On macOS, weak link the framework since Contacts.framework is not
> +        available on the Base System.

Perfect!

It looks like the changes in https://trac.webkit.org/changeset/269109/webkit

broke TestWebKitAPI.ContentFiltering.LazilyLoadPlatformFrameworks

History:
https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.ContentFiltering.LazilyLoadPlatformFrameworks

diff:
Failed

    TestWebKitAPI.ContentFiltering.LazilyLoadPlatformFrameworks
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:374
        Expected equality of these values:
          static_cast<bool>(networkExtensionShouldBeLoaded)
            Which is: false
          static_cast<bool>(networkExtensionLoaded)
            Which is: true
        
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:374
        Expected equality of these values:
          static_cast<bool>(networkExtensionShouldBeLoaded)
            Which is: false
          static_cast<bool>(networkExtensionLoaded)
            Which is: true
        
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:374
        Expected equality of these values:
          static_cast<bool>(networkExtensionShouldBeLoaded)
            Which is: false
          static_cast<bool>(networkExtensionLoaded)
            Which is: true
        
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:374
        Expected equality of these values:
          static_cast<bool>(networkExtensionShouldBeLoaded)
            Which is: false
          static_cast<bool>(networkExtensionLoaded)
            Which is: true
        
        
        /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:374
        Expected equality of these values:
          static_cast<bool>(networkExtensionShouldBeLoaded)
            Which is: false
          static_cast<bool>(networkExtensionLoaded)
            Which is: true

(In reply to Truitt Savell from comment #8)
> It looks like the changes in https://trac.webkit.org/changeset/269109/webkit
> 
> broke TestWebKitAPI.ContentFiltering.LazilyLoadPlatformFrameworks
> 
> History:
> https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.
> ContentFiltering.LazilyLoadPlatformFrameworks
> 
> diff:
> Failed
> 
>     TestWebKitAPI.ContentFiltering.LazilyLoadPlatformFrameworks
>         
>        
> /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ContentFiltering.mm:374
>         Expected equality of these values:
>           static_cast<bool>(networkExtensionShouldBeLoaded)
>             Which is: false
>           static_cast<bool>(networkExtensionLoaded)
>             Which is: true
>         
>         
>        
> /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ContentFiltering.mm:374
>         Expected equality of these values:
>           static_cast<bool>(networkExtensionShouldBeLoaded)
>             Which is: false
>           static_cast<bool>(networkExtensionLoaded)
>             Which is: true
>         
>         
>        
> /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ContentFiltering.mm:374
>         Expected equality of these values:
>           static_cast<bool>(networkExtensionShouldBeLoaded)
>             Which is: false
>           static_cast<bool>(networkExtensionLoaded)
>             Which is: true
>         
>         
>        
> /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ContentFiltering.mm:374
>         Expected equality of these values:
>           static_cast<bool>(networkExtensionShouldBeLoaded)
>             Which is: false
>           static_cast<bool>(networkExtensionLoaded)
>             Which is: true
>         
>         
>        
> /Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ContentFiltering.mm:374
>         Expected equality of these values:
>           static_cast<bool>(networkExtensionShouldBeLoaded)
>             Which is: false
>           static_cast<bool>(networkExtensionLoaded)
>             Which is: true

Heh, that's kind of a silly test. Contacts must be dragging in NE, but that's OK.

Reverted r269109 for reason:

Introduced dependency cycle

Committed r269154: <https://trac.webkit.org/changeset/269154>

Created attachment 412700 [details]
Patch

Comment on attachment 412700 [details]
Patch

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

> Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.mm:42
> +// on macOS, or when Contacts can be upward linked.

Are there radars that track either of these changes? We should link to them here so we know when it's safe to stop soft-linking on macOS.

Created attachment 412781 [details]
Patch for landing

Comment on attachment 412781 [details]
Patch for landing

Added links to radars.

Committed r269263: <https://trac.webkit.org/changeset/269263>

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