Bug 209244 - [Cocoa] Fix launch time regression with CF prefs direct mode enabled
Summary: [Cocoa] Fix launch time regression with CF prefs direct mode enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on: 209620
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-18 12:00 PDT by Per Arne Vollan
Modified: 2020-03-26 14:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.12 KB, patch)
2020-03-18 12:03 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (10.15 KB, patch)
2020-03-18 12:15 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (11.26 KB, patch)
2020-03-18 12:38 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (10.86 KB, patch)
2020-03-18 15:34 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (20.88 KB, patch)
2020-03-19 10:24 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (21.19 KB, patch)
2020-03-19 10:53 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (20.87 KB, patch)
2020-03-19 13:21 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (22.60 KB, patch)
2020-03-20 10:27 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.18 KB, patch)
2020-03-20 10:44 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.39 KB, patch)
2020-03-20 14:59 PDT, Per Arne Vollan
darin: review+
Details | Formatted Diff | Diff
Patch (25.75 KB, patch)
2020-03-20 15:48 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.89 KB, patch)
2020-03-20 16:25 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.99 KB, patch)
2020-03-23 08:10 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.91 KB, patch)
2020-03-24 09:53 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2020-03-18 12:00:20 PDT
When CF prefs direct mode was enabled in https://trac.webkit.org/changeset/258064/webkit, it introduced a significant launch time regression. This regression should be fixed.
Comment 1 Per Arne Vollan 2020-03-18 12:03:03 PDT
Created attachment 393882 [details]
Patch
Comment 2 Per Arne Vollan 2020-03-18 12:15:47 PDT
Created attachment 393884 [details]
Patch
Comment 3 Per Arne Vollan 2020-03-18 12:38:01 PDT
Created attachment 393888 [details]
Patch
Comment 4 Per Arne Vollan 2020-03-18 15:34:46 PDT
Created attachment 393909 [details]
Patch
Comment 5 Per Arne Vollan 2020-03-18 15:36:10 PDT
rdar://problem/60542149
Comment 6 Per Arne Vollan 2020-03-19 10:24:06 PDT
Created attachment 393993 [details]
Patch
Comment 7 Per Arne Vollan 2020-03-19 10:53:14 PDT
Created attachment 393995 [details]
Patch
Comment 8 Per Arne Vollan 2020-03-19 13:21:07 PDT
Created attachment 394011 [details]
Patch
Comment 9 Per Arne Vollan 2020-03-20 10:27:42 PDT
Created attachment 394099 [details]
Patch
Comment 10 Per Arne Vollan 2020-03-20 10:44:55 PDT
Created attachment 394102 [details]
Patch
Comment 11 Sam Weinig 2020-03-20 11:13:23 PDT
Comment on attachment 394102 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:878
> +#if PLATFORM(COCOA)
> +#define ENABLE_CFPREFS_DIRECT_MODE 1
> +#else
>  #define ENABLE_CFPREFS_DIRECT_MODE 0
> +#endif

Please put this in PlatformEnableCocoa.h, and add a default value of ENABLE_CFPREFS_DIRECT_MODE above.

> Source/WebKit/ChangeLog:16
> +

Why is ok to wait for ui process activation? It seems like this will have a change in behavior. 

If this change is ok, please add a test showing the change in behavior.
Comment 12 Per Arne Vollan 2020-03-20 14:59:20 PDT
Created attachment 394134 [details]
Patch
Comment 13 Per Arne Vollan 2020-03-20 15:02:05 PDT
(In reply to Sam Weinig from comment #11)
> Comment on attachment 394102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394102&action=review
> 
> > Source/WTF/wtf/PlatformEnable.h:878
> > +#if PLATFORM(COCOA)
> > +#define ENABLE_CFPREFS_DIRECT_MODE 1
> > +#else
> >  #define ENABLE_CFPREFS_DIRECT_MODE 0
> > +#endif
> 
> Please put this in PlatformEnableCocoa.h, and add a default value of
> ENABLE_CFPREFS_DIRECT_MODE above.
> 

Fixed.

> > Source/WebKit/ChangeLog:16
> > +
> 
> Why is ok to wait for ui process activation? It seems like this will have a
> change in behavior. 
> 
> If this change is ok, please add a test showing the change in behavior.

In general, a user would have to deactivate Safari to be able to change preferences we are observing, so I think waiting for UI process activation should be ok. I have added a test for this.

Thanks for reviewing!
Comment 14 Darin Adler 2020-03-20 15:22:39 PDT
Comment on attachment 394134 [details]
Patch

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:-537
> -    (global-name "com.apple.cfprefsd.daemon")

Seems strange that this is not inside a conditional.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:87
>      std::initializer_list<NSString*> domains = {

How can we determine if we have registered the correct set of domains, and keep this list updated over time? Do we have to notice bugs due to incorrect behavior or is there a system for detecting mistakes in the list?

Is this a scalable approach? Will we run into performance problems again later if we have to add more domains? Created by someone else not thinking about WebKit when they start fetching more preferences in some component used by WebKit?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:681
> +#if ENABLE(CFPREFS_DIRECT_MODE)
> +    m_activationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:@"UIApplicationDidBecomeActiveNotification" object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *notification) {
> +        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> +            // Start observing preference changes.
> +            [WKPreferenceObserver sharedInstance];
> +        });
> +    }];
> +#endif

Can we cut down on the copied and pasted code somehow? Maybe a helper function named startObservingPreferenceChanges?

Is there a race condition for preferences that we fetch before we call this block and create a WKPreferenceObserver, where we can miss an early change?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:-680
> -    (global-name "com.apple.cfprefsd.daemon")

