Add SPI to disable JIT in a WKWebView
Created attachment 355229 [details] Patch
Comment on attachment 355229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355229&action=review > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:120 > +static bool& jitDisabled() > +{ > + static bool disabled = false; > + return disabled; > +} Let's pick a consistent phrase and use it everywhere -- rather than the family of jitDisabled, disableJIT, canUseJIT, and enableJIT. I usually prefer boolean conditions to be named after the true condition, to avoid mental gymnastics around double negatives when the conditions are false. (E.g., "Enable the JIT" and "Do not enable the JIT" parse OK to me, but "Do not not enable the JIT" is hard for me to parse.) So, normally I would recommend "setJITEnabled" and "isJITEnabled" as the canonical phrases. > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:122 > static bool allowJIT() Let's pair this with the function above and call them "setJITEnabled" and "isJITEnabled". > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:125 > + return processHasEntitlement("dynamic-codesigning") && !jitDisabled(); Note the use of "not not enabled" here, which I would prefer to avoid. > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:131 > +void ExecutableAllocator::disableJIT() This can be the implementation of the setter in the false condition. > Source/WebKit/UIProcess/WebPageProxy.cpp:6530 > +void WebPageProxy::canUseJIT(CompletionHandler<void(bool)>&& completionHandler) I'd call this function and its message isJITEnabled. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4771 > +- (void)_canUseJIT:(void(^)(BOOL))completionHandler isJITEnabled > Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:71 > +@property (nonatomic) BOOL enableJIT WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); I'd call this JITEnabled getter=isJITEnabled > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:51 > + virtual bool enableJIT() const { return true; } isJITEnabled > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:178 > + xpc_dictionary_set_bool(bootstrapMessage.get(), "disable-jit", true); Maybe it's ok for this message to be "disable-jit". The best alternative I can think of is "set-jit-enabled-false". I'm not sure if that's better. > Source/WebKit/WebProcess/WebProcess.h:207 > + void canUseJIT(CompletionHandler<void(bool)>&&); isJITEnabled > Source/WebKit/WebProcess/WebProcess.messages.in:144 > + CanUseJIT() -> (bool canUse) Async IsJITEnabled
Usually when we have a one way toggle for X we call it “setX” or “clearX”. So I guess my recommendation is to name the setter in JSC and the bootstrap xpc message “clearJITEnabled” and “clear-jit-enabled”. (I think part of why you chose “disableJIT” might be that it’s not terribly productive to call setJITEnabled(true). So it doesn’t feel right for JSC to offer setJITEnabled(bool).)
Created attachment 355287 [details] Patch
Comment on attachment 355287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355287&action=review r=me Still a few opportunities to increase naming consistency. > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:116 > +static bool s_jitEnabled = true; s_isJITEnabled > Source/JavaScriptCore/jit/ExecutableAllocator.cpp:128 > +void ExecutableAllocator::setJITEnabled(bool enabled) > +{ > + s_jitEnabled = enabled; Probably worth an early return if the setting didn't change. > Source/WebKit/UIProcess/WebProcessProxy.h:261 > + bool JITEnabled() const final; isJITEnabled() > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:226 > + bool m_JITEnabled { true }; m_isJITEnabled > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4773 > + _page->isJITEnabled([completionHandler = makeBlockPtr(completionHandler)] (bool canUse) { canUse => isJITEnabled > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:51 > + virtual bool JITEnabled() const { return true; } isJITEnabled
Created attachment 355300 [details] Patch
Comment on attachment 355300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355300&action=review > Source/WebKit/UIProcess/WebProcessProxy.cpp:54 > +#include <JavaScriptCore/VM.h> This isn't needed. Won't commit it.
Comment on attachment 355300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355300&action=review > Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:73 > + if (initializerMessage && xpc_dictionary_get_bool(initializerMessage, "disable-jit")) Does this happen before initializeThreading? I believe it does. It’d be good to add such an assertion to the API you added in JSC as well. I think such an assertion is better than relying on the OS only allocating X memory once.
Comment on attachment 355300 [details] Patch Good idea! I'm adding ASSERT(!allocator); at the beginning of ExecutableAllocator::setJITEnabled to remind us when it can be called.
http://trac.webkit.org/r238388
<rdar://problem/46176350>