Enable CF prefs direct mode, in order to avoid connecting to the CF prefs daemon in the WebContent process. In direct mode, the prefs files will be accessed directly, and not through the CF prefs daemon.
Created attachment 381935 [details] Patch
I'd suggest doing a PLT5 A/B test on this before landing; in the past, loading a CF pref .plist into each WebContent process was a significant slowdown to process startup.
(In reply to Geoffrey Garen from comment #2) > I'd suggest doing a PLT5 A/B test on this before landing; in the past, > loading a CF pref .plist into each WebContent process was a significant > slowdown to process startup. That's a good point! Perhaps we also should run a memory benchmark?
(In reply to Per Arne Vollan from comment #3) > (In reply to Geoffrey Garen from comment #2) > > I'd suggest doing a PLT5 A/B test on this before landing; in the past, > > loading a CF pref .plist into each WebContent process was a significant > > slowdown to process startup. > > That's a good point! Perhaps we also should run a memory benchmark? I think it's OK to rely on our standard bots for memory. Two more tests you should try, though: OpenSource/PerformanceTests/LaunchTime/new_tab.py OpenSource/PerformanceTests/LaunchTime/startup.py These specifically isolate process startup time. (You may need to enable osascript in Terminal to run these, since it is disabled by default now.)
(In reply to Geoffrey Garen from comment #4) > (In reply to Per Arne Vollan from comment #3) > > (In reply to Geoffrey Garen from comment #2) > > > I'd suggest doing a PLT5 A/B test on this before landing; in the past, > > > loading a CF pref .plist into each WebContent process was a significant > > > slowdown to process startup. > > > > That's a good point! Perhaps we also should run a memory benchmark? > > I think it's OK to rely on our standard bots for memory. > > Two more tests you should try, though: > > OpenSource/PerformanceTests/LaunchTime/new_tab.py > OpenSource/PerformanceTests/LaunchTime/startup.py > > These specifically isolate process startup time. > > (You may need to enable osascript in Terminal to run these, since it is > disabled by default now.) Sounds good! I will perform these tests :)
(In reply to Geoffrey Garen from comment #4) > (In reply to Per Arne Vollan from comment #3) > > (In reply to Geoffrey Garen from comment #2) > > > I'd suggest doing a PLT5 A/B test on this before landing; in the past, > > > loading a CF pref .plist into each WebContent process was a significant > > > slowdown to process startup. > > > > That's a good point! Perhaps we also should run a memory benchmark? > > I think it's OK to rely on our standard bots for memory. > > Two more tests you should try, though: > > OpenSource/PerformanceTests/LaunchTime/new_tab.py > OpenSource/PerformanceTests/LaunchTime/startup.py > > These specifically isolate process startup time. > > (You may need to enable osascript in Terminal to run these, since it is > disabled by default now.) This patch appears to be PLT neutral on iOS and macOS. I will test launch time next.
Created attachment 388788 [details] Patch
(In reply to Per Arne Vollan from comment #7) > Created attachment 388788 [details] > Patch WIP.
Created attachment 388794 [details] Patch
Comment on attachment 388794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388794&action=review > Source/WTF/wtf/Platform.h:196 > +#define ENABLE_CFPREFS_DIRECT_MODE 1 This should probably be more specific, since this is presumably only for WebKit helper services and say, would not apply to WebKit 1. Also, as this is an ENABLE, it should go in PlatformEnable.h
Created attachment 388874 [details] Patch
(In reply to Sam Weinig from comment #10) > Comment on attachment 388794 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388794&action=review > > > Source/WTF/wtf/Platform.h:196 > > +#define ENABLE_CFPREFS_DIRECT_MODE 1 > > This should probably be more specific, since this is presumably only for > WebKit helper services and say, would not apply to WebKit 1. Also, as this > is an ENABLE, it should go in PlatformEnable.h Made define more specific, and changed to a USE macro. Thanks for reviewing!
Created attachment 388878 [details] Patch
Comment on attachment 388878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388878&action=review > Source/WTF/wtf/Platform.h:197 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || PLATFORM(IOS_FAMILY) > +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1 > +#endif Use macros should go in PlatformUse.h!
Created attachment 388992 [details] Patch
(In reply to Sam Weinig from comment #14) > Comment on attachment 388878 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388878&action=review > > > Source/WTF/wtf/Platform.h:197 > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || PLATFORM(IOS_FAMILY) > > +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1 > > +#endif > > Use macros should go in PlatformUse.h! Fixed. Thanks for reviewing!
Created attachment 388998 [details] Patch
Created attachment 389136 [details] Patch
Comment on attachment 389136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389136&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:858 > +void WebProcess::notifyPreferencesChanged(const String& domain) > +{ > + _CFPreferencesFlushCachesForIdentifier(domain.createCFString().get(), kCFPreferencesCurrentUser); > +} Is this approach sufficient? My understanding is that, after a preference changes, the change is not written to disk immediately. Therefore, flushing the cache may trigger a re-read from disk, but the re-read may not see the new value. (I thought we were going to address this by setting the new preference explicitly in the WebContent process. Did our plan change?)
(In reply to Geoffrey Garen from comment #19) > Comment on attachment 389136 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389136&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:858 > > +void WebProcess::notifyPreferencesChanged(const String& domain) > > +{ > > + _CFPreferencesFlushCachesForIdentifier(domain.createCFString().get(), kCFPreferencesCurrentUser); > > +} > > Is this approach sufficient? My understanding is that, after a preference > changes, the change is not written to disk immediately. Therefore, flushing > the cache may trigger a re-read from disk, but the re-read may not see the > new value. (I thought we were going to address this by setting the new > preference explicitly in the WebContent process. Did our plan change?) That is a good point. I thought this would be sufficient, but I think you are right in that the new preference value might not be on disk yet. I will update the patch. Thanks for reviewing!
Created attachment 390179 [details] Patch
Comment on attachment 390179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390179&action=review I see multiple problems here, but I don’t know the entire background. > Source/WTF/wtf/PlatformUse.h:313 > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) > +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1 > +#endif This seems unnecessary. It’s true on all Cocoa platforms, and the only place it’s used is in a Cocoa-only source file. > Source/WebCore/testing/Internals.cpp:5464 > +long Internals::readPreferenceInteger(const String& domain, const String& key) long in IDL is the same as "int" in .h/.cpp so this should be int, not long > Source/WebCore/testing/Internals.h:936 > + long readPreferenceInteger(const String& domain, const String& key); long in IDL is the same as "int" in .h/.cpp so this should be int, not long > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:39 > +extern "C" void _CFPrefsSetDirectModeEnabled(BOOL enabled); This should be in an SPI header, rather than right here. > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:121 > +#if USE(CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS) This conditional seems unnecessary. This is a Cocoa-only source file. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:28 > +@interface NSObject (PrivateKVOMethods) This should be in an SPI header instead of here. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:37 > +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues; I don’t think this needs to be declared. It’s part of the implementation, not the interface. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:39 > +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSKeyValueChangeKey, id> *)change context:(void *)context; Ditto. Overridding something in the inherited class isn’t normally needed. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:44 > +@implementation WKUserDefaults Why is the class implementation in a header file? I don’t think it should be. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:46 > +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues Is this a good technique, allocating a subclass of NSUserDefaults? Is it guaranteed to work? > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:70 > + for (auto* processPool : WebKit::WebProcessPool::allProcessPools()) Each process pool is going to have its own WKPreferenceObserver. Each WKPreferenceObserver is going to have its own sets of many WKUserDefaults objects. But all of them are going to call out to all process pools. Does this mean we are getting multiple notifyPreferencesChanged calls? I don’t think this is right. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:71 > + processPool->notifyPreferencesChanged(m_suiteName, String(key), String(encodedString)); Should not need to explicitly write String() here. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:95 > +@implementation WKPreferenceObserver Why is the class implementation in a header file? I don’t think it should be. > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:190 > + [userDefaults.get() addObserver:userDefaults.get() forKeyPath:@"testkey" options:NSKeyValueObservingOptionNew context:nil]; What is this about? Why are we observing a single test key in each defaults object? This is the kind of non-obvious code that needs comments explaining why it works. The technique here is very difficult to understand and I don’t see sufficient test coverage, just one test. > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:561 > +#if PLATFORM(COCOA) Should not need #if PLATFORM(COCOA) in a file named ...Cocoa.mm > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:741 > + process->send(Messages::WebProcess::NotifyPreferencesChanged(domain, key, encodedValue), 0); > + > +} Extra blank line here.
(In reply to Darin Adler from comment #22) > Comment on attachment 390179 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390179&action=review > > I see multiple problems here, but I don’t know the entire background. > > > Source/WTF/wtf/PlatformUse.h:313 > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) > > +#define USE_CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS 1 > > +#endif > > This seems unnecessary. It’s true on all Cocoa platforms, and the only place > it’s used is in a Cocoa-only source file. > > > Source/WebCore/testing/Internals.cpp:5464 > > +long Internals::readPreferenceInteger(const String& domain, const String& key) > > long in IDL is the same as "int" in .h/.cpp so this should be int, not long > > > Source/WebCore/testing/Internals.h:936 > > + long readPreferenceInteger(const String& domain, const String& key); > > long in IDL is the same as "int" in .h/.cpp so this should be int, not long > > > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:39 > > +extern "C" void _CFPrefsSetDirectModeEnabled(BOOL enabled); > > This should be in an SPI header, rather than right here. > > > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:121 > > +#if USE(CFPREFS_DIRECT_MODE_IN_WEBCONTENT_PROCESS) > > This conditional seems unnecessary. This is a Cocoa-only source file. > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:28 > > +@interface NSObject (PrivateKVOMethods) > > This should be in an SPI header instead of here. > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:37 > > +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues; > > I don’t think this needs to be declared. It’s part of the implementation, > not the interface. > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:39 > > +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSKeyValueChangeKey, id> *)change context:(void *)context; > > Ditto. Overridding something in the inherited class isn’t normally needed. > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:44 > > +@implementation WKUserDefaults > > Why is the class implementation in a header file? I don’t think it should be. > I will create an implementation file, PreferenceObserver.mm. > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:46 > > +- (void)_notifyObserversOfChangeFromValuesForKeys:(NSDictionary<NSString *, id> *)oldValues toValuesForKeys:(NSDictionary<NSString *, id> *)newValues > > Is this a good technique, allocating a subclass of NSUserDefaults? Is it > guaranteed to work? > I am not aware of any issues doing this and believe it should be safe, but I will investigate and double-check to make sure. > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:70 > > + for (auto* processPool : WebKit::WebProcessPool::allProcessPools()) > > Each process pool is going to have its own WKPreferenceObserver. Each > WKPreferenceObserver is going to have its own sets of many WKUserDefaults > objects. But all of them are going to call out to all process pools. Does > this mean we are getting multiple notifyPreferencesChanged calls? I don’t > think this is right. > Ah, good point! I think I will create a static NeverDestroyed<PreferenceObserver>. > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:71 > > + processPool->notifyPreferencesChanged(m_suiteName, String(key), String(encodedString)); > > Should not need to explicitly write String() here. > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:95 > > +@implementation WKPreferenceObserver > > Why is the class implementation in a header file? I don’t think it should be. > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:190 > > + [userDefaults.get() addObserver:userDefaults.get() forKeyPath:@"testkey" options:NSKeyValueObservingOptionNew context:nil]; > > What is this about? Why are we observing a single test key in each defaults > object? This is the kind of non-obvious code that needs comments explaining > why it works. > > The technique here is very difficult to understand and I don’t see > sufficient test coverage, just one test. > This is to make the preference daemon become aware of our NSUserDefaults instances, so that we get the KVO notifications. I will add a comment in the code. > > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:561 > > +#if PLATFORM(COCOA) > > Should not need #if PLATFORM(COCOA) in a file named ...Cocoa.mm > > > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:741 > > + process->send(Messages::WebProcess::NotifyPreferencesChanged(domain, key, encodedValue), 0); > > + > > +} > > Extra blank line here. Thanks for reviewing! I will update the patch accordingly.
(In reply to Per Arne Vollan from comment #23) > (In reply to Darin Adler from comment #22) > > Comment on attachment 390179 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=390179&action=review > > > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.h:190 > > > + [userDefaults.get() addObserver:userDefaults.get() forKeyPath:@"testkey" options:NSKeyValueObservingOptionNew context:nil]; > > > > What is this about? Why are we observing a single test key in each defaults > > object? This is the kind of non-obvious code that needs comments explaining > > why it works. > > > > The technique here is very difficult to understand and I don’t see > > sufficient test coverage, just one test. > > > > This is to make the preference daemon become aware of our NSUserDefaults > instances, so that we get the KVO notifications. I will add a comment in the > code. > We cannot use normal KVO techniques here, since we are looking for _any_ changes in a preference domain. For normal KVO techniques to work, we need to provide the specific key(s) we want to observe, but that set of keys is unknown to us.
Created attachment 390589 [details] Patch
Created attachment 390593 [details] Patch
Created attachment 390601 [details] Patch
<rdar://problem/58493414>
I have tested on iOS and macOS now, and have not encountered any issues with the notification mechanism so far.
Comment on attachment 390601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390601&action=review r=me > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:90 > + std::initializer_list<NSString*> domains = { Do we have a way to identify other domains we will need to access if some are missing from this list?
(In reply to Brent Fulgham from comment #30) > Comment on attachment 390601 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390601&action=review > > r=me > > > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:90 > > + std::initializer_list<NSString*> domains = { > > Do we have a way to identify other domains we will need to access if some > are missing from this list? Yes, I believe that whenever we need to expand the WebContent sandbox with read privileges to a new preference domain, we also need to add the domain to this list. Thanks for reviewing!
Committed r256756: <https://trac.webkit.org/changeset/256756/webkit>