RFR for JDK-8028711: TEST_BUG: Tests should pass through VM options: corelibs tests
Hi,
Please review the following code changes for
https://bugs.openjdk.java.net/browse/JDK-8028711
The bug and fix are for enforcing the following rules:
launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS} should
be passed in.
launch javac from shell script, ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS}
should be passed in.
So almost all the scripts who failed to follow above rules are touched
by this fix.
There are very few exceptions since test cases explicitly state that
they are intended to launch java without any vm options.
webrev
http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/
Thanks,
Michael Cui
Re: RFR for JDK-8028711: TEST_BUG: Tests should pass through VM options: corelibs tests
On 26/01/2014 08:10, michael cui wrote:
Hi,
Please review the following code changes for
https://bugs.openjdk.java.net/browse/JDK-8028711
The bug and fix are for enforcing the following rules:
launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS}
should be passed in.
launch javac from shell script, ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS}
should be passed in.
So almost all the scripts who failed to follow above rules are touched
by this fix.
There are very few exceptions since test cases explicitly state that
they are intended to launch java without any vm options.
webrev
http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/
You might be aware of this area but we've had two previous efforts ([1]
[2]) to update these tests to pass through options. I have to admit that
I wasn't aware of TESTJAVAOPTS or TESTJAVACOPTS. Looking at the jtreg
-help all page then these seem to correspond to -javaoption and
-javacoption. I see -vmoption used all the time to specify options, I
don't think I've seen -javaoption being used but I see your point. One
thing that might be helpful here is some guidance or examples as it
confusing to have so many options that initially appear to do the same
thing.
Mike Duigou might want to have a quick look to see if jdk/Makefile needs
to be updated to support -javacoption. As these tests aren't really
testing javac (except incidentally) then it might not be important but
it's yet another place where you can loose a finger.
I haven't looked at all the files but just notice that you've updated
the genXXX scripts. They are used to generate tests, they aren't tests
themselves and so aren't run by jtreg.
So are you planning to also update the java tests that create VMs to
pass through the options? This point came up during previous efforts
(and some test infrastructure has been added) but it isn't clear to me
where we are on that.
-Alan.
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae5d04dbacd6
[2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7da291690aa0
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/25/2014 05:35 AM, srikalyan chandrashekar wrote:
Hi Peter, if you are a committer would you like to take this further
(OR) perhaps david could sponsor this change.
Hi,
Here's new webrev that takes into account Kaylan's and David's review
comments:
cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.02/
I changed into using Class.forName() instead of Unsafe for class
preloading and initialization just to be on the safe side regarding
unwanted premature initialization of Unsafe class. I also took the
liberty of removing an unneeded semicolon (line 114) and fixing a JDK 8
compile time error in generics (line 189):
incompatible types: java.lang.ref.ReferenceQueuecapture#1 of ?
super java.lang.Object cannot be converted to
java.lang.ref.ReferenceQueuejava.lang.Object
I re-ran the java/lang/ref tests and they pass.
Can I count you as a reviewer, Kalyan? If I get a go also from David,
I'll commit this to jdk9/dev...
Regards, Peter
--
Thanks
kalyan
On 1/24/14 4:05 PM, Peter Levart wrote:
On 01/24/2014 02:53 AM, srikalyan chandrashekar wrote:
Hi David, yes thats right, only benefit i see is we can avoid
assignment to 'r' if pending is null.
Hi Kalyan,
Good to hear that test runs without failures so far.
Regarding assignment of 'r'. What I tried to accomplish with the
change was eliminate double reading of 'pending' field. I have a
mental model of local variable being a register and field being a
memory location. This may be important if the field is volatile, but
for normal fields, I guess the optimizer knows how to compile such
code most optimally in either case. The old (your) version is better
from logical perspective, since it guarantees that dereferencing the
'r', wherever it is possible, will never throw NPE (dereferencing
where 'r' is not assigned is not possible because of definitive
assignment rules). So I support going back to your version...
Regards, Peter
--
Thanks
kalyan
On 1/23/14 4:33 PM, David Holmes wrote:
On 24/01/2014 6:10 AM, srikalyan wrote:
Hi Peter, i have modified your code from
r = pending;
if (r != null) {
..
TO
if (pending != null) {
r = pending;
This is because the r is used later in the code and must not be
assigned
pending unless it is not null(this was as is earlier).
If r is null, because pending is null then you perform the wait()
and then continue - back to the top of the loop. There is no bug in
Peter's code.
The new webrev is
posted here
http://cr.openjdk.java.net/~srikchan/Regression/JDK-8022321_OOMEInReferenceHandler-webrev-V2/
. I ran a 1000 run and no failures so far, however i would like to
run a
couple more 1000 runs to assert the fix.
PS: The description section of JEP-122
(http://openjdk.java.net/jeps/122) says meta-data would be in native
memory(not heap).
The class_mirror is a Java object not meta-data.
David
--
Thanks
kalyan
Ph: (408)-585-8040
On 1/21/14, 2:31 PM, Peter Levart wrote:
On 01/21/2014 07:17 PM, srikalyan wrote:
Hi Peter/David, catching up after long weekend. Why would there
be an
OOME in object heap due to class loading in perm gen space ?
The perm gen is not a problem her (JDK 8 does not have it and we see
OOME on JDK8 too). Each time a class is loaded, new java.lang.Class
object is allocated on heap.
Regards, Peter
Please correct if i am missing something here. Meanwhile i will
give
the version of Reference Handler you both agreed on a try.
--
Thanks
kalyan
Ph: (408)-585-8040
On 1/21/14, 7:24 AM, Peter Levart wrote:
On 01/21/2014 07:54 AM, Peter Levart wrote:
*[Loaded sun.misc.Cleaner from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]*
[Loaded java.io.ByteArrayInputStream from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$ZoneOffsetTransitionRule
from /home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
...
I'm on linux, 64bit and using official EA build 121 of JDK 8...
But if I try with JDK 7u45, I don't see it.
So what changed between JDK 7 and JDK 8?
I suspect the following: 8007572: Replace existing jdk timezone
data
at java.home/lib/zi with JSR310's tzdb
Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 1/26/14 11:07 AM, Peter Levart wrote:
On 01/25/2014 05:35 AM, srikalyan chandrashekar wrote:
Hi Peter, if you are a committer would you like to take this further
(OR) perhaps david could sponsor this change.
Hi,
Here's new webrev that takes into account Kaylan's and David's review
comments:
cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.02/
I changed into using Class.forName() instead of Unsafe for class
preloading and initialization just to be on the safe side regarding
unwanted premature initialization of Unsafe class. I also took the
liberty of removing an unneeded semicolon (line 114) and fixing a JDK
8 compile time error in generics (line 189):
incompatible types: java.lang.ref.ReferenceQueuecapture#1 of ?
super java.lang.Object cannot be converted to
java.lang.ref.ReferenceQueuejava.lang.Object
I re-ran the java/lang/ref tests and they pass.
Can I count you as a reviewer, Kalyan? If I get a go also from
David, I'll commit this to jdk9/dev...
Hi Peter, I do not have review rights. So it has to be someone else from
core-libs-dev.
Regards, Peter
--
Thanks
kalyan
--
Thanks
kalyan
On 1/24/14 4:05 PM, Peter Levart wrote:
On 01/24/2014 02:53 AM, srikalyan chandrashekar wrote:
Hi David, yes thats right, only benefit i see is we can avoid
assignment to 'r' if pending is null.
Hi Kalyan,
Good to hear that test runs without failures so far.
Regarding assignment of 'r'. What I tried to accomplish with the
change was eliminate double reading of 'pending' field. I have a
mental model of local variable being a register and field being a
memory location. This may be important if the field is volatile, but
for normal fields, I guess the optimizer knows how to compile such
code most optimally in either case. The old (your) version is better
from logical perspective, since it guarantees that dereferencing the
'r', wherever it is possible, will never throw NPE (dereferencing
where 'r' is not assigned is not possible because of definitive
assignment rules). So I support going back to your version...
Regards, Peter
--
Thanks
kalyan
On 1/23/14 4:33 PM, David Holmes wrote:
On 24/01/2014 6:10 AM, srikalyan wrote:
Hi Peter, i have modified your code from
r = pending;
if (r != null) {
..
TO
if (pending != null) {
r = pending;
This is because the r is used later in the code and must not be
assigned
pending unless it is not null(this was as is earlier).
If r is null, because pending is null then you perform the wait()
and then continue - back to the top of the loop. There is no bug
in Peter's code.
The new webrev is
posted here
http://cr.openjdk.java.net/~srikchan/Regression/JDK-8022321_OOMEInReferenceHandler-webrev-V2/
. I ran a 1000 run and no failures so far, however i would like
to run a
couple more 1000 runs to assert the fix.
PS: The description section of JEP-122
(http://openjdk.java.net/jeps/122) says meta-data would be in native
memory(not heap).
The class_mirror is a Java object not meta-data.
David
--
Thanks
kalyan
Ph: (408)-585-8040
On 1/21/14, 2:31 PM, Peter Levart wrote:
On 01/21/2014 07:17 PM, srikalyan wrote:
Hi Peter/David, catching up after long weekend. Why would there
be an
OOME in object heap due to class loading in perm gen space ?
The perm gen is not a problem her (JDK 8 does not have it and we
see
OOME on JDK8 too). Each time a class is loaded, new java.lang.Class
object is allocated on heap.
Regards, Peter
Please correct if i am missing something here. Meanwhile i will
give
the version of Reference Handler you both agreed on a try.
--
Thanks
kalyan
Ph: (408)-585-8040
On 1/21/14, 7:24 AM, Peter Levart wrote:
On 01/21/2014 07:54 AM, Peter Levart wrote:
*[Loaded sun.misc.Cleaner from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]*
[Loaded java.io.ByteArrayInputStream from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$ZoneOffsetTransitionRule
from /home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
...
I'm on linux, 64bit and using official EA build 121 of JDK 8...
But if I try with JDK 7u45, I don't see it.
So what changed between JDK 7 and JDK 8?
I suspect the following: 8007572: Replace existing jdk
timezone data
at java.home/lib/zi with JSR310's tzdb
Regards, Peter
Re: RFR for JDK-8028711: TEST_BUG: Tests should pass through VM options: corelibs tests
On 01/26/2014 05:14 PM, Alan Bateman wrote:
On 26/01/2014 08:10, michael cui wrote:
Hi,
Please review the following code changes for
https://bugs.openjdk.java.net/browse/JDK-8028711
The bug and fix are for enforcing the following rules:
launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS}
should be passed in.
launch javac from shell script, ${TESTTOOLVMOPTS} and
${TESTJAVACOPTS} should be passed in.
So almost all the scripts who failed to follow above rules are
touched by this fix.
There are very few exceptions since test cases explicitly state that
they are intended to launch java without any vm options.
webrev
http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/
You might be aware of this area but we've had two previous efforts
([1] [2]) to update these tests to pass through options.
I am aware of efforts [1] JDK-8003890. Initially JDK-8028711 is opened
for double check the result of JDK-8003890 and fix them if there are
uncovered scripts.
Initial list scripts need to touch are :
java/lang/Thread/UncaughtExceptions.sh
sun/tools/common/ApplicationSetup.sh
sun/tools/jrunscript/jrunscript-argsTest.sh
sun/tools/jrunscript/jrunscript-cpTest.sh
sun/tools/jrunscript/jrunscript-DTest.sh
sun/tools/jrunscript/jrunscript-eTest.sh
sun/tools/jrunscript/jrunscript-fTest.sh
sun/tools/jrunscript/jrunscriptTest.sh
Later on, by reading http://openjdk.java.net/jtreg/vmoptions.html and
current last comments of JDK-8019502 (which is the parent of
JDK-8028711), also by exploring current existing usage (I mean large
part of scripts passing TESTJAVAOPTS or TESTJAVACOPTS.), I feel that we
might want to make the consistent usage of these two env variables as well.
I am not aware of efforts [2]. Thanks for the link.
I have to admit that I wasn't aware of TESTJAVAOPTS or TESTJAVACOPTS.
Looking at the jtreg -help all page then these seem to correspond to
-javaoption and -javacoption. I see -vmoption used all the time to
specify options, I don't think I've seen -javaoption being used but I
see your point. One thing that might be helpful here is some guidance
or examples as it confusing to have so many options that initially
appear to do the same thing.
I just attached a very simple example to show the behaviours described
at http://openjdk.java.net/jtreg/vmoptions.html.
Sample can download it from
https://bugs.openjdk.java.net/secure/attachment/18467/sample.zip
My understanding of options is :
When we start jtreg with -vmoptions , jtreg will set its value to
TESTVMOPTS and TESTTOOLVMOPTS before running shell script test (shell
scripts identified by jtreg tags @test and @run shell ). So if we call
java with TESTVMOPTS and call javac with TESTTOOLVMOPTS in shell script,
then it impact to both.
If we want to set some options only for launching java without impact
other javac or other java tools, then we need to use -javaoptions with
jtreg. (Precondition : TESTJAVAOPTS was passed)
Similarly, -javacoptions will only impact launching javac from all
shell scripts. (Precondition : TESTJAVACOPTS was passed)
It would be less confusing if we could keep use them in a consistent way.
Mike Duigou might want to have a quick look to see if jdk/Makefile
needs to be updated to support -javacoption. As these tests aren't
really testing javac (except incidentally) then it might not be
important but it's yet another place where you can loose a finger.
I haven't looked at all the files but just notice that you've updated
the genXXX scripts. They are used to generate tests, they aren't tests
themselves and so aren't run by jtreg.
Then I will rollback those changes. I found three scripts belongs to
these category.
test/java/nio/Buffer/genBasic.sh
test/java/nio/Buffer/genCopyDirectMemory.sh
test/java/util/Formatter/genBasic.sh
So are you planning to also update the java tests that create VMs to
pass through the options? This point came up during previous efforts
(and some test infrastructure has been added) but it isn't clear to me
where we are on that.
I don't plan to cover that in this bug fix. I though it will be covered
by JDK-8019502. I could work on that bug if you want me to cover that as
well.
-Alan.
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae5d04dbacd6
[2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7da291690aa0
-Michael Cui
