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: harden the tick sampling logic #32190

Closed
wants to merge 3 commits into from

Conversation

HarshithaKP
Copy link
Member

Under peculiar system load conditions, the profiler thread
does not get enough CPU slices to perform the sampling.
Improve the interaction between worker and parent thread
by performing a large disc read, which is a better blend of
CPU and I/O bound work, than earlier versions.
This produces x10 more samples than the existing one,
in 10 iterations, as opposed to 1024.

Also capture worker error situations to improve debugging

Refs: #26401 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Under peculiar system load conditions, the profiler thread
does not get enough CPU slices to perform the sampling.
Improve the interaction between worker and parent thread
by performing a large disc read, which is a better blend of
CPU and I/O bound work, than earlier versions.
This produces x10 more samples than the existing one,
in 10 iterations, as opposed to 1024.

Also capture worker error situations to improve debugging

Refs: nodejs#26401 (comment)
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 11, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

@HarshithaKP Looks like the test failed in CI in debug mode?

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2020
@gireeshpunathil
Copy link
Member

Error [ERR_WORKER_OUT_OF_MEMORY]: Worker terminated due to reaching memory limit: JS heap out of memory

looks like both linux-containerized and for arm the worker went out of memory, roughly matching with what @bnoordhuis had predicted before.

probably we should limit the volume of either the disc read, or the transport?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Mar 30, 2020
Under peculiar system load conditions, the profiler thread
does not get enough CPU slices to perform the sampling.
Improve the interaction between worker and parent thread
by performing a large disc read, which is a better blend of
CPU and I/O bound work, than earlier versions.
This produces x10 more samples than the existing one,
in 10 iterations, as opposed to 1024.

Also capture worker error situations to improve debugging

Refs: #26401 (comment)

PR-URL: #32190
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member

Landed in bffc932

@addaleax addaleax closed this Mar 30, 2020
addaleax pushed a commit that referenced this pull request Mar 30, 2020
Under peculiar system load conditions, the profiler thread
does not get enough CPU slices to perform the sampling.
Improve the interaction between worker and parent thread
by performing a large disc read, which is a better blend of
CPU and I/O bound work, than earlier versions.
This produces x10 more samples than the existing one,
in 10 iterations, as opposed to 1024.

Also capture worker error situations to improve debugging

Refs: #26401 (comment)

PR-URL: #32190
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
Under peculiar system load conditions, the profiler thread
does not get enough CPU slices to perform the sampling.
Improve the interaction between worker and parent thread
by performing a large disc read, which is a better blend of
CPU and I/O bound work, than earlier versions.
This produces x10 more samples than the existing one,
in 10 iterations, as opposed to 1024.

Also capture worker error situations to improve debugging

Refs: #26401 (comment)

PR-URL: #32190
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants