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.

Sign up for GitHub

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

Jump to bottom

Enable time-based load test mechanism in openjdk-systemtest load tests #396

Merged
merged 1 commit into from
Jan 29, 2021
Merged

Enable time-based load test mechanism in openjdk-systemtest load tests #396

merged 1 commit into from
Jan 29, 2021

Conversation

Mesbah-Alam
Copy link
Contributor

@Mesbah-Alam Mesbah-Alam commented Jan 24, 2021 •
edited
Loading

Signed-off-by: Mesbah_Alam@ca.ibm.com Mesbah_Alam@ca.ibm.com

@Mesbah-Alam Mesbah-Alam self-assigned this Jan 24, 2021
@Mesbah-Alam
Copy link
Contributor Author

Mesbah-Alam commented Jan 24, 2021 •
edited
Loading

Tests:

@Mesbah-Alam Mesbah-Alam mentioned this pull request Jan 24, 2021
Merged
@Mesbah-Alam Mesbah-Alam changed the title Add time-based execution mechanism in STF load tests Enable time-based load test mechanism in openjdk-systemtest load tests Jan 24, 2021
@Mesbah-Alam Mesbah-Alam marked this pull request as ready for review January 25, 2021 06:34
llxia
llxia reviewed Jan 26, 2021
View reviewed changes
openjdk.test.load/src/test.load/net/adoptopenjdk/stf/ClassloadingLoadTest.java
Comment on lines +86 to +97
if (isTimeBasedLoadTest) {
loadTestInvocation = loadTestInvocation.setTimeLimit(timeLimit); // If it's a time based test, stop execution after given time duration
}

loadTestInvocation = loadTestInvocation.setAbortIfOutOfMemory(false)
.addSuite("classloading")
.setSuiteThreadCount(cpuCount - 1, 10)
.setSuiteInventory(inventoryFile)
.setSuiteNumTests(totalTests * testCountMultiplier)
.setSuiteRandomSelection();
.setSuiteInventory(inventoryFile);

