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.ReferenceQueue<capture#1 of ?
super java.lang.Object> cannot be converted to
java.lang.ref.ReferenceQueue<java.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:

Reference<Object> 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 /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







Reply via email to