benchmark: conditionally use spawn with taskset for CPU pinning by thisalihassan · Pull Request #52253 · nodejs/node

thisalihassan · 2024-03-28T15:21:09Z

This PR updates our process spawning mechanism from fork to spawn to incorporate the taskset command, enabling CPU pinning for benchmark tasks. This change aims to improve benchmark consistency and reliability on systems with heterogeneous CPU cores.

closes#52233

mcollina

lgtm

mcollina · 2024-03-31T20:37:12Z

Can you please amend the first commit so it passes our linting rules? You can use npx core-validate-commit.

This change enhances the benchmarking tool by conditionally using the, spawn method with taskset for CPU pinning, improving consistency of benchmark results across different environments. Fixes: nodejs#52233

thisalihassan · 2024-03-31T21:27:10Z

Can you please amend the first commit so it passes our linting rules? You can use npx core-validate-commit.

thanks, it's passing all now except

mcollina

lgtm

mcollina · 2024-04-01T08:08:55Z

I've run the benchmarking script myself and verified that it does not switch CPU.

nodejs-github-bot · 2024-04-01T08:09:21Z

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

thisalihassan · 2024-04-01T21:25:36Z

is it normal that CI takes more than 14hours?

nodejs-github-bot · 2024-04-04T08:37:06Z

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

mcollina · 2024-04-04T08:37:12Z

No, let's see if I was able to kick it back up.

mertcanaltin

lgtm

nodejs-github-bot · 2024-04-04T14:18:22Z

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

thisalihassan · 2024-04-05T00:46:16Z

Build from tarball / test-tarball-linux (pull_request) is stuck again, it's been running for 3d, can someone let me know if something needs to be fixed in the PR?

nodejs-github-bot · 2024-04-06T22:35:47Z

Commit Queue failed