if (!isTimeBasedLoadTest) {
loadTestInvocation = loadTestInvocation.setSuiteNumTests(totalTests * testCountMultiplier);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have a simpler logic?

if (isTimeBasedLoadTest) { 
...
} else {
...
}

Same for the rest of PR

Copy link
Contributor Author

@Mesbah-Alam Mesbah-Alam Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is done the way it is because STF framework enforces a particular sequence in which we can add methods in a process definition. For example, in the code snippet above, if instead we did the following:

if (isTimeBasedLoadTest) { 
			loadTestInvocation = loadTestInvocation.setTimeLimit(timeLimit); // If it's a time based test, stop execution after given time duration
		} else { 
			loadTestInvocation = loadTestInvocation.setSuiteNumTests(totalTests * testCountMultiplier);
		}
		
		loadTestInvocation = loadTestInvocation.setAbortIfOutOfMemory(false)
				.addSuite("classloading")
				.setSuiteThreadCount(cpuCount - 1, 10)
				.setSuiteInventory(inventoryFile);

We will have an error like this:

GEN 16:00:20.802 - Using Mode NoOptions. Values = ''
GEN stderr Exception in thread "main" net.adoptopenjdk.stf.StfException: Java invocation built out of sequence. LOAD_TEST_ARGS cannot be set after SUITE
GEN stderr 	at net.adoptopenjdk.stf.processes.definitions.LoadTestProcessDefinition.checkAndUpdateLevel(LoadTestProcessDefinition.java:784)
GEN stderr 	at net.adoptopenjdk.stf.processes.definitions.LoadTestProcessDefinition.setAbortIfOutOfMemory(LoadTestProcessDefinition.java:297)
GEN stderr 	at net.adoptopenjdk.stf.ClassloadingLoadTest.execute(ClassloadingLoadTest.java:92)
GEN stderr 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
GEN stderr 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
GEN stderr 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
GEN stderr 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
GEN stderr 	at net.adoptopenjdk.stf.runner.StfRunner.runStage(StfRunner.java:549)
GEN stderr 	at net.adoptopenjdk.stf.runner.StfRunner.executeTest(StfRunner.java:297)
GEN stderr 	at net.adoptopenjdk.stf.runner.StfRunner.main(StfRunner.java:69)
Generation failed

Please see here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, we can have a separate issue to investigate if we need to revisit this feature in STF.

Copy link
Contributor

@llxia llxia Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we should. Please open an issue in STF. Thanks
Also, could you try one last thing? Could you try to move if-else code after setAbortIfOutOfMemory but before addSuite? According to STF code, setAbortIfOutOfMemory is set to LOAD_TEST_ARGS , setSuiteNumTests is set to SUITE , setTimeLimit is set to LOAD_TEST_ARGS and addSuite is set to SUITE .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move if-else code after setAbortIfOutOfMemory but before addSuite?

This had the same error as before. But it worked if I moved the if-else after addSuite and before setSuiteThreadCount :

LoadTestProcessDefinition loadTestInvocation = test.createLoadTestSpecification()
				.addJvmOption("-Djava.classloading.dir=" + notonclasspathDirRoot)  // Expose the bin_notonclasspath root directory to the deadlock test
				.addJvmOption("-Djava.version.number=" + javaVersion)
				.addModules(modulesAdd)
				.addPrereqJarToClasspath(JavaProcessDefinition.JarId.JUNIT)
				.addPrereqJarToClasspath(JavaProcessDefinition.JarId.HAMCREST)
				.addProjectToClasspath("openjdk.test.classloading")	
				.setAbortIfOutOfMemory(false)
				.addSuite("classloading");
		
		if (isTimeBasedLoadTest) { 
			loadTestInvocation = loadTestInvocation.setTimeLimit(timeLimit); // If it's a time based test, stop execution after given time duration
		} else { 
			loadTestInvocation = loadTestInvocation.setSuiteNumTests(totalTests * testCountMultiplier);
		}
		
		loadTestInvocation = loadTestInvocation.setSuiteThreadCount(cpuCount - 1, 10)
				.setSuiteInventory(inventoryFile)
				.setSuiteRandomSelection();

So, this simplifies the logic too. I will make similar update in the rest of the test classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Mesbah-Alam

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New STF issue raised: adoptium/STF#101

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to recap conversation with Lan about the above issue over slack:

I tried the change proposed above locally, and the if-else doesn't really work for both the scenarios when a timeLimit is provided (time-based test) and when it's no (iteration based test). Meaning, one particular sequence works when isTimeBasedLoadTest==true, and a different sequence is expected by STF when isTimeBasedLoadTest=false. Hence we have decided to stick to what the original change was.

Further discussion to commence in adoptium/STF#101

openjdk.test.load/src/test.load/net/adoptopenjdk/stf/ClassloadingLoadTest.java
loadTestInvocation = loadTestInvocation.setSuiteNumTests(totalTests * testCountMultiplier);
}

loadTestInvocation = loadTestInvocation.setSuiteRandomSelection();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set setSuiteRandomSelection , setAbortIfOutOfMemory , and others together in line 78? Same for the rest of PR

@llxia llxia requested a review from lumpfish January 26, 2021 18:48
@Mesbah-Alam
Add time-based execution mechanism in STF load tests
3eea79b 
Signed-off-by: Mesbah_Alam@ca.ibm.com <Mesbah_Alam@ca.ibm.com>
@Mesbah-Alam
Copy link
Contributor Author

Updated global default timeout value to 1h.

Started a sanity.system build on hotspot / jdk11: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/6132/

@karianna karianna added this to the January 2021 milestone Jan 29, 2021
@smlambert smlambert removed the request for review from lumpfish January 29, 2021 18:22
@smlambert smlambert merged commit 1ca5d5b into adoptium:master Jan 29, 2021
@Mesbah-Alam Mesbah-Alam deleted the add-timebased-tests branch March 10, 2021 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llxia llxia llxia approved these changes

@smlambert smlambert smlambert approved these changes

Assignees

@Mesbah-Alam Mesbah-Alam

Labels
enhancement
Projects
None yet
Milestone
January 2021
Development

Successfully merging this pull request may close these issues.
4 participants
@Mesbah-Alam @smlambert @llxia @karianna