Skip to content
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

Jump to bottom

lib: speed up MessageEvent creation internally #52951

Merged
merged 2 commits into from
Jun 16, 2024
Merged

lib: speed up MessageEvent creation internally #52951

merged 2 commits into from
Jun 16, 2024

Conversation

KhafraDev
Copy link
Member

createFastMessageEvent can be used when the arguments passed to the MessageEvent constructor do not need to be validated.

Refs: nodejs/undici#3170
Refs: #52370 (review)

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels May 12, 2024
@KhafraDev KhafraDev force-pushed the use-fast-create-messageevent branch from 98377de to 43aa76e Compare May 12, 2024 02:54
@KhafraDev KhafraDev changed the title perf: speed up MessageEvent creation internally lib: speed up MessageEvent creation internally May 12, 2024
@benjamingr
Copy link
Member

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1561/

benjamingr
benjamingr approved these changes May 12, 2024
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM with benchmark ci

@benjamingr
Copy link
Member

Benchmarks look great:

10:03:02                                                                         confidence improvement accuracy (*)   (**)  (***)
10:03:02 worker/atomics-wait.js n=10000000                                                       0.05 %       ±0.59% ±0.79% ±1.03%
10:03:02 worker/bench-eventlooputil.js method='ELU_passed' n=1000000                             0.64 %       ±0.67% ±0.90% ±1.18%
10:03:02 worker/bench-eventlooputil.js method='ELU_simple' n=1000000                             0.07 %       ±0.52% ±0.69% ±0.90%
10:03:02 worker/echo.js n=100000 sendsPerBroadcast=1 payload='object' workers=1                  0.12 %       ±0.69% ±0.92% ±1.19%
10:03:02 worker/echo.js n=100000 sendsPerBroadcast=1 payload='string' workers=1                  0.18 %       ±0.71% ±0.95% ±1.24%
10:03:02 worker/echo.js n=100000 sendsPerBroadcast=10 payload='object' workers=1        ***      2.73 %       ±0.71% ±0.95% ±1.24%
10:03:02 worker/echo.js n=100000 sendsPerBroadcast=10 payload='string' workers=1        ***      3.17 %       ±1.13% ±1.51% ±1.96%
10:03:02 worker/messageport.js n=1000000 style='eventemitter' payload='object'                  -0.06 %       ±0.36% ±0.48% ±0.63%
10:03:02 worker/messageport.js n=1000000 style='eventemitter' payload='string'                  -0.54 %       ±0.55% ±0.74% ±0.97%
10:03:02 worker/messageport.js n=1000000 style='eventtarget' payload='object'           ***     46.14 %       ±0.81% ±1.09% ±1.43%
10:03:02 worker/messageport.js n=1000000 style='eventtarget' payload='string'           ***     90.95 %       ±0.54% ±0.72% ±0.93%
10:03:02   0.55 false positives, when considering a   5% risk acceptance (*, **, ***),
10:03:02   0.11 false positives, when considering a   1% risk acceptance (**, ***),
10:03:02   0.01 false positives, when considering a 0.1% risk acceptance (***)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59164/

@KhafraDev KhafraDev added the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2024
@KhafraDev KhafraDev added the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2024
@KhafraDev KhafraDev mentioned this pull request May 15, 2024
Closed
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59254/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59272/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59287/

mcollina
mcollina approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59542/

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 16, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 16, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/52951
✔  Done loading data for nodejs/node/pull/52951
----------------------------------- PR info ------------------------------------
Title      lib: speed up MessageEvent creation internally (#52951)
Author     Khafra  (@KhafraDev)
Branch     KhafraDev:use-fast-create-messageevent -> nodejs:main
Labels     author ready, worker, needs-ci
Commits    2
 - lib: speed up MessageEvent creation internally
 - fixup
Committers 1
 - Khafra 
PR-URL: https://github.com/nodejs/node/pull/52951
Refs: https://github.com/nodejs/undici/pull/3170
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52951
Refs: https://github.com/nodejs/undici/pull/3170
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 12 May 2024 02:53:03 GMT
   ✔  Approvals: 2
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/52951#pullrequestreview-2052434926
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52951#pullrequestreview-2071390736
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2024-05-12T06:53:30Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1561/
   ℹ  Last Full PR CI on 2024-05-30T16:17:26Z: https://ci.nodejs.org/job/node-test-pull-request/59542/
- Querying data for job/node-test-pull-request/59542/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 52951
From https://github.com/nodejs/node
 * branch                  refs/pull/52951/merge -> FETCH_HEAD
✔  Fetched commits as 75fe4f35d4e5..29c44f31e535
--------------------------------------------------------------------------------
[main bac899986b] lib: speed up MessageEvent creation internally
 Author: Khafra 
 Date: Sat May 11 22:49:27 2024 -0400
 1 file changed, 6 insertions(+), 5 deletions(-)
[main 849a35ded2] fixup
 Author: Khafra 
 Date: Mon May 13 01:47:55 2024 -0400
 1 file changed, 2 insertions(+)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

lib: speed up MessageEvent creation internally


PR-URL: #52951

Refs: nodejs/undici#3170

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Matteo Collina [email protected]


[detached HEAD 4e7a2d0a67] lib: speed up MessageEvent creation internally

Author: Khafra [email protected]

Date: Sat May 11 22:49:27 2024 -0400

1 file changed, 6 insertions(+), 5 deletions(-)

Rebasing (3/4)

Rebasing (4/4)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup


PR-URL: #52951

Refs: nodejs/undici#3170

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Matteo Collina [email protected]


[detached HEAD 28d971d85f] fixup

Author: Khafra [email protected]

Date: Mon May 13 01:47:55 2024 -0400

1 file changed, 2 insertions(+)


Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/9532972963

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 16, 2024
@richardlau richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 16, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 16, 2024
@nodejs-github-bot nodejs-github-bot merged commit 672e4cc into nodejs:main Jun 16, 2024
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 672e4cc

@KhafraDev KhafraDev deleted the use-fast-create-messageevent branch June 16, 2024 04:44
targos pushed a commit that referenced this pull request Jun 20, 2024
@KhafraDev @targos
lib: speed up MessageEvent creation internally
452011b 
PR-URL: #52951
Refs: nodejs/undici#3170
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
@KhafraDev @eliphazbouye
lib: speed up MessageEvent creation internally
0928eb2 
PR-URL: nodejs#52951
Refs: nodejs/undici#3170
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@KhafraDev @bmeck
lib: speed up MessageEvent creation internally
0c507da 
PR-URL: nodejs#52951
Refs: nodejs/undici#3170
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Jun 25, 2024
Merged
@Karliz24 Karliz24 mentioned this pull request Jul 9, 2024
Open
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig left review comments

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@KhafraDev @benjamingr @nodejs-github-bot @mcollina @anonrig @targos @richardlau @aduh95