Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 17, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a506cc
perf(ext/event): optimize Event constructor
marcosc90 Aug 16, 2023
2931d84
disable isTrusted test
marcosc90 Aug 16, 2023
0d93208
reenable test
marcosc90 Aug 16, 2023
2224dc2
remove EventInit dictionary converter
marcosc90 Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions cli/tests/unit/event_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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]> = [
Expand Down
78 changes: 31 additions & 47 deletions ext/web/02_event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]]");
Expand All @@ -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,
Expand All @@ -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) {
Expand Down Expand Up @@ -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", {
Comment on lines +415 to +417
Copy link
Member

@littledivy littledivy Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice

enumerable: true,
get: isTrusted,
});

function defineEnumerableProps(
Ctor,
props,
Expand Down
4 changes: 2 additions & 2 deletions tools/wpt/expectation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down