- Loading data for nodejs/node/pull/52253 ✔ Done loading data for nodejs/node/pull/52253 ----------------------------------- PR info ------------------------------------ Title benchmark: conditionally use spawn with taskset for CPU pinning (#52253) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch thisalihassan:taskset_support_benchmark -> nodejs:main Labels benchmark, performance, author ready Commits 6 - benchmark: conditionally use spawn with taskset for cpu pinning - benchmark: run lint - benchmark: refactor code for benchmark cpu settings - benchmark: prefer node import syntax for built-in child_process - docs: update benchmarks md to explain CPUSET variable - benchmark: fix stdio on stderr for child process Committers 1 - Ali Hassan PR-URL: https://github.com/nodejs/node/pull/52253 Reviewed-By: Matteo Collina Reviewed-By: Raz Luvaton Reviewed-By: Yagiz Nizipli ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52253 Reviewed-By: Matteo Collina Reviewed-By: Raz Luvaton Reviewed-By: Yagiz Nizipli -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 28 Mar 2024 15:21:09 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52253#pullrequestreview-1970850302 ✔ - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/52253#pullrequestreview-1984706306 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/52253#pullrequestreview-1981518776 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ 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 52253 From https://github.com/nodejs/node * branch refs/pull/52253/merge -> FETCH_HEAD ✔ Fetched commits as ba07e4e5e643..5f700f58ec81 -------------------------------------------------------------------------------- [main 3c831d10f9] benchmark: conditionally use spawn with taskset for cpu pinning Author: Ali Hassan Date: Thu Mar 28 20:18:15 2024 +0500 2 files changed, 50 insertions(+), 9 deletions(-) [main a1a9ea842b] benchmark: run lint Author: Ali Hassan Date: Fri Mar 29 02:21:59 2024 +0500 2 files changed, 4 insertions(+), 4 deletions(-) [main 511f2a7dc2] benchmark: refactor code for benchmark cpu settings Author: Ali Hassan Date: Sat Mar 30 04:53:37 2024 +0500 3 files changed, 38 insertions(+), 15 deletions(-) [main b9577be4c0] benchmark: prefer node import syntax for built-in child_process Author: Ali Hassan Date: Sat Mar 30 15:10:36 2024 +0500 2 files changed, 2 insertions(+), 2 deletions(-) [main 137610d821] docs: update benchmarks md to explain CPUSET variable Author: Ali Hassan Date: Fri Apr 5 12:44:29 2024 +0500 1 file changed, 36 insertions(+) [main d44dadece0] benchmark: fix stdio on stderr for child process Author: Ali Hassan Date: Sat Apr 6 04:09:09 2024 +0500 2 files changed, 6 insertions(+), 8 deletions(-) ✔ Patches applied There are 6 commits in the PR. Attempting autorebase. Rebasing (2/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: conditionally use spawn with taskset for cpu pinning

This change enhances the benchmarking tool by conditionally using the,
spawn method with taskset for CPU pinning, improving consistency of
benchmark results across different environments.

Fixes: #52233
PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 7362ac3807] benchmark: conditionally use spawn with taskset for cpu pinning
Author: Ali Hassan ali-hassan27@outlook.com
Date: Thu Mar 28 20:18:15 2024 +0500
2 files changed, 50 insertions(+), 9 deletions(-)
Rebasing (3/12)
Rebasing (4/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: run lint

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 1282101554] benchmark: run lint
Author: Ali Hassan ali-hassan27@outlook.com
Date: Fri Mar 29 02:21:59 2024 +0500
2 files changed, 4 insertions(+), 4 deletions(-)
Rebasing (5/12)
Rebasing (6/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: refactor code for benchmark cpu settings

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 23a8358636] benchmark: refactor code for benchmark cpu settings
Author: Ali Hassan ali-hassan27@outlook.com
Date: Sat Mar 30 04:53:37 2024 +0500
3 files changed, 38 insertions(+), 15 deletions(-)
Rebasing (7/12)
Rebasing (8/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: prefer node import syntax for built-in child_process

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 052a8c9d28] benchmark: prefer node import syntax for built-in child_process
Author: Ali Hassan ali-hassan27@outlook.com
Date: Sat Mar 30 15:10:36 2024 +0500
2 files changed, 2 insertions(+), 2 deletions(-)
Rebasing (9/12)
Rebasing (10/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
docs: update benchmarks md to explain CPUSET variable

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD a2eb5d2273] docs: update benchmarks md to explain CPUSET variable
Author: Ali Hassan ali-hassan27@outlook.com
Date: Fri Apr 5 12:44:29 2024 +0500
1 file changed, 36 insertions(+)
Rebasing (11/12)
Rebasing (12/12)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: fix stdio on stderr for child process

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 92e603f9cb] benchmark: fix stdio on stderr for child process
Author: Ali Hassan ali-hassan27@outlook.com
Date: Sat Apr 6 04:09:09 2024 +0500
2 files changed, 6 insertions(+), 8 deletions(-)

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/8584238305

nodejs-github-bot · 2024-04-06T22:43:55Z

Landed in 47c934e

This change enhances the benchmarking tool by conditionally using the, spawn method with taskset for CPU pinning, improving consistency of benchmark results across different environments. Fixes: #52233 PR-URL: #52253 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>

nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. performance Issues and PRs related to the performance of Node.js. labels Mar 28, 2024

mcollina reviewed Mar 28, 2024
View reviewed changes

Comment threadbenchmark/compare.js Outdated

thisalihassan requested a review from mcollina March 28, 2024 20:08

mcollina reviewed Mar 28, 2024
View reviewed changes

Comment threadbenchmark/run.js Outdated

thisalihassan force-pushed the taskset_support_benchmark branch from 072af25 to 3fb5b79Compare March 28, 2024 20:53

thisalihassan requested a review from mcollina March 28, 2024 20:57

thisalihassan changed the title ~~taskset support for benchmarks~~benchmark: conditionally use spawn with taskset for CPU pinningMar 28, 2024

anonrig reviewed Mar 29, 2024
View reviewed changes

Comment threadbenchmark/compare.js Outdated
Comment threadbenchmark/compare.js Outdated
Comment threadbenchmark/run.js Outdated

