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.

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

Support starting benchmark tasks using taskset on Linux #52233

Closed
mcollina opened this issue Mar 27, 2024 · 4 comments · Fixed by #52253
Closed

Support starting benchmark tasks using taskset on Linux #52233

mcollina opened this issue Mar 27, 2024 · 4 comments · Fixed by #52253
Labels
good first issue Issues that are suitable for first-time contributors.

Comments

@mcollina
Copy link
Member

Our benchmark tasks must be modified to add a taskset command to pin the CPU the benchmark will run on. This is needed because now CPUs come with performance cores and energy efficient cores, making it harder to have reliable results otherwise.

It's also needed by nodejs/build#3657.

const child = fork(path.resolve(__dirname, job.filename), cli.optional.set, {
execPath: cli.optional[job.binary],
});

https://github.com/nodejs/node/blob/main/benchmark/run.js#L43-L46

cc @nodejs/performance

@mcollina mcollina added the good first issue Issues that are suitable for first-time contributors. label Mar 27, 2024
@ryanaslett
Copy link

FYI: It appears as though the p cores are 2 threads each, and the e cores are only one. This is the output of lscpu on one of the machinees:

root@test-hetzner-ubuntu2204-x64-2 ~ #  lscpu --all --extended
CPU NODE SOCKET CORE L1d:L1i:L2:L3 ONLINE    MAXMHZ   MINMHZ      MHZ
  0    0      0    0 0:0:0:0          yes 4800.0000 800.0000 2500.000
  1    0      0    0 0:0:0:0          yes 4800.0000 800.0000 2500.000
  2    0      0    1 4:4:1:0          yes 4800.0000 800.0000 2500.000
  3    0      0    1 4:4:1:0          yes 4800.0000 800.0000 2500.000
  4    0      0    2 8:8:2:0          yes 4800.0000 800.0000 2500.000
  5    0      0    2 8:8:2:0          yes 4800.0000 800.0000 2500.000
  6    0      0    3 12:12:3:0        yes 4800.0000 800.0000 2500.000
  7    0      0    3 12:12:3:0        yes 4800.0000 800.0000 2500.000
  8    0      0    4 16:16:4:0        yes 4800.0000 800.0000 2500.000
  9    0      0    4 16:16:4:0        yes 4800.0000 800.0000 2500.000
 10    0      0    5 20:20:5:0        yes 4800.0000 800.0000  800.047
 11    0      0    5 20:20:5:0        yes 4800.0000 800.0000 2500.000
 12    0      0    6 24:24:6:0        yes 3500.0000 800.0000 2500.000
 13    0      0    7 25:25:6:0        yes 3500.0000 800.0000 2500.000
 14    0      0    8 26:26:6:0        yes 3500.0000 800.0000 2500.000
 15    0      0    9 27:27:6:0        yes 3500.0000 800.0000 2500.000
 16    0      0   10 28:28:7:0        yes 3500.0000 800.0000 2500.000
 17    0      0   11 29:29:7:0        yes 3500.0000 800.0000 2500.000
 18    0      0   12 30:30:7:0        yes 3500.0000 800.0000 2500.000
 19    0      0   13 31:31:7:0        yes 3500.0000 800.0000 2500.000

So it should just be a matter of taskset -c 0-11 <command> for the perf cores.

@RafaelGSS
Copy link
Member

RafaelGSS commented Mar 27, 2024

FWIW A long time ago I did research to see the impact of CPU 0 on Node.js tasks and apparently, it doesn't affect Node.js workload. https://github.com/RafaelGSS/lies-and-benchmark/blob/76a261381ab1fae378bc24ff1853d39e48e8bcde/cpu-0-variation/cpu-http.js#L25

I did a more comprehensive benchmark during that research, but I couldn't find any reference to it in my personal notes.

@thisalihassan
Copy link
Contributor

I will make a PR for this

@mcollina
Copy link
Member Author

@RafaelGSS the reason why this is needed is to remove variance from the tests, because on new machines there are different types of cores.

thisalihassan added a commit to thisalihassan/node that referenced this issue Mar 28, 2024
thisalihassan added a commit to thisalihassan/node that referenced this issue Mar 30, 2024
This change enhances the benchmarking tool by conditionally using the `spawn` method with `taskset` for CPU pinning, improving accuracy and consistency of benchmark results across different environments.

Fixes: nodejs#52233
thisalihassan added a commit to thisalihassan/node that referenced this issue Mar 31, 2024
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
nodejs-github-bot pushed a commit that referenced this issue Apr 6, 2024
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>
thisalihassan added a commit to thisalihassan/node that referenced this issue Apr 10, 2024
Enabled inter-process communication (ipc) in the stdio configuration
of the spawn function within the benchmark subsystem.
This change allows for improved data exchange between parent
and benchmarked child processes, addressing limitations in performance
testing scenarios.

Fixes: nodejs#52233
Refs: nodejs/performance#161
nodejs-github-bot pushed a commit that referenced this issue Apr 13, 2024
Enabled inter-process communication (ipc) in the stdio configuration
of the spawn function within the benchmark subsystem.
This change allows for improved data exchange between parent
and benchmarked child processes, addressing limitations in performance
testing scenarios.

Fixes: #52233
Refs: nodejs/performance#161
PR-URL: #52456
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Apr 13, 2024
PR-URL: #52456
Fixes: #52233
Refs: nodejs/performance#161
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
aduh95 pushed a commit that referenced this issue Apr 29, 2024
Enabled inter-process communication (ipc) in the stdio configuration
of the spawn function within the benchmark subsystem.
This change allows for improved data exchange between parent
and benchmarked child processes, addressing limitations in performance
testing scenarios.

Fixes: #52233
Refs: nodejs/performance#161
PR-URL: #52456
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
aduh95 pushed a commit that referenced this issue Apr 29, 2024
PR-URL: #52456
Fixes: #52233
Refs: nodejs/performance#161
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
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>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
Enabled inter-process communication (ipc) in the stdio configuration
of the spawn function within the benchmark subsystem.
This change allows for improved data exchange between parent
and benchmarked child processes, addressing limitations in performance
testing scenarios.

Fixes: #52233
Refs: nodejs/performance#161
PR-URL: #52456
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
PR-URL: #52456
Fixes: #52233
Refs: nodejs/performance#161
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
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>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
Enabled inter-process communication (ipc) in the stdio configuration
of the spawn function within the benchmark subsystem.
This change allows for improved data exchange between parent
and benchmarked child processes, addressing limitations in performance
testing scenarios.

Fixes: #52233
Refs: nodejs/performance#161
PR-URL: #52456
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
PR-URL: #52456
Fixes: #52233
Refs: nodejs/performance#161
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors.
Projects
None yet
4 participants