[PGO] Fix sandboxing issues and simplify collection

https://bugs.webkit.org/show_bug.cgi?id=261014 rdar://113870737 Reviewed by Justin Michaud and Alexey Proskuryakov. Make a number of simplifications to our PGO instrumentation logic to fix instances where profile collection would fail silently due to sandboxing: Instead of initializing and writing out instrumented profiles ourselves, use LLVM's built-in logic. We bake in a default profile path into instrumented binaries, which tells the instrumentation machinery to write profiles to /private/tmp/WebKitPGO. This is done by declaring __llvm_profile_filename in each instrumented binary, rather than using the -fprofile-generate=<name> compiler argument, because there is no Xcode build setting to easily set it, see rdar://114792050. At process launch, profiling begins via a static initializer compiled in to each instrumented binary from LLVM. Auto-initalization works great for starting PGO collection in a predictably, across all WebKit processes, but termination requires extra care. On Darwin-based platforms, we kill WebKit XPC processes via _exit, without running atexit handlers, which is normally when profiles would be written. To accomodate this, run profile collection in "continuous mode", denoted by a %c in the profile string. LLVM mmaps the profile file and records function calls directly in the mapped file, with no writeback step needed. Add sandbox rules to permit writing to the WebKitPGO directory when built for instrumentation. Teach the benchmark runner to copy from WebKitPGO when Safari exits, instead of parsing the system log to find written profiles. * Source/JavaScriptCore/runtime/InitializeThreading.cpp: Declare default profile name for JSC. * Source/WTF/WTF.xcodeproj/project.pbxproj: Remove GenerateProfiles.h. * Source/WTF/wtf/CMakeLists.txt: Remove GenerateProfiles.h. * Source/WTF/wtf/GenerateProfiles.h: Removed. * Source/WebCore/bindings/js/ScriptController.cpp: Declare default profile name for WebCore. * Source/WebKit/Configurations/BaseXPCService.xcconfig: XPC executables (i.e. the binary that contains XPCServiceMain) were being instrumented, but the profiles were being discarded due to sandboxing. We can safely turn off instrumentation -- the only binaries we care about are the executables in the WebKit framework stack. * Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in: Allow access to the WebKitPGO directory. * Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in: Ditto. * Source/WebKit/Shared/Cocoa/WebKit2InitializeCocoa.mm: Declare default profile name for WebKit. * Source/WebKit/Shared/WebKit2Initialize.cpp: Remove spurious include. * Source/WebKit/WebProcess/com.apple.WebProcess.sb.in: Ditto above. * Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py: When collecting profiles, each invocation will populate /tmp/WebKitPGO. Ensure the directory is empty before launching the browser, and copy its contents to the diagnostic directory upon exit. (BenchmarkRunner._run_benchmark): * Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py: (run_benchmark_plan): Use a fixed path for writing profiles to. * Tools/Scripts/webkitpy/benchmark_runner/webserver_benchmark_runner.py: (WebServerBenchmarkRunner._get_result): Remove the logic that listened to the system log for profiling messages and moved profiles out ad hoc. * Tools/TestWebKitAPI/Configurations/TestIPC.xcconfig: It static-links libWebKitPlatform.a, so when that library is compiled with profiling it needs it too. This has likely always been broken, and not been an issue since we never attempt to build for profiling with tools. Canonical link: https://commits.webkit.org/267741@main
WebKit · Sep 7, 2023 · 6fc61a3 · 6fc61a3
1 parent c57c928
commit 6fc61a3
Show file tree

Hide file tree

Showing 14 changed files with 36 additions and 125 deletions.
diff --git a/Source/JavaScriptCore/runtime/InitializeThreading.cpp b/Source/JavaScriptCore/runtime/InitializeThreading.cpp
@@ -45,7 +45,6 @@
 #include "WasmFaultSignalHandler.h"
 #include "WasmThunks.h"
 #include <mutex>
-#include <wtf/GenerateProfiles.h>
 #include <wtf/Threading.h>
 #include <wtf/threads/Signals.h>
 
@@ -56,6 +55,10 @@
 #endif
 #endif
 
+#if ENABLE(LLVM_PROFILE_GENERATION)
+extern "C" char __llvm_profile_filename[] = "/private/tmp/WebKitPGO/JavaScriptCore_%m_pid%p%c.profraw";
+#endif
+
 namespace JSC {
 
 static_assert(sizeof(bool) == 1, "LLInt and JIT assume sizeof(bool) is always 1 when touching it directly from assembly code.");
@@ -138,8 +141,6 @@ void initialize()
         WTF::compilerFence();
         RELEASE_ASSERT(!g_jscConfig.initializeHasBeenCalled);
         g_jscConfig.initializeHasBeenCalled = true;
-
-        WTF::registerProfileGenerationCallback<JSCProfileTag>("JavaScriptCore");
     });
 }
 

