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

test_runner: run single test file benchmark #56479

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pmarchini
Copy link
Member

With reference to #55723:

As I'm exploring the possibility of adding a new kind of isolation (worker thread), I think we should add at least one benchmark to measure the differences in overhead between various isolation methods.
For example, I would expect a worker thread to have less computational overhead than a process.
This benchmark is meant to trace and highlight these kinds of differences

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. test_runner Issues and PRs related to the test runner subsystem. labels Jan 5, 2025
@pmarchini pmarchini requested review from cjihrig and MoLow January 5, 2025 15:56
@cjihrig
Copy link
Contributor

cjihrig commented Jan 5, 2025

As I'm exploring the possibility of adding a new kind of isolation (worker thread)

Is there any benefit to using worker threads in the test runner? My guess is that there isn't. Worker threads will be slower than no isolation, have less isolation than child processes, and a number of Node APIs either don't work, or work differently in worker threads.

@pmarchini
Copy link
Member Author

As I'm exploring the possibility of adding a new kind of isolation (worker thread)

Is there any benefit to using worker threads in the test runner? My guess is that there isn't. Worker threads will be slower than no isolation, have less isolation than child processes, and a number of Node APIs either don't work, or work differently in worker threads.

I agree on the "somewhere in between offering limited benefits"

The only significant advantage I see is the overhead reduction.

I've experimented with a poc and the reduction seems to be decent but not incredible on a large test suite.

My idea would be to start with an isolation based on a "single worker" moving incrementally then to a "pool" of workers.
This could avoid even more overhead avoiding the creation of new workers.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 5, 2025

I'm not opposed to supporting a pool, but I still think worker threads are not a great choice due to API differences.

FWIW, you might find these benchmarks interesting.

@pmarchini
Copy link
Member Author

Thanks @cjihrig for the link! I've taken a quick look and I think I'll extend this bench 😁
Do you have any specific suggestions regarding this PR?

I'm not opposed to supporting a pool, but I still think worker threads are not a great choice due to API differences.

On this point, are you thinking about a pool of processes?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM

On this point, are you thinking about a pool of processes?

I think that would be less surprising to users, albeit not as performant as using worker threads. No matter which approach you take, you'll also have to deal with certain edge cases. For example, only tests will require the --test-only flag. There might also be edge cases related to watch mode.

benchmark/test_runner/run-single-test-file.js Show resolved Hide resolved
benchmark/test_runner/run-single-test-file.js Outdated Show resolved Hide resolved
@pmarchini pmarchini force-pushed the test_runner/run-single-test-file-benchmarks branch from 622ddbc to 0fe0976 Compare January 5, 2025 19:53
@pmarchini pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@pmarchini pmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants