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
8254799: runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java fails with release VMs #673
Conversation
…ils with release VMs
/issue add JDK-8254799 |
|
@DamonFool This issue is referenced in the PR title - it will now be updated. |
@DamonFool |
@DamonFool The |
Webrevs
|
For test requires debug version, please use "@requires vm.debug" in test. |
@DamonFool This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 13 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
Mailing list message from David Holmes on hotspot-runtime-dev: Hi Yumin, On 15/10/2020 10:16 am, Yumin Qi wrote:
I disagree with that approach. The test is run for any release and this What Jie has done seems the best solution to me - a logical equivalent of DEBUG_ONLY(-XX:-VerifyDependencies) Cheers, |
The original fix has problem, don't know how it went through test! Disabling this flag is to reduce run time in debug. I am OK with the workaround though it looks strange. |
Maybe use Platform.isDebugBuild()? Anyway up to Jie's choice. |
"-XX:+IgnoreUnrecognizedVMOptions", | ||
"-XX:-VerifyDependencies", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding "-XX:+IgnoreUnrecognizedVMOptions", you can
check the JDK type like this:
String jdkType = System.getProperty("jdk.debug", "release");
boolean addNonReleaseOptions = false;
if (!jdkType.equals("release")) {
addNonReleaseOptions = true;
}
and then only include the "-XX:-VerifyDependencies" option
when addNonReleaseOptions
is true... I'm not sure how to
do optional parameters with ProcessTools.createJavaProcessBuilder().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about Platform.isDebugBuild()
...
Mailing list message from David Holmes on hotspot-runtime-dev: On 15/10/2020 10:48 am, Daniel D.Daugherty wrote:
I think this is a trivial issue being over engineered. :) We run many Cheers, |
Mailing list message from Daniel D. Daugherty on hotspot-runtime-dev: On 10/14/20 8:59 PM, David Holmes wrote:
My understanding is that we are trying to stop using Dan
|
Mailing list message from David Holmes on hotspot-runtime-dev: On 15/10/2020 11:01 am, Daniel D. Daugherty wrote:
I don't recall that. But in this case it is not an issue at all. We are using I suppose a trivial change for Jie would be: - "-XX:-VerifyDependencies", Cheers,
|
I like @dholmes-ora 's solution and will update the PR soon. |
Unfortunately, the following fix doesn't work.
It seems that ProcessTools.createJavaProcessBuilder doesn't allow empty string as an arg. Would you mind something like:
If you don't like it, I still prefer -XX:+IgnoreUnrecognizedVMOptions. |
Mailing list message from David Holmes on hotspot-runtime-dev: On 15/10/2020 12:06 pm, Jie Fu wrote:
Well that is annoying - not quite sure how that arises but anyway ...
Just use "-Dx" as the dummy argument. Thanks, |
Hi, Jie, David and Dan, I would like to reorg the code a little bit: `index a889ae11ed1..596ca348c30 100644 import java.io.File; public class TestHeapDumpOnOutOfMemoryError { @@ -64,22 +66,29 @@ public class TestHeapDumpOnOutOfMemoryError {
What do you think? |
Aha, the diff looks ugly. Basically ProcessTools.createJavaProcessBuilder(List<?>) so you can manage the code there. |
Hi @yminqi |
Hi all, I prefer David's solution because I think it's a good idea to avoid -XX:+IgnoreUnrecognizedVMOptions without the effort to reorg the code. I've updated the PR. Best regards, |
Sorry for the trouble my change caused. :(
It would have been fine also to just run this test on debug only, or even just to remove the option; the speedup that the reduced MaxMetaspaceSize brought would have been enough.
Thanks a lot for fixing this.
I wondered how it got past gatekeeper tests but I see now that it was excluded from tier1, probably exactly because the slow runtime.
Hi @dcubed-ojdk , |
Thumbs up on this version.
I would have used "-Dignored_option" so it was self documenting,
but that's me... "-Dx" works just fine.
Thanks @yminqi , @dholmes-ora , @dcubed-ojdk and @tstuefe for your helpful comments and review. |
@DamonFool Since your change was applied there have been 13 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit f3ce45f. |
runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java and runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryErrorInMetaspace.java fail with release VMs due to VerifyDependencies is develop and is available only in debug version of VM.
-XX:+IgnoreUnrecognizedVMOptions is added to fix it.
Progress
Testing
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/673/head:pull/673
$ git checkout pull/673