Merge pull request #3107 from cloudflare/yagiz/enable-compile-cache

enable built-in compile cache behind autogate

Author: anonrig

src/workerd/jsg/BUILD.bazel

@@ -62,6 +62,7 @@ wd_cc_library(
         ":observer",
         ":url",
         "//src/workerd/util",
+        "//src/workerd/util:autogate",
         "//src/workerd/util:sentry",
         "//src/workerd/util:thread-scopes",
         "//src/workerd/util:uuid",

src/workerd/jsg/jsg-test.h

@@ -46,8 +46,8 @@ class Evaluator {
         auto modules = ModuleRegistryImpl<IsolateType_TypeWrapper>::from(js);
         auto p = kj::Path::parse("main");
         modules->add(p,
-            jsg::ModuleRegistry::ModuleInfo(
-                lock, "main", code, ModuleInfoCompileOption::BUNDLE, observer));
+            jsg::ModuleRegistry::ModuleInfo(lock, "main", code, nullptr /* compile cache */,
+                ModuleInfoCompileOption::BUNDLE, observer));
 
         // Instantiate the module
         auto& moduleInfo = KJ_REQUIRE_NONNULL(modules->resolve(js, p));

src/workerd/jsg/modules.c++

@@ -6,6 +6,8 @@
 #include "jsg.h"
 #include "setup.h"
 
+#include <workerd/util/autogate.h>
+
 #include <kj/mutex.h>
 
 #include <set>
@@ -365,53 +367,46 @@ static CompilationObserver::Option convertOption(ModuleInfoCompileOption option)
 v8::Local<v8::Module> compileEsmModule(jsg::Lock& js,
     kj::StringPtr name,
     kj::ArrayPtr<const char> content,
+    kj::ArrayPtr<const kj::byte> compileCache,
     ModuleInfoCompileOption option,
     const CompilationObserver& observer) {
   // destroy the observer after compilation finished to indicate the end of the process.
   auto compilationObserver =
       observer.onEsmCompilationStart(js.v8Isolate, name, convertOption(option));
 
   // Must pass true for `is_module`, but we can skip everything else.
-  const int resourceLineOffset = 0;
-  const int resourceColumnOffset = 0;
-  const bool resourceIsSharedCrossOrigin = false;
-  const int scriptId = -1;
-  const bool resourceIsOpaque = false;
-  const bool isWasm = false;
-  const bool isModule = true;
+  constexpr int resourceLineOffset = 0;
+  constexpr int resourceColumnOffset = 0;
+  constexpr bool resourceIsSharedCrossOrigin = false;
+  constexpr int scriptId = -1;
+  constexpr bool resourceIsOpaque = false;
+  constexpr bool isWasm = false;
+  constexpr bool isModule = true;
   v8::ScriptOrigin origin(v8StrIntern(js.v8Isolate, name), resourceLineOffset, resourceColumnOffset,
       resourceIsSharedCrossOrigin, scriptId, {}, resourceIsOpaque, isWasm, isModule);
   v8::Local<v8::String> contentStr;
-  v8::ScriptCompiler::CachedData* existingCacheData = nullptr;
-  auto compileOptions = v8::ScriptCompiler::kNoCompileOptions;
-  const auto& compileCache = CompileCache::get();
 
   if (option == ModuleInfoCompileOption::BUILTIN) {
     // TODO(later): Use of newExternalOneByteString here limits our built-in source
     // modules (for which this path is used) to only the latin1 character set. We
     // may need to revisit that to import built-ins as UTF-16 (two-byte).
     contentStr = jsg::newExternalOneByteString(js, content);
-
-    // We only enable compile cache for built-in modules for now.
-    KJ_IF_SOME(cached, compileCache.find(name)) {
-      compileOptions = v8::ScriptCompiler::kConsumeCodeCache;
-      existingCacheData = cached.AsCachedData().release();
-    }
   } else {
     contentStr = jsg::v8Str(js.v8Isolate, content);
   }
 
-  v8::ScriptCompiler::Source source(contentStr, origin, existingCacheData);
-  auto module =
-      jsg::check(v8::ScriptCompiler::CompileModule(js.v8Isolate, &source, compileOptions));
-
-  if (existingCacheData == nullptr) {
-    auto cachedData = std::shared_ptr<v8::ScriptCompiler::CachedData>(
-        v8::ScriptCompiler::CreateCodeCache(module->GetUnboundModuleScript()));
-    compileCache.add(name, kj::mv(cachedData));
+  if (util::Autogate::isEnabled(util::AutogateKey::COMPILE_CACHE_FOR_BUILTINS)) {
+    if (compileCache.size() > 0 && compileCache.begin() != nullptr) {
+      auto cached = std::make_unique<v8::ScriptCompiler::CachedData>(
+          compileCache.begin(), compileCache.size());
+      v8::ScriptCompiler::Source source(contentStr, origin, cached.release());
+      return jsg::check(v8::ScriptCompiler::CompileModule(
+          js.v8Isolate, &source, v8::ScriptCompiler::kConsumeCodeCache));
+    }
   }
 
-  return module;
+  v8::ScriptCompiler::Source source(contentStr, origin);
+  return jsg::check(v8::ScriptCompiler::CompileModule(js.v8Isolate, &source));
 }
 
 v8::Local<v8::Module> createSyntheticModule(
@@ -438,9 +433,10 @@ ModuleRegistry::ModuleInfo::ModuleInfo(
 ModuleRegistry::ModuleInfo::ModuleInfo(jsg::Lock& js,
     kj::StringPtr name,
     kj::ArrayPtr<const char> content,
+    kj::ArrayPtr<const kj::byte> compileCache,
     ModuleInfoCompileOption flags,
     const CompilationObserver& observer)
-    : ModuleInfo(js, compileEsmModule(js, name, content, flags, observer)) {}
+    : ModuleInfo(js, compileEsmModule(js, name, content, compileCache, flags, observer)) {}
 
 ModuleRegistry::ModuleInfo::ModuleInfo(jsg::Lock& js,
     kj::StringPtr name,

src/workerd/jsg/modules.h

@@ -352,6 +352,7 @@ class ModuleRegistry {
     ModuleInfo(jsg::Lock& js,
         kj::StringPtr name,
         kj::ArrayPtr<const char> content,
+        kj::ArrayPtr<const kj::byte> compileCache,
         ModuleInfoCompileOption flags,
         const CompilationObserver& observer);
 
@@ -506,7 +507,8 @@ class ModuleRegistryImpl final: public ModuleRegistry {
       }
     }
     // TODO: asChars() might be wrong for wide characters
-    addBuiltinModule(module.getName(), module.getSrc().asChars(), module.getType());
+    addBuiltinModule(module.getName(), module.getSrc().asChars(), module.getType(),
+        module.getCompileCache().asBytes());
   }
 
   void addBuiltinBundle(Bundle::Reader bundle, kj::Maybe<Type> maybeFilter = kj::none) {
@@ -521,11 +523,13 @@ class ModuleRegistryImpl final: public ModuleRegistry {
   // sourceCode has to exist while this ModuleRegistry exists.
   // The expectation is for this method to be called during the assembly of worker global context
   // after registering all user modules.
-  void addBuiltinModule(
-      kj::StringPtr specifier, kj::ArrayPtr<const char> sourceCode, Type type = Type::BUILTIN) {
+  void addBuiltinModule(kj::StringPtr specifier,
+      kj::ArrayPtr<const char> sourceCode,
+      Type type = Type::BUILTIN,
+      kj::ArrayPtr<const kj::byte> compileCache = {}) {
     KJ_ASSERT(type != Type::BUNDLE);
     auto path = kj::Path::parse(specifier);
-    entries.insert(kj::heap<Entry>(path, type, sourceCode));
+    entries.insert(kj::heap<Entry>(path, type, sourceCode, compileCache));
   }
 
   void addBuiltinModule(
@@ -728,15 +732,22 @@ class ModuleRegistryImpl final: public ModuleRegistry {
     // Either instantiated module or module source code.
     Info info;
 
+    // Optional compileCache.
+    kj::ArrayPtr<const kj::byte> compileCache;
+
     Entry(const kj::Path& specifier, Type type, ModuleInfo info)
         : specifier(specifier.clone()),
           type(type),
           info(kj::mv(info)) {}
 
-    Entry(const kj::Path& specifier, Type type, kj::ArrayPtr<const char> src)
+    Entry(const kj::Path& specifier,
+        Type type,
+        kj::ArrayPtr<const char> src,
+        kj::ArrayPtr<const kj::byte> compileCache)
         : specifier(specifier.clone()),
           type(type),
-          info(src) {}
+          info(src),
+          compileCache(compileCache) {}
 
     Entry(const kj::Path& specifier, Type type, ModuleCallback factory)
         : specifier(specifier.clone()),
@@ -756,8 +767,8 @@ class ModuleRegistryImpl final: public ModuleRegistry {
           return kj::Maybe<ModuleInfo&>(moduleInfo);
         }
         KJ_CASE_ONEOF(src, kj::ArrayPtr<const char>) {
-          info =
-              ModuleInfo(js, specifier.toString(), src, ModuleInfoCompileOption::BUILTIN, observer);
+          info = ModuleInfo(js, specifier.toString(), src, compileCache,
+              ModuleInfoCompileOption::BUILTIN, observer);
           return info.tryGet<ModuleInfo>();
         }
         KJ_CASE_ONEOF(src, ModuleCallback) {

src/workerd/jsg/resource-test.c++

@@ -5,6 +5,7 @@
 #include "jsg-test.h"
 
 #include <workerd/jsg/resource-test.capnp.h>
+#include <workerd/util/autogate.h>
 
 namespace workerd::jsg::test {
 namespace {
@@ -590,6 +591,7 @@ struct JsBundleContext: public ContextGlobalObject {
 JSG_DECLARE_ISOLATE_TYPE(JsBundleIsolate, JsBundleContext);
 
 KJ_TEST("expectEvalModule function works") {
+  util::Autogate::initAutogate({});
   Evaluator<JsBundleContext, JsBundleIsolate, decltype(nullptr), JsBundleIsolate_TypeWrapper> e(
       v8System);
   e.expectEvalModule("export function run() { return 123; }", "number", "123");

src/workerd/server/workerd-api.c++

@@ -398,8 +398,9 @@ kj::Maybe<jsg::ModuleRegistry::ModuleInfo> WorkerdApi::tryCompileModule(jsg::Loc
               lock, Impl::compileJsonGlobal(lock, module.getJson())));
     }
     case config::Worker::Module::ES_MODULE: {
+      // TODO(soon): Make sure passing nullptr to compile cache is desired.
       return jsg::ModuleRegistry::ModuleInfo(lock, module.getName(), module.getEsModule(),
-          jsg::ModuleInfoCompileOption::BUNDLE, observer);
+          nullptr /* compile cache */, jsg::ModuleInfoCompileOption::BUNDLE, observer);
     }
     case config::Worker::Module::COMMON_JS_MODULE: {
       kj::Maybe<kj::Array<kj::StringPtr>> named = kj::none;

src/workerd/util/autogate.c++

@@ -19,6 +19,8 @@ kj::StringPtr KJ_STRINGIFY(AutogateKey key) {
       return "test-workerd"_kj;
     case AutogateKey::PYTHON_EXTERNAL_BUNDLE:
       return "python-external-bundle"_kj;
+    case AutogateKey::COMPILE_CACHE_FOR_BUILTINS:
+      return "compile-cache-for-builtins"_kj;
     case AutogateKey::NumOfKeys:
       KJ_FAIL_ASSERT("NumOfKeys should not be used in getName");
   }

src/workerd/util/autogate.h

@@ -15,6 +15,7 @@ namespace workerd::util {
 enum class AutogateKey {
   TEST_WORKERD,
   PYTHON_EXTERNAL_BUNDLE,
+  COMPILE_CACHE_FOR_BUILTINS,
   NumOfKeys  // Reserved for iteration.
 };