Skip to content

test: fix test-worker-memory.js for large cpu #s #27090

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 2 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Apr 4, 2019

This test consistently failed on a system with a
large number of cores (~120). Cap the number of
concurrent workers so we'll stay consistently within
the "slack" allowed with respect to rss.

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

This test consistently failed on a system with a
large number of cores (~120). Cap the number of
concurrent workers so we'll stay consistently within
the "slack" allowed with respect to rss.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 4, 2019
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2019
@nodejs-github-bot
Copy link
Collaborator

@gireeshpunathil
Copy link
Member

As a side note: I had come across the same issue and attempted to address it in a similar manner, but did not proceed when I realized that there is another problem, that may be harder to solve: when too many workers run in truly parallel manner, the rss growth pattern is observed to be non-uniform and non-deterministic (not a function of the worker count, but the degree of parallelism - order in which threads were accessing the memory, scheduling, the time gap between worker exeution and the rss data collection etc.

This fix looks reasonable; but if the issue recurs even with this, we should suspect the non-deterministic behavior of rss and may be we should fix the number of workers to a lowest possible number of CPUs in our CI.

@nodejs-github-bot
Copy link
Collaborator

Co-Authored-By: mhdawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member Author

mhdawson commented Apr 8, 2019

CI was good landing.

@mhdawson
Copy link
Member Author

mhdawson commented Apr 8, 2019

See that comment was changed sinc last CI run. Lite CI to be safe: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3198/

@mhdawson
Copy link
Member Author

mhdawson commented Apr 8, 2019

Landed as f96a660

@mhdawson mhdawson closed this Apr 8, 2019
mhdawson added a commit that referenced this pull request Apr 8, 2019
This test consistently failed on a system with a
large number of cores (~120). Cap the number of
concurrent workers so we'll stay consistently within
the "slack" allowed with respect to rss.

PR-URL: #27090
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@mhdawson mhdawson deleted the test-worker-memory-fix branch September 30, 2019 13:14
# for free to join this conversation on GitHub. Already have an account? # 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