Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/28/2014 03:17 AM, David Holmes wrote:
On 27/01/2014 5: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
Seems somewhat odd given there is no supertype for Object but it is
consistent with the field declaration:
ReferenceQueue? super T queue;
The generics here is a little odd as we don't really know the type of
T we just play fast-and-loose by declaring:
ReferenceObject r;
Which only works because of erasure. I guess it wouldn't work to try
and use a simple wildcard '?' for both 'r' and 'q' as they would be
different captures to javac.
Yes, I tried that too and it results in even more unsafe casts.
It's odd yes, since the compile-time error is not present when building
via OpenJDK build system make files (using make images in top
directory for example) but only if I compile the class from command line
(using javac directly) or from IDEA. I use JDK 8 ea-b121 in all cases as
a build JDK. Are there any special options passed to javac for compiling
those classes in JDK build system that allow such code?
Regards, Peter
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...
I can be counted as the Reviewer. Kalyan can be listed as a reviewer.
Thanks Peter.
David
-
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
Re: [8] RFR (XS): 8032585: JSR292: IllegalAccessError when attempting to invoke protected method from different package
Chris, John, thank you for reviewing the fix. I'll proceed with choice #1 then. Filed a bug [1] to track cleanup activities. Best regards, Vladimir Ivanov [1] https://bugs.openjdk.java.net/browse/JDK-8032881 On 1/28/14 4:50 AM, John Rose wrote: This is safe as a point fix, since (a) Lookup.checkSymbolicClass vets 'refc' before checkMemberAccess is called, and (b) the JVMS does not in fact call for 'defc' to also be accessible, if it is inherited via 'refc'. There is a slightly different problem here, a code cleanup problem. The javadoc for checkMemberAccess does not seem to accurately reflect what the method does or how it works. We need to re-align the javadoc, the JVM spec., and (if necessary) the code. For example, I would prefer either an explicit call from checkMemberAccess to isClassAccessible, or at least have an assert in there. This will take a little more time and care to do right. I see two choices; either are OK with me as a reviewer: 1. Do the point fix as proposed and file a followup bug. 2. Fix the javadoc, add the assert, and re-review correspondence between checkMemberAccess (javadoc+code) and the JVM spec. for member access. — John On Jan 27, 2014, at 8:05 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8032585 JSR292 access verification logic refuses method handle lookup access to methods which are defined on inaccessible classes. This is usually correct, but in the corner case of inheritance through a public class, it is wrong. 8029507 makes the JVM provide more correct information about the defining class of a looked-up method and this corrected information is causing the old and wrong checks to fail where they didn't fail before. The fix is to relax the check: don't require the class where protected member is declared to be public. It is enough to check that defining class is a super class of the class lookup request comes from to ensure there are enough privileges to access protected member. Testing: regression test, enumeration tests on access checks, jdk/test/java/lang/invoke, vm.mlvm.testlist Thanks! Best regards, Vladimir Ivanov ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: RFR: 8022854 : (s) Cleaner re-initialization of System properties
Hi Mike, Your patch is slightly out of sync with 9: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/c8c4f441fc76 http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/src/share/classes/sun/misc/VM.java -- Generally looks OK. Bikeshed-wise i prefer something like getPublicSavedProperties rather than getSanitizedSavedProperties. Paul. On Jan 27, 2014, at 9:17 PM, Mike Duigou mike.dui...@oracle.com wrote: Hello all; This is a bit of cleanup I did back during Java 8 that got deferred due to it's late arrival during the development cycle. I've updated it for Java 9. http://cr.openjdk.java.net/~mduigou/JDK-8022854/0/webrev/ This change improves the implementation of System.setProperties(null) which restores the system properties to their boot time defaults. The existing semantics are preserved. Mike
hg: jdk8/tl/jdk: 8032585: JSR292: IllegalAccessError when attempting to invoke protected method from different package
Changeset: 56d05f260123 Author:vlivanov Date: 2014-01-28 13:46 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/56d05f260123 8032585: JSR292: IllegalAccessError when attempting to invoke protected method from different package Reviewed-by: twisti, jrose ! src/share/classes/sun/invoke/util/VerifyAccess.java + test/java/lang/invoke/ProtectedMemberDifferentPackage/Test.java + test/java/lang/invoke/ProtectedMemberDifferentPackage/p1/T2.java + test/java/lang/invoke/ProtectedMemberDifferentPackage/p2/T3.java
Re: RFR: 8022854 : (s) Cleaner re-initialization of System properties
On 27/01/2014 20:17, Mike Duigou wrote: Hello all; This is a bit of cleanup I did back during Java 8 that got deferred due to it's late arrival during the development cycle. I've updated it for Java 9. http://cr.openjdk.java.net/~mduigou/JDK-8022854/0/webrev/ This change improves the implementation of System.setProperties(null) which restores the system properties to their boot time defaults. The existing semantics are preserved. Mike I agree with Paul on the naming, getSanitizedSavedProperties is a bit odd (but not a big deal as it's internal). On the wording then reloaded is better than is forgotten but I wonder if it could be a bit clearer, like reset to the default system properties. On the test then I remember Erik's late change to fix a build issue [1]. He added a simple test as part of that and maybe we should remove that one now and maybe beef up your test to check the properties listed System.getProperties's table are present. A minor comment is that the date in the header suggests you wrote this test in 2013. Also just to keep things consistent then you might want a space in if(. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2437ccbf3504
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 28/01/2014 08:44, Peter Levart wrote: Yes, I tried that too and it results in even more unsafe casts. It's odd yes, since the compile-time error is not present when building via OpenJDK build system make files (using make images in top directory for example) but only if I compile the class from command line (using javac directly) or from IDEA. I use JDK 8 ea-b121 in all cases as a build JDK. Are there any special options passed to javac for compiling those classes in JDK build system that allow such code? jdk/make/Setup.gmk has the -Xlint options that are used in the build but I suspect it more than that all the classes in java/lang/ref are compiled together. -Alan
Re: RFR: 8022854 : (s) Cleaner re-initialization of System properties
On 1/28/14 4:06 AM, Paul Sandoz wrote: Hi Mike, Your patch is slightly out of sync with 9: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/c8c4f441fc76 This was the changeset I pushed yesterday causing this and removed sun.lang.ClassLoader.allowArraySyntax. Sorry for the timing. http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/src/share/classes/sun/misc/VM.java -- Generally looks OK. Bikeshed-wise i prefer something like getPublicSavedProperties rather than getSanitizedSavedProperties. Would getDefaultSystemProperties() be better? I never like the savedProps name and the related method names that I added and thanks for doing the clean up. Mandy Paul. On Jan 27, 2014, at 9:17 PM, Mike Duigou mike.dui...@oracle.com wrote: Hello all; This is a bit of cleanup I did back during Java 8 that got deferred due to it's late arrival during the development cycle. I've updated it for Java 9. http://cr.openjdk.java.net/~mduigou/JDK-8022854/0/webrev/ This change improves the implementation of System.setProperties(null) which restores the system properties to their boot time defaults. The existing semantics are preserved. Mike
hg: jdk8/tl/hotspot: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: ce0320cdb075 Author:jeff Date: 2014-01-28 20:09 + URL: http://hg.openjdk.java.net/jdk8/tl/hotspot/rev/ce0320cdb075 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl/corba: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: 6d40c0d49c7a Author:jeff Date: 2014-01-28 20:09 + URL: http://hg.openjdk.java.net/jdk8/tl/corba/rev/6d40c0d49c7a 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl/jaxws: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: 2b44c111e153 Author:jeff Date: 2014-01-28 20:09 + URL: http://hg.openjdk.java.net/jdk8/tl/jaxws/rev/2b44c111e153 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl/jdk: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: 72d0cc723560 Author:jeff Date: 2014-01-28 20:10 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/72d0cc723560 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl/langtools: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: afa91c54ff00 Author:jeff Date: 2014-01-28 20:10 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/afa91c54ff00 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl/nashorn: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: d3b293a4d554 Author:jeff Date: 2014-01-28 20:10 + URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/d3b293a4d554 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl: 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8
Changeset: 4f590c2cec75 Author:jeff Date: 2014-01-28 20:09 + URL: http://hg.openjdk.java.net/jdk8/tl/rev/4f590c2cec75 8032816: THIRDPARTYREADME LittleCMS preamble missing JRE 8 JDK 8 Reviewed-by: lana ! THIRD_PARTY_README
hg: jdk8/tl/jdk: 8032697: Issues with Lambda
Changeset: e385bd6f7338 Author:rfield Date: 2014-01-28 17:23 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e385bd6f7338 8032697: Issues with Lambda Reviewed-by: ahgross, briangoetz, dlsmith, rfield Contributed-by: daniel.sm...@oracle.com ! src/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java + test/java/lang/invoke/lambda/T8032697.java + test/java/lang/invoke/lambda/T8032697_anotherpkg/T8032697_A.java
RFR(s): 8023541 Race condition in rmid initialization
Hi all, Please review this fix to a race condition in rmid initialization. Briefly, rmid subclasses the RMI registry implementation and provides special handling for its own stub. Unfortunately the registry is exported in the super() call, making remote calls possible before rmid's stub initialization is complete. The fix is to ensure that all remote calls wait for initialization before proceeding. Bug: https://bugs.openjdk.java.net/browse/JDK-8023541 Webrev: http://cr.openjdk.java.net/~smarks/reviews/8023541/webrev.0/ Thanks, s'marks
Re: RFR: JDK-8032050: TEST_BUG: java/rmi/activation/Activatable/shutdownGracefully/ShutdownGracefully.java fails intermittently
Hi Tristan, I don't want to put the workaround into ActivationLibrary.rmidRunning() for a null return from the lookup, because this is only a workaround for an actual bug in rmid initialization. See the review I just posted for JDK-8023541. Adding JavaVM.waitFor(timeout) is something that would be useful in general, but it needs to be handled carefully. It uses the new Process.waitFor(timeout, unit) which is new in Java SE 8; this makes backporting to 7 more complicated. Not clear whether we'll do so, but I don't want to forclose the opportunity without discussion. It's also not clear how one can get the vm's exit status after JavaVM.waitFor() has returned true. With the Process API it's possible simply to call waitFor() or exitValue(). With JavaVM, a new API needs to be created, or the rule has to be established that one must call JavaVM.waitFor() to collect the exit status as well as to close the pipes from the subprocess. If JavaVM.waitFor(timeout, unit) is called without subsequently calling JavaVM.waitFor(), the pipes are leaked. In ShutdownGracefully.java, the finally-block needs to check to see if rmid is still running, and if it is, to shut it down. Simply calling waitFor(timeout, unit) isn't sufficient, because if the rmid process is still running, it will be left running. The straightforward approach would be to call ActivationLibrary.rmidRunning() to test if it's still running. Unfortunately this isn't quite right, because rmidRunning() has a timeout loop in it -- which should probably be removed. (I think there's a bug for this.) Another approach would be simply to call rmid.destroy(). This calls rmid's shutdown() method first, which is reasonable, but I'm not sure it kills the process if that fails. In any case, this already has a timeout loop waiting for the process to die, so ShutdownGracefully.java needn't use a new waitFor(timeout, unit) call. Removing the commented-out code that starts with no longer needed is good, and removing the ShutdownDetectThread is also good, since that's unnecessary. There are some more cleanups I have in mind here but I'd like to see a revised webrev before proceeding. Thanks, s'marks On 1/25/14 8:57 PM, Tristan Yan wrote: Hi Stuart Thank you for your review and suggestion. Yes, since this failure mode is very hard to be reproduced. I guess it's make sense to do some hack. And I also noticed in ActivationLibrary.rmidRunning. It does try to look up ActivationSystem but doesn't check if it's null. So I add the logic to make sure we will look up the non-null ActivationSystem. Also I did some cleanup if you don't mind. Add a waitFor(long timeout, TimeUnit unit) for JavaVM. Which we can have a better waitFor control. I appreciate you can review the code again. http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.01/ Thank you Tristan On 01/25/2014 10:20 AM, Stuart Marks wrote: On 1/23/14 10:34 PM, Tristan Yan wrote: Hi All Could you review the bug fix for JDK-8032050. http://cr.openjdk.java.net/~tyan/JDK-8032050/webrev.00/ Description: This rare happened failure caused because when RMID starts. It don't guarantee sun.rmi.server.Activation.startActivation finishes. Fix by adding a iterative getSystem with a 5 seconds timeout. Hi Tristan, Adding a timing/retry loop into this test isn't the correct approach for fixing this test. The timing loop isn't necessary because there is already a timing loop in RMID.start() in the RMI test library. (There's another timing loop in ActivationLibrary.rmidRunning() which should probably be removed.) So the intent of this library call is that it start rmid and wait for it to become ready. That logic doesn't need to be added to the test. In the bug report JDK-8032050 you had mentioned that the NullPointerException was suspicious. You're right! I took a look and it seemed like it was related to JDK-8023541, and I added a note to this effect to the bug report. The problem here is that rmid can come up and transiently return null instead of the stub of the activation system. That's what JDK-8023541 covers. I think that rmid itself needs to be fixed, though modifying the timing loop in the RMI test library to wait for rmid to come up *and* for the lookup to return non-null is an easy way to fix the problem. (Or at least cover it up.) The next step in the analysis is to determine, given that ActivationLibrary.getSystem can sometimes return null, whether this has actually caused this test failure. This is pretty easy to determine; just hack in a line system = null in the right place and run the test. I've done this, and the test times out and the output log is pretty much identical to the one in the bug report. (I recommend you try this yourself.) So I think it's fairly safe to say that the problem in JDK-8023541 has caused the failure listed in JDK-8032050. I can see a couple ways to proceed here. One way is just to close this out as a duplicate of
Re: RFR(s): 8023541 Race condition in rmid initialization
Hi Stuart Should variable initialized be volatile here? Otherwise looks good. Thank you Tristan On Jan 29, 2014, at 2:51 PM, Stuart Marks stuart.ma...@oracle.com wrote: Hi all, Please review this fix to a race condition in rmid initialization. Briefly, rmid subclasses the RMI registry implementation and provides special handling for its own stub. Unfortunately the registry is exported in the super() call, making remote calls possible before rmid's stub initialization is complete. The fix is to ensure that all remote calls wait for initialization before proceeding. Bug: https://bugs.openjdk.java.net/browse/JDK-8023541 Webrev: http://cr.openjdk.java.net/~smarks/reviews/8023541/webrev.0/ Thanks, s'marks
