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

[MyPerf4J-101] Fix flaky test testMultiThread4HighRace and testMultiThread4LowRace #101 #1

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

Conversation

CaseyPan
Copy link
Owner

@CaseyPan CaseyPan commented Oct 11, 2023

What is the purpose of the change

Fix #101

Brief ChangeLog

  1. Test Overview:
    Location: Myperf4j-Base/util/concurrent/AtomicIntHashCounterTest.java
    Test : testMultiThread4HighRace and testMultiThread4LowRace

    The two test are designed to assess the behavior of a multi-threaded scenario with varying levels of contention, and they use random parameters to perform a large number of iterations to test the stability and correctness of the tested code.

  2. Reason of flakiness:
    ExecutorService newFixedThreadPool(int nThreads) method creates a thread pool that reuses a fixed number of threads operating off a shared unbounded queue. And it will throw error message IllegalArgumentException if nThreads <= 0.
    The author initially set the threadCnt to availableProcessors() - 2 to maybe avoid occupying too many computing resources. However, when the machine we use has less than 2 processors, an error will arise.

  3. Changes:

  • Introduced a new variable availableThreads to store the number of available processors.
  • Added conditional check to threadCnt to handle cases with different number of available processors.
    • If the machine has less than 2 available processors, it throws an error message indicating that multithreading is not supported
    • If the machine has 2 or 3 available processors, it use 2 threads for testing.
    • For all other cases, it use availableThreads - 2 threads, which is consistent with the original code.

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GITHUB_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [MyPerf4J-XXX] Fix NullPointerException when host is null #XXX. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist.
  • Run mvn clean package -Dmaven.test.skip=false to make sure unit-test pass.

@yashdeep97
Copy link

Thanks for adding the thorough PR description (Explains the changes really well!)
One minor change - Could you please update the "Purpose of the change"? Currently it says "Fix #XXX"

if (availableThreads <= 1) {
throw new RuntimeException("Multithreading is not supported on this system with " +
availableThreads + " available processors.");
} else if (availableThreads >= 2 && availableThreads <= 3) {

Choose a reason for hiding this comment

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

Could you please elaborate on why you chose to assign threadCnt = 2 when there are 2 or 3 available processors?
I think the authors might have an issue if both processors get used since their primary motive was to keep 2 processors available for other computing tasks.
In case of 3 available threads, maybe we can assign just one thread since that is what the authors initially intended? (However, I'm not 100% sure if this is the best approach)

@CaseyPan CaseyPan changed the title [MyPerf4J-XXX] Fix flaky test testMultiThread4HighRace and testMultiThread4LowRace #XXX [MyPerf4J-101] Fix flaky test testMultiThread4HighRace and testMultiThread4LowRace #101 Oct 13, 2023
@yesh385
Copy link

yesh385 commented Nov 2, 2023

Could you please update the PR by addressing the above comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test testMultiThread4HighRace and testMultiThread4LowRace
3 participants