Hi all,
I'm waiting some reviews from (R)eviewer.
And here's a new webrev which includes Chris' comments.
- Move fflush() outside of 'if' statement at jni.cpp.
- Remove #ifdef _WIN32
webrev:
http://cr.openjdk.java.net/~sangheki/8139801/webrev.02/
Thanks,
Sangheon
On 10/20/2015 05:17 PM, sangheon.kim wrote:
Hi Chris,
Thank you for reviewing this.
My answers are below.
On 10/20/2015 04:35 PM, Chris Plummer wrote:
Hi Sangheon,
Thanks for doing this. A few comments/suggestions:
I don't think the #ifdef __WIN32__ should be needed. Although we are
doing the fflush mainly to address issues we only currently see on
win32, there's no reason why the fflush can't be done on all
platforms. I think that will help keep the code cleaner.
From cleaner code point of view, I totally agree with you.
Let me hear others opinion on this.
In jni.cpp, is there a reason not to flush unconditionally (outside
the if/else)?
Hmm.
I thought we will not have strings to be flushed from the create_vm()
success case.
But I think I have to move outside of 'if statement'.
Please make sure your commit comment references all 3 CRs this fix is
addressing:
8139374 TlabSize.java argument test fails
8078328 AppCDS jtreg test result missing expected error message in
stderr
8139801 Error message from validation check has wrong order on Windows
Okay.
Thanks,
Sangheon
I'm not a "R"eviewer.
thanks,
Chris
On 10/20/15 3:03 PM, sangheon.kim wrote:
Hi all,
Initially I only tested validation error messages but later I
noticed that other error messages also have same problem on Windows.
(thanks to Chris Plummer for the discussion)
When we fail to create VM, we end with 1) returning to the caller or
2) call vm_abort().
For the 1) case, all error messages have wrong order on Windows.
eg) an error message on Windows. (this is NG)
$ java -XX:VMOptionsFile=foo -XX:VMOptionsFile=bar
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
Only one VM Options file is supported on the command line <- this
should be printed first.
We can see easily this wrong order on Windows but the problem is
there's no guarantee that 'stdout/stderr' will be flushed on that
situation.
As a solution I added fflush(stdout) and fflush(stderr) for above
two cases.
CR: https://bugs.openjdk.java.net/browse/JDK-8139801
Webrev: http://cr.openjdk.java.net/%7Esangheki/8139801/webrev.01/
Testing: JPRT
Thanks,
Sangheon
On 10/19/2015 01:00 PM, sangheon.kim wrote:
Hi all again,
Let me cancel this patch as this kind of problem seems general on
Windows.
Thanks,
Sangheon
On 10/19/2015 08:56 AM, sangheon.kim wrote:
Hi all,
Can I get some reviews for this change of adding 'fflush()' on
validation message print?
The order of 'validation error message' and 'vm exit message' is
wrong on Windows.
- Windows is buffering the validation error message which is sent
to 'stderr'.
- 'stderr' from 'jvm.dll' is flushed later than 'stderr' from the
caller of 'jvm.dll'.
eg) Expected message order (all other platforms except Windows)
$ java -XX:MinTLABSize=1 -version
1) MinTLABSize (1) must be greater than or equal to reserved area
in TLAB (16)
2) Error: Could not create the Java Virtual Machine. \n Error: A
fatal exception has occurred. Program will exit.
On Windows:
$ java -XX:MinTLABSize=1 -version
2) Error: Could not create the Java Virtual Machine. \n Error: A
fatal exception has occurred. Program will exit.
1) MinTLABSize (1) must be greater than or equal to reserved area
in TLAB (16)
As a solution, I added 'fflush()' on print function of validation
check (CommandLineError::print).
And any other functions that are printing to 'stderr' would have
similar fix for Windows.
CR: https://bugs.openjdk.java.net/browse/JDK-8139801
Webrev: http://cr.openjdk.java.net/~sangheki/8139801/webrev.00/
Testing: JPRT
Thanks,
Sangheon
