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

benchmark: forcefully close processes #43557

Closed
wants to merge 2 commits into from

Conversation

ShogunPanda
Copy link
Contributor

This PR makes sure a benchmark always exit after reporting results.
The timer is created in order to allow the process to exit normally.

This solves situations in which some resources are wrongfully reported as active (like HTTP sockets closed in a bad way by wrk) and prevent the benchmarks to continue.

Fixes: nodejs/build#2968

@ShogunPanda
Copy link
Contributor Author

CC: @nodejs/build

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jun 24, 2022
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I am fine to do this but I fear we might hide an actual bug doing it. Even though that's not really critical as it's only in our own benchmark suite.

Should we maybe increase the timeout to e.g., 5 seconds? The machines could be under heavy load.

@richardlau
Copy link
Member

Should we maybe increase the timeout to e.g., 5 seconds? The machines could be under heavy load.

FWIW the benchmark machines in the CI are baremetal machines (donated by Intel for the purposes of benchmark work). The only load on the machines should be Jenkins and the code excuted by the benchmark job.

@ShogunPanda
Copy link
Contributor Author

Yes, I tested it, it's not a load problem.
We can definitely increase to 5 just to give more time to resolve a bad situation. But we do need a upper limit to make sure a job is not stuck for days (literally).

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I guess it does not hurt to do this.

@ShogunPanda ShogunPanda force-pushed the force-close-benchmarks branch from ca74cb6 to fe5dc88 Compare June 27, 2022 07:29
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

ShogunPanda added a commit that referenced this pull request Jun 27, 2022
PR-URL: #43557
Fixes: nodejs/build#2968
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@ShogunPanda
Copy link
Contributor Author

Landed in 684e107

@ShogunPanda ShogunPanda deleted the force-close-benchmarks branch June 27, 2022 10:31
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43557
Fixes: nodejs/build#2968
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jul 20, 2022
PR-URL: #43557
Fixes: nodejs/build#2968
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43557
Fixes: nodejs/build#2968
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43557
Fixes: nodejs/build#2968
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access to @ShogunPanda to test-nearform_intel-ubuntu1604-x64
6 participants