Merge pull request #5339 from cloudflare/jasnell/jsg-struct-template

Author: jasnell

src/workerd/api/streams/standard.c++

@@ -2252,7 +2252,7 @@ jsg::Promise<void> ReadableStreamJsController::cancel(
 
   const auto doCancel = [&](auto& consumer) {
     auto reason = js.v8Ref(maybeReason.orDefault([&] { return js.v8Undefined(); }));
-    KJ_DEFER(state.init<StreamStates::Closed>());
+    KJ_DEFER(doClose(js));
     return consumer->cancel(js, reason.getHandle(js));
   };
 

src/workerd/io/compatibility-date.capnp

@@ -1192,4 +1192,13 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef {
     $compatDisableFlag("disable_python_external_sdk")
     $experimental;
   # Don't include the Python sdk from the runtime, use a vendored copy.
+
+  fastJsgStruct @141 :Bool
+    $compatEnableFlag("enable_fast_jsg_struct")
+    $compatDisableFlag("disable_fast_jsg_struct");
+  # Enables the fast jsg::Struct optimization. With this enabled, JSG_STRUCTS
+  # will use a more efficient creation pattern that reduces construction time.
+  # However, optional fields will be explicitly set to undefined rather than
+  # being omitted, which is an observable behavior change.
+  # TODO(soon): Once proven in production, add a default on date
 }

src/workerd/io/worker.c++

@@ -1065,6 +1065,9 @@ Worker::Isolate::Isolate(kj::Own<Api> apiParam,
     if (features.getEnhancedErrorSerialization()) {
       lock->setUsingEnhancedErrorSerialization();
     }
+    if (features.getFastJsgStruct()) {
+      lock->setUsingFastJsgStruct();
+    }
 
     if (impl->inspector != kj::none || ::kj::_::Debug::shouldLog(::kj::LogSeverity::INFO)) {
       lock->setLoggerCallback([this](jsg::Lock& js, kj::StringPtr message) {

src/workerd/jsg/jsg.c++

@@ -191,6 +191,14 @@ void Lock::setUsingEnhancedErrorSerialization() {
   IsolateBase::from(v8Isolate).setUsingEnhancedErrorSerialization();
 }
 
+void Lock::setUsingFastJsgStruct() {
+  IsolateBase::from(v8Isolate).setUsingFastJsgStruct();
+}
+
+bool Lock::isUsingFastJsgStruct() const {
+  return IsolateBase::from(v8Isolate).getUsingFastJsgStruct();
+}
+
 bool Lock::isUsingEnhancedErrorSerialization() const {
   return IsolateBase::from(v8Isolate).getUsingEnhancedErrorSerialization();
 }

src/workerd/jsg/jsg.h

@@ -717,6 +717,14 @@ struct HasStructTypeScriptDefine<T, decltype(T::_JSG_STRUCT_TS_DEFINE_DO_NOT_USE
   template <typename TypeWrapper, typename Self>                                                   \
   using JsgFieldWrappers =                                                                         \
       ::workerd::jsg::TypeTuple<JSG_FOR_EACH(JSG_STRUCT_FIELD, , __VA_ARGS__)>;                    \
+  template <typename Self>                                                                         \
+  static v8::Local<v8::DictionaryTemplate> jsgGetTemplate(v8::Isolate* isolate) {                  \
+    kj::Vector<std::string_view> names;                                                            \
+    JSG_FOR_EACH(JSG_STRUCT_FIELD_COL, , __VA_ARGS__);                                             \
+    auto namesPtr = names.asPtr().asConst();                                                       \
+    return v8::DictionaryTemplate::New(                                                            \
+        isolate, v8::MemorySpan<const std::string_view>(namesPtr.begin(), namesPtr.size()));       \
+  }                                                                                                \
   template <typename Registry, typename Self, typename Config>                                     \
   static void registerMembersInternal(Registry& registry, Config arg) {                            \
     JSG_FOR_EACH(JSG_STRUCT_REGISTER_MEMBER, , __VA_ARGS__);                                       \
@@ -761,6 +769,10 @@ consteval size_t prefixLengthToStrip(const char (&s)[N]) {
 // it stripped.
 #define JSG_STRUCT_FIELD_NAME(_, name) name##_JSG_NAME_DO_NOT_USE_DIRECTLY[] = #name
 
+#define JSG_STRUCT_FIELD_COL(_, name)                                                              \
+  ::workerd::jsg::jsgAddToStructNames<decltype(::kj::instance<Self>().name),                       \
+      name##_JSG_NAME_DO_NOT_USE_DIRECTLY, ::workerd::jsg::prefixLengthToStrip(#name)>(names)
+
 // (Internal implementation details for JSG_STRUCT.)
 #define JSG_STRUCT_FIELD(_, name)                                                                  \
   ::workerd::jsg::FieldWrapper<TypeWrapper, Self, decltype(::kj::instance<Self>().name),           \
@@ -1061,6 +1073,16 @@ class SelfRef: public V8Ref<v8::Object> {
   inline Value asValue(Lock& js) const;
 };
 
+template <typename U>
+static constexpr bool isUsableStructField = !kj::isSameType<U, SelfRef>() &&
+    !kj::isSameType<U, Unimplemented>() && !kj::isSameType<U, WontImplement>();
+
+template <typename T, const char* name, size_t prefix>
+void jsgAddToStructNames(auto& names) {
+  constexpr const char* exportedName = name + prefix;
+  if constexpr (isUsableStructField<T>) names.add(exportedName);
+}
+
 // TODO(cleanup): This class was meant to be a ByteString (characters in the range [0,255]), but
 //   its only use so far is in api::Headers. But making the Headers class use ByteStrings turned
 //   out to be unwise. Nevertheless, it is still useful to keep around in order to provide
@@ -2626,6 +2648,8 @@ class Lock {
 
   void setCaptureThrowsAsRejections(bool capture);
   void setUsingEnhancedErrorSerialization();
+  void setUsingFastJsgStruct();
+  bool isUsingFastJsgStruct() const;
   bool isUsingEnhancedErrorSerialization() const;
 
   void setNodeJsCompatEnabled();

src/workerd/jsg/setup.h

@@ -255,6 +255,14 @@ class IsolateBase {
     return usingEnhancedErrorSerialization;
   }
 
+  void setUsingFastJsgStruct() {
+    usingFastJsgStruct = true;
+  }
+
+  bool getUsingFastJsgStruct() const {
+    return usingFastJsgStruct;
+  }
+
   bool pumpMsgLoop() {
     return v8System.pumpMsgLoop(ptr);
   }
@@ -319,6 +327,7 @@ class IsolateBase {
   bool allowTopLevelAwait = true;
   bool usingNewModuleRegistry = false;
   bool usingEnhancedErrorSerialization = false;
+  bool usingFastJsgStruct = false;
 
   // Only used when the original module registry is used.
   bool throwOnUnrecognizedImportAssertion = false;

src/workerd/jsg/struct.h

@@ -91,6 +91,9 @@ class FieldWrapper {
   explicit FieldWrapper(v8::Isolate* isolate)
       : nameHandle(isolate, v8StrIntern(isolate, exportedName)) {}
 
+  // The is the original, slow-path wrap implementation that uses Set(). Prefer the other overload
+  // for better performance. It is, however, a breaking change to remove this overload so we
+  // need to keep it with a compatibility flag.
   void wrap(Lock& js,
       TypeWrapper& wrapper,
       v8::Isolate* isolate,
@@ -112,6 +115,24 @@ class FieldWrapper {
     }
   }
 
+  void wrap(Lock& js,
+      TypeWrapper& wrapper,
+      v8::Isolate* isolate,
+      v8::Local<v8::Context> context,
+      kj::Maybe<v8::Local<v8::Object>> creator,
+      Struct& in,
+      v8::MaybeLocal<v8::Value>& out,
+      size_t& idx) {
+    if constexpr (kj::isSameType<T, SelfRef>()) {
+      // Ignore SelfRef when converting to JS.
+    } else if constexpr (kj::isSameType<T, Unimplemented>() || kj::isSameType<T, WontImplement>()) {
+      // Fields with these types are required NOT to be present, so don't try to convert them.
+    } else {
+      idx++;
+      out = wrapper.wrap(js, context, creator, kj::mv(in.*field));
+    }
+  }
+
   Type unwrap(TypeWrapper& wrapper,
       v8::Isolate* isolate,
       v8::Local<v8::Context> context,
@@ -149,10 +170,42 @@ class StructWrapper<Self, T, TypeTuple<FieldWrappers...>, kj::_::Indexes<indices
     return typeid(T);
   }
 
+  // A count of the JSG_STRUCT fields that are usable for the v8::DictionaryTemplate
+  // version of wrap (i.e. not SelfRef, Unimplemented, or WontImplement).
+  static constexpr size_t kCountOfUsableFields =
+      ((isUsableStructField<typename FieldWrappers::Type> ? 1 : 0) + ...);
+
   v8::Local<v8::Object> wrap(
       Lock& js, v8::Local<v8::Context> context, kj::Maybe<v8::Local<v8::Object>> creator, T&& in) {
     auto isolate = js.v8Isolate;
     auto& fields = getFields(isolate);
+
+    // Fast path using a cached dictionary template.
+    if (js.isUsingFastJsgStruct()) {
+      v8::MaybeLocal<v8::Value> values[kCountOfUsableFields]{};
+
+      size_t idx = 0;
+      (kj::get<indices>(fields).wrap(
+           js, static_cast<Self&>(*this), isolate, context, creator, in, values[idx], idx),
+          ...);
+
+      // We use a cached dictionary template to improve performance on repeated struct wraps.
+
+      v8::Local<v8::DictionaryTemplate> tmpl;
+      if (templateHandle.IsEmpty()) {
+        tmpl = T::template jsgGetTemplate<T>(isolate);
+        templateHandle.Reset(isolate, tmpl);
+      } else {
+        tmpl = templateHandle.Get(isolate);
+      }
+
+      // Make sure we filled in the expected number of fields.
+      KJ_ASSERT(idx == kCountOfUsableFields);
+
+      return tmpl->NewInstance(context, values);
+    }
+
+    // Original slow path.
     v8::Local<v8::Object> out = v8::Object::New(isolate);
     (kj::get<indices>(fields).wrap(
          js, static_cast<Self&>(*this), isolate, context, creator, in, out),
@@ -217,6 +270,7 @@ class StructWrapper<Self, T, TypeTuple<FieldWrappers...>, kj::_::Indexes<indices
   void getTemplate() = delete;
 
  private:
+  v8::Global<v8::DictionaryTemplate> templateHandle;
   kj::Maybe<kj::Tuple<FieldWrappers...>> lazyFields;
 
   kj::Tuple<FieldWrappers...>& getFields(v8::Isolate* isolate) {

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

@@ -115,6 +115,7 @@ JSG_DECLARE_ISOLATE_TYPE(OptionalIsolate,
 
 KJ_TEST("optionals and maybes") {
   Evaluator<OptionalContext, OptionalIsolate> e(v8System);
+  e.getIsolate().setUsingFastJsgStruct();
   e.expectEval("takeOptional(new NumberBox(123))", "number", "123");
   e.expectEval("takeOptional()", "number", "321");
   e.expectEval("takeOptional(undefined)", "number", "321");
@@ -161,14 +162,14 @@ KJ_TEST("optionals and maybes") {
 
   e.expectEval(
       "var object = makeTestOptionalFields(undefined, undefined, null);\n" ENUMERATE_OBJECT,
-      "string", "nullable: null");
+      "string", "optional: undefined, lenient: undefined, nullable: null");
   e.expectEval("var object = makeTestOptionalFields('foo', 'bar', null);\n" ENUMERATE_OBJECT,
       "string", "optional: foo, lenient: bar, nullable: null");
   e.expectEval("var object = makeTestOptionalFields('foo', 'bar', 'baz');\n" ENUMERATE_OBJECT,
       "string", "optional: foo, lenient: bar, nullable: baz");
   e.expectEval(
       "var object = makeTestOptionalFields(undefined, undefined, 'bar');\n" ENUMERATE_OBJECT,
-      "string", "nullable: bar");
+      "string", "optional: undefined, lenient: undefined, nullable: bar");
 #undef ENUMERATE_OBJECT
 
   e.expectEval("readTestAllOptionalFields({})", "string", "(absent), 321");

src/wpt/BUILD.bazel

@@ -108,6 +108,7 @@ wpt_test(
 wpt_test(
     name = "streams",
     size = "large",
+    compat_flags = ["enable_fast_jsg_struct"],
     config = "streams-test.ts",
     wpt_directory = "@wpt//:streams@module",
 )

src/wpt/streams-test.ts

@@ -329,7 +329,6 @@ export default {
     comment: 'To be investigated',
     expectedFailures: [
       'ReadableStream with byte source (empty): calling getReader with invalid arguments should throw appropriate errors',
-      'ReadableStream with byte source (empty) default reader: canceling via the reader should cause the reader to act closed',
       'ReadableStream with byte source (empty) BYOB reader: canceling via the reader should cause the reader to act closed',
     ],
   },
@@ -338,7 +337,6 @@ export default {
     comment: 'To be investigated',
     expectedFailures: [
       'Async iterator instances should have the correct list of properties',
-      'return(); next()',
       'return(); next() [no awaiting]',
       'return(); return() [no awaiting]',
       'return(); next() with delayed cancel() [no awaiting]',
@@ -348,8 +346,6 @@ export default {
       'next() that succeeds; return() [no awaiting]',
       'next() that succeeds; next() that reports an error; next()',
       'next() that succeeds; next() that reports an error(); return()',
-      'Async-iterating a pull source manually',
-      'return(); next() with delayed cancel()',
     ],
   },
   'readable-streams/bad-strategies.any.js': {
@@ -444,11 +440,6 @@ export default {
       'ReadableStreamDefaultReader closed promise should be rejected with undefined if that is the error',
       // TODO(conform): The spec expects this to be a TypeError, not a RangeError
       'getReader() should call ToString() on mode',
-      // In the actual WPT, the result is expected to explicitly contain `value: undefined`,
-      // but we omit that.
-      'Reading twice on a stream that gets closed',
-      // TODO: Investigate why this test is failing
-      'Reading twice on a closed stream',
     ],
   },
   'readable-streams/floating-point-total-queue-size.any.js': {
@@ -478,20 +469,7 @@ export default {
       'ReadableStream.from: cancel() rejects when return() throws synchronously',
       'ReadableStream.from accepts an array of promises',
       'ReadableStream.from accepts a sync iterable of promises',
-      'ReadableStream.from accepts an empty iterable',
-      'ReadableStream.from accepts an array of values',
-      'ReadableStream.from accepts an array iterator',
-      'ReadableStream.from accepts a Set',
-      'ReadableStream.from accepts a Set iterator',
-      'ReadableStream.from accepts a sync generator',
-      'ReadableStream.from accepts a sync iterable of values',
-      'ReadableStream.from accepts an async iterable',
-      'ReadableStream.from accepts an async generator',
-      'ReadableStream.from accepts a ReadableStream',
-      'ReadableStream.from accepts a ReadableStream async iterator',
       'ReadableStream.from ignores a null @@asyncIterator',
-      'ReadableStream.from: return() is not called when iterator completes normally',
-      'ReadableStream.from(array), push() to array while reading',
       'ReadableStream.from: cancelling the returned stream calls and awaits return()',
     ],
   },
@@ -593,14 +571,11 @@ export default {
   'readable-streams/tee.any.js': {
     comment: 'To be investigated',
     expectedFailures: [
-      'ReadableStream teeing: should be able to read one branch to the end without affecting the other',
       'ReadableStream teeing: errors in the source should propagate to both branches',
       'ReadableStreamTee should only pull enough to fill the emptiest queue',
       'ReadableStreamTee stops pulling when original stream errors while branch 1 is reading',
       'ReadableStreamTee stops pulling when original stream errors while branch 2 is reading',
       'ReadableStreamTee stops pulling when original stream errors while both branches are reading',
-      'ReadableStream teeing: canceling branch1 should finish when branch2 reads until end of stream',
-      'ReadableStream teeing: enqueue() and close() while both branches are pulling',
       'ReadableStream teeing: canceling both branches should aggregate the cancel reasons into an array',
       'ReadableStream teeing: canceling both branches in reverse order should aggregate the cancel reasons into an array',
       'ReadableStream teeing: failing to cancel the original stream should cause cancel() to reject on branches',
@@ -616,25 +591,12 @@ export default {
     ],
     expectedFailures: [
       'ReadableStream (empty): calling getReader with invalid arguments should throw appropriate errors',
-      'ReadableStream (empty) reader: canceling via the reader should cause the reader to act closed',
-      'ReadableStream reader (closed before getting reader): read() should fulfill with { value: undefined, done: true }',
-      'ReadableStream reader (closed before getting reader): read() multiple times should fulfill with { value: undefined, done: true }',
-      'ReadableStream reader (closed before getting reader): read() should work when used within another read() fulfill callback',
       'ReadableStream reader (closed before getting reader): releasing the lock should cause closed to reject and change identity',
-      'ReadableStream reader (closed after getting reader): read() should fulfill with { value: undefined, done: true }',
-      'ReadableStream reader (closed after getting reader): read() multiple times should fulfill with { value: undefined, done: true }',
-      'ReadableStream reader (closed after getting reader): read() should work when used within another read() fulfill callback',
       'ReadableStream reader (closed after getting reader): releasing the lock should cause closed to reject and change identity',
-      'ReadableStream reader (closed via cancel after getting reader): read() should fulfill with { value: undefined, done: true }',
-      'ReadableStream reader (closed via cancel after getting reader): read() multiple times should fulfill with { value: undefined, done: true }',
-      'ReadableStream reader (closed via cancel after getting reader): read() should work when used within another read() fulfill callback',
       'ReadableStream reader (closed via cancel after getting reader): releasing the lock should cause closed to reject and change identity',
       'ReadableStream (errored via returning a rejected promise in start) reader: releasing the lock should cause closed to reject and change identity',
       'ReadableStream reader (errored before getting reader): releasing the lock should cause closed to reject and change identity',
       'ReadableStream reader (errored after getting reader): releasing the lock should cause closed to reject and change identity',
-      'ReadableStream (two chunks enqueued, still open) reader: cancel() after a read() should still give that single read result',
-      'ReadableStream (two chunks enqueued, then closed) reader: third read(), without waiting, should give { value: undefined, done: true } (sequential)',
-      'ReadableStream (two chunks enqueued, then closed) reader: third read(), without waiting, should give { value: undefined, done: true } (nested)',
     ],
   },