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

Removed duplicate System.gc() in test #18339

Merged
merged 8 commits into from
Nov 8, 2023
Merged

Removed duplicate System.gc() in test #18339

merged 8 commits into from
Nov 8, 2023

Conversation

flooxo
Copy link
Contributor

@flooxo flooxo commented Oct 24, 2023 •
edited
Loading

Removes that System.gc() is not called repeatedly, but only once in all test files

This PR closes #18335 due to #18044

@pshipton pshipton requested a review from llxia October 24, 2023 18:35
@llxia
Copy link
Contributor

llxia commented Oct 24, 2023

https://openj9-jenkins.osuosl.org/job/Grinder/2996/

@hangshao0
Copy link
Contributor

FYI @theresa-m ValueTypeTests.java is being updated here.

@llxia
Copy link
Contributor

llxia commented Oct 27, 2023

jenkins test sanity,extended xlinux jdk8,jdk11

@flooxo
Copy link
Contributor Author

flooxo commented Oct 29, 2023

I'm not sure right now what to change to make the 3 tests pass. Could you please give me a hint?

@llxia
Copy link
Contributor

llxia commented Oct 30, 2023 •
edited
Loading

PR build: https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/4638/

llxia
llxia approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

LGTM

pshipton
pshipton reviewed Oct 31, 2023
View reviewed changes
test/functional/JIT_Test/src/jit/test/finalizer/TestObject.java
@@ -66,12 +66,10 @@ public static boolean isFinalized(String name) {
}

protected static void thoroughGCandFinalization() {
System.gc();
System.gc();
System.runFinalization();
Copy link
Member

@pshipton pshipton Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@dmitripivkine Do we still need to call runFinaliization() and do the additional System.gc()? According to our doc update the finalization should occur from the single System.gc() now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dmitripivkine Do we still need to call runFinaliization() and do the additional System.gc()? According to our doc update the finalization should occur from the single System.gc() now.

In theory your statement is correct. However I don't know the reason why we call System.gc() and System.runFinalization() multiple times. I guess there is a reason. We can try to simplify this code (calling System.gc() twice?) and watch for intermittent failures.

pshipton
pshipton reviewed Oct 31, 2023
View reviewed changes
test/functional/VM_Test/src/j9vm/test/classunloading/ClassUnloadingTestParent.java
@@ -61,12 +61,10 @@ public void runTest() throws Exception {
}

protected static void thoroughGCandFinalization() {
System.gc();
System.gc();
System.runFinalization();
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as TestObject.java, do we still need to call runFinaliization() and do the additional System.gc()? According to our doc update the finalization should occur from the single System.gc() now.

dmitripivkine
dmitripivkine reviewed Oct 31, 2023
View reviewed changes
test/functional/VM_Test/src/com/ibm/j9/monitor/tests/LockNurseryTest.java Outdated
@@ -72,8 +72,6 @@ public static void generateGarbageAndCollect(){
}
System.out.println("Finished generating garbage, requesting gc");
System.gc();
System.gc();
System.gc();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should watch this case too. I don't know exact reason we called System.gc() three times. We do have race condition in Metronome when we can not guarantee Global GC execution even we call it twice.

Copy link
Member

Choose a reason for hiding this comment

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

This should retain two calls to System.gc()

@dmitripivkine
Copy link
Contributor

dmitripivkine commented Oct 31, 2023 •
edited
Loading

@pshipton What approach do you prefer? We can update all double and triple SystemGC() calls to do single call. However I am almost sure it will introduce number of highly intermittent failures. And we can address these failures by adjusting tests again. Alternatively we can do adjustments more accurate assuming current code is doing necessary operations. So, replace triple SystemGC() call to double, call extra runFinaliization() etc.

@pshipton
Copy link
Member

We can reduce the calls to what we think is the minimum required now and then monitor the tests. If System.gc() does finalization then do we just need to call System.gc() twice and remove the rest?

@pshipton
Copy link
Member

Or maybe this is the min for those two places.

System.gc();
System.runFinaliziation();
System.gc();
System.runFinalization();

@dmitripivkine
Copy link
Contributor

Or maybe this is the min for those two places.

System.gc(); System.runFinaliziation(); System.gc(); System.runFinalization();

Yes, there is what I am thinking about. New single call of SystemGC() includes two calls of GC and single runFinalization(). We can do replacement accordingly:

  • replace triple call of SystemGC() to double
  • in the case of System.gc(); System.gc(); System.runFinaliziation(); System.runFinaliziation(); replace it to System.gc(); System.runFinaliziation(); etc.

@pshipton
Copy link
Member

pshipton commented Nov 1, 2023

@flooxo pls make the changes as described in #18339 (comment)

@flooxo
Copy link
Contributor Author

flooxo commented Nov 1, 2023

Sure, let me know if it is now as intended or if I understood something wrong :)

@llxia
Copy link
Contributor

llxia commented Nov 6, 2023

@pshipton @dmitripivkine could you take a second look? Thanks

@llxia
Copy link
Contributor

llxia commented Nov 6, 2023

jenkins test sanity,extended xlinux jdk8,jdk11

@llxia
Copy link
Contributor

llxia commented Nov 7, 2023 •
edited
Loading

rerun:
https://openj9-jenkins.osuosl.org/job/Test_openjdk8_j9_sanity.functional_x86-64_linux_Personal_testList_1/212/
https://openj9-jenkins.osuosl.org/job/Test_openjdk8_j9_extended.functional_x86-64_linux_Personal/38/
https://openj9-jenkins.osuosl.org/job/Test_openjdk11_j9_extended.functional_x86-64_linux_Personal_testList_1/54/

@llxia llxia merged commit 0a0620e into eclipse-openj9:master Nov 8, 2023
3 of 7 checks passed
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

@dmitripivkine dmitripivkine dmitripivkine approved these changes

@pshipton pshipton pshipton approved these changes

Assignees
No one assigned
Labels
comp:test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove duplicate System.gc() in test
5 participants
@flooxo @llxia @hangshao0 @dmitripivkine @pshipton