diff --git a/Source/WTF/WTF.xcodeproj/project.pbxproj b/Source/WTF/WTF.xcodeproj/project.pbxproj
@@ -103,7 +103,6 @@
 		6311592628989A55006A9A12 /* LibraryPathDiagnostics.h in Headers */ = {isa = PBXBuildFile; fileRef = 6311592428989A55006A9A12 /* LibraryPathDiagnostics.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		6311592728989A55006A9A12 /* LibraryPathDiagnostics.mm in Sources */ = {isa = PBXBuildFile; fileRef = 6311592528989A55006A9A12 /* LibraryPathDiagnostics.mm */; };
 		63F9BED42898D96400371416 /* CFPrivSPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 63F9BED32898D96400371416 /* CFPrivSPI.h */; settings = {ATTRIBUTES = (Private, ); }; };
-		641C2E5B27FBAAA5002E0814 /* GenerateProfiles.h in Headers */ = {isa = PBXBuildFile; fileRef = 641C2E5A27FBAAA5002E0814 /* GenerateProfiles.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		70A993FE1AD7151300FA615B /* SymbolRegistry.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 70A993FC1AD7151300FA615B /* SymbolRegistry.cpp */; };
 		70ECA60D1B02426800449739 /* AtomStringImpl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 70ECA60A1B02426800449739 /* AtomStringImpl.cpp */; };
 		7A05093F1FB9DCC500B33FB8 /* JSONValues.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7A05093E1FB9DCC500B33FB8 /* JSONValues.cpp */; };
@@ -1202,7 +1201,6 @@
 		6311592428989A55006A9A12 /* LibraryPathDiagnostics.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = LibraryPathDiagnostics.h; sourceTree = "<group>"; };
 		6311592528989A55006A9A12 /* LibraryPathDiagnostics.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = LibraryPathDiagnostics.mm; sourceTree = "<group>"; };
 		63F9BED32898D96400371416 /* CFPrivSPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CFPrivSPI.h; sourceTree = "<group>"; };
-		641C2E5A27FBAAA5002E0814 /* GenerateProfiles.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = GenerateProfiles.h; sourceTree = "<group>"; };
 		70A993FC1AD7151300FA615B /* SymbolRegistry.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = SymbolRegistry.cpp; sourceTree = "<group>"; };
 		70A993FD1AD7151300FA615B /* SymbolRegistry.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SymbolRegistry.h; sourceTree = "<group>"; };
 		70ECA60A1B02426800449739 /* AtomStringImpl.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = AtomStringImpl.cpp; sourceTree = "<group>"; };
@@ -2098,7 +2096,6 @@
 				1A1D8B9B173186CE00141DA4 /* FunctionDispatcher.h */,
 				FEC26BF2289DBBD300639A59 /* FunctionPtr.h */,
 				53F1D98620477B9800EBC6BF /* FunctionTraits.h */,
-				641C2E5A27FBAAA5002E0814 /* GenerateProfiles.h */,
 				E3FD6D6627A1F6AD00935000 /* GenericHashKey.h */,
 				E3831F922703101A00EF5EB3 /* GenericTimeMixin.h */,
 				A8A472A8151A825A004123FF /* GetPtr.h */,
@@ -3077,7 +3074,6 @@
 				DD3DC94E27A4BF8E007E5B61 /* FunctionTraits.h in Headers */,
 				DDF3080227C08A77006A526F /* generate-unified-source-bundles.rb in Headers */,
 				DDF3080327C08A77006A526F /* GeneratePreferences.rb in Headers */,
-				641C2E5B27FBAAA5002E0814 /* GenerateProfiles.h in Headers */,
 				DDCAF31D27B1E7C500C45308 /* GenericHashKey.h in Headers */,
 				DD3DC93127A4BF8E007E5B61 /* GenericTimeMixin.h in Headers */,
 				DD3DC94727A4BF8E007E5B61 /* GetPtr.h in Headers */,

diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt
@@ -91,7 +91,6 @@ set(WTF_PUBLIC_HEADERS
     FunctionDispatcher.h
     FunctionPtr.h
     FunctionTraits.h
-    GenerateProfiles.h
     GenericHashKey.h
     GenericTimeMixin.h
     GetPtr.h

diff --git a/Source/WTF/wtf/GenerateProfiles.h b/Source/WTF/wtf/GenerateProfiles.h
diff --git a/Source/WebCore/bindings/js/ScriptController.cpp b/Source/WebCore/bindings/js/ScriptController.cpp
@@ -74,14 +74,17 @@
 #include <JavaScriptCore/SyntheticModuleRecord.h>
 #include <JavaScriptCore/WeakGCMapInlines.h>
 #include <JavaScriptCore/WebAssemblyModuleRecord.h>
-#include <wtf/GenerateProfiles.h>
 #include <wtf/SetForScope.h>
 #include <wtf/SharedTask.h>
 #include <wtf/Threading.h>
 #include <wtf/text/TextPosition.h>
 
 #define SCRIPTCONTROLLER_RELEASE_LOG_ERROR(channel, fmt, ...) RELEASE_LOG_ERROR(channel, "%p - ScriptController::" fmt, this, ##__VA_ARGS__)
 
+#if ENABLE(LLVM_PROFILE_GENERATION)
+extern "C" char __llvm_profile_filename[] = "/private/tmp/WebKitPGO/WebCore_%m_pid%p%c.profraw";
+#endif
+
 namespace WebCore {
 using namespace JSC;
 
@@ -94,7 +97,6 @@ void ScriptController::initializeMainThread()
     WTF::initializeMainThread();
     WebCore::populateJITOperations();
 #endif
-    WTF::registerProfileGenerationCallback<WebCoreProfileTag>("WebCore");
 }
 
 ScriptController::ScriptController(LocalFrame& frame)

diff --git a/Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in b/Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in
@@ -1285,3 +1285,8 @@
 
 (when (defined? 'SYS_map_with_linking_np)
     (allow syscall-unix (syscall-number SYS_map_with_linking_np)))
+
+
+#if ENABLE(LLVM_PROFILE_GENERATION)
+(allow file-write* (subpath "/private/tmp/WebKitPGO"))
+#endif
diff --git a/Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in b/Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in
@@ -851,3 +851,7 @@
     (global-name "com.apple.WebPrivacy.Service"))
 
 #endif
+
+#if ENABLE(LLVM_PROFILE_GENERATION)
+(allow file-write* (subpath "/private/tmp/WebKitPGO"))
+#endif
diff --git a/Source/WebKit/Shared/Cocoa/WebKit2InitializeCocoa.mm b/Source/WebKit/Shared/Cocoa/WebKit2InitializeCocoa.mm
@@ -30,7 +30,6 @@
 #import <WebCore/CommonAtomStrings.h>
 #import <WebCore/WebCoreJITOperations.h>
 #import <mutex>
-#import <wtf/GenerateProfiles.h>
 #import <wtf/MainThread.h>
 #import <wtf/RefCounted.h>
 #import <wtf/WorkQueue.h>
@@ -40,6 +39,10 @@
 #import <WebCore/WebCoreThreadSystemInterface.h>
 #endif
 
+#if ENABLE(LLVM_PROFILE_GENERATION)
+extern "C" char __llvm_profile_filename[] = "/private/tmp/WebKitPGO/WebKit_%m_pid%p%c.profraw";
+#endif
+
 namespace WebKit {
 
 static std::once_flag flag;
@@ -61,8 +64,6 @@ static void runInitializationCode(void* = nullptr)
     WTF::RefCountedBase::enableThreadingChecksGlobally();
 
     WebCore::populateJITOperations();
-
-    WTF::registerProfileGenerationCallback<WebKitProfileTag>("WebKit");
 }
 
 void InitializeWebKit2()

diff --git a/Source/WebKit/Shared/WebKit2Initialize.cpp b/Source/WebKit/Shared/WebKit2Initialize.cpp
@@ -29,7 +29,6 @@
 #include <JavaScriptCore/InitializeThreading.h>
 #include <WebCore/CommonAtomStrings.h>
 #include <WebCore/WebCoreJITOperations.h>
-#include <wtf/GenerateProfiles.h>
 #include <wtf/MainThread.h>
 #include <wtf/RefCounted.h>
 #include <wtf/RunLoop.h>

diff --git a/Source/WebKit/WebProcess/com.apple.WebProcess.sb.in b/Source/WebKit/WebProcess/com.apple.WebProcess.sb.in
@@ -2410,3 +2410,7 @@
     (deny mach-task-special-port-get (with telemetry))
     (deny mach-task-special-port-set (with telemetry)))
 #endif
+
+#if ENABLE(LLVM_PROFILE_GENERATION)
+(allow file-write* (subpath "/private/tmp/WebKitPGO"))
+#endif
diff --git a/Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py b/Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py
@@ -1,5 +1,6 @@
 import json
 import logging
+import shutil
 import sys
 import os
 from collections import defaultdict, OrderedDict
@@ -151,6 +152,9 @@ def _run_benchmark(self, count, web_root):
             self._browser_driver.prepare_initial_env(self._config)
             for iteration in range(1, count + 1):
                 _log.info('Start the iteration {current_iteration} of {iterations} for current benchmark'.format(current_iteration=iteration, iterations=count))
