perf(ext/event): optimize Event constructor (#20181)

This PR optimizes `Event` constructor - ~Added a fast path for empty `eventInitDict`~ Removed `EventInit` dictionary converter - Don't make `isTrusted` a [LegacyUnforgeable](https://webidl.spec.whatwg.org/#LegacyUnforgeable) property. Doing so makes it non-spec compliant but calling `Object/Reflect.defineProperty` on the constructor is a big bottleneck. Node did the same a few months ago nodejs/node#46974. In my opinion, the performance gains are worth deviating from the spec for a browser-related property. **This PR** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 36.69 ns/iter 27,257,504.6 (33.36 ns … 42.45 ns) 37.71 ns 39.61 ns 40.07 ns event constructor 36.7 ns/iter 27,246,776.6 (33.35 ns … 56.03 ns) 37.73 ns 40.14 ns 41.74 ns ``` **main** ``` cpu: 13th Gen Intel(R) Core(TM) i9-13900H runtime: deno 1.36.1 (x86_64-unknown-linux-gnu) benchmark time (avg) iter/s (min … max) p75 p99 p995 ------------------------------------------------------------------------------- ----------------------------- event constructor no init 380.48 ns/iter 2,628,275.8 (366.66 ns … 399.39 ns) 384.58 ns 398.27 ns 399.39 ns event constructor 480.33 ns/iter 2,081,882.6 (466.67 ns … 503.47 ns) 484.27 ns 501.28 ns 503.47 ns ``` ```js Deno.bench("event constructor no init", () => { const event = new Event("foo"); }); Deno.bench("event constructor", () => { const event = new Event("foo", { bubbles: true, cancelable: false }); }); ``` towards #20167
denoland · Aug 17, 2023 · ec63b36 · ec63b36
1 parent 1b0e394
commit ec63b36
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 63 deletions.
diff --git a/cli/tests/unit/event_test.ts b/cli/tests/unit/event_test.ts
@@ -1,5 +1,5 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
-import { assert, assertEquals, assertStringIncludes } from "./test_util.ts";
+import { assertEquals, assertStringIncludes } from "./test_util.ts";
 
 Deno.test(function eventInitializedWithType() {
   const type = "click";
@@ -80,19 +80,6 @@ Deno.test(function eventInitializedWithNonStringType() {
   assertEquals(event.cancelable, false);
 });
 
-// ref https://github.com/web-platform-tests/wpt/blob/master/dom/events/Event-isTrusted.any.js
-Deno.test(function eventIsTrusted() {
-  const desc1 = Object.getOwnPropertyDescriptor(new Event("x"), "isTrusted");
-  assert(desc1);
-  assertEquals(typeof desc1.get, "function");
-
-  const desc2 = Object.getOwnPropertyDescriptor(new Event("x"), "isTrusted");
-  assert(desc2);
-  assertEquals(typeof desc2!.get, "function");
-
-  assertEquals(desc1!.get, desc2!.get);
-});
-
 Deno.test(function eventInspectOutput() {
   // deno-lint-ignore no-explicit-any
   const cases: Array<[any, (event: any) => string]> = [

diff --git a/ext/web/02_event.js b/ext/web/02_event.js
@@ -122,20 +122,6 @@ const isTrusted = ObjectGetOwnPropertyDescriptor({
   },
 }, "isTrusted").get;
 
-webidl.converters.EventInit = webidl.createDictionaryConverter("EventInit", [{
-  key: "bubbles",
-  defaultValue: false,
-  converter: webidl.converters.boolean,
-}, {
-  key: "cancelable",
-  defaultValue: false,
-  converter: webidl.converters.boolean,
-}, {
-  key: "composed",
-  defaultValue: false,
-  converter: webidl.converters.boolean,
-}]);
-
 const _attributes = Symbol("[[attributes]]");
 const _canceledFlag = Symbol("[[canceledFlag]]");
 const _stopPropagationFlag = Symbol("[[stopPropagationFlag]]");
@@ -161,36 +147,7 @@ class Event {
     this[_isTrusted] = false;
     this[_path] = [];
 
-    if (!eventInitDict[_skipInternalInit]) {
-      webidl.requiredArguments(
-        arguments.length,
-        1,
-        "Failed to construct 'Event'",
-      );
-      type = webidl.converters.DOMString(
-        type,
-        "Failed to construct 'Event'",
-        "Argument 1",
-      );
-      const eventInit = webidl.converters.EventInit(
-        eventInitDict,
-        "Failed to construct 'Event'",
-        "Argument 2",
-      );
-      this[_attributes] = {
-        type,
-        ...eventInit,
-        currentTarget: null,
-        eventPhase: Event.NONE,
-        target: null,
-        timeStamp: DateNow(),
-      };
-      // [LegacyUnforgeable]
-      ReflectDefineProperty(this, "isTrusted", {
-        enumerable: true,
-        get: isTrusted,
-      });
-    } else {
+    if (eventInitDict?.[_skipInternalInit]) {
       this[_attributes] = {
         type,
         data: eventInitDict.data ?? null,
@@ -202,10 +159,30 @@ class Event {
         target: null,
         timeStamp: 0,
       };
-      // TODO(@littledivy): Not spec compliant but performance is hurt badly
-      // for users of `_skipInternalInit`.
-      this.isTrusted = false;
+      return;
     }
+
+    webidl.requiredArguments(
+      arguments.length,
+      1,
+      "Failed to construct 'Event'",
+    );
+    type = webidl.converters.DOMString(
+      type,
+      "Failed to construct 'Event'",
+      "Argument 1",
+    );
+
+    this[_attributes] = {
+      type,
+      bubbles: !!eventInitDict.bubbles,
+      cancelable: !!eventInitDict.cancelable,
+      composed: !!eventInitDict.composed,
+      currentTarget: null,
+      eventPhase: Event.NONE,
+      target: null,
+      timeStamp: DateNow(),
+    };
   }
 
   [SymbolFor("Deno.privateCustomInspect")](inspect) {
@@ -435,6 +412,13 @@ class Event {
   }
 }
 
+// Not spec compliant. The spec defines it as [LegacyUnforgeable]
+// but doing so has a big performance hit
+ReflectDefineProperty(Event.prototype, "isTrusted", {
+  enumerable: true,
+  get: isTrusted,
+});
+
 function defineEnumerableProps(
   Ctor,
   props,

diff --git a/tools/wpt/expectation.json b/tools/wpt/expectation.json
@@ -2229,8 +2229,8 @@
       ],
       "AddEventListenerOptions-signal.any.html": true,
       "AddEventListenerOptions-signal.any.worker.html": true,
-      "Event-isTrusted.any.html": true,
-      "Event-isTrusted.any.worker.html": true,
+      "Event-isTrusted.any.html": false,
+      "Event-isTrusted.any.worker.html": false,
       "EventTarget-add-remove-listener.any.html": true,
       "EventTarget-add-remove-listener.any.worker.html": true,
       "EventTarget-addEventListener.any.html": true,