Seems strange that this is not inside a conditional.
Comment 15 Per Arne Vollan 2020-03-20 15:48:33 PDT
Created attachment 394142 [details]
Patch
Comment 16 Sam Weinig 2020-03-20 16:06:15 PDT
Comment on attachment 394134 [details]
Patch

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

> Source/WTF/wtf/PlatformEnable.h:876
> +#if !defined(ENABLE_CFPREFS_DIRECT_MODE)
>  #define ENABLE_CFPREFS_DIRECT_MODE 0
> +#endif

This should really go in the section above with all the other defaults, not down here in the "Asserts, invariants for macro definitions" section.
Comment 17 Per Arne Vollan 2020-03-20 16:18:58 PDT
(In reply to Darin Adler from comment #14)
> Comment on attachment 394134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394134&action=review
> 
> > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:-537
> > -    (global-name "com.apple.cfprefsd.daemon")
> 
> Seems strange that this is not inside a conditional.
> 

AFAIK, we are not preprocessing the sandbox on iOS, so I don't believe we can use a conditional here.

> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:87
> >      std::initializer_list<NSString*> domains = {
> 
> How can we determine if we have registered the correct set of domains, and
> keep this list updated over time? Do we have to notice bugs due to incorrect
> behavior or is there a system for detecting mistakes in the list?
> 

We have done, and are doing a test sweep now to determine the initial list of domains we need. New preference domains are denied in the sandbox by default, so when we are asked to add them to the sandbox, we should also evaluate if that domain needs to go into this list. So I don't think we need to rely on noticing incorrect behavior going forward, once we have the initial list.

> Is this a scalable approach? Will we run into performance problems again
> later if we have to add more domains? Created by someone else not thinking
> about WebKit when they start fetching more preferences in some component
> used by WebKit?
> 

Given that this patch shows 0% regression on the liftoff benchmark, and that the initialization is running on a secondary thread after most other CF prefs work on startup is done, I believe we can add some domains without creating a new regression. However, I don't think we can add a lot of domains without seeing some sort of regression, either in power/memory usage or launch time. At the moment, I am not sure how many we can add before we see a regression, but I don't expect a substantial net growth in this list over time.

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:681
> > +#if ENABLE(CFPREFS_DIRECT_MODE)
> > +    m_activationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:@"UIApplicationDidBecomeActiveNotification" object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *notification) {
> > +        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> > +            // Start observing preference changes.
> > +            [WKPreferenceObserver sharedInstance];
> > +        });
> > +    }];
> > +#endif
> 
> Can we cut down on the copied and pasted code somehow? Maybe a helper
> function named startObservingPreferenceChanges?
> 

Fixed.

> Is there a race condition for preferences that we fetch before we call this
> block and create a WKPreferenceObserver, where we can miss an early change?
> 

That is a very good point. I think it could be possible in some scenarios, but unlikely, since the user will have to deactivate Safari before going into Settings and make changes.

> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:-680
> > -    (global-name "com.apple.cfprefsd.daemon")
> 
> Seems strange that this is not inside a conditional.

On macOS we are actually preprocessing the sandbox file, but I have observed some unexpected behavior related to ENABLE macros here earlier, so I did not use a conditional.

Thanks for reviewing!
Comment 18 Per Arne Vollan 2020-03-20 16:25:30 PDT
Created attachment 394147 [details]
Patch
Comment 19 Per Arne Vollan 2020-03-20 16:26:05 PDT
(In reply to Sam Weinig from comment #16)
> Comment on attachment 394134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394134&action=review
> 
> > Source/WTF/wtf/PlatformEnable.h:876
> > +#if !defined(ENABLE_CFPREFS_DIRECT_MODE)
> >  #define ENABLE_CFPREFS_DIRECT_MODE 0
> > +#endif
> 
> This should really go in the section above with all the other defaults, not
> down here in the "Asserts, invariants for macro definitions" section.

Thanks, fixed!
Comment 20 Geoffrey Garen 2020-03-21 11:25:03 PDT
Comment on attachment 394147 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:606
> +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

You might as well put a dispatch_once here, so we don't do a throw-away dispatch_async on every activation.
Comment 21 Per Arne Vollan 2020-03-23 08:10:39 PDT
Created attachment 394261 [details]
Patch
Comment 22 Per Arne Vollan 2020-03-23 08:11:36 PDT
(In reply to Geoffrey Garen from comment #20)
> Comment on attachment 394147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394147&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:606
> > +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
> 
> You might as well put a dispatch_once here, so we don't do a throw-away
> dispatch_async on every activation.

Fixed.

Thanks for reviewing!
Comment 23 Per Arne Vollan 2020-03-24 09:53:50 PDT
Created attachment 394378 [details]
Patch
Comment 24 EWS 2020-03-24 15:41:19 PDT
Committed r258949: <https://trac.webkit.org/changeset/258949>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394378 [details].
Comment 25 Chris Dumez 2020-03-26 14:24:11 PDT
Comment on attachment 394378 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> +    [[NSNotificationCenter defaultCenter] removeObserver:m_activationObserver.get()];

This introduced this crash: rdar://problem/60930466

The m_activationObserver is no longer properly unregistered on macOS... This block does not seem to be built on macOS at all.

Why did you move this?
Comment 26 Per Arne Vollan 2020-03-26 14:51:25 PDT
(In reply to Chris Dumez from comment #25)
> Comment on attachment 394378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394378&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> > +    [[NSNotificationCenter defaultCenter] removeObserver:m_activationObserver.get()];
> 
> This introduced this crash: rdar://problem/60930466
> 
> The m_activationObserver is no longer properly unregistered on macOS... This
> block does not seem to be built on macOS at all.
> 
> Why did you move this?

I intended to move it so that it would be unregistered for both macOS and iOS, but did not move it to the correct place.

Thanks for catching this!