+                if self._pgo_profile_output_dir:
+                    shutil.rmtree(self._pgo_profile_output_dir, ignore_errors=True)
+                    os.mkdir(self._pgo_profile_output_dir)
                 try:
                     self._browser_driver.prepare_env(self._config)
                     if 'entry_point' in self._plan:
@@ -176,6 +180,8 @@ def _run_benchmark(self, count, web_root):
 
                 finally:
                     self._browser_driver.restore_env()
+                if self._pgo_profile_output_dir:
+                    shutil.copytree(self._pgo_profile_output_dir, self._diagnose_dir, dirs_exist_ok=True)
 
                 _log.info('End the iteration {current_iteration} of {iterations} for current benchmark'.format(current_iteration=iteration, iterations=count))
         finally:

diff --git a/Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py b/Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py
@@ -19,6 +19,7 @@
     WebServerBenchmarkRunner.name: WebServerBenchmarkRunner,
 }
 
+WEBKIT_PGO_DIR = '/private/tmp/WebKitPGO'
 
 def default_platform():
     if sys.platform.startswith('linux'):
@@ -99,7 +100,7 @@ def run_benchmark_plan(args, plan):
                                     args.local_copy, args.count, args.timeout, args.build_dir, args.output_file,
                                     args.platform, args.browser, args.browser_path, args.subtests, args.scale_unit,
                                     args.show_iteration_values, args.device_id, args.diagnose_dir,
-                                    args.diagnose_dir if args.generate_pgo_profiles else None,
+                                    WEBKIT_PGO_DIR if args.generate_pgo_profiles else None,
                                     args.diagnose_dir if args.trace_type else None,
                                     args.trace_type, args.profiling_interval,
                                     args.browser_args)

diff --git a/Tools/Scripts/webkitpy/benchmark_runner/webserver_benchmark_runner.py b/Tools/Scripts/webkitpy/benchmark_runner/webserver_benchmark_runner.py
@@ -33,18 +33,6 @@ def __init__(self, plan_file, local_copy, count_override, timeout_override, buil
     def _get_result(self, test_url):
         result = self._browser_driver.add_additional_results(test_url, self._http_server_driver.fetch_result())
         assert(not self._http_server_driver.get_return_code())
-
-        if self._pgo_profile_output_dir:
-            _log.info('Getting benchmark PGO profile results for {} and copying them to {}.'.format(test_url, self._pgo_profile_output_dir))
-            copy_output = subprocess.Popen(r"""log stream --style json --color none | perl -mFile::Basename -mFile::Copy -nle 'if (m/<WEBKIT_LLVM_PROFILE>.*<BEGIN>(.*)<END>/) { (my $l = $1) =~ s/\\\//\//g; my $b = File::Basename::basename($l); my $d = """ + "\"" + self._pgo_profile_output_dir + """/$b"; print "Moving $l to $d"; File::Copy::move($l, $d); }'""", shell=True, bufsize=0, preexec_fn=os.setsid)
-            time.sleep(1)
-            subprocess.call(['notifyutil', '-p', 'com.apple.WebKit.profiledata'])
-            time.sleep(7)
-            # We can kill the shell with kill(), but killing children is harder.
-            os.killpg(os.getpgid(copy_output.pid), signal.SIGINT)
-            copy_output.kill()
-            time.sleep(1)
-            _log.debug('Hopefully the benchmark profile has finished writing to disk, moving on.')
         return result
 
     def _construct_subtest_url(self, subtests):

diff --git a/Tools/TestWebKitAPI/Configurations/TestIPC.xcconfig b/Tools/TestWebKitAPI/Configurations/TestIPC.xcconfig
@@ -25,6 +25,9 @@ PRODUCT_NAME = TestIPC;
 
 PROJECT_HEADER_SEARCH_PATHS = $(SRCROOT)/../../Source/WebKit/Platform $(SRCROOT)/../../Source/WebKit/Platform/IPC $(SRCROOT)/../../Source/WebKit/Platform/IPC/darwin $(SRCROOT)/../../Source/WebKit/Platform/IPC/cocoa $(BUILT_PRODUCTS_DIR)/DerivedSources/WebKit $(inherited);
 
+CLANG_INSTRUMENT_FOR_OPTIMIZATION_PROFILING = $(CLANG_INSTRUMENT_FOR_OPTIMIZATION_PROFILING_$(ENABLE_LLVM_PROFILE_GENERATION));
+CLANG_INSTRUMENT_FOR_OPTIMIZATION_PROFILING_ENABLE_LLVM_PROFILE_GENERATION = YES;
+
 GCC_ENABLE_OBJC_EXCEPTIONS = YES;
 
 GCC_PREPROCESSOR_DEFINITIONS = $(inherited) BUILDING_TEST_IPC GTEST_API_=