thisalihassan requested a review from anonrig March 30, 2024 00:18

mertcanaltin reviewed Mar 30, 2024
View reviewed changes

Comment threadbenchmark/run.js Outdated

thisalihassan force-pushed the taskset_support_benchmark branch from bf8e66e to 9ac9d79Compare March 30, 2024 12:17

thisalihassan requested a review from mertcanaltin March 30, 2024 12:18

mcollina approved these changes Mar 31, 2024
View reviewed changes

thisalihassan added 4 commits April 1, 2024 02:23

benchmark: conditionally use spawn with taskset for cpu pinning
28c68f0
This change enhances the benchmarking tool by conditionally using the, spawn method with taskset for CPU pinning, improving consistency of benchmark results across different environments. Fixes: nodejs#52233

benchmark: run lint
a559149

benchmark: refactor code for benchmark cpu settings
3270cef

benchmark: prefer node import syntax for built-in child_process
d6b0a82

thisalihassan force-pushed the taskset_support_benchmark branch from 9ac9d79 to d6b0a82Compare March 31, 2024 21:25

mcollina approved these changes Apr 1, 2024
View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

mertcanaltin approved these changes Apr 4, 2024
View reviewed changes

rluvaton mentioned this pull request Apr 4, 2024
events: improve addAbortListener perf by saving the same options object #52367
Merged

rluvaton approved these changes Apr 4, 2024
View reviewed changes

anonrig approved these changes Apr 5, 2024
View reviewed changes

Comment threadbenchmark/_cli.js

thisalihassan added 2 commits April 5, 2024 12:44

docs: update benchmarks md to explain CPUSET variable
e87f992

benchmark: fix stdio on stderr for child process
5f700f5

thisalihassan force-pushed the taskset_support_benchmark branch from b070fa2 to 5f700f5Compare April 5, 2024 23:09

rluvaton added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 6, 2024

rluvaton approved these changes Apr 6, 2024
View reviewed changes

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 6, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2024

nodejs-github-bot merged commit 47c934e into nodejs:mainApr 6, 2024

rluvaton mentioned this pull request Apr 7, 2024
Hetzner Benchmarking Machine Replacements nodejs/build#3657
Open

rluvaton reviewed Apr 10, 2024
View reviewed changes

Comment threadbenchmark/compare.js

rluvaton reviewed Apr 10, 2024
View reviewed changes

Comment threadbenchmark/compare.js

rluvaton mentioned this pull request Apr 10, 2024
run compare on perf cores nodejs/performance#162
Merged

thisalihassan mentioned this pull request Apr 15, 2024
Force push does not invalidate existing approved reviews nodejs/node-core-utils#677
Open

RafaelGSS mentioned this pull request Apr 15, 2024
2024-04-24, Version 22.0.0 (Current) #52505
Merged

marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
Merged

Uh oh!

Uh oh!

Conversation

thisalihassan commented Mar 28, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 31, 2024

Uh oh!

thisalihassan commented Mar 31, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Apr 1, 2024

Uh oh!

nodejs-github-bot commented Apr 1, 2024

Uh oh!

thisalihassan commented Apr 1, 2024

Uh oh!

nodejs-github-bot commented Apr 4, 2024

Uh oh!

mcollina commented Apr 4, 2024

Uh oh!

mertcanaltin left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 4, 2024

Uh oh!

thisalihassan commented Apr 5, 2024

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 6, 2024

Fixes: #52233 PR-URL: #52253 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Raz Luvaton rluvaton@gmail.com Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Raz Luvaton rluvaton@gmail.com Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Raz Luvaton rluvaton@gmail.com Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Raz Luvaton rluvaton@gmail.com Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Raz Luvaton rluvaton@gmail.com Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253 Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Raz Luvaton rluvaton@gmail.com Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

Successfully rebased and updated refs/heads/main.

Uh oh!

nodejs-github-bot commented Apr 6, 2024

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thisalihassan commented Mar 28, 2024•
edited
Loading

Fixes: #52233
PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

PR-URL: #52253
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io