Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/01/2013 10:11 PM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: In Finalizer.java, at line 97 you look up the JavaLangAccess object every single time. Is it worth caching that earlier, maybe when the finalize thread starts, since it will never change? I was expecting that would get optimized during runtime and it's a simple getter method. It's a good suggestion to cache it at the finalize thread start time and here is the revised webrev: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.01/ Thanks for the review. Mandy Hi Mandy, FWIW, I don't know if you gain anything by caching static field in a static field, performance wise.It looks nicer at use site though. If performance was really extremely important in this use case (which I think is not) then jla should be a local variable inside FinalizerThread.run() (and possibly in the anonymous Runnables too) and passed as an argument to Finalizer.runFinalizer(). Looking at code once more, I see a possible data race. Is it remotely possible, that either Finalizer.runFinalization() or Finalizer.runAllFinalizers() is executed and new secondary finalizer thread spawned and executed before FinalizerThread manages to call ensureAccessAvailable(), therefore executing the Finalizer.runFinalizer() methods before static Finalizer.jla is initialized? Even though jla field was initialized, what guarantees it's value being visible in spawned secondary finalizer threads? FinalizerThread is started in Finalizer's static initializer, yes, but ensureAccessAvailable() is called by FinalizerThread after it starts... Considering all this, it might be safer to just ready the SharedSecrets field every time it's needed. Regards, Peter
Assorted annotation optimizations
Hi, I propose a set of straightforward optimizations of annotations in the field of minimizing garbage creation and execution performance. Here's a webrev containing the changes: http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationOptimizations/webrev.01/ Changes grouped by class/method: AnnotationInvocationHandler.invoke(): Use reference comparison among interned strings instead of equals() to dispatch the invocation and use Method.getParameterCount() instead of Method.getParameterTypes() to avoid creating Class[] array for each invocation. AnnotationInvocationHandler.toStringImpl(): Use StringBuilder instead of StringBuffer AnnotationInvocationHandler.equalsImpl(): Don't call asOneOfUs(o) in a loop for each member and restructure the code to have basically two independent implementations: the one based on memberValues map if the other annotation is OneOfUs (the common case) and the one based on AnnotationType.members() map of member methods. Caching of member methods privately in each AnnotationInvocationHandler instance is not needed - use the AnnotationType.members() map instead - this improves annotation instance footprint too. AnnotationInvocationHandler.getMemberValue(): new static package-private method used in AnnotationSupport for fast retrieval of shared value array in container annotations. AnnotationSupport.getDirectlyAndIndirectlyPresent(): minimize number of garbage objects and array copies. This method delegates to getSharedIndirectlyPresent() which might return a shared array which is only cloned if needed. AnnotationSupport.getSharedIndirectlyPresent(): private method instead of getIndirectlyPresent() that may return a shared array. AnnotationSupport.getSharedValueArray(): private method that may return a shared array. This method is based on new AnnotationInvocationHandler.getMemberValue() method and now also incorporates a call to checkTypes(). AnnotationSupport.getValueArray(): now based on getSharedValueArray() and cloneArray() AnnotationSupport.cloneArray(): new private method which clones the array AnnotationSupport.newArray(): new private constructor for annotation arrays (to localize @SuppressWarnings(unchecked)) AnnotationType constructor: member Methods are now all made setAccessible(true) in advance so that value member method for container annotations doesn't need to be made setAccessible(true) each time a container of a particular type is flattened. All in all these changes make common operations such as retrieving the annotation member values and obtaining repeatable annotations faster and with less produced garbage objects. Here are the results of some micro-benchmarks: http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationOptimizations/test/perf_results.txt These results are obtained with the following test: http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationOptimizations/test/AnnotationDispatchPerfTest.java http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationOptimizations/test/si/pele/microbench/TestRunner.java In short: - Invoking Annotation.annotationType() is 6x faster - Retrieving a scalar member value is 2x faster - Annotation.equals for annotation with 3 int members is 2.5x faster - Class.getDeclaredAnnotationsByType for direct-only case is 2x faster - Class.getDeclaredAnnotationsByType for in-direct-only case is 2.2x faster - Class.getDeclaredAnnotationsByType for mixed case is 1.7x faster So what do you think? Regards, Peter
Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/04/2013 05:45 AM, Mandy Chung wrote:
On 11/3/2013 5:32 PM, David Holmes wrote:
Hi Mandy,
On 2/11/2013 7:11 AM, Mandy Chung wrote:
On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote:
2013/11/1 4:15 -0700, mandy.ch...@oracle.com:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/
Looks good.
Just one question: In Finalizer.java, at line 97 you look up the
JavaLangAccess object every single time. Is it worth caching that
earlier, maybe when the finalize thread starts, since it will never
change?
I was expecting that would get optimized during runtime and it's a
simple getter method. It's a good suggestion to cache it at the
finalize
thread start time and here is the revised webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.01/
I'm missing something basic - how did you get this to compile:
public void invokeFinalize(Object o) throws Throwable {
o.finalize();
}
given finalize is protected ??
protected members can be accessed by the same package and subclasses.
This is the implementation of JavaLangAccess in java.lang.System that
is in the same package as java.lang.Object.
Also VM.awaitBooted seems inherently risky as a general method as you
would have to make sure that it is never called by the main VM
initialization thread. Perhaps handle this in
sun.misc.SharedSecrets.getJavaLangAccess so it is less 'general'?
That sounds a good idea. Let me think about it and get back to this.
That said I think Peter may be right that there could be races with
agents triggerring explicit finalization requests early in the VM
initialization process - which means any blocking operation dependent
on other parts of the initialization sequence could be problematic.
Hmm... agents calling System.runFinalization during startup - like
Alan described, the agent is playing fire.
Hi Mandy,
Isn't System.runFinalization() just a hint? Like System.gc() for
example...
* p
* Calling this method suggests that the Java Virtual Machine expend
* effort toward running the codefinalize/code methods of objects
* that have been found to be discarded but whose codefinalize/code
* methods have not yet been run. When control returns from the
* method call, the Java Virtual Machine has made a *best effort* to
* complete all outstanding finalizations.
* p
Couldn't the request just be ignored when called before VM.isBooted() ?
The finalizers will be executed nevertheless asynchronously later by the
finalizer thread...
Regards, Peter
The potential issue that could happen is that during the VM
initialization the heap is so small that triggers GC and also the
startup code has finalizers and those objects with finalizers are
awaiting for finalization in order for the sufficient memory to be
freed up. The VM initialization couldn't get completed and the
Finalizer thread is blocked and thus due to insufficient memory,
eventually it would get out of memory. An agent instrumenting classes
early in the startup and creates lots of objects and finalizers, that
might also cause problem.
I think it's good to have the secondary finalizer thread to call
ensureAccessAvailable (with some modification to ensure jla is
initialized).
Overall I think a safer approach may be to fix the native JNI code so
that if it gets a private version of finalize() it looks up the
method in the superclass.
There is other issue (e.g. static method with same name/descriptor)
that JNI_GetMethodID has to resolve. This will be a bigger change in
the VM that probably can't make jdk8.
I think the proposed patch with slight change in the secondary
finalizer thread is a relative safe approach (I wil revise the patch
and send out another rev tomorrow).
thanks
Mandy
Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/04/2013 07:52 AM, Peter Levart wrote:
Also VM.awaitBooted seems inherently risky as a general method as
you would have to make sure that it is never called by the main VM
initialization thread. Perhaps handle this in
sun.misc.SharedSecrets.getJavaLangAccess so it is less 'general'?
That sounds a good idea. Let me think about it and get back to this.
That said I think Peter may be right that there could be races with
agents triggerring explicit finalization requests early in the VM
initialization process - which means any blocking operation
dependent on other parts of the initialization sequence could be
problematic.
Hmm... agents calling System.runFinalization during startup - like
Alan described, the agent is playing fire.
Hi Mandy,
Isn't System.runFinalization() just a hint? Like System.gc() for
example...
* p
* Calling this method suggests that the Java Virtual Machine expend
* effort toward running the codefinalize/code methods of objects
* that have been found to be discarded but whose
codefinalize/code
* methods have not yet been run. When control returns from the
* method call, the Java Virtual Machine has made a *best effort* to
* complete all outstanding finalizations.
* p
Couldn't the request just be ignored when called before VM.isBooted()
? The finalizers will be executed nevertheless asynchronously later by
the finalizer thread...
...if you do it like the following:
/* Called by Runtime.runFinalization() */
static void runFinalization() {
if (VM.isBooted()) { // ignore requests early in the VM boot-up sequence
forkSecondaryFinalizer(new Runnable() {
private volatile boolean running;
public void run() {
if (running)
return;
running = true;
for (;;) {
Finalizer f = (Finalizer)queue.poll();
if (f == null) break;
f.runFinalizer();
}
}
});
}
}
*and* use the SharedSecrets.getJavaLangAccess() directly, the code also
passes the formal proof of having no data races, since you have the
following sequence:
boot-up thread:
write to SharedSecrets.javaLangAccess
volatile write to VM.booted
thread calling getJavaLangAccess():
volatile read from VM.booted
read from SharedSecrets.javaLangAccess
I still don't see any advantage of caching JavaLangAccess instance in a
private static Finalizer.jla field other than to complicate synchronization.
Regards, Peter
Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/04/2013 05:45 AM, Mandy Chung wrote:
That said I think Peter may be right that there could be races with
agents triggerring explicit finalization requests early in the VM
initialization process - which means any blocking operation dependent
on other parts of the initialization sequence could be problematic.
Hmm... agents calling System.runFinalization during startup - like
Alan described, the agent is playing fire.
The potential issue that could happen is that during the VM
initialization the heap is so small that triggers GC and also the
startup code has finalizers and those objects with finalizers are
awaiting for finalization in order for the sufficient memory to be
freed up. The VM initialization couldn't get completed and the
Finalizer thread is blocked and thus due to insufficient memory,
eventually it would get out of memory. An agent instrumenting classes
early in the startup and creates lots of objects and finalizers, that
might also cause problem.
I think it's good to have the secondary finalizer thread to call
ensureAccessAvailable (with some modification to ensure jla is
initialized).
Hi Mandy,
What about the following helper class in java.lang package:
package java.lang;
import sun.misc.VM;
import sun.reflect.CallerSensitive;
import sun.reflect.Reflection;
/**
* Access to protected Object methods. Only accessible to system classes.
*/
public final class ObjectAccess {
private static final ObjectAccess theInstance = new ObjectAccess();
@CallerSensitive
public static ObjectAccess getInstance() {
Class? caller = Reflection.getCallerClass();
if (!VM.isSystemDomainLoader(caller.getClassLoader()))
throw new SecurityException(ObjectAccess);
return theInstance;
}
public void finalizeObject(Object o) throws Throwable {
o.finalize();
}
public Object cloneObject(Object o) throws CloneNotSupportedException {
return o.clone();
}
}
...is such code permissible to be executed in the Finalizer's static
initializer even before VM.isBooted() ?
Regards, Peter
Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/04/2013 09:48 AM, Alan Bateman wrote:
On 04/11/2013 08:33, Peter Levart wrote:
:
What about the following helper class in java.lang package:
If public then it leaks into the Java SE API. I think using
SharedSecrets should be okay, assuming we can sort out any potential
races getting to JavaLangAccess.
-Alan.
The problem is that we would like Finalizers to be executed even before
VM.isBooted(). Currently this is possible, because native code is used
to access Object.finalize(). java.lang.Thread is initialized earlier,
but initializing SharedSecrets.javaLangAccess in Thread's static
initializer doesn't work (VM crashes). I think this is because
SharedSecrets also initialize sun.misc.Unsafe via .getUnsafe() method
and this needs the caller ClassLoader to check access... SharedSecrets
is already too encumbered to be used that early in boot-up sequence.
So what about the following simple class in sun.misc:
package sun.misc;
/**
* Access to protected Object methods.
*/
public abstract class ObjectAccess {
private static ObjectAccess theInstance;
public static ObjectAccess getInstance() {
return theInstance;
}
public static void setInstance(ObjectAccess instance) {
theInstance = instance;
}
public abstract void finalizeObject(Object o) throws Throwable;
}
...with the following initialization in java.lang.Thread:
public
class Thread implements Runnable {
/* Make sure registerNatives is the first thing clinit does. */
private static native void registerNatives();
static {
registerNatives();
*sun.misc.ObjectAccess.setInstance(new ObjectAccess() {**
**@Override**
**public void finalizeObject(Object o) throws Throwable {**
**o.finalize();**
**}**
**});*
}
I tried executing JVM with these two changes and is seems to work - i.e.
does not cause JVM to crash at startup.
Regards, Peter
Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/05/2013 03:21 AM, Mandy Chung wrote:
On 11/4/2013 6:03 PM, David Holmes wrote:
Hi Mandy,
This all seems quite reasonable to me.
Two minor nits:
1. The delay ntil typo in Finalizer.java is still present.
Missed that :(
2. In VM.java. booted need not be volatile now that it is only
accessed within a locked region. Also awaitBooted might as well be
void as it can only ever return true.
Fixed. Revised webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.03/
Hi Mandy,
Just a nit. You are assigning SharedSecrets.getJavaLangAccess() to a
local 'jla' variable declared outside Runnables, which makes it
effectively a final field. You could declare and assign the 'jla' inside
run() methods of both Runnables, just before the loop - like it is done
in the FinalizerThread...
Regards, Peter
P.S. I'm curious about the strange seemingly unneeded code fragments in
FinalizerThread and both Runnables. For example:
169 forkSecondaryFinalizer(new Runnable() {
170*private volatile boolean running;*
171 public void run() {
172*if (running)*
173*return;*
174*running = true;*
The FinalizerThread and each Runnable instance is only executed once.
Why these checks then? Does anybody know?
Regards, Peter
Thanks for the review.
Mandy
Thanks,
David
On 5/11/2013 6:04 AM, Mandy Chung wrote:
Thank you all for the suggestions.
Before the VM initialization is completed, any agent ought to be
careful
in what things it can do and what shouldn't do. I think it's
reasonable
to ignore System.runFinalization if it's called at early startup phase.
I'm unclear if there is any use case that we really require
finalization
to happen before VM is booted (I can imagine other problems may
happen).
I change the runFinalization and runAllFinalizers methods to return if
it's not booted and also change runFinalizer method to take
JavaLangAccess to simplify the synchronization instead of caching it in
the static field.
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.02/
David - I decided to leave VM.awaitBooted as it is. Having the
callsite
to call awaitBooted makes it explicit and clear that it's blocking and
we will keep SharedSecrets.getJavaLangAccess method consistent with the
other SharedSecrets methods that they are all getter methods. If any
future change calls SharedSecrets.getJavaLangAccess before it's booted,
it's a little easier to diagnose (NPE vs hangs).
Peter - thanks for the ObjectAccess idea. I don't think supporting
finalization to happen before VM is booted buys us anything and I would
rather keep this change simple.
Mandy
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
Hi Robert,
I think this fix is not complete. When one sets the system property
sun.reflect.noInflation=true, reflection proxy is still attempted to be
generated for anonymous classes (see
ReflectionFactory.newMethodAccessor/newConstructorAccessor).
I would also restructure the Method/Constructor accessor logic
differently. The check for ReflectUtil.isVMAnonymousClass() can be
performed just once (in the newMethodAccessor/newConstructorAccessor
methods) and based on this check, create accessor:
- for classic declaring class - as is / unchanged
- for anonymous declaring class - just create and return
NativeMethodAccessorImpl without a parent
Then in NativeMethodAccessorImpl (and same for constructor), modify the
inflation checking logic:
if (*parent != null *++numInvocations
ReflectionFactory.inflationThreshold()) {
MethodAccessorImpl acc = (MethodAccessorImpl)
new MethodAccessorGenerator().
generateMethod(method.getDeclaringClass(),
method.getName(),
method.getParameterTypes(),
method.getReturnType(),
method.getExceptionTypes(),
method.getModifiers());
parent.setDelegate(acc);
}
Regards, Peter
On 11/04/2013 07:12 PM, robert.fi...@oracle.com wrote:
Changeset: 51b002381b35
Author:rfield
Date: 2013-11-04 10:12 -0800
URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/51b002381b35
7194897: JSR 292: Cannot create more than 16 instances of an anonymous class
8027681: Lambda serialization fails once reflection proxy generation kicks in
Reviewed-by: ksrini, briangoetz, jfranck
Contributed-by: joel.fra...@oracle.com, brian.go...@oracle.com,
robert.fi...@oracle.com
! src/share/classes/sun/reflect/NativeConstructorAccessorImpl.java
! src/share/classes/sun/reflect/NativeMethodAccessorImpl.java
! src/share/classes/sun/reflect/misc/ReflectUtil.java
+ test/java/lang/invoke/lambda/RepetitiveLambdaSerialization.java
!
test/java/util/stream/test/org/openjdk/tests/java/lang/invoke/SerializedLambdaTest.java
+ test/sun/reflect/AnonymousNewInstance/ManyNewInstanceAnonTest.java
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
On 11/04/2013 07:12 PM, robert.fi...@oracle.com wrote:
Changeset: 51b002381b35
Author:rfield
Date: 2013-11-04 10:12 -0800
URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/51b002381b35
7194897: JSR 292: Cannot create more than 16 instances of an anonymous class
8027681: Lambda serialization fails once reflection proxy generation kicks in
Reviewed-by: ksrini, briangoetz, jfranck
Contributed-by: joel.fra...@oracle.com, brian.go...@oracle.com,
robert.fi...@oracle.com
! src/share/classes/sun/reflect/NativeConstructorAccessorImpl.java
! src/share/classes/sun/reflect/NativeMethodAccessorImpl.java
! src/share/classes/sun/reflect/misc/ReflectUtil.java
+ test/java/lang/invoke/lambda/RepetitiveLambdaSerialization.java
!
test/java/util/stream/test/org/openjdk/tests/java/lang/invoke/SerializedLambdaTest.java
+ test/sun/reflect/AnonymousNewInstance/ManyNewInstanceAnonTest.java
Hi Robert,
I also propose a much faster variant of:
+ /**
+ * Checks if {@code Class cls} is a VM-anonymous class
+ * as defined by {@link sun.misc.Unsafe#defineAnonymousClass}
+ * (not to be confused with a Java Language anonymous inner class).
+ */
+ public static boolean isVMAnonymousClass(Class? cls) {
+ return cls.getSimpleName().contains(/);
+ }
The following:
public static boolean isVMAnonymousClassFAST(Class? cls) {
String name = cls.getName();
for (int i = name.length() - 1; i = 0; i--) {
char c = name.charAt(i);
if (c == '.') return false;
if (c == '/') return true;
}
return false; // root package
}
It's about 12..25x faster for typical class names and doesn't produce
any garbage.
Regards, Peter
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
Hi John,
Speaking of names, the following test:
package pkg;
public class Test {
public static void main(String[] args)
{
Runnable r = () - {};
Class? c = r.getClass();
Class? ac = java.lang.reflect.Array.newInstance(c, 0).getClass();
System.out.println(c: + c.getName() + / +
c.getSimpleName());
System.out.println(ac: + ac.getName() + / +
ac.getSimpleName());
}
}
Prints:
c: pkg.Test$$Lambda$1/798154996 / Test$$Lambda$1/798154996
ac: [Lpkg.Test$$Lambda$1; / Test$$Lambda$1/798154996[]
I think the array class name is missing the trailing '/798154996' just
before ';'
Regards, Peter
On 11/05/2013 09:55 AM, Peter Levart wrote:
On 11/04/2013 07:12 PM, robert.fi...@oracle.com wrote:
Changeset: 51b002381b35
Author:rfield
Date: 2013-11-04 10:12 -0800
URL:http://hg.openjdk.java.net/jdk8/tl/jdk/rev/51b002381b35
7194897: JSR 292: Cannot create more than 16 instances of an anonymous class
8027681: Lambda serialization fails once reflection proxy generation kicks in
Reviewed-by: ksrini, briangoetz, jfranck
Contributed-by:joel.fra...@oracle.com,brian.go...@oracle.com,robert.fi...@oracle.com
! src/share/classes/sun/reflect/NativeConstructorAccessorImpl.java
! src/share/classes/sun/reflect/NativeMethodAccessorImpl.java
! src/share/classes/sun/reflect/misc/ReflectUtil.java
+ test/java/lang/invoke/lambda/RepetitiveLambdaSerialization.java
!
test/java/util/stream/test/org/openjdk/tests/java/lang/invoke/SerializedLambdaTest.java
+ test/sun/reflect/AnonymousNewInstance/ManyNewInstanceAnonTest.java
Hi Robert,
I also propose a much faster variant of:
+ /**
+ * Checks if {@code Class cls} is a VM-anonymous class
+ * as defined by {@link sun.misc.Unsafe#defineAnonymousClass}
+ * (not to be confused with a Java Language anonymous inner class).
+ */
+ public static boolean isVMAnonymousClass(Class? cls) {
+ return cls.getSimpleName().contains(/);
+ }
The following:
public static boolean isVMAnonymousClassFAST(Class? cls) {
String name = cls.getName();
for (int i = name.length() - 1; i = 0; i--) {
char c = name.charAt(i);
if (c == '.') return false;
if (c == '/') return true;
}
return false; // root package
}
It's about 12..25x faster for typical class names and doesn't produce
any garbage.
Regards, Peter
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
On 11/05/2013 10:33 AM, Joel Borggrén-Franck wrote:
I would also restructure the Method/Constructor accessor logic differently.
The check for ReflectUtil.isVMAnonymousClass() can be performed just once (in the
newMethodAccessor/newConstructorAccessor methods) and based on this check, create
accessor:
- for classic declaring class - as is / unchanged
- for anonymous declaring class - just create and return
NativeMethodAccessorImpl without a parent
Then in NativeMethodAccessorImpl (and same for constructor), modify the
inflation checking logic:
if (parent != null ++numInvocations
ReflectionFactory.inflationThreshold()) {
MethodAccessorImpl acc = (MethodAccessorImpl)
new MethodAccessorGenerator().
generateMethod(method.getDeclaringClass(),
method.getName(),
method.getParameterTypes(),
method.getReturnType(),
method.getExceptionTypes(),
method.getModifiers());
parent.setDelegate(acc);
}
I don't like adding even more special cases to this check. IMHO a better way
that we discussed and rejected, opting for a smaller change, is to create a
NonInflatingMethodAccessor and just drop in one of those without a delegate for
when creating the accessor for methods/ctors on anonymous classes.
Even better. I would name the new class NativeMethodAccessorImpl and the
one doing inflation InflatingNativeMethodAccessorImpl which would extend
NativeMethodAccessorImpl, override invoke() and call super.invoke()...
This way, no native methods duplication is needed. invoke() is already
an interface method with 2 implementations. Now it will have 3. Does
this present any difference for dispatch optimization?
Regards, Peter
cheers
/Joel
Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass
On 11/05/2013 12:33 PM, Chris Hegarty wrote:
On 05/11/2013 10:59, Paul Sandoz wrote:
On Nov 5, 2013, at 8:26 AM, Peter Levartpeter.lev...@gmail.com wrote:
P.S. I'm curious about the strange seemingly unneeded code fragments
in FinalizerThread and both Runnables. For example:
169 forkSecondaryFinalizer(new Runnable() {
170*private volatile boolean running;*
171 public void run() {
172*if (running)*
173*return;*
174*running = true;*
The FinalizerThread and each Runnable instance is only executed
once. Why these checks then? Does anybody know?
I was wondering that too. A Thread is an instance of Runnable. So a
finalize method could re-exec the current finalizer thread on another
another thread.
Right. I added these a while back to ensure that this does not happen.
-Chris.
That's really mean 8-)
Peter
Paul.
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
On 11/06/2013 11:37 AM, Remi Forax wrote:
On 11/05/2013 11:11 AM, Joel Borggrén-Franck wrote:
On 5 nov 2013, at 10:51, Peter Levart peter.lev...@gmail.com wrote:
On 11/05/2013 10:33 AM, Joel Borggrén-Franck wrote:
I would also restructure the Method/Constructor accessor logic
differently. The check for ReflectUtil.isVMAnonymousClass() can
be performed just once (in the
newMethodAccessor/newConstructorAccessor methods) and based on
this check, create accessor:
- for classic declaring class - as is / unchanged
- for anonymous declaring class - just create and return
NativeMethodAccessorImpl without a parent
Then in NativeMethodAccessorImpl (and same for constructor),
modify the inflation checking logic:
if (parent != null ++numInvocations
ReflectionFactory.inflationThreshold()) {
MethodAccessorImpl acc = (MethodAccessorImpl)
new MethodAccessorGenerator().
generateMethod(method.getDeclaringClass(),
method.getName(),
method.getParameterTypes(),
method.getReturnType(),
method.getExceptionTypes(),
method.getModifiers());
parent.setDelegate(acc);
}
I don't like adding even more special cases to this check. IMHO a
better way that we discussed and rejected, opting for a smaller
change, is to create a NonInflatingMethodAccessor and just drop in
one of those without a delegate for when creating the accessor for
methods/ctors on anonymous classes.
Even better. I would name the new class NativeMethodAccessorImpl and
the one doing inflation InflatingNativeMethodAccessorImpl which
would extend NativeMethodAccessorImpl, override invoke() and call
super.invoke()... This way, no native methods duplication is needed.
invoke() is already an interface method with 2 implementations. Now
it will have 3. Does this present any difference for dispatch
optimization?
FWIW i think the magic number is 4, but I don't know where I got
that. Anyhow, this might be slightly controversial, but for all code
I care about (reflective invocation of methods/ctors on
non-VM-anonymous classes) the check happens exactly once as is.
No, the magic number is 2 (by default),
you can look for TypeProfileWidth in the vm code.
I forgot that each generated method accessor is a separate class with a
separate method. So the number of implementations is already much more
than 2 today...
Regards, Peter
regards,
Rémi
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
On 11/05/2013 10:10 AM, Brian Goetz wrote:
Ineexof(char) sounds like as fast and simpler?
Well, indexOf(char) or lastIndexOf(char) searches for a single char. We
can do better searching backwards for two chars at the same time.
If the name of VM-anonymous class is always ending with pattern:
slash followed by some decimal digits then the following would be even
faster:
public static boolean isVMAnonymousClassFAST(Class? cls) {
String name = cls.getName();
for (int i = name.length() - 1; i = 0; i--) {
char c = name.charAt(i);
if (c == '/') return true;
if (c '0' || c '9') return false;
}
return false;
}
Regards, Peter
Sent from my iPhone
On Nov 5, 2013, at 8:55 AM, Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com wrote:
On 11/04/2013 07:12 PM, robert.fi...@oracle.com wrote:
Changeset: 51b002381b35
Author:rfield
Date: 2013-11-04 10:12 -0800
URL:http://hg.openjdk.java.net/jdk8/tl/jdk/rev/51b002381b35
7194897: JSR 292: Cannot create more than 16 instances of an anonymous class
8027681: Lambda serialization fails once reflection proxy generation kicks in
Reviewed-by: ksrini, briangoetz, jfranck
Contributed-by:joel.fra...@oracle.com,brian.go...@oracle.com,robert.fi...@oracle.com
! src/share/classes/sun/reflect/NativeConstructorAccessorImpl.java
! src/share/classes/sun/reflect/NativeMethodAccessorImpl.java
! src/share/classes/sun/reflect/misc/ReflectUtil.java
+ test/java/lang/invoke/lambda/RepetitiveLambdaSerialization.java
!
test/java/util/stream/test/org/openjdk/tests/java/lang/invoke/SerializedLambdaTest.java
+ test/sun/reflect/AnonymousNewInstance/ManyNewInstanceAnonTest.java
Hi Robert,
I also propose a much faster variant of:
+ /**
+ * Checks if {@code Class cls} is a VM-anonymous class
+ * as defined by {@link sun.misc.Unsafe#defineAnonymousClass}
+ * (not to be confused with a Java Language anonymous inner class).
+ */
+ public static boolean isVMAnonymousClass(Class? cls) {
+ return cls.getSimpleName().contains(/);
+ }
The following:
public static boolean isVMAnonymousClassFAST(Class? cls) {
String name = cls.getName();
for (int i = name.length() - 1; i = 0; i--) {
char c = name.charAt(i);
if (c == '.') return false;
if (c == '/') return true;
}
return false; // root package
}
It's about 12..25x faster for typical class names and doesn't produce
any garbage.
Regards, Peter
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
On 11/07/2013 02:20 AM, John Rose wrote:
On Nov 6, 2013, at 11:30 AM, Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com wrote:
Well, indexOf(char) or lastIndexOf(char) searches for a single char.
We can do better searching backwards for two chars at the same time.
If the name of VM-anonymous class is always ending with pattern:
slash followed by some decimal digits then the following would be
even faster:
Although this reasoning is plausible, it is not a reliable conclusion,
and should not drive edits of Java code without careful measurement.
The reasoning assumes a performance model based on the interpreter
and bytecode count. But performance depends on native code produced
by the JIT.
I agree. I should have measured it...
An optimizing JIT will usually transform the code deeply. For string
scanning, for example, HotSpot has an intrinsic for
String.indexOf(String) that uses totally different code from a
user-coded loop. (It is not currently so for String.indexOf(int), but
String.indexOf(/) is potentially very fast.)
Also, with your example code, the combined loop may often be faster
than two back-to-back loops, but simpler loops can sometimes be
vectorized more robustly, to the point where back-to-back simple
loops, if vectorized, may be competitive with a hand-fused loop.
So loop complexity and method intrinsics can create surprises for
those who rely on simple performance modesl
Some day we will get to a world where loops are specified stream-wise,
and robustly optimized without this sort of manual intervention. In
the mean time, be careful about advising hand-optimizations of Java
code. They can backfire, by confusing the JIT and preventing
optimizations that would apply to simpler code.
— John
So I did measure it. I took two classes with the following names:
com.test.pkg.PerfTest$Classic$1
com.test.pkg.PerfTest$$Lambda$1/925858445
Typically package names will be even relatively longer than in this example.
I measured the following implementations:
public static boolean isVMAnonymousClass(Class? cls) {
return cls.getSimpleName().contains(/);
}
public static boolean isVMAnonymousClass_FAST1(Class? cls) {
String name = cls.getName();
for (int i = name.length() - 1; i = 0; i--) {
char c = name.charAt(i);
if (c == '.') return false;
if (c == '/') return true;
}
return false;
}
public static boolean isVMAnonymousClass_FAST2(Class? cls) {
String name = cls.getName();
for (int i = name.length() - 1; i = 0; i--) {
char c = name.charAt(i);
if (c == '/') return true;
if (c '0' || c '9') return false;
}
return false;
}
public static boolean isVMAnonymousClass_indexOf(Class? cls) {
return cls.getName().indexOf(/) -1;
}
I also tried String.lastIndexOf(String) and it is a little faster for
true return, since it only scans backwards until the / is found, but
it is slower than String.indexOf(String) for false return. Is it not
intrinsified?
Here are the results:
http://cr.openjdk.java.net/~plevart/jdk8-tl/isVmAnonymousClass/results.txt
I think that any of the above implementations that doesn't use
Class.getSimpleName() is good. The FAST2 variant is favourable for false
return since it typically decides after examining a single character at
end of class name and is still among the fastest for true return...
Here's the code I used for testing:
http://cr.openjdk.java.net/~plevart/jdk8-tl/isVmAnonymousClass/com/test/pkg/PerfTest.java
http://cr.openjdk.java.net/~plevart/jdk8-tl/isVmAnonymousClass/si/pele/microbench/TestRunner.java
Regards, Peter
Re: Signature of MethodHandleInfo.reflectAs is not specific enough
On 11/11/2013 02:24 AM, Ali Ebrahimi wrote: This is another workaround: public T extends MemberAnnotatedElement, R R reflectAs(Class? super T expected, Lookup lookup); info.reflectAs(Member.class, lookup);//works info.reflectAs(AnnotatedElement.class, lookup);//works info.reflectAs(Member.class, lookup);//works info.reflectAs(AnnotatedElement.class, lookup);//works info.reflectAs(Object.class, lookup);doesn't work. info.reflectAs(Other.class, lookup);doesn't work. with this does not need to your javadoc and is more type safe. . Hm... it doesn't look very compile-time type-safe: String s = info.reflectAs(Method.class, lookup); // compiles !!! IMO, I would rather remove the Class parameter altogether. It serves no purpose in the method implementation other than to perform a cast-check on the returned object. The method could simply be: public T extends Member T reflect(Lookup lookup); This would not solve the problem Remi put forward. I.e. this would not compile: AnnotatedElement ae = info.reflect(lookup); But with an explicit cast, It compiles: AnnotatedElement ae = (AnnotatedElement) info.reflect(lookup); And compared to what we would have with Class parameter and loosened compile-time type-safety as per Remi's suggestion, it is still shorter: AnnotatedElement ae = info.reflectAs(AnnotatedElement.class, lookup); // this is longer! A type-unsafe variant is possible too (I'm not advocating it): public T T reflect(Lookup lookup); Now that Generalized Target-Type Inference http://openjdk.java.net/projects/jdk8/features#101 is part of Java 8, using ClassT parameters just as hints to the compiler is not needed in many cases. But if one needs to hint the compiler, explicit type parameters can be used as an escape hatch as always: Object o = info.Methodreflect(lookup); Regards, Peter On Mon, Nov 11, 2013 at 1:59 AM, Remi Forax fo...@univ-mlv.fr mailto:fo...@univ-mlv.fr wrote: The is a stupid issue with the signature of MethodHandleInfo.reflectAs, j.l.r.Field, Method or Constructor implement two interfaces Member and AnnotatedElement, with the current signature, the code info.reflectAs(Member.class, lookup) works but the code info.reflectAs(AnnotatedElement.class, lookup) doesn't work. Because there is no way to do an 'or' between several bounds of a type variable, I think that the signature of reflectAs should be changed from : public T extends Member T reflectAs(ClassT expected, Lookup lookup); to public T T reflectAs(ClassT expected, Lookup lookup); and the javadoc should be modified to explain that a Member or AnnotatedElement are valid bounds of T. As a side effect, the signature of MethodHandles.reflectAs(ClassT, MethodHandle) should be updated accordingly. There is a workaround, one can write: (AnnotatedElement)info.reflectAs(Member.class, lookup) but it's at best weird. cheers, Rémi ___ mlvm-dev mailing list mlvm-...@openjdk.java.net mailto:mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-...@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: Signature of MethodHandleInfo.reflectAs is not specific enough
On 11/11/2013 08:14 AM, Peter Levart wrote: The method could simply be: public T extends Member T reflect(Lookup lookup); But if one needs to hint the compiler, explicit type parameters can be used as an escape hatch as always: Object o = info.Methodreflect(lookup); Well, well, explicit type parameters for method invocation are not needed in above example: Object o = info.reflect(lookup); // compiles One would only need them in situations like: info.Methodreflect(lookup).invoke(...); Regards, Peter
Re: hg: jdk8/tl/jdk: 7194897: JSR 292: Cannot create more than 16 instances of an anonymous class; ...
On 11/05/2013 12:29 AM, Remi Forax wrote: But you are right that there is a performance pothole in the interoperation between lambdas and reflection. This implementation RFE, to use method handles instead of code spinning, would take care of that particular problem: https://bugs.openjdk.java.net/browse/JDK-6824466 I remember writing a code like that a long time ago, to see if it was possible, as far as I remember, the main issue was to be able to say, please compiler compile this method handle chain into a blob that I can reuse. — John regards, Rémi Hi John, Remi, I tried to create a prototype to see how it compares to bytecode-generated method accessor. Here it is: https://bugs.openjdk.java.net/browse/JDK-6824466#comment-13426571 It seems that lambda forms do their job quite well. Do experts have any suggestion how to improve it? Regards, Peter
Re: Why stream from BufferedReader::lines is not closing the reader?
On 11/18/2013 03:28 PM, Brian Goetz wrote:
Which means that, if your stream holds non-memory resources, the
flatMapper should create a stream that closes the underlying stream,
like:
blah.flatMap(path - {
BufferedReader br = new BufferedReader(path);
return br.lines.onClose(br::close);
}
...
...the only problem with above code is that it doesn't compile, because
of IOException declared on BufferedReader (FileReader actually!)
constructor and BufferedReader.close() method. The solutions to this
have already been discussed on the list some time ago, and one of the
propositions was to create interfaces like:
public interface IOFunctionT, R extends FunctionT, R {
default R apply(T t) {
try {
return applyIO(t);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
R applyIO(T t) throws IOException;
}
public interface IORunnable extends Runnable {
default void run() {
try {
runIO();
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
void ruinIO() throws IOException;
}
...etc, and use them in code like this:
ListString paths = ...
paths
.stream()
.flatMap((IOFunctionString, StreamString) path - {
BufferedReader br = new BufferedReader(new FileReader(path));
return br.lines().onClose((IORunnable) br::close);
})
.forEach(System.out::println);
Regards, Peter
Re: CallerSensitive access rights problems
On 11/18/2013 04:31 PM, Alan Bateman wrote:
On 18/11/2013 14:59, Jochen Theodorou wrote:
Hi,
java.lang.Class has multiple methods annotated with CallerSensitive
(see
http://hg.openjdk.java.net/jdk8/jdk8-gate/jdk/file/tip/src/share/classes/java/lang/Class.java).
Now if we in Groovy here want to build our runtime structure for this
class, and the security manager is not allowing access to
sun.reflect, then we get into trouble.
https://jira.codehaus.org/browse/GROOVY-6405 is caused by this.
What do you suggest people with this problem, if adding
accessClassInPackage.sun.reflect is no option?
Is it sun.reflect.CallerSensitive.class.getDeclaredMethods that is
failing?
-Alan.
From GROOVY-6405 discussion I think it is, yes.
The work-around suggested in GROOVY-6405 does not work, because it has a
bug. It should be written as:
private static void setAnnotationMetaData(Annotation[] annotations
/*, AnnotatedNode an */) {
for (Annotation annotation : annotations) {
if (annotation*.annotationType()*.getPackage() == null ||
!sun.reflect.equals(annotation*.annotationType()*.getPackage().getName()))
{
System.out.println(Processing: +
annotation.annotationType().getName());
} else {
System.out.println(Skipping: +
annotation.annotationType().getName());
}
}
}
... i.e. don't call annotation.*getClass()* because what you get is a
dynamic Proxy class implementing the annotation interface and such Proxy
class does not live in the same package as the annotation interface...
There is another such annotation to watch for, in another protected
package: *sun.misc.Contended* ...
Regards, Peter
Re: Map.Entry.setValue as a default method
On 11/21/2013 03:56 PM, Remi Forax wrote:
Maybe one of:
interface KoolReusablePair {
default boolean defaultEquals(Object x) { ... }
static int hashCode(KoolReusablePair self) { ... }
...
}
class KoolImpl implements KoolReusablePair {
@Override //manual opt-in:
public boolean equals(Object x) { return
KoolReusablePair.super.defaultEquals(x); }
@Override //manual opt-in:
public int hashCode() { return KoolReusablePair.hashCode(this); }
...
}
— John
The plumber in me think that a static method unlike a default method
will not pollute the itable.
...and static interface method, unlike default, will not pollute the
public API (static interface methods are not inherited).
Regards, Peter
regards,
Rémi
Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method
On 11/27/2013 08:40 AM, David Holmes wrote:
On 27/11/2013 2:16 AM, David Chase wrote:
On 2013-11-26, at 8:12 AM, David Chase david.r.ch...@oracle.com wrote:
On 2013-11-26, at 7:32 AM, David Holmes david.hol...@oracle.com
wrote:
On 26/11/2013 10:16 PM, David Chase wrote:
On 2013-11-26, at 7:12 AM, David Holmes david.hol...@oracle.com
wrote:
On 26/11/2013 9:56 PM, David Chase wrote:
On 2013-11-25, at 9:18 PM, David Holmes
david.hol...@oracle.com wrote:
We do have the jdk.internal namespace. But I think Unsafe is as
good a place as any - though maybe sun.misc.VM is marginally
better?
Does anyone have any problems with sun.misc.VM as a choice?
I have to do a minor revision to the hotspot commit anyway.
Is sun.misc.VM also loaded very early anyway?
No you would have to add it as for Unsafe.
But it's loaded early anyway as a normal consequence of other
class loading, right?
What do you mean by early? It isn't a pre-loaded class but it
will be loaded during system initialization. It is approx the 120th
class to be loaded. Unsafe is about 135th.
120 is earlier than 135, so by that measure it is superior.
Do you see any other problems with the change?
The method's not at all Unsafe in the technical sense of the word,
so it is just a matter of choosing a good home.
On further investigation, change to sun.misc.VM would be the first
time that hotspot knows of the existence of sun.misc.VM;
sun.misc.Unsafe is already filled with methods that the runtime knows
about (intrinsics, etc). I think Unsafe is better.
Okay.
David
Hi David(s),
Excuse me for my ignorance, but does pre-loading the class involve it's
initialization? Is static initializer called at that time? Even if it is
not at that time, it surely should be called before first invocation of
a method on that class (the throwIllegalAccessError() method in this
case). You need a reference to this method very early - even before
system initialization starts. How early do you expect first faulty
invocations could occur that need to call this method? What if calling
that method triggers non-trivial initialization which in turn encounters
another faulty invocation?
sun.misc.Unsafe has a non-trivial static initialization which involves
registerNatives() native method invocation, and it also calls:
sun.reflect.Reflection.registerMethodsToFilter(Unsafe.class, getUnsafe);
...which initializes hell lot of things (like java.util.HashMap for
example, which in who knows which incarnation included random hash seed
initialization which triggered random number generator initialization
with gathering of random seed from various sources, ...)
sun.misc.VM on the other hand, only has the following static initialization:
private static final Object lock = new Object();
static {
initialize();
}
private native static void initialize();
Are you okay with all possible interactions of this initialization on
all platforms?
I think a new class with only this method would be a safer choice.
Regarding back-porting, even if sun.misc.Unsafe is used as the holder
for that method, this new method will have to be added to
sun.misc.Unsafe on those legacy platforms in order to obtain this new
Method *. If the method is not found, the VM would have to behave as
before. Couldn't the pre-loading of classes be made such that some of
them are optional?
Regard, Peter
David
Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method
On 11/27/2013 11:03 AM, David Holmes wrote:
Hi Peter,
On 27/11/2013 7:20 PM, Peter Levart wrote:
On 11/27/2013 08:40 AM, David Holmes wrote:
On 27/11/2013 2:16 AM, David Chase wrote:
On 2013-11-26, at 8:12 AM, David Chase david.r.ch...@oracle.com
wrote:
On 2013-11-26, at 7:32 AM, David Holmes david.hol...@oracle.com
wrote:
On 26/11/2013 10:16 PM, David Chase wrote:
On 2013-11-26, at 7:12 AM, David Holmes david.hol...@oracle.com
wrote:
On 26/11/2013 9:56 PM, David Chase wrote:
On 2013-11-25, at 9:18 PM, David Holmes
david.hol...@oracle.com wrote:
We do have the jdk.internal namespace. But I think Unsafe is as
good a place as any - though maybe sun.misc.VM is marginally
better?
Does anyone have any problems with sun.misc.VM as a choice?
I have to do a minor revision to the hotspot commit anyway.
Is sun.misc.VM also loaded very early anyway?
No you would have to add it as for Unsafe.
But it's loaded early anyway as a normal consequence of other
class loading, right?
What do you mean by early? It isn't a pre-loaded class but it
will be loaded during system initialization. It is approx the 120th
class to be loaded. Unsafe is about 135th.
120 is earlier than 135, so by that measure it is superior.
Do you see any other problems with the change?
The method's not at all Unsafe in the technical sense of the word,
so it is just a matter of choosing a good home.
On further investigation, change to sun.misc.VM would be the first
time that hotspot knows of the existence of sun.misc.VM;
sun.misc.Unsafe is already filled with methods that the runtime knows
about (intrinsics, etc). I think Unsafe is better.
Okay.
David
Hi David(s),
Excuse me for my ignorance, but does pre-loading the class involve it's
initialization? Is static initializer called at that time?
No, pre-load is simply loading not initialization. Static
initialization gets triggerred via a controlled process as it is very
delicate.
Even if it is
not at that time, it surely should be called before first invocation of
a method on that class (the throwIllegalAccessError() method in this
case). You need a reference to this method very early - even before
system initialization starts. How early do you expect first faulty
invocations could occur that need to call this method? What if calling
that method triggers non-trivial initialization which in turn encounters
another faulty invocation?
These faults should only appear in application classes so generally
everything should be initialized well before you need to use this
method. If a core library class has a bug that triggered this need to
call this method then yes it is possible that it might happen too
early to succeed - but that is quite normal for the core classes,
there are a number of things that can happen too early in the
initialization process to work (eg throwing exceptions, using
assertions).
That said I'm not sure how this could fail in that all we need is a
reference to the method to put in the vtable and we have that after
loading. Then all that can go wrong is that we can't actually throw
the exception.
sun.misc.Unsafe has a non-trivial static initialization which involves
registerNatives() native method invocation, and it also calls:
registerNative is not an issue
sun.reflect.Reflection.registerMethodsToFilter(Unsafe.class,
getUnsafe);
...which initializes hell lot of things (like java.util.HashMap for
example, which in who knows which incarnation included random hash seed
initialization which triggered random number generator initialization
with gathering of random seed from various sources, ...)
sun.misc.VM on the other hand, only has the following static
initialization:
private static final Object lock = new Object();
static {
initialize();
}
private native static void initialize();
Are you okay with all possible interactions of this initialization on
all platforms?
The only time the initialization order would be changed by this fix is
if one of the classes initialized before-hand had a bug that required
this fix to come into affect. That is obviously a JDK bug and is
extremely unlikely to happen.
Note that sun.misc.VM is currently initialized 44th while Unsafe is 61st.
I don't see any issues with this fix in that regard.
I see. Thanks for explanation, David.
I think a new class with only this method would be a safer choice.
Regarding back-porting, even if sun.misc.Unsafe is used as the holder
for that method, this new method will have to be added to
sun.misc.Unsafe on those legacy platforms in order to obtain this new
Method *. If the method is not found, the VM would have to behave as
before. Couldn't the pre-loading of classes be made such that some of
them are optional?
It already supports optional classes.
Ah, I have misunderstood the back-porting issue. It was not about not
having new class but about which existing class to use as a host that
might not exist in older version
Re: RFR (S + L test) : 8016839 : JSR292: AME instead of IAE when calling a method
On 11/27/2013 02:40 PM, David Chase wrote:
On 2013-11-27, at 6:53 AM, Peter Levart peter.lev...@gmail.com wrote:
Ah, I have misunderstood the back-porting issue. It was not about not having
new class but about which existing class to use as a host that might not exist
in older version of platform...
Sorry for noise.
Noise is okay. This fix was a PITA, init order can hide surprises, and I don't
mind going over the details.
As to the activation of the code in question, practically never, except in tests or when
bozo-bytecode-generators (a remark I resemble) are making mistakes. The
initialize of the throwIAE method is deferred until loading of a class that
performs an illegal-access-override of an interface method -- e.g., interface I { int
m(); } class C implements I { private int m(); } so even that requires an inconsistent
set of classes, which is
unlikely in any normal initialization.
Longer-term/hoped-for plan is to create a custom throwIAE method each time this happens
so that an informative error message can be included -- C.m() overrides I.m() but
is private (or protected, or package-inaccessible) -- but that is tricky because we
don't have a good place to put the method; by the time we learn this about class C, we
have rewritten its constant pool and it is difficult to add more, and the constant pool
cache is fragile-yet-critical-to-interpreter-performance. So for now, I did this.
I did test it as much as I could figure out how, including running the
regression test in Netbeans and poking around, and also running it under jdb on
some embedded platforms and trying to think of commands that might trigger
embarrassing failures.
The push is in the pipeline, but if you have other tests you can suggest, it is
not too late to pull the emergency stop (in particular, I am gatekeeper for
hs-comp this month, so I can put off the next turn of the crank till the last
moment).
David
Your and David Holmes' answers have pretty much silenced me. I can't
think of any more troubles...
About creating an informative error message: is there a way to
dynamically dig-out from the VM, while executing the single system-wide
throwIAE method, the symbolic invocation attributes of a call-site
together with the target object in case of instance method invocation?
From that data it could be possible to re-compute the context in
question, I assume...
Regards, Peter
Re: 8029281: Synchronization issues in Logger and LogManager
Hi Daniel,
I have started looking at the LogManager change first, and here's the
1st race I found...
The new method LoggerWeakRef.dispose() can be called from 3 places:
- LoggerContext.addLocalLogger
- LoggerContext.findLogger
- LogManager.drainLoggerRefQueueBounded which is called from public
method LogManager.addLogger
The 1st and 2nd are guarded by LoggerContext.this lock, but the 3rd is
unguarded.
What can happen is the following:
Thread1:
LogManager.addLogger(someLogger)
LogManager.drainLoggerRefQueueBounded() // finds some enqueued
LoggerWeakRef for name == 'X.Y' and...
LoggerWeakRef.dispose() // this is 1st call to dispose, so...
synchronized (LoggerWeakRef.this) {
disposed = true;
}
---context switch---
Thread2:
LogManager.addLogger(logger{name=='X.Y'})
LogManager.drainLoggerRefQueueBounded() // no enqueued refs...
LoggerContext.addLocalLogger(logger{name=='X.Y'}, true)
LoggerWeakRef ref = namedLoggers.get(name); // returns
a non-null ref ('X.Y' still points to a cleared weak ref)
if (ref.get() == null) {
ref.dispose(); // this is a 2nd call to this ref,
so it returns quickly
}
// ... namedLoggers gets new entry, etc...
LogNode node = getNode(name); // get node for 'X.Y'
*node.loggerRef = ref;*
---context switch---
Thread1: (still in LoggerWeakRef.dispose())
if (node != null) {
*node.loggerRef = null;* // clear LogNode's weak ref to
us *!!! NOT QUITE !!!*
... so Thread1 erroneously clears the reference from Node to new
LoggerWeakRef that was just added by Thread2 ...
One way to fix this is to synchronize the clearing of node.logerRef on
node.context too. I would make LoggerWeakRef.node and parentRef fields
volatile and do the following:
final class LoggerWeakRef extends WeakReferenceLogger {
private String name; // for
namedLoggers cleanup
private volatile LogNode node; // for loggerRef
cleanup
private volatile WeakReferenceLogger parentRef; // for kids
cleanup
LoggerWeakRef(Logger logger) {
super(logger, loggerRefQueue);
name = logger.getName(); // save for namedLoggers cleanup
}
// dispose of this LoggerWeakRef object
void dispose() {
LogNode node = this.node;
if (node != null) {
synchronized (node.context) {
node = this.node; // double-checked locking
if (node != null) {
// if we have a LogNode, then we were a named
Logger
// so clear namedLoggers weak ref to us
node.context.removeLoggerRef(name, this);
name = null; // clear our ref to the Logger's name
node.loggerRef = null; // clear LogNode's weak
ref to us
this.node = null; // clear our ref to LogNode
}
}
}
WeakReferenceLogger parentRef = this.parentRef;
if (parentRef != null) {
// this LoggerWeakRef has or had a parent Logger
this.parentRef = null; // clear our weak ref to the
parent Logger (racy, but harmless)
Logger parent = parentRef.get();
if (parent != null) {
// the parent Logger is still there so clear the
// parent Logger's weak ref to us
parent.removeChildLogger(this);
}
}
}
...
What do you think?
That's all for today. Will check the rest of patch tomorrow.
Regards, Peter
On 11/27/2013 08:23 PM, Daniel Fuchs wrote:
Hi,
Please find below a webrev for:
8029281: Synchronization issues in Logger and LogManager
https://bugs.openjdk.java.net/browse/JDK-8029281:
I believe this changeset will also fix JDK-8027670 and
JDK-8029092 - which I have thus closed as duplicates of JDK-8029281.
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.00/
Now some comments on the fix:
LogManager:
When a logger was garbage collected, and then a new logger
of the same name requested, addlocalLogger used to call
LoggerContext.removeLogger to remove the stale reference
and replace it by a new one. But sometime, the stale reference
was still in the reference queue, which caused the *new* logger
to be wrongly removed later when the reference queue was drained.
LogManager was also missing some synchronization for its 'props'
variable.
Logger:
userBundle resourceBundleName were sometimes accessed within
a synchronized block, and sometimes without. In particular the
getters were not synchronized, which could cause race conditions
because an other thread
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: The problem is that we use a logger in CustomerFormatter and this causes Logger.log call Logger.log itself. As FileHandler.publish and StreamHandler.publish is synchronized, but the iteration to call publish method for all handlers in Logger.log is not synchronized, the deadlock happens. Hello Shi Jun Zhang, Why do you use Logger.log in the CustomerFormatter? What are you achieving by it? Do you want to re-route and re-format messages destined for one handler to some other Logger and consequently handler? On 11/28/2013 08:53 AM, Shi Jun Zhang wrote: This violates the Java doc for java.util.logging.Logger that says All methods on Logger are multi-thread safe. I don't know for sure, but I think that multi-thread-safe does not imply dead-lock-safe. It would be good if java logging used less locks and be less deadlock-prone though. So we should see if it is possible to remove some locks, not to add more locking... Regards, Peter
Re: 8029281: Synchronization issues in Logger and LogManager
Hi Daniel,
1st sorry for the delay. I promised looking at the patch, but was then
distracted by other things. I think that synchronization in LogManager
is correct now. The fact that Mandy thinks so is also reassuring.
I wonder if calling LoggerWeakRef.dispose() from
LoggerContext.addLocalLoger() is needed now that you check the
LogNode.loggerRef before clearing in LoggerWeakRef.dispose():
if (n.loggerRef == this) {
n.loggerRef = null; // clear LogNode's weak
ref to us
}
The LoggerContext.addLocalLoger() will already overwrite the
namedLoggers Hashtable entry for the logger's name with new LoggerWeakRef:
namedLoggers.put(name, ref);
...and it will already overwrite the LogNode.loggerRef with new
LoggerWeakRef:
// Find the new node and its parent.
LogNode node = getNode(name);
node.loggerRef = ref;
So the only effect of calling LoggerWeakRef.dispose() from
LoggerContext.addLocalLoger() is a more prompt unlinking of
LoggerWeakRef child, which has already been cleared but not yet
disposed(), from parent logger. This will eventually happen in a call to
LogManager.drainLoggerRefQueueBounded(), called from public
LogManager.addLogger(), which is the only entry point to
LoggerContext.addLocalLoger(). But it's not wrong as it is now when you
also ensure more prompt unlinking of cleared LoggerWeakRef and code is
more uniform and less fragile then it would be if you created and called
a hypothetical disposeFromParent() which only took care of unlinking
from parent logger...
Regards, Peter
On 11/29/2013 12:41 PM, Daniel Fuchs wrote:
Hi,
Here is a new revision that includes Peter's findings.
Thanks again for that Peter!
http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.01/
It has passed all jdk_core tests and running the new tests
in a loop hasn't triggered any of the issues that appeared
before the fix.
-- daniel
On 11/28/13 3:26 PM, Daniel Fuchs wrote:
On 11/27/13 10:34 PM, Peter Levart wrote:
Hi Daniel,
I have started looking at the LogManager change first, and here's the
1st race I found...
Hi Peter,
Thanks a lot for your feedback! see below...
The new method LoggerWeakRef.dispose() can be called from 3 places:
- LoggerContext.addLocalLogger
- LoggerContext.findLogger
- LogManager.drainLoggerRefQueueBounded which is called from public
method LogManager.addLogger
The 1st and 2nd are guarded by LoggerContext.this lock, but the 3rd is
unguarded.
What can happen is the following:
Thread1:
LogManager.addLogger(someLogger)
LogManager.drainLoggerRefQueueBounded() // finds some enqueued
LoggerWeakRef for name == 'X.Y' and...
LoggerWeakRef.dispose() // this is 1st call to dispose,
so...
synchronized (LoggerWeakRef.this) {
disposed = true;
}
---context switch---
Thread2:
LogManager.addLogger(logger{name=='X.Y'})
LogManager.drainLoggerRefQueueBounded() // no enqueued
refs...
LoggerContext.addLocalLogger(logger{name=='X.Y'}, true)
LoggerWeakRef ref = namedLoggers.get(name); // returns
a non-null ref ('X.Y' still points to a cleared weak ref)
if (ref.get() == null) {
ref.dispose(); // this is a 2nd call to this ref,
so it returns quickly
}
// ... namedLoggers gets new entry, etc...
LogNode node = getNode(name); // get node for 'X.Y'
*node.loggerRef = ref;*
---context switch---
Thread1: (still in LoggerWeakRef.dispose())
if (node != null) {
*node.loggerRef = null;* // clear LogNode's weak
ref to
us *!!! NOT QUITE !!!*
... so Thread1 erroneously clears the reference from Node to new
LoggerWeakRef that was just added by Thread2 ...
Damn. I believe that you're right! I overlooked the fact that LogNode is
reused.
Locking on LogNode.context is indeed a good way of solving the issue.
The only method that we will call from within the lock is
LogNode.context.rmoveLoggerRef(name this) - and that already requires
a lock on LogNode.context so it would be safe.
I have also double checked the places where LogNode.loggerRef is set,
and all those places (except in dispose() so far) are already from
within a lock on LoggerContext.
I think I would prefer however to keep the 'disposed' flag - and to
check whether LogNode.loggerRef == this before clearing it.
Given that - as I said - LogNode.loggerRef is always modified
from within a lock on LoggerContext I think the following would
work:
void dispose() {
synchronized(this) {
if (disposed) return;
disposed = true;
}
final LogNode n = node;
if (n != null) {
synchronized (n.context) {
n.context.removeLoggerRef(name, this);
name = null;
if (n.loggerRef
Theoretical data race on java.util.logging.Handler.sealed
Hi, While browsing the code of java.util.logging.Handler, I noticed a theoretical possibility that a security check in a j.u.l.StreamHandler be circumvented using a data race. There is a plain boolean instance field 'sealed' in j.u.l.Handler that is pre-initialized to 'true' in field initializer. StreamHandler sublcass' constructors overwrite this value with 'false' at the beginning, then issue some operations which circumvent security checks, and finally they reset the 'sealed' value back to 'true' at the end. If a reference to an instance of StreamHandler or subclass is passed to some thread without synchronization via data-race, this thread can see 'true' or 'false' as the possible values of 'sealed' variable, thus it is possible to circumvent security checks. One possibility to fix this is moving the field to StreamHandler and making it final: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.01/ Just making the field volatile might not work. There is an ongoing debate on concurrency-interest which suggests that volatile fields are not exceptional in constructors like final fields are... Regards, Peter
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/03/2013 09:51 AM, Peter Levart wrote: Hi, While browsing the code of java.util.logging.Handler, I noticed a theoretical possibility that a security check in a j.u.l.StreamHandler be circumvented using a data race. There is a plain boolean instance field 'sealed' in j.u.l.Handler that is pre-initialized to 'true' in field initializer. StreamHandler sublcass' constructors overwrite this value with 'false' at the beginning, then issue some operations which circumvent security checks, and finally they reset the 'sealed' value back to 'true' at the end. If a reference to an instance of StreamHandler or subclass is passed to some thread without synchronization via data-race, this thread can see 'true' or 'false' as the possible values of 'sealed' variable, thus it is possible to circumvent security checks. One possibility to fix this is moving the field to StreamHandler and making it final: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.01/ Just making the field volatile might not work. There is an ongoing debate on concurrency-interest which suggests that volatile fields are not exceptional in constructors like final fields are... Regards, Peter The proposed patch is not complete. There are several subclasses of StreamHandler in the java.util.logging package that also need a way to bypass security checks for some operations in their constructors. So here's the updated webrev which updates them with the same code as StreamHandler. This means that there are two copies of 'sealed' flag in object of type ConsoleHandler, for example, but only the one declared in ConsoleHandler is relevant for governing access checks: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.02/ Before filing the bug, I'm asking the list whether this can be considered a bug... Regards, Peter
Re: RFR [6968459] JNDI timeout fails before timeout is reached
On 11/29/2013 09:06 PM, Ivan Gerasimov wrote:
Thank you Alan for the reply!
On 29.11.2013 21:03, Alan Bateman wrote:
On 19/11/2013 17:58, Ivan Gerasimov wrote:
Hello all!
Would you please help review a fix for the bug?
https://bugs.openjdk.java.net/browse/JDK-6968459
It was reported that creating new InitialLdapContext() can fail with
javax.naming.NamingException: LDAP response read timed out, timeout
used:3ms, even though the specified timeout hadn't been elapsed.
The fix was provided by the filer of the bug some time ago.
Here's the webrev with this fix:
http://cr.openjdk.java.net/~igerasim/6968459/0/webrev/
I haven't seen any replies to this but I've cc'ed Vinnie and Xuelei
as they are more familiar with this area.
If I understand correctly then the issue is that the timeout handling
doesn't take account of wakeups when aren't any BerDecoders to
dequeue. The changes mean it will retry the wait with a decreasing
timeout until a reply is received or the timeout elapses. That seems
reasonable, assuming the time doesn't change :-) You might find the
code is a bit clearer if you have a remaining time as that would
allow you get rid of timedOut, timeOut and endTime.
I modified the patch in the way you suggest.
http://cr.openjdk.java.net/~igerasim/6968459/1/webrev/
The timeOut variable now holds the remaining time.
If the system time had changed back, we start counting from the
beginning.
If it had changed forward, we have no way to catch it and the timeout
gets elapsed earlier.
I see the patch doesn't come with a test. Is there any test
infrastructure for testing LDAP without require a complete server?
I didn't find anything like that, that's why I set 'noreg-hard' label.
Sincerely yours,
Ivan
-Alan.
Hi Ivan,
From quick look I notice a danger that Connection.readReply() pauses
(for the timeOut time) in spite of the fact that a reply is ready and
enqueued before waiting.
Imagine a situation where the 1st try of obtaining result returns null:
450 // Get the reply if it is already available.
451 BerDecoder rber = ldr.getReplyBer();
...after that, but before reaching the following lines:
471 synchronized (ldr) {
472 ldr.wait(timeOut);
473 }
The other thread enqueues a reply (LdapRequest.addReplyBer()) because
the code in readReply() is executing without holding a lock on ldr.
When this thread (executing readReply()) finally enters synchronized
block and waits, it will wait until wait() times out or another reply is
enqueued which will issue another notify(). This can lead to unnecessary
pauses. Old code does not exhibit this problem, because it re-checks the
readiness of a reply immediately after entering the synchronized block.
Regards, Peter
Re: RFR [6968459] JNDI timeout fails before timeout is reached
On 12/03/2013 03:35 PM, Ivan Gerasimov wrote:
Hi Peter!
Thank you for your review!
You are right, the patch changed the behavior of the code.
I've reverted back all the unnecessary changes. This should minimize
the risk.
I've also made another correction: After decrementing the remaining
timeOut, the startTime should be set to currTime.
Would you please help review the updated webrev:
http://cr.openjdk.java.net/~igerasim/6968459/2/webrev/
Sincerely yours,
Ivan
Hi Ivan,
That's better. You could move the initial request for ldr.getReplyBer()
(line 447) out of while loop, since it is not needed in the loop (all
further ldr.getReplyBer() calls in loop are performed in synchronized
block already - line 465). else { break; } (line 469) is not needed in
that case. I would also make timeOut variable long to avoid overflows.
Simplified logic could look like this:
BerDecoder readReply(LdapRequest ldr)
throws IOException, NamingException {
long timeOut = (readTimeout 0) ?
readTimeout : 15 * 1000; // Default read timeout of 15 sec
long currTime = System.currentTimeMillis();
BerDecoder rber = ldr.getReplyBer();
while (rber == null timeOut 0L) {
long startTime = currTime;
// If socket closed, don't even try
synchronized (this) {
if (sock == null) {
throw new ServiceUnavailableException(host + : +
port +
; socket closed);
}
}
synchronized (ldr) {
// check if condition has changed since our last check
rber = ldr.getReplyBer();
if (rber == null) {
try {
ldr.wait(timeOut);
} catch (InterruptedException ex) {
throw new InterruptedNamingException(
Interrupted during LDAP operation);
}
}
}
currTime = System.currentTimeMillis();
if (startTime currTime) {
timeOut -= (currTime - startTime);
}
// else system time must have changed backwards (or not at all)
}
if (rber == null readTimeout 0) {
removeRequest(ldr);
throw new NamingException(LDAP response read timed out,
timeout used:
+ readTimeout + ms. );
}
return rber;
}
What do you think?
Regards, Peter
From quick look I notice a danger that Connection.readReply() pauses
(for the timeOut time) in spite of the fact that a reply is ready and
enqueued before waiting.
Imagine a situation where the 1st try of obtaining result returns null:
450 // Get the reply if it is already available.
451 BerDecoder rber = ldr.getReplyBer();
...after that, but before reaching the following lines:
471 synchronized (ldr) {
472 ldr.wait(timeOut);
473 }
The other thread enqueues a reply (LdapRequest.addReplyBer())
because the code in readReply() is executing without holding a lock
on ldr. When this thread (executing readReply()) finally enters
synchronized block and waits, it will wait until wait() times out or
another reply is enqueued which will issue another notify(). This can
lead to unnecessary pauses. Old code does not exhibit this problem,
because it re-checks the readiness of a reply immediately after
entering the synchronized block.
Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/05/2013 07:54 AM, Shi Jun Zhang wrote: On 11/30/2013 12:05 AM, Daniel Fuchs wrote: On 11/29/13 4:56 PM, Alan Bateman wrote: On 29/11/2013 10:08, Daniel Fuchs wrote: However, removing or just moving the lock around might well introduce new unknown issues - so it will need to be carefully anaIyzed, and I am not sure it can/should be attempted in a minor JDK release. Yes, we have to be very careful as the logging code has a history of biting the hand of those that try to improve it. For various reasons, it seems there is a lot of code that has subtle dependencies on the implementation, on the initialization in particular. In any case, you are to be applauded for tackling the synchronization issues and it would be a good project to re-examine all of this in JDK 9 to see how it would be simplified. On documenting the locking details in an @implNote (which seems to be one of the suggestions here) then we also need to be careful as I think we need some flexibility to change some of this going forward. Yes - that's a two edged sword indeed. We certainly don't want to set that in stone... On the other hand I empathizes with developers who struggle to find out what they can - and can't do - when extending j.u.l APIs... Anyway, if usage of the 'synchronized' keyword never appears in the Javadoc I guess that's for good reasons... Thanks Alan, -- daniel -Alan Hi Daniel, This thread is silent for several days, do you have any finding in Handler.publish? Hi Shi Jun Zhang, I have looked at this, creating a prototype. It re-arranged synchronization in a way so that all Formatter methods are invoked out of synchronized sections. I haven't come forward with this yet, because of two issues: - Formatter implementations would suddenly be called multi-threaded. Currently they are invoked from within Handler-instance synchronized sections. - Formatter would have to be invoked optimistically to obtain head and tail strings, so it could happen that a head, for example, would be requested concurrently multiple times, but only one of returned heads would be written to stream then. The 1st thing seems problematic. I can imagine there exist Formatters that are not thread-safe (for example, using single instance of MessageFormat, which is not multi-threaded) and now just happen to work as a consequence of current StreamHandler implementation detail, but would break if called multi-threaded. One way to remedy this is to add a boolean property to Formatter API, say Formatter.isMultiThreaded(), and arrange so that appropriate instances return appropriate values also considering backwards-compatibility... So all-in-all this is not a simple patch and I doubt it can be made for JDK8. In JDK9, I think, it will be possible to re-visit this issue, so It would be good to file it as a BUG or RFI. Regards, Peter
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/04/2013 12:27 AM, Mandy Chung wrote: On 12/3/2013 1:44 AM, Peter Levart wrote: On 12/03/2013 09:51 AM, Peter Levart wrote: Hi, While browsing the code of java.util.logging.Handler, I noticed a theoretical possibility that a security check in a j.u.l.StreamHandler be circumvented using a data race. There is a plain boolean instance field 'sealed' in j.u.l.Handler that is pre-initialized to 'true' in field initializer. StreamHandler sublcass' constructors overwrite this value with 'false' at the beginning, then issue some operations which circumvent security checks, and finally they reset the 'sealed' value back to 'true' at the end. If a reference to an instance of StreamHandler or subclass is passed to some thread without synchronization via data-race, this thread can see 'true' or 'false' as the possible values of 'sealed' variable, thus it is possible to circumvent security checks. One possibility to fix this is moving the field to StreamHandler and making it final: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.01/ Just making the field volatile might not work. There is an ongoing debate on concurrency-interest which suggests that volatile fields are not exceptional in constructors like final fields are... Regards, Peter The proposed patch is not complete. There are several subclasses of StreamHandler in the java.util.logging package that also need a way to bypass security checks for some operations in their constructors. So here's the updated webrev which updates them with the same code as StreamHandler. This means that there are two copies of 'sealed' flag in object of type ConsoleHandler, for example, but only the one declared in ConsoleHandler is relevant for governing access checks: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.02/ Before filing the bug, I'm asking the list whether this can be considered a bug... This does look a possible data race that might return a partially constructed object with sealed = false. I am not sure how likely we will run into this race though. W.r.t. the patch, it might be better to get rid of the sealed field and wrap the calls with doPrivileged with limited privilege (just LoggingPermission(control)) Mandy H Mandy, I created an issue for it nevertheless: https://bugs.openjdk.java.net/browse/JDK-8029781 You're right, doPrivileged() is a more straight-forward approach than 'sealed' variable. Since this might only be considered for inclusion in JDK9 when lambdas are already a tried technology, how do you feel about using them for platform code like logging? As far as I know (just checked), lambda meta-factory is not using any j.u.logging, so there is no danger of initialization loops or similar: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/ Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/09/2013 09:28 AM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter I think the best solution to your problem (or a work-around if you say so) was suggested by Jason Mehrens few messages ago - decoupling of user code from publishing of log records - by creating an AsyncLogHandler that is just feeding a FIFO queue with log records which are processed by a background thread by pop-ing them out of queue and sticking them in the actual LogHandler.publish(). If the queue is unbouded or large enough so that it never fills up, there's no danger of dead-locks even if you invoke Logger.log from custom formatters in such background publishing thread. Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/09/2013 09:51 AM, Shi Jun Zhang wrote: On 12/9/2013 4:28 PM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter Hi Peter, We have thought about moving formatter out of the synchronized section of Handler.publish(), it can avoid the deadlock. However, we can reproduce the similar deadlock by extending the Writer in Handler and using logger in the customized Writer. That's right. And the remedy for that situation would also be what Jason Mehrens suggested - asynchronous publishing. Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/09/2013 10:50 AM, Shi Jun Zhang wrote: On 12/9/2013 4:40 PM, Peter Levart wrote: On 12/09/2013 09:28 AM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter I think the best solution to your problem (or a work-around if you say so) was suggested by Jason Mehrens few messages ago - decoupling of user code from publishing of log records - by creating an AsyncLogHandler that is just feeding a FIFO queue with log records which are processed by a background thread by pop-ing them out of queue and sticking them in the actual LogHandler.publish(). If the queue is unbouded or large enough so that it never fills up, there's no danger of dead-locks even if you invoke Logger.log from custom formatters in such background publishing thread. Regards, Peter Hi Peter, Would the following situation happen if we use AsyncLogHandler? Some error happens and it causes the jvm crashes or System.exit() is called, however the related log record which contains important message is still in the queue and not printed. If so, I think it's unacceptable. You could install a shutdown-hook that would make sure the remaining queue is flushed before completing the VM exit... Regards, Peter
Re: Deadlock between FileHandler and ConsoleHandler when using customized formatter
On 12/09/2013 11:12 AM, Daniel Fuchs wrote: On 12/9/13 9:58 AM, Peter Levart wrote: On 12/09/2013 09:51 AM, Shi Jun Zhang wrote: On 12/9/2013 4:28 PM, Peter Levart wrote: On 12/09/2013 08:02 AM, Shi Jun Zhang wrote: Peter, I think you are misunderstanding this problem. This deadlock is not related to the formatter synchronization, we can make CustomerFormatter.format not synchronized and not call super.format, the deadlock still happens. I'm not saying that your formatters are explicitly synchronized - all formatters are currently effectively synchronized by LogHandlers. The Formatter is invoked from within LogHandler's publish() method which is synchronized (on LogHandler.this). If formatters were invoked out of this synchronized section, there would be no danger of deadlocks when using Logger.log from within custom formatters. But then other issues would arise as a consequence of non-multithreaded formatters being invoked concurrently... Regards, Peter Hi Peter, We have thought about moving formatter out of the synchronized section of Handler.publish(), it can avoid the deadlock. However, we can reproduce the similar deadlock by extending the Writer in Handler and using logger in the customized Writer. That's right. And the remedy for that situation would also be what Jason Mehrens suggested - asynchronous publishing. Hi, I agree with Peter Jason - asynchronous publishing seems like a good solution. I believe the LogManager will close all handlers on exiting, so you might want to make sure that your asynchronous handler flushes the queue before quitting - which could still be tricky if flushing the queue produces new log messages for the queue - and also because you will want Handler.close() to wait until the queue is empty. You're right, Daniel. There already is a global shut-down hook installed in LogManager that close()s all active handlers when VM is shutting down. Shi Jun Zhang, here's a quick mock-up of a prototype AsyncHandler that might work for you: http://cr.openjdk.java.net/~plevart/misc/jul.AsyncHandler/AsyncHandler.java Regards, Peter Anyway - the best advice still is IMHO - don't call Logger.log while publishing a log message. This should save you from a lot of issues, like the one you encountered - but also possible stack overflows etc... best regards, -- daniel Regards, Peter
Re: Theoretical data race on java.util.logging.Handler.sealed
Hi Mandy, On 12/13/2013 12:37 AM, Mandy Chung wrote: Hi Peter, On 12/8/2013 11:19 AM, Peter Levart wrote: H Mandy, I created an issue for it nevertheless: https://bugs.openjdk.java.net/browse/JDK-8029781 You're right, doPrivileged() is a more straight-forward approach than 'sealed' variable. Since this might only be considered for inclusion in JDK9 when lambdas are already a tried technology, how do you feel about using them for platform code like logging? As far as I know (just checked), lambda meta-factory is not using any j.u.logging, so there is no danger of initialization loops or similar: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/ Sorry for the delay to get to this. No rush. We have time before JDK9 gets set-up and running... Alan is right that java.lang.invoke.ProxyClassesDumper does use PlatformLogger which will forward calls to j.u.logging if -Djava.util.logging.config.file is set or java.util.logging has been initialized (via other j.u.logging call). It means that it may lead to recursive initialization. Also the current PlatformLogger implementation formats the message in the same way as j.u.logging that may load locale providers and other classes. I am afraid there are issues to tease out and resolve. It's unfortunate that a lambda debugging feature prevents us from using a basic language feature in j.u.logging code. As far as I know, java.lang.invoke.ProxyClassesDumper is only used if 'jdk.internal.lambda.dumpProxyClasses' system property is set to point to a directory where lambda proxy class files are to be dumped as they are generated - a debugging hook therefore. Wouldn't it be good-enough if error messages about not-being able to set-up/use the dump facility were output to System.err directly - not using PlatformLogger at all? The overloads the doPrivileged method makes it cumbersome to use lambda that causes you to workaround it by adding a new PrivilegedVoidAction interface which is clever. So I think it isn't too bad for this patch to use anonymous inner class and have the doPrivileged call in the constructor. Right. I have prepared a modified webrev which doesn't use lambdas: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.04/ In attempt to minimize the boilerplate, I haven't just replaced lambdas with anonymous inner classes, but rather turned all private configure() methods into ConfigureAction inner classes. In two occasions (SocketHandler and StreamHandler), they are extended with anonymous inner classes to append some actions. In SocketHandler I kept the mechanics of transporting the checked IOException out of PrivilegedAction by wrapping it with Unchecked IOException and later unwrapping and throwing it, rather than using PrivilegedExceptionAction which would further complicate exception handling, since it declares throwing a general j.l.Exception, but the SocketHandler constructor only declares throwing IOException... I think this could be backported to 7u as-is. Regards, Peter Mandy
Re: Review request for 8021368: Launch of Java Web Start app fails with ClassCircularityError exception
On 12/12/2013 07:29 PM, Mandy Chung wrote:
JDK-8021368: Launch of Java Web Start app fails with
ClassCircularityError exception in 7u25
https://bugs.openjdk.java.net/browse/JDK-8021368
This is a fix for 7u60 only. It's a regression in 7u25 due to
JDK-8010117 where it calls Class.getMethod to determine if the
checkMemberAccess method is overridden in a SecurityManager subclass
but Class.getMethod causes parameter types and returned type of other
public methods to be resolved and hence ClassCircularityError. It is
not an issue in JDK 8 as SecurityManager.checkMemberAccess is
deprecated and not called by the JDK (see JDK-8007035).
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk7u/webrevs/8021368/webrev.00/
An alternative implementation is to add a new VM entry point to look
up the declaring class of an overridden method instead of using
JNI_GetMethodID and get a reflective method for a faster query. Since
this check is only performed once in most cases, this performance cost
of using JNI is not too bad that the new VM entry point doesn't
necessarily buy much more.
thanks
Mandy
Hi Mandy,
I tried the following:
public class Test {
static class A {}
static class B {}
static class X {
public void x() { }
}
static class Y extends X {
public A a(B b) { return null; }
public B b(A a) { return null; }
public void x() { }
}
public static void main(String[] args) throws Exception {
MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodHandle mh = lookup.findVirtual(Y.class, x,
MethodType.methodType(void.class, new Class[0]));
MethodHandleInfo mhi = lookup.revealDirect(mh);
System.out.println(mhi.getDeclaringClass() == Y.class);
}
}
The above code does not trigger loading of classes A or B. But
unfortunately it prints true even if I comment-out the declaration of
method Y.x(). I don't know if this is a bug though. I should ask on the
mlvm-dev list...
Anyway, your approach seems more appropriate as it doesn't depend on
method handles and their peculiarities...
Some nits:
2241 * Finds the checkMemberAccess method of the given
SecurityManager*instance*.
...I think it should read ...of the given SecurityManager*class*. instead,
since the method parameter is of type Class.
2210 private static class SecurityManagerHelper {
2211 private final SecurityManager smgr;
2212 private final boolean overrideCheckMemberAccess;
...the fields could be declared package-private so that no synthetic access
methods are generated...
Regards, Peter
Re: Theoretical data race on java.util.logging.Handler.sealed
Hi, Daniel reminded me of a couple of issues the 4th revision of the patch would have when backporting to 7u. So here's another variant that tries to be more backport-friendly: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.05/ This variant could be backported by simply replacing a limited variant of doPrivileged (introduced in JDK 8) with full variant and still not elevate the privilege of Socket creation in SocketHandler. I also removed the need to subclass various ConfigureAction(s) with annonymous inner subclasses by introducing overloaded constructors to ConfigureActions(s) that follow the overloaded constructors of various Handlers. Regards, Peter On 12/14/2013 12:25 PM, Peter Levart wrote: Hi Mandy, On 12/13/2013 12:37 AM, Mandy Chung wrote: Hi Peter, On 12/8/2013 11:19 AM, Peter Levart wrote: H Mandy, I created an issue for it nevertheless: https://bugs.openjdk.java.net/browse/JDK-8029781 You're right, doPrivileged() is a more straight-forward approach than 'sealed' variable. Since this might only be considered for inclusion in JDK9 when lambdas are already a tried technology, how do you feel about using them for platform code like logging? As far as I know (just checked), lambda meta-factory is not using any j.u.logging, so there is no danger of initialization loops or similar: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/ Sorry for the delay to get to this. No rush. We have time before JDK9 gets set-up and running... Alan is right that java.lang.invoke.ProxyClassesDumper does use PlatformLogger which will forward calls to j.u.logging if -Djava.util.logging.config.file is set or java.util.logging has been initialized (via other j.u.logging call). It means that it may lead to recursive initialization. Also the current PlatformLogger implementation formats the message in the same way as j.u.logging that may load locale providers and other classes. I am afraid there are issues to tease out and resolve. It's unfortunate that a lambda debugging feature prevents us from using a basic language feature in j.u.logging code. As far as I know, java.lang.invoke.ProxyClassesDumper is only used if 'jdk.internal.lambda.dumpProxyClasses' system property is set to point to a directory where lambda proxy class files are to be dumped as they are generated - a debugging hook therefore. Wouldn't it be good-enough if error messages about not-being able to set-up/use the dump facility were output to System.err directly - not using PlatformLogger at all? The overloads the doPrivileged method makes it cumbersome to use lambda that causes you to workaround it by adding a new PrivilegedVoidAction interface which is clever. So I think it isn't too bad for this patch to use anonymous inner class and have the doPrivileged call in the constructor. Right. I have prepared a modified webrev which doesn't use lambdas: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.04/ In attempt to minimize the boilerplate, I haven't just replaced lambdas with anonymous inner classes, but rather turned all private configure() methods into ConfigureAction inner classes. In two occasions (SocketHandler and StreamHandler), they are extended with anonymous inner classes to append some actions. In SocketHandler I kept the mechanics of transporting the checked IOException out of PrivilegedAction by wrapping it with Unchecked IOException and later unwrapping and throwing it, rather than using PrivilegedExceptionAction which would further complicate exception handling, since it declares throwing a general j.l.Exception, but the SocketHandler constructor only declares throwing IOException... I think this could be backported to 7u as-is. Regards, Peter Mandy
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/17/2013 11:29 AM, Daniel Fuchs wrote:
On 12/16/13 10:14 PM, Mandy Chung wrote:
On 12/14/2013 9:38 AM, Peter Levart wrote:
Hi,
Daniel reminded me of a couple of issues the 4th revision of the patch
would have when backporting to 7u. So here's another variant that
tries to be more backport-friendly:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.05/
This looks good in general.
SocketHandler line 200 - it looks to me that this is an existing bug
that should call setOutputStream within doPrivileged.
I was asking myself the same question.
I'm not sure I would qualify this as a bug: if you want to create a
SocketHandler that sends to a specific host/port you need the
LoggingPermission(control).
What is being protected with permission checking in Handler(s) is
changing various properties of Handler(s). What we are granting with
doPriviliged is a temporary elevation of privilege for changing the
properties of the instance of Handler that is being constructed - just
for the act of constructing the instance of Handler. Anybody should be
able to instantiate new Handler instance - that doesn't present any
special capability. Putting new Handler into service is already
additionally protected (for example Logger.addHandler()). So in case of
SocketHandler, one should be able to instantiate new SocketHandler for
specified host/port if she has permission to create a Socket to that
host/port, but nothing more should be required. To put such Handler into
service (to direct logging messages generated by logging system to it),
additional LoggingPermission(control) is already required.
So I think Mandy is right and that is an existing bug, although not very
critical, since it only affects those that want to instantiate new
SocketHandler although they are not able to use it in the logging system
(maybe they use it somewhere else?).
Regards, Peter
I think it'd be simpler if SocketHandler no-arg constructor can first
get the port and host from the logging properties so that it doesn't
need to differentiate hostAndPortSet is set and ConfigureAction no-arg
constructor can be removed.
Daniel/Peter - do we have tests to cover these permission check for
these handlers?
Shockingly:
$ cd jdk/test/java/util/logging
$ find . -type f -exec grep SocketHandler {} /dev/null \;
reveals nothing. So it seams we have no unit tests for SocketHandler!
-- daniel
Mandy
This variant could be backported by simply replacing a limited variant
of doPrivileged (introduced in JDK 8) with full variant and still not
elevate the privilege of Socket creation in SocketHandler. I also
removed the need to subclass various ConfigureAction(s) with
annonymous inner subclasses by introducing overloaded constructors to
ConfigureActions(s) that follow the overloaded constructors of various
Handlers.
Regards, Peter
On 12/14/2013 12:25 PM, Peter Levart wrote:
Hi Mandy,
On 12/13/2013 12:37 AM, Mandy Chung wrote:
Hi Peter,
On 12/8/2013 11:19 AM, Peter Levart wrote:
H Mandy,
I created an issue for it nevertheless:
https://bugs.openjdk.java.net/browse/JDK-8029781
You're right, doPrivileged() is a more straight-forward approach
than 'sealed' variable. Since this might only be considered for
inclusion in JDK9 when lambdas are already a tried technology, how
do you feel about using them for platform code like logging? As far
as I know (just checked), lambda meta-factory is not using any
j.u.logging, so there is no danger of initialization loops or
similar:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/
Sorry for the delay to get to this.
No rush. We have time before JDK9 gets set-up and running...
Alan is right that java.lang.invoke.ProxyClassesDumper does use
PlatformLogger which will forward calls to j.u.logging if
-Djava.util.logging.config.file is set or java.util.logging has been
initialized (via other j.u.logging call). It means that it may lead
to recursive initialization. Also the current PlatformLogger
implementation formats the message in the same way as j.u.logging
that may load locale providers and other classes. I am afraid there
are issues to tease out and resolve.
It's unfortunate that a lambda debugging feature prevents us from
using a basic language feature in j.u.logging code. As far as I know,
java.lang.invoke.ProxyClassesDumper is only used if
'jdk.internal.lambda.dumpProxyClasses' system property is set to
point to a directory where lambda proxy class files are to be dumped
as they are generated - a debugging hook therefore. Wouldn't it be
good-enough if error messages about not-being able to set-up/use the
dump facility were output to System.err directly - not using
PlatformLogger at all?
The overloads the doPrivileged method makes it cumbersome to use
lambda that causes you to workaround it by adding a new
PrivilegedVoidAction interface which is clever. So I think it isn't
too bad for this patch to use anonymous
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/17/2013 04:29 PM, Jason Mehrens wrote:
Handlers can be subclassed. Is it a security concern when
doPrivileged is invoking non-final public/protected methods? For example,
@Override
public void setOutputStream(OutputStream out) {
LogManager.getLogManager().reset();
}
@Override
public void setLevel(Level l) {
LogManager.getLogManager().reset();
}
Jason
Good catch! The 'sealed' field did not allow that, since there was no
elevation of privilege. So doPrivileged is out of question, right?
Peter
Date: Mon, 16 Dec 2013 13:14:03 -0800
From: mandy.ch...@oracle.com
To: peter.lev...@gmail.com
Subject: Re: Theoretical data race on java.util.logging.Handler.sealed
CC: core-libs-dev@openjdk.java.net
On 12/14/2013 9:38 AM, Peter Levart wrote:
Hi,
Daniel reminded me of a couple of issues the 4th revision of the
patch
would have when backporting to 7u. So here's another variant that
tries to be more backport-friendly:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.05/
This looks good in general.
SocketHandler line 200 - it looks to me that this is an existing bug
that should call setOutputStream within doPrivileged.
I think it'd be simpler if SocketHandler no-arg constructor can first
get the port and host from the logging properties so that it doesn't
need to differentiate hostAndPortSet is set and ConfigureAction no-arg
constructor can be removed.
Daniel/Peter - do we have tests to cover these permission check for
these handlers?
Mandy
This variant could be backported by simply replacing a limited
variant
of doPrivileged (introduced in JDK 8) with full variant and still not
elevate the privilege of Socket creation in SocketHandler. I also
removed the need to subclass various ConfigureAction(s) with
annonymous inner subclasses by introducing overloaded constructors to
ConfigureActions(s) that follow the overloaded constructors of
various
Handlers.
Regards, Peter
On 12/14/2013 12:25 PM, Peter Levart wrote:
Hi Mandy,
On 12/13/2013 12:37 AM, Mandy Chung wrote:
Hi Peter,
On 12/8/2013 11:19 AM, Peter Levart wrote:
H Mandy,
I created an issue for it nevertheless:
https://bugs.openjdk.java.net/browse/JDK-8029781
You're right, doPrivileged() is a more straight-forward approach
than 'sealed' variable. Since this might only be considered for
inclusion in JDK9 when lambdas are already a tried technology, how
do you feel about using them for platform code like logging? As
far
as I know (just checked), lambda meta-factory is not using any
j.u.logging, so there is no danger of initialization loops or
similar:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/
Sorry for the delay to get to this.
No rush. We have time before JDK9 gets set-up and running...
Alan is right that java.lang.invoke.ProxyClassesDumper does use
PlatformLogger which will forward calls to j.u.logging if
-Djava.util.logging.config.file is set or java.util.logging has
been
initialized (via other j.u.logging call). It means that it may lead
to recursive initialization. Also the current PlatformLogger
implementation formats the message in the same way as j.u.logging
that may load locale providers and other classes. I am afraid there
are issues to tease out and resolve.
It's unfortunate that a lambda debugging feature prevents us from
using a basic language feature in j.u.logging code. As far as I
know,
java.lang.invoke.ProxyClassesDumper is only used if
'jdk.internal.lambda.dumpProxyClasses' system property is set to
point to a directory where lambda proxy class files are to be dumped
as they are generated - a debugging hook therefore. Wouldn't it be
good-enough if error messages about not-being able to set-up/use the
dump facility were output to System.err directly - not using
PlatformLogger at all?
The overloads the doPrivileged method makes it cumbersome to use
lambda that causes you to workaround it by adding a new
PrivilegedVoidAction interface which is clever. So I think it isn't
too bad for this patch to use anonymous inner class and have the
doPrivileged call in the constructor.
Right. I have prepared a modified webrev which doesn't use lambdas:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.04/
In attempt to minimize the boilerplate, I haven't just replaced
lambdas with anonymous inner classes, but rather turned all private
configure() methods into ConfigureAction inner classes. In two
occasions (SocketHandler and StreamHandler), they are extended with
anonymous inner classes to append some actions. In SocketHandler I
kept the mechanics of transporting the checked IOException out of
PrivilegedAction by wrapping it with Unchecked IOException and later
unwrapping and throwing it, rather than using
PrivilegedExceptionAction which would further
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/17/2013 05:26 PM, Mandy Chung wrote:
This is a good point. The patch for JDK 8 and above uses limited
doPrivileged that only grants LoggingPermission(control) access even
though the system class has all permissions and it should be no
difference than the current implementation. When we backport to 7u,
it would be an issue.
I think it's an issue for limited doPrivileged use too (JDK 8).
LoggingPermission(control) is a permission used to protect the whole
logging system. So what was not possible with 'sealed' field would be
possible with doPermission's temporary elevation of privilege even if it
is just for LoggingPermission(control).
We shall look into this and it's an existing issue to the current
implementation.
I don't think so. The 'sealed' field only pertains to permission checks
inside the Handler instance - not globally. I think we must fix the bug
by keeping 'sealed' field. One variant is by having final 'sealed'
field(s) in each Handler's subclass (as proposed initialy), the other
would be to replace the primitive boolean sealed field in Handler with:
final AtomicBoolean sealed = new AtomicBoolean(true);
Which one do you prefer?
Regards, Peter
thanks
Mandy
On 12/17/2013 7:29 AM, Jason Mehrens wrote:
Handlers can be subclassed. Is it a security concern when
doPrivileged is invoking non-final public/protected methods? For
example,
@Override
public void setOutputStream(OutputStream out) {
LogManager.getLogManager().reset();
}
@Override
public void setLevel(Level l) {
LogManager.getLogManager().reset();
}
Jason
Date: Mon, 16 Dec 2013 13:14:03 -0800
From: mandy.ch...@oracle.com
To: peter.lev...@gmail.com
Subject: Re: Theoretical data race on java.util.logging.Handler.sealed
CC: core-libs-dev@openjdk.java.net
On 12/14/2013 9:38 AM, Peter Levart wrote:
Hi,
Daniel reminded me of a couple of issues the 4th revision of the
patch
would have when backporting to 7u. So here's another variant that
tries to be more backport-friendly:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.05/
This looks good in general.
SocketHandler line 200 - it looks to me that this is an existing bug
that should call setOutputStream within doPrivileged.
I think it'd be simpler if SocketHandler no-arg constructor can first
get the port and host from the logging properties so that it doesn't
need to differentiate hostAndPortSet is set and ConfigureAction no-arg
constructor can be removed.
Daniel/Peter - do we have tests to cover these permission check for
these handlers?
Mandy
This variant could be backported by simply replacing a limited
variant
of doPrivileged (introduced in JDK 8) with full variant and still
not
elevate the privilege of Socket creation in SocketHandler. I also
removed the need to subclass various ConfigureAction(s) with
annonymous inner subclasses by introducing overloaded
constructors to
ConfigureActions(s) that follow the overloaded constructors of
various
Handlers.
Regards, Peter
On 12/14/2013 12:25 PM, Peter Levart wrote:
Hi Mandy,
On 12/13/2013 12:37 AM, Mandy Chung wrote:
Hi Peter,
On 12/8/2013 11:19 AM, Peter Levart wrote:
H Mandy,
I created an issue for it nevertheless:
https://bugs.openjdk.java.net/browse/JDK-8029781
You're right, doPrivileged() is a more straight-forward approach
than 'sealed' variable. Since this might only be considered for
inclusion in JDK9 when lambdas are already a tried technology,
how
do you feel about using them for platform code like logging?
As far
as I know (just checked), lambda meta-factory is not using any
j.u.logging, so there is no danger of initialization loops or
similar:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/
Sorry for the delay to get to this.
No rush. We have time before JDK9 gets set-up and running...
Alan is right that java.lang.invoke.ProxyClassesDumper does use
PlatformLogger which will forward calls to j.u.logging if
-Djava.util.logging.config.file is set or java.util.logging has
been
initialized (via other j.u.logging call). It means that it may
lead
to recursive initialization. Also the current PlatformLogger
implementation formats the message in the same way as j.u.logging
that may load locale providers and other classes. I am afraid
there
are issues to tease out and resolve.
It's unfortunate that a lambda debugging feature prevents us from
using a basic language feature in j.u.logging code. As far as I
know,
java.lang.invoke.ProxyClassesDumper is only used if
'jdk.internal.lambda.dumpProxyClasses' system property is set to
point to a directory where lambda proxy class files are to be
dumped
as they are generated - a debugging hook therefore. Wouldn't it be
good-enough if error messages about not-being able to set-up/use
the
dump facility were output to System.err
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/17/2013 11:02 PM, Daniel Fuchs wrote:
On 12/17/13 6:02 PM, Peter Levart wrote:
On 12/17/2013 05:26 PM, Mandy Chung wrote:
This is a good point. The patch for JDK 8 and above uses limited
doPrivileged that only grants LoggingPermission(control) access even
though the system class has all permissions and it should be no
difference than the current implementation. When we backport to 7u,
it would be an issue.
I think it's an issue for limited doPrivileged use too (JDK 8).
LoggingPermission(control) is a permission used to protect the whole
logging system. So what was not possible with 'sealed' field would be
possible with doPermission's temporary elevation of privilege even if it
is just for LoggingPermission(control).
Hi Peter,
I don't think that is an issue - because if Handler is subclassed
and the custom subclass attempts to call something requiring
LoggingPermission(control) then the custom subclass will
*still need* to have the control permission - otherwise a
security exception will be thrown. This is because the
custom subclass's protection domain will be in the code
path:
- callers call new XxxxHandler
- XxxxHandler call Handler's super constructor
- super constructor calls doPrivileged
- super constructor calls e.g. this.setLevel()
which is redefined as XxxxHandler.setLevel
- setLevel calls something else requiring control
At this point you have XxxxHandler.class in the code path so
the security manager will check that XxxxHandler.class has the
control permission.
Ah, I see. Thanks for the explanation. I haven't thought of different
protection domains. I have studied the AccessControler javadocs and I
think I understand the behavior now. So there's no problem even if
unlimited doPrivileged is used, or am I missing something?
But then we have another problem with doPrivileged approach, since it is
even more restrictive than 'sealed' field approach. Currently the
Handler's subclass that overrides a setter and calls super, works:
@Override
public void setOutputStream(OutputStream out) {
super.setOutputStream(out);
}
With doPrivileged wrapping setOutputStream in the superclass
constructor, it will throw SecurityException if the subclass protection
domain does not have the control permission...
Can this break custom handlers in some environment?
Regards, Peter
We shall look into this and it's an existing issue to the current
implementation.
I don't think so. The 'sealed' field only pertains to permission checks
inside the Handler instance - not globally. I think we must fix the bug
by keeping 'sealed' field. One variant is by having final 'sealed'
field(s) in each Handler's subclass (as proposed initialy), the other
would be to replace the primitive boolean sealed field in Handler with:
final AtomicBoolean sealed = new AtomicBoolean(true);
I don't think that would solve anything - it wouldn't prevent
the race condition - would it?
Which one do you prefer?
I think we should fix the potential race condition using
limited doPrivileged in 9. IMHO that does exactly what we
want.
best regards,
-- daniel
Regards, Peter
thanks
Mandy
On 12/17/2013 7:29 AM, Jason Mehrens wrote:
Handlers can be subclassed. Is it a security concern when
doPrivileged is invoking non-final public/protected methods? For
example,
@Override
public void setOutputStream(OutputStream out) {
LogManager.getLogManager().reset();
}
@Override
public void setLevel(Level l) {
LogManager.getLogManager().reset();
}
Jason
Date: Mon, 16 Dec 2013 13:14:03 -0800
From: mandy.ch...@oracle.com
To: peter.lev...@gmail.com
Subject: Re: Theoretical data race on
java.util.logging.Handler.sealed
CC: core-libs-dev@openjdk.java.net
On 12/14/2013 9:38 AM, Peter Levart wrote:
Hi,
Daniel reminded me of a couple of issues the 4th revision of the
patch
would have when backporting to 7u. So here's another variant that
tries to be more backport-friendly:
http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.05/
This looks good in general.
SocketHandler line 200 - it looks to me that this is an existing bug
that should call setOutputStream within doPrivileged.
I think it'd be simpler if SocketHandler no-arg constructor can
first
get the port and host from the logging properties so that it doesn't
need to differentiate hostAndPortSet is set and ConfigureAction
no-arg
constructor can be removed.
Daniel/Peter - do we have tests to cover these permission check for
these handlers?
Mandy
This variant could be backported by simply replacing a limited
variant
of doPrivileged (introduced in JDK 8) with full variant and still
not
elevate the privilege of Socket creation in SocketHandler. I also
removed the need to subclass various ConfigureAction(s) with
annonymous inner subclasses by introducing overloaded
constructors to
ConfigureActions(s) that follow the overloaded constructors
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/18/2013 12:05 PM, Daniel Fuchs wrote:
But then we have another problem with doPrivileged approach, since it is
even more restrictive than 'sealed' field approach. Currently the
Handler's subclass that overrides a setter and calls super, works:
@Override
public void setOutputStream(OutputStream out) {
super.setOutputStream(out);
}
With doPrivileged wrapping setOutputStream in the superclass
constructor, it will throw SecurityException if the subclass protection
domain does not have the control permission...
Can this break custom handlers in some environment?
Good question. It may yes - but on the other hand, a subclass of
handler that override such methods and call the super.method would
*need* to have the control permission too - wouldn't it?
Otherwise calling these methods when sealed is back to true would
always fail...
There might be no need to call the overridden setters after the Handler
instance is constructed. Apart from setLevel, Handler setters are only
called by Handlers themselves in construction phase and by user code...
If user code doesn't need to call the overridden setters after handlers
are constructed, user code doesn't need the control permission.
But than again, user would *need* to have control permission anyway to
make use of a newly constructed Handler. If all she can do is
instantiate new Handler instances and not add them to Loggers, what good
does it make?
Regards, Peter
best regards,
-- daniel
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/17/2013 06:43 PM, Mandy Chung wrote: Can you check what methods are called by the constructors whose access are denied in the current implementation but granted in the patch? I have to take another look to make sure but I believe they only calls the methods in the handler classes that calls Handler.checkPermission. Mandy Methods that are called by various Handler constructors and would be elevated by doPrivileged are: - Handler's own setters, protected by Handler.checkPermission - control permission required, - LogManager.getLevelProperty - no special permission required - LogManager.getFilterProperty - loads class configured in properties from system class loader and instantiates it. (since this is done by system class - LogManager, no special permission required besides those that might be imposed by constructor of the Filter (sub)class) - LogManager.getFormatterProperty - the same as getFilterProperty - LogManager.getStringProperty,getIntProperty - no special permission required That's about it. So the only methods that are not known and would be elevated by doPrivileged are no-args constructors of Filter and Formatter subclasses the names of which are obtained from logging properties and which must be loadable by system class loader. Regards, Peter
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/17/2013 06:43 PM, Mandy Chung wrote: Can you check what methods are called by the constructors whose access are denied in the current implementation but granted in the patch? I have to take another look to make sure but I believe they only calls the methods in the handler classes that calls Handler.checkPermission. Mandy Hi Mandy, Daniel, Here's yet another variant that reduces the doPrivileged code to just Handler's setters. This way no LogManager methods are invoked under elevated privilege: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.06/ I also factored-out common code into a single package-private Handler.configure() method + two package-private getters that provide this method with different defaults for different Handler subclasses. configure() is in this variant called only in immediate Handler subclasses: MemoryHandler StreamHandler and not in SocketHandler or ConsoleHandler. Each property is only set once this way at construction time - current code sets each property twice in SocketHandler or ConsoleHandler. SocketHandler's output stream is set with privileged action in both cases: whether constructed with configured or specified host/port. Regards, Peter
Re: Theoretical data race on java.util.logging.Handler.sealed
On 12/18/2013 11:55 PM, Mandy Chung wrote: On 12/18/2013 9:03 AM, Peter Levart wrote: Hi Mandy, Daniel, Here's yet another variant that reduces the doPrivileged code to just Handler's setters. This way no LogManager methods are invoked under elevated privilege: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.06/ This version looks good. I like the refactoring to have the subclass to call the common code Handler.configure method. It may be better to have the configure method (or a new one) that takes the default Level and default Formatter instead of the package-private getters. I don't see why the handler constructors are designed to call the overridden methods rather than the initialization and if a subclass has its custom field, it should initialize its custom fields in its constructor implementation.Anyway this would be a separate clean up task from this one. Can you also add a sanity test to verify that these handlers can be constructed successfully with a security manager installed? Hi Mandy, Daniel, I didn't like the package-protected getters either. So here's another variant that replaces Handler.configure() method with a package-protected constructor which is chained from JDK subclasses: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/ I filed another bug that is fixed by this patch: https://bugs.openjdk.java.net/browse/JDK-8030801 And I created a test (see webrev.07) that almost passes when run against unchanged JDK 8 (the failure is just at the end when calling new SocketHandler(host, port) - access denied (java.util.logging.LoggingPermission control)). If I comment-out the System.setSecurityManager() from the test, it passes with unchanged code. This is to verify the test itself. When run against the patched JDK 8, it passes even when SecurityManager is active - this verifies two things: - the behaviour of patched code is analogous to unpatched code as far as defaults and configured handler properties is concerned and it conforms to javadoc - the patched code does not require any new permissions - it actually requires less, because it fixes bug 8030801. All java/util/logging jtreg tests pass with patched code. I hope that localhost is a resolvable name on all environments and that new ServerSocket(0) creates a server socket bound at least to the IP address that localhost resolves to. Is this reasonable to assume? Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
Hi David,
Is it possible to get the test output when it fails? It can fail in two
different ways. I can't look at the bug (not authorized)...
On 12/20/2013 10:54 AM, Chris Hegarty wrote:
On 20 Dec 2013, at 04:33, Mandy Chung mandy.ch...@oracle.com wrote:
Hi Srikalyan,
Maybe you can get add an uncaught handler to see if you can get
any information.
+1. With this, at least the next time we see this failure we should have a
better idea where the OOM is coming from.
-Chris.
We can try, but I think the VM already prints the stack-trace of the
exception by default and as far as I remember, OOME thrown by VM is
preallocated and does not contain a stack trace. So I suspect we'll see
nothing more with the suggested UEH.
Is it possible to include in test, a modified version of Reference class
that would be prepended to boot-classpath? For example, containing the
following ReferenceHandler:
private static class ReferenceHandler extends Thread {
ReferenceHandler(ThreadGroup g, String name) {
super(g, name);
}
private volatile int state;
@Override
public String toString() {
return super.toString() + [state= + state + ];
}
public void run() {
for (;;) {
state = 1;
ReferenceObject r;
state = 2;
synchronized (lock) {
state = 3;
if (pending != null) {
state = 4;
r = pending;
state = 5;
pending = r.discovered;
state = 6;
r.discovered = null;
state = 7;
} else {
state = 8;
// The waiting on the lock may cause an OOME
because it may try to allocate
// exception objects, so also catch OOME here
to avoid silent exit of the
// reference handler thread.
//
// Explicitly define the order of the two
exceptions we catch here
// when waiting for the lock.
//
// We do not want to try to potentially load
the InterruptedException class
// (which would be done if this was its first
use, and InterruptedException
// were checked first) in this situation.
//
// This may lead to the VM not ever trying to
load the InterruptedException
// class again.
try {
state = 9;
try {
state = 10;
lock.wait();
state = 11;
} catch (InterruptedException x) { state =
12; }
state = 13;
} catch (OutOfMemoryError x) { state = 14; }
state = 15;
continue;
}
state = 16;
}
state = 17;
// Fast path for cleaners
if (r instanceof Cleaner) {
state = 18;
((Cleaner)r).clean();
state = 19;
continue;
}
state = 20;
ReferenceQueueObject q = (ReferenceQueue) r.queue;
state = 21;
if (q != ReferenceQueue.NULL) q.enqueue(r);
state = 22;
}
}
}
...then just include the toString of referenceHandlerThread instance as
part of the exception message at the end of the test:
...
...
// wait at most 10 seconds for success or failure
for (int i = 0; i 20; i++) {
if (refQueue.poll() != null) {
// Reference Handler thread still working - success
return;
}
System.gc();
Thread.sleep(500L); // wait a little to allow GC to do
it's work before allocating objects
if (!referenceHandlerThread.isAlive()) {
// Reference Handler thread died - failure
throw new Exception(Reference Handler thread died.
referenceHandlerThread: + referenceHandlerThread);
}
}
// no sure answer after 10 seconds
throw new IllegalStateException(Reference Handler thread
stuck. weakRef.get(): + weakRef.get() +
, referenceHandlerThread:
+ referenceHandlerThread);
}
This might be safer than using UEH since at the time the
UEH.uncaughtException() is called, the heap might still be full which
would prevent printing the message.
Re: Theoretical data race on java.util.logging.Handler.sealed
Hi Mandy, On 12/19/2013 10:38 PM, Mandy Chung wrote: On 12/19/13 7:49 AM, Peter Levart wrote: Hi Mandy, Daniel, I didn't like the package-protected getters either. So here's another variant that replaces Handler.configure() method with a package-protected constructor which is chained from JDK subclasses: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/ Looks good. Thanks for making the change and the new test. It'd be good to close the handlers by the test. The test is running in othervm mode and the Cleaner thread will close the handler when VM exits and the test is fine as it is. Well, not really. The Cleaner only closes Handlers that are attached to Loggers but the test just instantiates Handlers and doesn't add them to any Loggers. It's harmless as it is, othervm will exit nevertheless and resources will be freed... I tried closing Handlers at the end of test, but that requires control LoggingPermission and we don't want to run the test with control permission since we want to check that instantiating Handlers (SocketHandler too) doesn't require control permission. Digress: Just notice that the closeable handler classes are not AutoCloseable (they don't implement Closeable either). The close() method don't throw IOException but instead throws SecurityException an unchecked exception. Otherwise, we could use try-with-resources. I filed another bug that is fixed by this patch: https://bugs.openjdk.java.net/browse/JDK-8030801 And I created a test (see webrev.07) that almost passes when run against unchanged JDK 8 (the failure is just at the end when calling new SocketHandler(host, port) - access denied (java.util.logging.LoggingPermission control)). If I comment-out the System.setSecurityManager() from the test, it passes with unchanged code. This is to verify the test itself. When run against the patched JDK 8, it passes even when SecurityManager is active - this verifies two things: - the behaviour of patched code is analogous to unpatched code as far as defaults and configured handler properties is concerned and it conforms to javadoc - the patched code does not require any new permissions - it actually requires less, because it fixes bug 8030801. Yes I agree this is a bug that should get fixed. All java/util/logging jtreg tests pass with patched code. I hope that localhost is a resolvable name on all environments and that new ServerSocket(0) creates a server socket bound at least to the IP address that localhost resolves to. Is this reasonable to assume? localhost should be fine and there are other tests depending on it be resolvable. So should anything else be done before pushing this to jdk9/dev ? Regards, Peter thanks Mandy
Re: (reflect) Accessing members of inner annotations types
I think the problem is as follows... Annotations are implemented as Java
dynamic Proxy classes. The generated proxy class for a package-private
interface is created in the same package as the interface so that it has
access to the interface. The top level interface can be package private,
but nested interface (nested inside another interface) is by default
public (all interface members are by default public). The consequence is
that proxy class of an interface nested inside the interface is
generated in com.sun.proxy package:
package pkg;
import java.lang.reflect.Proxy;
public class ProxyTest {
public static void main(String[] args) throws Exception {
Class? iClass = Proxy.getProxyClass(I.class.getClassLoader(),
new Class[]{I.class});
Class? nClass = Proxy.getProxyClass(I.class.getClassLoader(),
new Class[]{I.N.class});
System.out.println(iClass: + iClass);
System.out.println(nClass: + nClass);
}
}
interface I {
interface N {}
}
prints:
iClass: class pkg.$Proxy0
nClass: class com.sun.proxy.$Proxy1
This is would be all right until such proxy class (com.sun.proxy.$Proxy1
in our example) has to access some package-private types in some
specific package. This happens in your Named.List annotation
implementation class. It implements a member method with the following
signature:
public Named[] value() {...
...where the return type is an array of a package-private type Named.
Public class in com.sun.proxy package can not access package-private
types in other packages!
Your best solution currently is to implement the top-level annotations
as public, or have all package-private annotations as top-level annotations.
In future, I think the logic to calculate the package of a proxy class
could be improved. It currently is as follows:
String proxyPkg = null; // package to define proxy class in
int accessFlags = Modifier.PUBLIC | Modifier.FINAL;
/*
* Record the package of a non-public proxy interface so
that the
* proxy class will be defined in the same package. Verify
that
* all non-public proxy interfaces are in the same package.
*/
for (Class? intf : interfaces) {
int flags = intf.getModifiers();
if (!Modifier.isPublic(flags)) {
accessFlags = Modifier.FINAL;
String name = intf.getName();
int n = name.lastIndexOf('.');
String pkg = ((n == -1) ? : name.substring(0, n
+ 1));
if (proxyPkg == null) {
proxyPkg = pkg;
} else if (!pkg.equals(proxyPkg)) {
throw new IllegalArgumentException(
non-public interfaces from different
packages);
}
}
}
if (proxyPkg == null) {
// if no non-public proxy interfaces, use com.sun.proxy
package
proxyPkg = ReflectUtil.PROXY_PACKAGE + .;
}
...It could be improved to take into account not only all interfaces
being implemented but also all types that are encountered in interface
method signatures. If any of those types is non-public than all of them
should be in the same package and proxy class would be generated in that
package...
I can file a bug/rfe for this.
Regards, Peter
On 01/03/2014 02:14 PM, Gunnar Morling wrote:
Hi,
In the course of running the Bean Validation TCK on JDK 8, I'm
investigating an issue around reflectively accessing members of annotations
which are declared as inner type of another, package-private annotation
type.
The following shows an example:
@Retention( RetentionPolicy.RUNTIME )
/*package-private */ @interface Named {
@Retention( RetentionPolicy.RUNTIME )
/*package-private */ @interface List {
Named[] value();
}
}
The @List annotation is used as this on a type in the same package:
public class Foo {
@Named.List( @Named )
public void getBar() {}
}
I'm then trying to access the @Named annotation using reflection like this:
Annotation listAnnotation = Foo.class.getMethod( getBar
).getAnnotations()[0];
Method method = listAnnotation.getClass().getMethod( value );
method.setAccessible( true );
//fails
Annotation namedAnnotation = ( (Annotation[]) method.invoke(
listAnnotation ) )[0];
This is the exception I get:
IllegalAccessError: tried to access class com.example.Named from class
com.sun.proxy.$Proxy3
at com.sun.proxy.$Proxy3.value(Unknown Source)
Interestingly, this only occurs when the List annotation is declared as an
inner type within the Named type; it works when the List annotation is a
top-level package-private type (the Named annotation still being
package-private) as well as when it is
Re: (reflect) Accessing members of inner annotations types
On 01/03/2014 03:52 PM, Peter Levart wrote:
This is would be all right until such proxy class
(com.sun.proxy.$Proxy1 in our example) has to access some
package-private types in some specific package. This happens in your
Named.List annotation implementation class. It implements a member
method with the following signature:
public Named[] value() {...
...where the return type is an array of a package-private type Named.
Public class in com.sun.proxy package can not access package-private
types in other packages!
Investigating this further, I found that the declaration itself is not
problematic. It's the code in the implemented proxy method that tries to
access the package-private Named class. Here's how the bytecode looks
like for such proxy method:
public final pkg.Named[] value() throws ;
Signature: ()[Lpkg/Named;
flags: ACC_PUBLIC, ACC_FINAL
Code:
stack=10, locals=2, args_size=1
0: aload_0
1: getfield #16 // Field
java/lang/reflect/Proxy.h:Ljava/lang/reflect/InvocationHandler;
4: aload_0
5: getstatic #67 // Field
m3:Ljava/lang/reflect/Method;
8: aconst_null
9: invokeinterface #28, 4 // InterfaceMethod
java/lang/reflect/InvocationHandler.invoke:(Ljava/lang/Object;Ljava/lang/reflect/Method;[Ljava/lang/Object;)Ljava/lang/Object;
*14: checkcast #69 // class [Lpkg/Named;*
17: areturn
18: athrow
19: astore_1
20: new #42 // class
java/lang/reflect/UndeclaredThrowableException
23: dup
24: aload_1
25: invokespecial #45 // Method
java/lang/reflect/UndeclaredThrowableException.init:(Ljava/lang/Throwable;)V
28: athrow
Exception table:
fromto target type
01818 Class java/lang/Error
01818 Class java/lang/RuntimeException
01819 Class java/lang/Throwable
... I think the error is thrown at the checkcast bytecode. The
improvement suggested still holds. If the proxy class was generated in
the specific package, error would not be thrown. But the requirement to
take into account all implemented interfaces and all types encountered
in the interface method signatures to calculate the package of proxy
class it too strict. Only implemented interfaces and return types of all
interface methods need to be taken into consideration. Here's an example
of bytecode that illustrates how method parameters are passed to
InvocationHandler:
public final void doWith(pkg.Named[]) throws ;
Signature: ([Lpkg/Named;)V
flags: ACC_PUBLIC, ACC_FINAL
Code:
stack=10, locals=3, args_size=2
0: aload_0
1: getfield #16 // Field
java/lang/reflect/Proxy.h:Ljava/lang/reflect/InvocationHandler;
4: aload_0
5: getstatic #57 // Field
m3:Ljava/lang/reflect/Method;
8: iconst_1
9: anewarray #22 // class java/lang/Object
12: dup
13: iconst_0
14: aload_1
15: aastore
16: invokeinterface #28, 4 // InterfaceMethod
java/lang/reflect/InvocationHandler.invoke:(Ljava/lang/Object;Ljava/lang/reflect/Method;[Ljava/lang/Object;)Ljava/lang/Object;
21: pop
22: return
23: athrow
24: astore_2
25: new #42 // class
java/lang/reflect/UndeclaredThrowableException
28: dup
29: aload_2
30: invokespecial #45 // Method
java/lang/reflect/UndeclaredThrowableException.init:(Ljava/lang/Throwable;)V
33: athrow
Exception table:
fromto target type
02323 Class java/lang/Error
02323 Class java/lang/RuntimeException
02324 Class java/lang/Throwable
... as can be seen, no parameter types are referenced in order to wrap
the parameters with Object[] array.
Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/07/2014 03:15 AM, srikalyan chandrashekar wrote: Sure David will give that a try, we have so far attempted to 1. Print state data(as per the test creator peter.levart's inputs), Hi Kalyan, Have you been able to reproduce the OOME in that set-up? What was the result? Regards, Peter 2. Use UEH(uncaught exception handler per Mandy's inputs) -- Thanks kalyan On 1/6/14 4:40 PM, David Holmes wrote: Back from vacation ... On 20/12/2013 4:49 PM, David Holmes wrote: On 20/12/2013 12:57 PM, srikalyan chandrashekar wrote: Hi David Thanks for your comments, the unguarded part(clean and enqueue) in the Reference Handler thread does not seem to create any new objects, so it is the application(the test in this case) which is adding objects to heap and causing the Reference Handler to die with OOME. The ReferenceHandler thread can only get OOME if it allocates (directly or indirectly) - so there has to be something in the unguarded part that causes this. Again it may be an implicit action in the VM - similar to the class load issue for InterruptedException. Run a debug VM with -XX:+TraceExceptions to see where the OOME is triggered. David - David I am still unsure about the side effects of the code change and agree with your thoughts(on memory exhaustion test's reliability). PS: hotspot dev alias removed from CC. -- Thanks kalyan On 12/19/13 5:08 PM, David Holmes wrote: Hi Kalyan, This is not a hotspot issue so I'm moving this to core-libs, please drop hotspot from any replies. On 20/12/2013 6:26 AM, srikalyan wrote: Hi all, I have been working on the bug JDK-8022321 https://bugs.openjdk.java.net/browse/JDK-8022321 , this is a sporadic failure and the webrev is available here http://cr.openjdk.java.net/~srikchan/Regression/JDK-8022321_OOMEInReferenceHandler-webrev/ I'm really not sure what to make of this. We have a test that triggers an out-of-memory condition but the OOME can actually turn up in the ReferenceHandler thread causing it to terminate and the test to fail. We previously accounted for the non-obvious occurrences of OOME due to the Object.wait and the possible need to load the InterruptedException class - but still the OOME can appear where we don't want it. So finally you have just placed the whole for(;;) loop in a try/catch(OOME) that ignores the OOME. I'm certain that makes the test happy, but I'm not sure it is really what we want for the ReferenceHandler thread. If the OOME occurs while cleaning, or enqueuing then we will fail to clean and/or enqueue but there would be no indication that has occurred and I think that is a bigger problem than this test failing. There may be no way to make this test 100% reliable. In fact I'd suggest that no memory exhaustion test can be 100% reliable. David * **Root Cause:Still not known* 2 places where there is a possibility for OOME 1) Cleaner.clean() 2) ReferenceQueue.enqueue() 1) The cleanup code in turn has 2 places where there is potential for throwing OOME, a) thunk Thread which is run from clean() method. This Runnable is passed to Cleaner and appears in the following classes java/nio/DirectByteBuffer.java sun/misc/Perf.java sun/nio/fs/NativeBuffer.java sun/nio/ch/IOVecWrapper.java sun/misc/Cleaner/ExitOnThrow.java However none of the above overridden implementations ever create an object in the clean() code. b) new PrivilegedAction created in try catch Exception block of clean() method but for this object to be created and to be held responsible for OOME an Exception(other than OOME) has to be thrown. 2) No new heap objects are created in the enqueue method nor anywhere in the deep call stack (VM.addFinalRefCount() etc) so this cannot be a potential cause. *Experimental change to java.lang.Reference.java* : - Put one more guard (try catch with OOME block) in the Reference Handler Thread which may give the Reference Handler a chance to cleanup. This is fixing the test failure (several 1000 runs with 0 failures) - Without the above change the test fails atleast 3-5 times for every 1000 run. *PS*: The code change is to a very critical part of JDK and i am fully not aware of the consequences of the change, hence seeking expert help here. Appreciate your time and inputs towards this.
Re: Theoretical data race on java.util.logging.Handler.sealed
Hi Mandy, Daniel, Thanks for reviews. I just pushed this change to jdk9-dev/jdk ... Regards, Peter On 12/23/2013 05:50 AM, Mandy Chung wrote: On 12/22/2013 5:23 AM, Peter Levart wrote: Hi Mandy, On 12/19/2013 10:38 PM, Mandy Chung wrote: On 12/19/13 7:49 AM, Peter Levart wrote: Hi Mandy, Daniel, I didn't like the package-protected getters either. So here's another variant that replaces Handler.configure() method with a package-protected constructor which is chained from JDK subclasses: http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/ Looks good. Thanks for making the change and the new test. It'd be good to close the handlers by the test. The test is running in othervm mode and the Cleaner thread will close the handler when VM exits and the test is fine as it is. Well, not really. The Cleaner only closes Handlers that are attached to Loggers but the test just instantiates Handlers and doesn't add them to any Loggers. It's harmless as it is, othervm will exit nevertheless and resources will be freed... I tried closing Handlers at the end of test, but that requires control LoggingPermission and we don't want to run the test with control permission since we want to check that instantiating Handlers (SocketHandler too) doesn't require control permission. Thanks and the test is fine as it is. So should anything else be done before pushing this to jdk9/dev ? Fix looks good and have a regression test. It's good to go and push to jdk9/dev.No other approval needed. thanks Mandy
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/08/2014 07:30 AM, David Holmes wrote:
On 8/01/2014 4:19 PM, David Holmes wrote:
On 8/01/2014 7:33 AM, srikalyan chandrashekar wrote:
Hi David, TraceExceptions with fastdebug build produced some nice trace
http://cr.openjdk.java.net/%7Esrikchan/OOME_exception_trace.log . The
native method wait(long) is where the OOME if being thrown, the deepest
call is in
src/share/vm/gc_interface/collectedHeap.inline.hpp, line 157
Yes but it is the caller that is of interest:
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/runtime/objectMonitor.cpp,
line 1649]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ae0} 'wait'
'(J)V' in 'java/lang/Object'
at bci 0 for thread 0x7f78c40d2800
The ReferenceHandler thread gets the OOME trying to allocate the
InterruptedException.
However we already have a catch block around the wait() so how is this
OOME getting through? A bug in exception handling in the interpreter ??
Might be. And it may have something to do with the fact that the
Thread.run() method is the 1st call frame on the thread's stack (seems
like corner case). The last few meaningful TraceExceptions records are:
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/gc_interface/collectedHeap.inline.hpp,
line 157]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/runtime/objectMonitor.cpp,
line 1649]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ae0} 'wait'
'(J)V' in 'java/lang/Object'
at bci 0 for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ca8} 'wait'
'()V' in 'java/lang/Object'
at *bci 2* for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b48d2250} 'run'
'()V' in 'java/lang/ref/Reference$ReferenceHandler'
at *bci 36* for thread 0x7f78c40d2800
Here's the relevant bytecodes:
public class java.lang.Object
public final void wait() throws java.lang.InterruptedException;
descriptor: ()V
flags: ACC_PUBLIC, ACC_FINAL
Code:
stack=3, locals=1, args_size=1
0: aload_0
1: lconst_0
* 2: invokevirtual #73 // Method wait:(J)V*
5: return
LineNumberTable:
line 502: 0
line 503: 5
Exceptions:
throws java.lang.InterruptedException
class java.lang.ref.Reference$ReferenceHandler extends java.lang.Thread
public void run();
descriptor: ()V
flags: ACC_PUBLIC
Code:
stack=2, locals=5, args_size=1
0: invokestatic #62 // Method
java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;
3: dup
4: astore_2
5: monitorenter
6: invokestatic #61 // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
9: ifnull33
12: invokestatic #61 // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
15: astore_1
16: aload_1
17: invokestatic #64 // Method
java/lang/ref/Reference.access$300:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
20: invokestatic #63 // Method
java/lang/ref/Reference.access$202:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
23: pop
24: aload_1
25: aconst_null
26: invokestatic #65 // Method
java/lang/ref/Reference.access$302:(Ljava/lang/ref/Reference;Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
29: pop
30: goto 52
33: invokestatic #62 // Method
java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;
*36: invokevirtual #59 // Method
java/lang/Object.wait:()V*
39: goto 43
42: astore_3
43: goto 47
46: astore_3
47: aload_2
48: monitorexit
49: goto 0
52: aload_2
53: monitorexit
54: goto 64
57: astore4
59: aload_2
60: monitorexit
61: aload 4
63: athrow
64: aload_1
65: instanceof#38 // class sun/misc/Cleaner
68: ifeq 81
71: aload_1
72: checkcast #38 // class sun/misc/Cleaner
75: invokevirtual #67 // Method
Re: (reflect) Accessing members of inner annotations types
On 01/08/2014 01:00 PM, Joel Borggren-Franck wrote:
Hi Peter,
On 2014-01-03, Peter Levart wrote:
On 01/03/2014 03:52 PM, Peter Levart wrote:
This is would be all right until such proxy class
(com.sun.proxy.$Proxy1 in our example) has to access some
package-private types in some specific package. This happens in
your Named.List annotation implementation class. It implements a
member method with the following signature:
public Named[] value() {...
...where the return type is an array of a package-private type
Named. Public class in com.sun.proxy package can not access
package-private types in other packages!
Investigating this further, I found that the declaration itself is
not problematic. It's the code in the implemented proxy method that
tries to access the package-private Named class. Here's how the
bytecode looks like for such proxy method:
... I think the error is thrown at the checkcast bytecode. The
improvement suggested still holds. If the proxy class was generated
in the specific package, error would not be thrown. But the
requirement to take into account all implemented interfaces and all
types encountered in the interface method signatures to calculate
the package of proxy class it too strict. Only implemented
interfaces and return types of all interface methods need to be
taken into consideration. Here's an example of bytecode that
illustrates how method parameters are passed to InvocationHandler:
Perhaps an alternative would be to check if the interface a proxy is
proxying is nested. In that case use the outermost interface's access
level for the package calculation.
This would probably lead to a few more proxies being generated into the
real package compared to your proposal. I don't know if that is ok or
not.
Hi Joel,
The nested public interface need not be referencing the outer
class/interface. In this case, proxy class can be generated in
com.sun.proxy. More importantly, even if the outer interface/class was
public, the nested interface could be referencing some other
package-private type. For example:
public class Outer {
@Retention(RUNTIME)
@interface PkgPrivate {}
@Retention(RUNTIME)
public @interface Public {
PkgPrivate value();
}
@Public(@PkgPrivate)
public void m() {}
}
I think the criteria for package selection should be closely tied to
requirements of the generated proxy class code. And the generated code
references the return type of the method.
Regards, Peter
cheers
/Joel
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/10/2014 12:59 AM, srikalyan chandrashekar wrote:
David/Peter you are right, the logs trace came from passed run, i am
trying to simulate the failure and get the logs for failed runs(2000+
runs done and still no failure), will get back to you once i have the
data from failed run. Sorry for the confusion.
I doubt the logs will be any different. A simple test that throws an
exception inside Thread.run() without catching it shows that
TraceExceptions doesn't report the fact that Thread.run() terminates
abruptly (as David pointed out, pending exception is reported after
every bytecode executed and there's no bytecode that invoked Thread.run()).
While you're at it, testing, could you also test the modified
ReferenceHandler (the one that calls private runImpl() from it's run()
method) so that we get a proof of incorrect behaviour.
Since we suspect there's something wrong with exception handling in
interpreter, I devised a hypothetical reproducer that tries to simulate
ReferenceHandler in many aspects, but doesn't require to be a
ReferenceHandler:
http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java
This is designed to run indefinitely and only terminate if/when thread
dies. Could you run this program in the environment that causes the
OOMEInReferenceHandler test to fail and see if it terminates?
Regards, Peter
---
Thanks
kalyan
On 01/08/2014 11:22 PM, David Holmes wrote:
Thanks Peter.
Kalyan: Can you confirm, as Peter asked, that the TraceExceptions
output came from a failed run?
AFAICS the Trace info is printed after each bytecode where there is a
pending exception - though I'm not 100% sure on the printing within
the VM runtime. Based on that I think we see the Trace output in
run() at the point where wait() returns, so it may well be caught
after that - in which case this was not a failing run.
I also can't reproduce the problem :(
David
On 8/01/2014 10:34 PM, Peter Levart wrote:
On 01/08/2014 07:30 AM, David Holmes wrote:
On 8/01/2014 4:19 PM, David Holmes wrote:
On 8/01/2014 7:33 AM, srikalyan chandrashekar wrote:
Hi David, TraceExceptions with fastdebug build produced some nice
trace
http://cr.openjdk.java.net/%7Esrikchan/OOME_exception_trace.log
. The
native method wait(long) is where the OOME if being thrown, the
deepest
call is in
src/share/vm/gc_interface/collectedHeap.inline.hpp, line 157
Yes but it is the caller that is of interest:
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/runtime/objectMonitor.cpp,
line 1649]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ae0} 'wait'
'(J)V' in 'java/lang/Object'
at bci 0 for thread 0x7f78c40d2800
The ReferenceHandler thread gets the OOME trying to allocate the
InterruptedException.
However we already have a catch block around the wait() so how is this
OOME getting through? A bug in exception handling in the
interpreter ??
Might be. And it may have something to do with the fact that the
Thread.run() method is the 1st call frame on the thread's stack (seems
like corner case). The last few meaningful TraceExceptions records are:
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/gc_interface/collectedHeap.inline.hpp,
line 157]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/runtime/objectMonitor.cpp,
line 1649]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ae0} 'wait'
'(J)V' in 'java/lang/Object'
at bci 0 for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ca8} 'wait'
'()V' in 'java/lang/Object'
at *bci 2* for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b48d2250} 'run'
'()V' in 'java/lang/ref/Reference$ReferenceHandler'
at *bci 36* for thread 0x7f78c40d2800
Here's the relevant bytecodes:
public class java.lang.Object
public final void wait() throws java.lang.InterruptedException;
descriptor: ()V
flags: ACC_PUBLIC, ACC_FINAL
Code:
stack=3, locals=1, args_size=1
0: aload_0
1: lconst_0
* 2: invokevirtual #73 // Method wait:(J)V*
5: return
LineNumberTable:
line 502: 0
line 503: 5
Exceptions:
throws java.lang.InterruptedException
class java.lang.ref.Reference$ReferenceHandler extends java.lang.Thread
public void run();
descriptor: ()V
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/10/2014 09:31 AM, Peter Levart wrote: Since we suspect there's something wrong with exception handling in interpreter, I devised a hypothetical reproducer that tries to simulate ReferenceHandler in many aspects, but doesn't require to be a ReferenceHandler: http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java This is designed to run indefinitely and only terminate if/when thread dies. Could you run this program in the environment that causes the OOMEInReferenceHandler test to fail and see if it terminates? I forgot to mention that in order for this long-running program to exhibit interpreter behaviour, it should be run with -Xint option. So I suggest: -Xmx24M -XX:-UseTLAB -Xint Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/10/2014 10:51 PM, srikalyan chandrashekar wrote:
Hi Peter the version you provided ran indefinitely(i put a 10 minute
timeout) and the program got interrupted(no error),
Did you run it with or without fastedbug -XX:+TraceExceptions ? If
with, it might be that fastdebug and/or -XX:+TraceExceptions changes the
execution a bit so that we can no longer reproduce the wrong behaviour.
even if there were to be an error you cannot print the string of
thread to console(these have been attempted earlier).
...it has been attempted to print toString in uncaught exception
handler. At that time, the heap is still full. I'm printing it after the
GC has cleared the heap. You can try that it works by commenting out the
try { and corresponding } catch (OOME x) {} exception handler...
- The test's running on interpreter mode, what i am watching for is
one error with trace. Without fastdebug build and -XX:+TraceExceptions
i am able to reproduce failure atleast 5 failures out of 1000 runs but
with fastdebug+Trace no luck yet(already past few 1000 runs).
It might be interesting to try with fastebug build but without the
-XX:+TraceExceptions option to see what has an effect on it. It might
also be interesting to try the modified ReferenceHandler (the one with
private runImpl() method called from run()) and with normal
non-fastdebug JDK. This info might be useful when one starts to inspect
the exception handling code in interpreter...
Regards, Peter
---
Thanks
kalyan
On 01/10/2014 02:57 AM, Peter Levart wrote:
On 01/10/2014 09:31 AM, Peter Levart wrote:
Since we suspect there's something wrong with exception handling in
interpreter, I devised a hypothetical reproducer that tries to
simulate ReferenceHandler in many aspects, but doesn't require to be
a ReferenceHandler:
http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java
This is designed to run indefinitely and only terminate if/when
thread dies. Could you run this program in the environment that
causes the OOMEInReferenceHandler test to fail and see if it
terminates?
I forgot to mention that in order for this long-running program to
exhibit interpreter behaviour, it should be run with -Xint option. So
I suggest:
-Xmx24M -XX:-UseTLAB -Xint
Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
from the above changes, Peter's suggestion to create and
call a
private runImpl() from run() in ReferenceHandler makes sense to me.
Why would we need this?
David
-
---
Thanks
kalyan
On 01/13/2014 03:57 PM, srikalyan wrote:
On 1/11/14, 6:15 AM, Peter Levart wrote:
On 01/10/2014 10:51 PM, srikalyan chandrashekar wrote:
Hi Peter the version you provided ran indefinitely(i put a 10
minute
timeout) and the program got interrupted(no error),
Did you run it with or without fastedbug -XX:+TraceExceptions
? If
with, it might be that fastdebug and/or -XX:+TraceExceptions
changes
the execution a bit so that we can no longer reproduce the wrong
behaviour.
With fastdebug -XX:TraceExceptions. I will try combination of
possible options(i.e without -XX:TraceEception on debug build etc)
soon.
even if there were to be an error you cannot print the
string of
thread to console(these have been attempted earlier).
...it has been attempted to print toString in uncaught exception
handler. At that time, the heap is still full. I'm printing it
after
the GC has cleared the heap. You can try that it works by
commenting
out the try { and corresponding } catch (OOME x) {} exception
handler...
Since there is a GC call prior to printing string i will give
that a
shot with non-debug build.
- The test's running on interpreter mode, what i am watching
for is
one error with trace. Without fastdebug build and
-XX:+TraceExceptions i am able to reproduce failure atleast 5
failures out of 1000 runs but with fastdebug+Trace no luck
yet(already past few 1000 runs).
It might be interesting to try with fastebug build but without the
-XX:+TraceExceptions option to see what has an effect on it. It
might
also be interesting to try the modified ReferenceHandler (the one
with private runImpl() method called from run()) and with normal
non-fastdebug JDK. This info might be useful when one starts to
inspect the exception handling code in interpreter...
Regards, Peter
--
Thanks
kalyan
Ph: (408)-585-8040
---
Thanks
kalyan
On 01/10/2014 02:57 AM, Peter Levart wrote:
On 01/10/2014 09:31 AM, Peter Levart wrote:
Since we suspect there's something wrong with exception
handling
in interpreter, I devised a hypothetical reproducer that
tries to
simulate ReferenceHandler in many aspects, but doesn't
require to
be a ReferenceHandler:
http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java
This is designed to run indefinitely and only terminate if/when
thread dies. Could you run this program in the environment that
causes the OOMEInReferenceHandler test to fail and see if it
terminates?
I forgot to mention that in order for this long-running
program to
exhibit interpreter behaviour, it should be run with -Xint
option.
So I suggest:
-Xmx24M -XX:-UseTLAB -Xint
Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/17/2014 02:00 PM, Peter Levart wrote:
On 01/17/2014 05:38 AM, David Holmes wrote:
On 17/01/2014 1:31 PM, srikalyan chandrashekar wrote:
Hi David, the disassembled code is also attached to the bug. Per my
Sorry missed that.
analysis the exception was thrown when Reference Handler was on line
143
as put in the earlier email.
But if the numbers in the dissassembly match the BCI then 65 shows:
65: instanceof#11 // class sun/misc/Cleaner
which makes more sense, the runtime instanceof check might encounter
an OOME condition. I wish there was some easy way to trace into the
full call chain as TraceExceptions doesn't show you any runtime
frames :(
Still, it is easy enough to check:
// Fast path for cleaners
boolean isCleaner = false;
try {
isCleaner = r instanceof Cleaner;
} catch (OutofMemoryError oome) {
continue;
}
if (isCleaner) {
((Cleaner)r).clean();
continue;
}
Hi David, Kalyan,
I've caught-up now. Just thinking: is instanceof Cleaner throwing
OOME as a result of loading the Cleaner class? Wouldn't the above code
then throw some error also in ((Cleaner)r) - the checkcast, since
Cleaner class would not be successfully initialized?
Well, no. The above code would just skip Cleaner processing in this
situation. And will never be doing it again after the heap is freed...
So it might be good to load and initialize Cleaner class as part of
ReferenceHandler initialization to ensure correct operation...
Peter
Perhaps we should pre-load and initialize the Cleaner class as part of
ReferenceHandler initialization...
Regards, Peter
Thanks,
David
--
Thanks
kalyan
On 1/16/14 6:16 PM, David Holmes wrote:
On 17/01/2014 4:48 AM, srikalyan wrote:
Hi David
On 1/15/14, 9:04 PM, David Holmes wrote:
On 16/01/2014 10:19 AM, srikalyan chandrashekar wrote:
Hi Peter/David, we could finally get a trace of exception with
fastdebug
build and ReferenceHandler modified (with runImpl() added and
called
from run()). The logs, disassembled code is available in JIRA
https://bugs.openjdk.java.net/browse/JDK-8022321 as attachments.
All I can see is the log for the OOMECatchingTest program not one
for
the actual ReferenceHandler ??
Please search for ReferenceHandler in the log.
Observations from the log:
Root Cause:
1) UncaughtException is being dispatched from Reference.java:143
141 ReferenceObject r;
142 synchronized (lock) {
143if (pending != null) {
144r = pending;
145pending = r.discovered;
146r.discovered = null;
pending field in Reference is touched and updated by the
collector, so
at line 143 when the execution context is in Reference handler
there
might have been an Exception pending due to allocation done by
collector
which causes ReferenceHandler thread to die.
Sorry but the GC does not trigger asynchronous exceptions so this
explanation does not make any sense to me. What part of the log led
you to this conclusion?
-- Log Excerpt begins --
Exception a 'java/lang/OutOfMemoryError' (0xff7808e8)
thrown
[/home/srikalyc/work/ora2013/infracleanup/jdk8/hotspot/src/share/vm/gc_interface/collectedHeap.inline.hpp,
line 168]
for thread 0x7feed80cf800
Exception a 'java/lang/OutOfMemoryError' (0xff7808e8)
thrown in interpreter method {method} {0x7feeddd3c600}
'runImpl'
'()V' in 'java/lang/ref/Reference$ReferenceHandler'
at bci 65 for thread 0x7feed80cf800
Exception a 'java/lang/OutOfMemoryError' (0xff7808e8)
thrown in interpreter method {method} {0x7feeddd3c478} 'run'
'()V' in 'java/lang/ref/Reference$ReferenceHandler'
at bci 1 for thread 0x7feed80cf800
Exception a 'java/lang/OutOfMemoryError' (0xff780868)
thrown
[/home/srikalyc/work/ora2013/infracleanup/jdk8/hotspot/src/share/vm/gc_interface/collectedHeap.inline.hpp,
line 157]
for thread 0x7feed80cf800
Exception a 'java/lang/OutOfMemoryError' (0xff780868)
thrown in interpreter method {method} {0x7feeddcaaf90}
'uncaughtException' '(Ljava/lang/Thread;Ljava/lang/Throwable;)V'
in '
at bci 48 for thread 0x7feed80cf800
Exception a 'java/lang/OutOfMemoryError' (0xff780868)
thrown in interpreter method {method} {0x7feeddca7298}
'dispatchUncaughtException' '(Ljava/lang/Throwable;)V' in
'java/lang/
at bci 6 for thread 0x7feed80cf800
-- Log Excerpt ends --
Sorry if it is a wrong understanding.
What you are seeing there is an OOME escaping the run() method which
will cause the uncaughtExceptionHandler to be run which then triggers
a second OOME (likely as it tries to report information about the
first OOME). The first exception occurred in runImpl at BCI 65. Can
you disassemble (javap -c) the class you used so we can see what is at
BCI 65.
Thanks,
David
Suggested fix
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/17/2014 02:13 PM, Peter Levart wrote:
// Fast path for cleaners
boolean isCleaner = false;
try {
isCleaner = r instanceof Cleaner;
} catch (OutofMemoryError oome) {
continue;
}
if (isCleaner) {
((Cleaner)r).clean();
continue;
}
Hi David, Kalyan,
I've caught-up now. Just thinking: is instanceof Cleaner throwing
OOME as a result of loading the Cleaner class? Wouldn't the above
code then throw some error also in ((Cleaner)r) - the checkcast,
since Cleaner class would not be successfully initialized?
Well, no. The above code would just skip Cleaner processing in this
situation. And will never be doing it again after the heap is freed...
So it might be good to load and initialize Cleaner class as part of
ReferenceHandler initialization to ensure correct operation...
Well, yes and no. Let me try once more:
Above code will skip Cleaner processing if the 1st time instanceof
Cleaner is executed, OOME is thrown as a consequence of full heap while
loading and initializing the Cleaner class. The 2nd time the instanceof
Cleaner is executed after such OOME, the same line would throw
NoClassDefFoundError as a consequence of referencing a class that failed
initialization. Am I right?
Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/20/2014 02:51 AM, David Holmes wrote:
Hi Peter,
On 17/01/2014 11:24 PM, Peter Levart wrote:
On 01/17/2014 02:13 PM, Peter Levart wrote:
// Fast path for cleaners
boolean isCleaner = false;
try {
isCleaner = r instanceof Cleaner;
} catch (OutofMemoryError oome) {
continue;
}
if (isCleaner) {
((Cleaner)r).clean();
continue;
}
Hi David, Kalyan,
I've caught-up now. Just thinking: is instanceof Cleaner throwing
OOME as a result of loading the Cleaner class? Wouldn't the above
code then throw some error also in ((Cleaner)r) - the checkcast,
since Cleaner class would not be successfully initialized?
Well, no. The above code would just skip Cleaner processing in this
situation. And will never be doing it again after the heap is freed...
So it might be good to load and initialize Cleaner class as part of
ReferenceHandler initialization to ensure correct operation...
Well, yes and no. Let me try once more:
Above code will skip Cleaner processing if the 1st time instanceof
Cleaner is executed, OOME is thrown as a consequence of full heap while
loading and initializing the Cleaner class.
Yes - I was assuming that this would not fail the very first time and
so the Cleaner class would already be loaded. Failing to be able to
load the Cleaner class was one of the potential issues flagged earlier
with this problem. I was actually assuming that Cleaner would be
loaded already due to some actual Cleaner subclasses being used, but
this does not happen as part of the default initialization. :( The
irony being that if the Cleaner class is not loaded then r can not be
an instance of Cleaner and so we would fail to load the class in a
case where we didn't need it anyway.
What I wanted to focus on here was an OOME from the instanceof itself,
but as you say that might trigger classloading of Cleaner (which is
not what I was interested in).
The 2nd time the instanceof
Cleaner is executed after such OOME, the same line would throw
NoClassDefFoundError as a consequence of referencing a class that failed
initialization. Am I right?
instanceof is not one of the class initialization triggers, so we
should not see an OOME generated due to a class initialization
exception and so the class will not be put into the Erroneous state
and so subsequent attempts to use the class will not automatically
trigger NoClassdefFoundError.
If OOME occurs during actual loading/linking of the class Cleaner it
is unclear what would happen on subsequent attempts. OOME is not a
LinkageError that must be rethrown on subsequent attempts, and it is
potentially a transient condition, so I would expect a re-load attempt
to be allowed. However we are now deep into the details of the VM and
it may well depend on the exact place from which the OOME originates.
The bottom line with the current problem is that there are multiple
non-obvious paths by which the ReferenceHandler can encounter an OOME.
In such cases we do not want the ReferenceHandler to terminate - which
implies catching the OOME and continuing. However we also do not want
to silently skip Cleaner processing or reference queue processing - as
that would lead to hard to diagnoze bugs. But trying to report the
problem may not be possible due to being out-of-memory. It may be that
we need to break things up into multiple try/catch blocks, where each
catch does a System.gc() and then reports that the OOME occurred. Of
course the reporting must still be in a try/catch for the OOME. Though
at some point letting the ReferenceHandler die may be the only way to
report a major memory problem.
David
Hm... If I give -verbose:class option to run a simple test program:
public class Test { public static void main(String... a) {} }
I see Cleaner class being loaded before Test class. I don't see by which
tread or if it might get loaded after main() starts, but I suspect that
loading of Cleaner is not a problem here. Initialization of Cleaner
class is not performed by ReferenceHandler thread as you pointed out.
The instanceof does not trigger it and if it returns true then Cleaner
has already been initialized. So there must be some other cause for
instanceof throwing OOME...
What do you say about this variant of ReferenceHandler.run() method:
public void run() {
for (;;) {
Reference r;
Cleaner c;
synchronized (lock) {
r = pending;
if (r != null) {
// instanceof operator might throw OOME
sometimes. Just retry after
// yielding - might have better luck next time...
try {
c = r instanceof Cleaner ? (Cleaner) r : null;
} catch (OutOfMemoryError x) {
Thread.yield();
continue;
}
pending = r.discovered
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
skipped a Reference because of OOME, but
if we just re-try until we eventually succeed, nothing is lost, nothing
to report (but a slow response)...
Your suggested approach seems okay though I'm not sure why we
shouldn't help things along by calling System.gc() ourselves rather
than just yielding and hoping things will get cleaned up elsewhere.
But for the present purposes your approach will suffice I think.
Maybe my understanding is wrong but isn't the fact that OOME is rised a
consequence of that VM has already attempted to clear things up
(executing a GC round synchronously) but didn't succeed to make enough
free space to satisfy the allocation request? If this is only how some
collectors/allocators are implemented and not a general rule, then we
should put a System.gc() in place of Thread.yield(). Should we also
combine that with Thread.yield()? I'm concerned of a possibility that we
spin, consume too much CPU (ReferenceHandler thread has MAX priority) so
that other threads dont' get enough CPU time to proceed and clean things
up (we hope other threads will also get OOME and release things as their
stacks unwind...).
Regards, Peter
Thanks,
David
On 20/01/2014 6:42 PM, Peter Levart wrote:
On 01/20/2014 09:00 AM, Peter Levart wrote:
On 01/20/2014 02:51 AM, David Holmes wrote:
Hi Peter,
On 17/01/2014 11:24 PM, Peter Levart wrote:
On 01/17/2014 02:13 PM, Peter Levart wrote:
// Fast path for cleaners
boolean isCleaner = false;
try {
isCleaner = r instanceof Cleaner;
} catch (OutofMemoryError oome) {
continue;
}
if (isCleaner) {
((Cleaner)r).clean();
continue;
}
Hi David, Kalyan,
I've caught-up now. Just thinking: is instanceof Cleaner throwing
OOME as a result of loading the Cleaner class? Wouldn't the above
code then throw some error also in ((Cleaner)r) - the checkcast,
since Cleaner class would not be successfully initialized?
Well, no. The above code would just skip Cleaner processing in this
situation. And will never be doing it again after the heap is
freed...
So it might be good to load and initialize Cleaner class as part of
ReferenceHandler initialization to ensure correct operation...
Well, yes and no. Let me try once more:
Above code will skip Cleaner processing if the 1st time instanceof
Cleaner is executed, OOME is thrown as a consequence of full heap
while
loading and initializing the Cleaner class.
Yes - I was assuming that this would not fail the very first time and
so the Cleaner class would already be loaded. Failing to be able to
load the Cleaner class was one of the potential issues flagged
earlier with this problem. I was actually assuming that Cleaner would
be loaded already due to some actual Cleaner subclasses being used,
but this does not happen as part of the default initialization. :(
The irony being that if the Cleaner class is not loaded then r can
not be an instance of Cleaner and so we would fail to load the class
in a case where we didn't need it anyway.
What I wanted to focus on here was an OOME from the instanceof
itself, but as you say that might trigger classloading of Cleaner
(which is not what I was interested in).
The 2nd time the instanceof
Cleaner is executed after such OOME, the same line would throw
NoClassDefFoundError as a consequence of referencing a class that
failed
initialization. Am I right?
instanceof is not one of the class initialization triggers, so we
should not see an OOME generated due to a class initialization
exception and so the class will not be put into the Erroneous state
and so subsequent attempts to use the class will not automatically
trigger NoClassdefFoundError.
If OOME occurs during actual loading/linking of the class Cleaner it
is unclear what would happen on subsequent attempts. OOME is not a
LinkageError that must be rethrown on subsequent attempts, and it is
potentially a transient condition, so I would expect a re-load
attempt to be allowed. However we are now deep into the details of
the VM and it may well depend on the exact place from which the OOME
originates.
The bottom line with the current problem is that there are multiple
non-obvious paths by which the ReferenceHandler can encounter an
OOME. In such cases we do not want the ReferenceHandler to terminate
- which implies catching the OOME and continuing. However we also do
not want to silently skip Cleaner processing or reference queue
processing - as that would lead to hard to diagnoze bugs. But trying
to report the problem may not be possible due to being out-of-memory.
It may be that we need to break things up into multiple try/catch
blocks, where each catch does a System.gc() and then reports that the
OOME occurred. Of course the reporting must still be in a try/catch
for the OOME. Though at some point letting the ReferenceHandler die
may be the only way to report a major memory problem.
David
Hm... If I give -verbose:class option to run a simple test program:
public class Test { public static void main(String
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
Hi, David, Kalyan,
Summing up the discussion, I propose the following patch for
ReferenceHandler:
http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/
all 10 java/lang/ref tests pass on my PC (including OOMEInReferenceHandler).
I kindly ask Kalyan to try to re-run the OOMEInReferenceHandler test
with this code and report any failure.
Thanks, Peter
On 01/21/2014 08:57 AM, David Holmes wrote:
On 21/01/2014 4:54 PM, Peter Levart wrote:
On 01/21/2014 03:22 AM, David Holmes wrote:
Hi Peter,
I do not see Cleaner being loaded prior to the main class on either
Windows or Linux. Which platform are you on? Did you see it loaded
before the main class or as part of executing it?
Before. The main class is empty:
public class Test { public static void main(String... a) {} }
Here's last few lines of -verbose:class:
[Loaded java.util.TimeZone from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfo from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$1 from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInput from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInputStream from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
*[Loaded sun.misc.Cleaner from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]*
Curious. I wonder what the controlling factor is ??
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 perhaps it would be good
to trigger Cleaner loading and initialization as part of
ReferenceHandler initialization to play things safe.
If we do that for Cleaner we may as well do it for
InterruptedException too.
Also, it is not that I think ReferenceHandler is responsible for
reporting OOME, but that it is responsible for reporting that it was
unable to perform a clean or enqueue because of OOME.
This would be necessary if we skipped a Reference because of OOME, but
if we just re-try until we eventually succeed, nothing is lost, nothing
to report (but a slow response)...
Agreed - just trying to clarify things.
Your suggested approach seems okay though I'm not sure why we
shouldn't help things along by calling System.gc() ourselves rather
than just yielding and hoping things will get cleaned up elsewhere.
But for the present purposes your approach will suffice I think.
Maybe my understanding is wrong but isn't the fact that OOME is rised a
consequence of that VM has already attempted to clear things up
(executing a GC round synchronously) but didn't succeed to make enough
free space to satisfy the allocation request? If this is only how some
collectors/allocators are implemented and not a general rule, then we
should put a System.gc() in place of Thread.yield(). Should we also
combine that with Thread.yield()? I'm concerned of a possibility that we
spin, consume too much CPU (ReferenceHandler thread has MAX priority) so
that other threads dont' get enough CPU time to proceed and clean things
up (we hope other threads will also get OOME and release things as their
stacks unwind...).
You are probably right about the System.gc() - OOME should be thrown
after GC fails to create space, so it really needs some other thread
to drop live references to allow further space to be reclaimed.
But note that Thread.yield() can behave badly on some linux systems
too, so spinning is still a possibility - but either way this would
only be really bad on a uniprocessor system where yield() is
unlikely to misbehave.
David
-
Regards, Peter
Thanks,
David
On 20/01/2014 6:42 PM, Peter Levart wrote:
On 01/20/2014 09:00 AM, Peter Levart wrote:
On 01/20/2014 02:51 AM, David Holmes wrote:
Hi Peter,
On 17/01/2014 11:24 PM, Peter Levart wrote:
On 01/17/2014 02:13 PM, Peter Levart wrote:
// Fast path for cleaners
boolean isCleaner = false;
try {
isCleaner = r instanceof Cleaner;
} catch (OutofMemoryError oome) {
continue;
}
if (isCleaner) {
((Cleaner)r).clean();
continue;
}
Hi David, Kalyan,
I've caught-up now. Just thinking: is instanceof Cleaner
throwing
OOME as a result of loading the Cleaner class? Wouldn't the above
code then throw some error also in ((Cleaner)r) - the checkcast,
since Cleaner class would not be successfully initialized?
Well, no. The above code would just skip Cleaner processing in
this
situation. And will never be doing it again after the heap is
freed...
So it might be good to load and initialize Cleaner class as
part of
ReferenceHandler initialization to ensure correct operation...
Well, yes and no. Let me try once more:
Above code will skip Cleaner processing if the 1st time instanceof
Cleaner is executed, OOME is thrown as a consequence of full heap
while
loading and initializing the Cleaner class.
Yes - I was assuming
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/21/2014 08:57 AM, David Holmes wrote:
[Loaded java.util.TimeZone from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfo from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$1 from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInput from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInputStream from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
*[Loaded sun.misc.Cleaner from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]*
Curious. I wonder what the controlling factor is ??
The Cleaner is usually loaded by ReferenceHandler in JDK8 in the 1st
execution of it's loop. It looks like JDK8 system initialization
produces at least one XXXReference that is cleared before main() method
is entered (debugging, I found it's a Finalizer for a FileInputStream -
perhaps of the stream that loads the TimeZone data), so ReferenceHandler
thread is woken-up, executes the instanceof Cleaner check and this loads
the class. I put the following printfs in an original ReferenceHandler:
System.out.println(Before using Cleaner...);
// Fast path for cleaners
if (r instanceof Cleaner) {
((Cleaner)r).clean();
continue;
}
System.out.println(After using Cleaner...);
...and the empty main() test with -verbose:class prints:
...
[Loaded java.io.DataInput from
/home/peter/work/hg/jdk8-tl/build/linux-x86_64-normal-server-release/images/j2sdk-image/jre/lib/rt.jar]
[Loaded java.io.DataInputStream from
/home/peter/work/hg/jdk8-tl/build/linux-x86_64-normal-server-release/images/j2sdk-image/jre/lib/rt.jar]
*Before using Cleaner...**
**[Loaded sun.misc.Cleaner from out/production/jdk]**
**After using Cleaner...*
[Loaded java.io.ByteArrayInputStream from
/home/peter/work/hg/jdk8-tl/build/linux-x86_64-normal-server-release/images/j2sdk-image/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$ZoneOffsetTransitionRule from
/home/peter/work/hg/jdk8-tl/build/linux-x86_64-normal-server-release/images/j2sdk-image/jre/lib/rt.jar]
...
But sometimes, It seems, the VM is not so quick in clearing the early
XXXReferences and/or the ReferenceHandler start-up is delayed and the
1st iteration of the loop is executed after the OOMEInReferenceHandler
test already fills the heap and consequently loading of Cleaner class
throws OOME in instanceof check...
My proposed fix is very aggressive. It pre-loads classes, initializes
them and watches for OOMEs thrown in all ocasions. It might be that
pre-loading Cleaner class in ReferenceHandler initialization would be
sufficient to fix this intermittent failure. Or do you think instanceof
check could throw OOME for some other reason besides loading of the class?
Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
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 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 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 01/22/2014 03:19 AM, David Holmes wrote:
Hi Peter,
On 22/01/2014 12:00 AM, Peter Levart wrote:
Hi, David, Kalyan,
Summing up the discussion, I propose the following patch for
ReferenceHandler:
http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/
I can live with it, though it maybe that once Cleaner has been
preloaded instanceof can no longer throw OOME. Can't be 100% sure. And
there's some duplication/verbosity in the commentary that could be
trimmed down :)
Any specific reason to use Unsafe to do the preload rather than
Class.forName ? Does this force Unsafe to be loaded earlier than it
otherwise would?
Good question. In systemDictionary.hpp they are both on the preloaded
list in this order:
do_klass(Reference_klass, java_lang_ref_Reference,
Pre ) \
...
do_klass(misc_Unsafe_klass,
sun_misc_Unsafe, Pre ) \
So when Reference is initialized, the Unsafe is already loaded. But I
don't know if it is already initialized. This should be studied.
I'll try to find out what is the case and get back to you.
Regards, Peter
Thanks,
David
all 10 java/lang/ref tests pass on my PC (including
OOMEInReferenceHandler).
I kindly ask Kalyan to try to re-run the OOMEInReferenceHandler test
with this code and report any failure.
Thanks, Peter
On 01/21/2014 08:57 AM, David Holmes wrote:
On 21/01/2014 4:54 PM, Peter Levart wrote:
On 01/21/2014 03:22 AM, David Holmes wrote:
Hi Peter,
I do not see Cleaner being loaded prior to the main class on either
Windows or Linux. Which platform are you on? Did you see it loaded
before the main class or as part of executing it?
Before. The main class is empty:
public class Test { public static void main(String... a) {} }
Here's last few lines of -verbose:class:
[Loaded java.util.TimeZone from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfo from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$1 from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInput from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInputStream from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
*[Loaded sun.misc.Cleaner from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]*
Curious. I wonder what the controlling factor is ??
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 perhaps it would be
good
to trigger Cleaner loading and initialization as part of
ReferenceHandler initialization to play things safe.
If we do that for Cleaner we may as well do it for
InterruptedException too.
Also, it is not that I think ReferenceHandler is responsible for
reporting OOME, but that it is responsible for reporting that it was
unable to perform a clean or enqueue because of OOME.
This would be necessary if we skipped a Reference because of OOME, but
if we just re-try until we eventually succeed, nothing is lost,
nothing
to report (but a slow response)...
Agreed - just trying to clarify things.
Your suggested approach seems okay though I'm not sure why we
shouldn't help things along by calling System.gc() ourselves rather
than just yielding and hoping things will get cleaned up elsewhere.
But for the present purposes your approach will suffice I think.
Maybe my understanding is wrong but isn't the fact that OOME is
rised a
consequence of that VM has already attempted to clear things up
(executing a GC round synchronously) but didn't succeed to make enough
free space to satisfy the allocation request? If this is only how some
collectors/allocators are implemented and not a general rule, then we
should put a System.gc() in place of Thread.yield(). Should we also
combine that with Thread.yield()? I'm concerned of a possibility
that we
spin, consume too much CPU (ReferenceHandler thread has MAX
priority) so
that other threads dont' get enough CPU time to proceed and clean
things
up (we hope other threads will also get OOME and release things as
their
stacks unwind...).
You are probably right about the System.gc() - OOME should be thrown
after GC fails to create space, so it really needs some other thread
to drop live references to allow further space to be reclaimed.
But note that Thread.yield() can behave badly on some linux systems
too, so spinning is still a possibility - but either way this would
only be really bad on a uniprocessor system where yield() is
unlikely to misbehave.
David
-
Regards, Peter
Thanks,
David
On 20/01/2014 6:42 PM, Peter Levart wrote:
On 01/20/2014 09:00 AM, Peter Levart wrote:
On 01/20/2014 02:51 AM, David Holmes wrote:
Hi Peter,
On 17/01/2014 11:24 PM, Peter Levart wrote:
On 01/17/2014 02:13 PM, Peter Levart
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 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 /home/peter/Apps64
Re: RFR(s): 8023541 Race condition in rmid initialization
Hi Stuart,
My eye was caught by the following new method:
328 private synchronized void awaitInitialized() {
329 try {
330 while (!initialized) {
331 wait();
332 }
333 } catch (InterruptedException ie) {
334 Thread.currentThread().interrupt();
335 }
336 }
...if the thread calling this method is interrupted while waiting, it
will set the interrupted status of the thread back, but will terminate
the waiting loop. So the initialization race is still possible. Why not
wait until initialized like:
boolean interrupted = false;
while (!initialized) {
try {
wait();
} catch (InterruptedException ie) {
interrupted = true;
}
}
if (interrupted) {
Thread.currentThread().interrupt();
}
Regards, Peter
boolean interrupted = false;
On 01/29/2014 07:51 AM, Stuart Marks 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
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/28/2014 04:46 PM, Alan Bateman wrote: 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 That's right. If I add the source for ReferenceQueue.java into a directory where Reference.java resides and then compile with: javac -d /tmp Reference.java ...then Reference as well as ReferenceQueue gets compiled and there's no error. If there is sole Reference.java in the directory, a compile time error is emitted. I checked the source of ReferenceQueue.java in JDK 8 ea-b121 (the JDK used for compiling) and it only differs in copyright year from the source in jdk9-dev. So there seems to be inconsistency in javac's handling of types that are read from .class vs. .java files. I'll try to create a reproducer example and post it to compiler-dev. Since I don't know what should be the correct behaviour of javac, I can leave the Reference.java changes as proposed since it compiles in both cases. Or should I revert the change to declaration of local variable 'q' ? Regards, Peter
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/30/2014 03:46 PM, Alan Bateman wrote: On 29/01/2014 19:10, Mandy Chung wrote: On 1/29/2014 5:09 AM, Peter Levart wrote: Since I don't know what should be the correct behaviour of javac, I can leave the Reference.java changes as proposed since it compiles in both cases. Or should I revert the change to declaration of local variable 'q' ? I slightly prefer to revert the change to ReferenceQueue? super Object for now as there is no supertype for Object and this looks a little odd. We can clean this up as a separate fix after we get clarification from compiler-dev. I see Peter has posted a question to compiler-dev on this and it can always be re-visited once it clear why it compiles when both Reference and ReferenceQueue are in the same compilation unit. -Alan I Just commited the version with no change to ReferenceQueueObject line to jdk9/dev. If there is a bug in javac and the code would not compile as is, the change to this line should be committed as part of javac fix, right? Regards, Peter
RFR - 6857566 (bf) DirectByteBuffer garbage creation can outpace reclamation
Hi, I'm proposing a patch for the following issue: https://bugs.openjdk.java.net/browse/JDK-6857566 The patch is basically the same as developed and tested as part of the following discussion 3 months ago: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-October/021547.html It's just modified to be applied on top of JDK9 which already changed since then. Here's the webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/DirectBufferAlloc/webrev.01/ As Alan Bateman pointed out: A related piece of work is the FileChannel map implementation where there is a gc + retry if mmap fails. This could be changed to have a similar back-off/retry. I'm planning to examine this code and come up with a similar patch for mapped memory. Regards, Peter
Re: ObjectIn/OutputStream improvements
On 02/05/2014 04:11 PM, Chris Hegarty wrote:
Thanks stuart, Mike, and Paul.
- Why not have getClassSignature() return an interned string? (that's if
interning is actually essential. Are we sure it's not just overhead?)
I didn’t want to change the existing use of interning here, just refactor the
code a little to make it cleaner.
I think that would be a better way to spend our performance investigation
budget. Thus, it might be better to remove the StringBuilder field from this
patch and submit another one focusing on UTF decoding; the other changes in the
patch look good.
Agreed. This could be looked at separately.
Latest webrev:
http://cr.openjdk.java.net/~chegar/serial_stupp.01/
Thanks,
-Chris.
Hi Chris,
What about the following even less garbage-producing-and-copying
ObjectStreamClass.get[Class|Method]Signature combo:
/**
* Returns JVM type signature for given class.
*/
static String getClassSignature(Class? cl) {
if (cl.isPrimitive())
return getPrimitiveSignature(cl);
else
return appendClassSignature(new StringBuilder(),
cl).toString();
}
private static StringBuilder appendClassSignature(StringBuilder
sbuf, Class? cl) {
while (cl.isArray()) {
sbuf.append('[');
cl = cl.getComponentType();
}
if (cl.isPrimitive())
sbuf.append(getPrimitiveSignature(cl));
else
sbuf.append('L').append(cl.getName().replace('.', '/')).append(';');
return sbuf;
}
/**
* Returns JVM type signature for given list of parameters and
return type.
*/
private static String getMethodSignature(Class?[] paramTypes,
Class? retType)
{
StringBuilder sbuf = new StringBuilder();
sbuf.append('(');
for (int i = 0; i paramTypes.length; i++) {
appendClassSignature(sbuf, paramTypes[i]);
}
sbuf.append(')');
appendClassSignature(sbuf, retType);
return sbuf.toString();
}
Regards, Peter
Re: RFR - 6857566 (bf) DirectByteBuffer garbage creation can outpace reclamation
On 02/05/2014 11:21 AM, Alan Bateman wrote: On 05/02/2014 08:08, Peter Levart wrote: Hi, I'm proposing a patch for the following issue: https://bugs.openjdk.java.net/browse/JDK-6857566 The patch is basically the same as developed and tested as part of the following discussion 3 months ago: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-October/021547.html It's just modified to be applied on top of JDK9 which already changed since then. Here's the webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/DirectBufferAlloc/webrev.01/ Hi Alan, I made another webrev with minor changes based on your feedback... http://cr.openjdk.java.net/~plevart/jdk9-dev/DirectBufferAlloc/webrev.03/ Thanks for re-basing the changes and from the previous discussion then I think there was agreement on the approach. So I'd say this is ready to go into jdk9/dev. A minor comments on the usages of tryHandlePendingReferences in Bits that it might be a bit neater to not split the line. At least when looking at the patch then it makes it a bit more obvious what is condition vs. statement in the while loop. I did that by extracting the reference to JavaLangRefAccess into a local variable. Another minor comment is that usually avoid System.exit in tests. It's probably okay here as it always runs in othervm mode but in other modes a System.exit(0) will be flagged by jtreg and will require it spin up another VM. No System.exit in this webrev any more. The test should still run in othervm though, since it uses special VM option -XX:MaxDirectMemorySize=128m. Without it there might be to much direct memory available by default on some machine (like mine with 16GB of RAM) to provoke an error in 5 seconds time... I completely agree with separating the mapped buffers issue to a separate issue. Thanks again for running with this one (it goes back to original implementation in JDK 1.4). -Alan. I think I'll need another official review before pushing this. Regards, Peter
Re: JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger
On 02/20/2014 11:20 AM, David Holmes wrote:
Hi Brian,
On 20/02/2014 7:44 AM, Brian Burkhalter wrote:
This patch concerns cleaning up some ugly internal deprecations.
Issue:https://bugs.openjdk.java.net/browse/JDK-8035279
Webrev:http://cr.openjdk.java.net/~bpb/8035279/webrev.00/
All JTREG BigInteger tests pass, and the serialized form has been
unaltered as verified by bidirectional read-write testing between
Java 7 and this version.
I would appreciate scrutiny of the arithmetic to make sure that I've
made no silly errors. Also I would be interested in opinions as to
whether the volatile keyword should in fact be used for the
affected instance variables.
re: volatile
The basic approach being taken with these primitive fields is lazy
initialization to a value that will always be the same if recomputed.
If the variables are not volatile then in the worst-case (this is
theoretical only) they will be recomputed by each thread that accesses
them. If they are volatile then they will only be recomputed by
threads that truly race on the initialization logic, but you will
thereafter pay the price of a volatile read on each access.
Either way you will be functionally correct so it all comes down to
performance. Practically speaking I would expect you to be better off
overall without the volatile.
As Paul notes the powerCache array does not need to be volatile as it
is initialized as part of class initialization.
Cheers,
David
Hi Brian,
I would also be concerned about correctness here. You changed fields to
be volatile and assigned them with different that default (zero) values
in field initializers. Take for example the field bitCount:
| 156 private volatile int bitCount = Integer.MIN_VALUE;|
In code you check this initial value and decide whether to lazily
compute the real value or not based on this check:
3298 public int bitCount() {
3299 // This computation has a known, acceptable non-critical race
condition.
3300 if (bitCount == Integer.MIN_VALUE) { // bitCount not initialized
yet
There has recently been a discussion on concurrency-dev mailing list
about whether this is safe:
http://cs.oswego.edu/pipermail/concurrency-interest/2013-November/011951.html
...in cases where the instance of such constructed object escapes to
non-constructing thread via data-race. I think the conclusion was that
such semantics is desirable and might be guaranteed in a future updated
JMM, but currently it is not, although most JVMs are implemented in such
a way that this is safe. Citing Doug Lea:
Right. To summarize:
* Programmers do not expect that even though final fields are specifically
publication-safe, volatile fields are not always so.
* For various implementation reasons, JVMs arrange that
volatile fields are publication safe anyway, at least in
cases we know about.
* Actually updating the JMM/JLS to mandate this is not easy
(no small tweak that I know applies). But now is a good time
to be considering a full revision for JDK9.
* In the mean time, it would make sense to further test
and validate JVMs as meeting this likely future spec.
-Doug
Is there a special reason you changed the uninitialized value from
default (zero) to non-zero?
Regards, Peter
Re: JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger
On 02/21/2014 10:06 AM, Paul Sandoz wrote:
I think we should try and use zero, as John says (alas @Stable is package
private to j.l.invoke), and replace 0 with another value such as MIN_VALUE if
one is unsure of the bounds:
int lsb;
if ((lsb = lowestSetBit) == 0) {
...
lowestSetBit = lsb != 0 ? lsb : Integer.MIN_VALUE;
}
return lsb != Integer.MIN_VALUE : lsb ? 0;
Hm, would the following be any better or worse?
int lsb = lowestSetBit;
if (lsb = 0) {
if (lsb == Integer.MIN_VALUE) {
lsb = 0;
} else {
...
lowestSetBit = lsb != 0 ? lsb : Integer.MIN_VALUE;
}
}
return lsb;
I wonder if any of the above is better than current code that offsets
the value space. Current code only contains a single
compare+conditional-branch on fast-path while both of above variants
contain more than one. Does eliminating a single add instruction on
fast-path pay up for that? What do measurements show?
Regards, Peter
Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations
Hi Dmeetry, On 02/22/2014 01:22 PM, dmeetry degrave wrote: Hi all, I would like to ask for a review of combined back port for 7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it also integrates the changes from 8005232, 7185456, 8022721 https://bugs.openjdk.java.net/browse/JDK-7122142 https://bugs.openjdk.java.net/browse/JDK-8005232 https://bugs.openjdk.java.net/browse/JDK-7185456 https://bugs.openjdk.java.net/browse/JDK-8022721 Original jdk8 changes: 7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0 8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92 7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501 8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738 back port: http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/ Patches can't be applied cleanly, hence it was a manual back port, though the final result is equivalent to applying the patches in chronological order (8005232, 7185456, 7122142, 8022721) and applying all the relevant rejected parts It's good to see those patches being back-ported to 7u. By browsing the webrev, I don't see any obvious difference between the original patches and the backport. Do you happen to remember in what part of code there were rejects so that you had to manually apply the changes? (with one exception, AnnotationTypeRuntimeAssumptionTest.java test was not included due to jdk8 API). Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8. Here's the changed test that only uses the JDK7 API so you can include this test too: http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java All tests in test/java/lang/annotation passed. thanks, dmeetry Regards, Peter
Re: JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger
On 02/25/2014 09:38 PM, Brian Burkhalter wrote: On Feb 20, 2014, at 1:42 AM, Paul Sandoz wrote: Not sure the static powerCache field, in the original code, needs to be volatile either: 1137 private static volatile BigInteger[][] powerCache; Is there consensus on whether volatile is necessary here? I think it has to be volatile. The powerCache implementation was added in the following changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7546 ...and improved later in the following: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/7586 It uses a copy-on-write technique to extend the cache with new values when needed. volatile is mandatory here to safely publish the newly constructed array-of-arrays and the newly constructed sub-array to other threads. Without volatile, other threads could see null slots where BigInteger[] and/or BigInteger objects should be... Regards, Peter Thanks, Brian
Re: Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods
On 02/26/2014 09:54 PM, Martin Buchholz wrote:
I don't recall having added this particular wart
to test/java/lang/ProcessBuilder/Basic.java, and history suggests that
others did that.
It does seem that being able to tell whether a java object monitor is
currently locked is useful for debugging and monitoring - there should be a
way to do that.
Thread.holdsLock(Object) ?
Regards, Peter
On Wed, Feb 26, 2014 at 7:12 AM, Paul Sandoz paul.san...@oracle.com wrote:
Hi,
Out of all the methods on Unsafe i think the
monitorEnter/monitorExit/tryMonitorEnter are the least used and are very
strong candidates for removal.
99% of use-cases are supported by classes in the
java.util.concurrent.locks package.
Within the JDK itself it is only used in one JDK test file
test/java/lang/ProcessBuilder/Basic.java:
while (unsafe.tryMonitorEnter(s)) {
unsafe.monitorExit(s);
Thread.sleep(1);
}
for a test verifying an EOF is received on pending reads and it is polling
to check when the process builder acquires the lock before destroying the
process, presumably to avoid some sort of race condition that occasionally
causes the test to fail.
I believe this test as been through a number of rounds, i stared at things
for a bit, but cannot quickly work out a replacement; i lack the knowledge
on this area.
Outside of the JDK i can only find one usage of monitorExit/Enter
(according to grep code) in JBoss modules, and i believe this is only used
on Java 1.6 to work around some limitations in class loading that were
fixed in 1.7.
Given such very limited use i propose to remove these methods after having
worked out a fix for ProcessBuilder/Basic.java test.
Paul.
Re: Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods
On 02/27/2014 08:29 AM, Peter Levart wrote:
On 02/26/2014 09:54 PM, Martin Buchholz wrote:
I don't recall having added this particular wart
to test/java/lang/ProcessBuilder/Basic.java, and history suggests that
others did that.
It does seem that being able to tell whether a java object monitor is
currently locked is useful for debugging and monitoring - there
should be a
way to do that.
Thread.holdsLock(Object) ?
Ah, you meant to query from some other thread, right?
Peter
Regards, Peter
On Wed, Feb 26, 2014 at 7:12 AM, Paul Sandoz paul.san...@oracle.com
wrote:
Hi,
Out of all the methods on Unsafe i think the
monitorEnter/monitorExit/tryMonitorEnter are the least used and are
very
strong candidates for removal.
99% of use-cases are supported by classes in the
java.util.concurrent.locks package.
Within the JDK itself it is only used in one JDK test file
test/java/lang/ProcessBuilder/Basic.java:
while (unsafe.tryMonitorEnter(s)) {
unsafe.monitorExit(s);
Thread.sleep(1);
}
for a test verifying an EOF is received on pending reads and it is
polling
to check when the process builder acquires the lock before
destroying the
process, presumably to avoid some sort of race condition that
occasionally
causes the test to fail.
I believe this test as been through a number of rounds, i stared at
things
for a bit, but cannot quickly work out a replacement; i lack the
knowledge
on this area.
Outside of the JDK i can only find one usage of monitorExit/Enter
(according to grep code) in JBoss modules, and i believe this is
only used
on Java 1.6 to work around some limitations in class loading that were
fixed in 1.7.
Given such very limited use i propose to remove these methods after
having
worked out a fix for ProcessBuilder/Basic.java test.
Paul.
Re: Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods
On 02/27/2014 08:41 AM, David Holmes wrote:
On 27/02/2014 5:30 PM, Peter Levart wrote:
On 02/27/2014 08:29 AM, Peter Levart wrote:
On 02/26/2014 09:54 PM, Martin Buchholz wrote:
I don't recall having added this particular wart
to test/java/lang/ProcessBuilder/Basic.java, and history suggests that
others did that.
It does seem that being able to tell whether a java object monitor is
currently locked is useful for debugging and monitoring - there
should be a
way to do that.
Thread.holdsLock(Object) ?
Ah, you meant to query from some other thread, right?
Right - that is the usage in Basic.java but I'm still scratching my
head trying to understand what finding the BufferedInputStream locked
is supposed to signify.
Here's the relevant code block:
while (unsafe.tryMonitorEnter(s)) {
unsafe.monitorExit(s);
Thread.sleep(1);
}
This code block waits until it finds that some thread has acquired the
lock on 's' for at least 1 ms (or else it can miss it and will wait
further) ...
Since this block is executed for s instanceof BufferedInputStream, what
this block does, is wait until one of synchronized methods on
BufferedInputStream is called. The thread that does that, calls read()
or read(byte[]) and blocks while holding a lock, since the child process
that is spawned and whose stdout the thread is trying to read in this
situation does a 10 minute sleep. I think the following could be used
instead for the same purpose:
while (!isLocked(s, 100L)) {
Thread.sleep(1);
}
// using the following utility method:
static boolean isLocked(final Object monitor, long millis) throws
InterruptedException {
Thread t = new Thread() {
@Override
public void run() {
synchronized (monitor) { }
}
};
t.start();
t.join(millis);
return t.isAlive();
}
Regards, Peter
David
-
Peter
Regards, Peter
On Wed, Feb 26, 2014 at 7:12 AM, Paul Sandoz paul.san...@oracle.com
wrote:
Hi,
Out of all the methods on Unsafe i think the
monitorEnter/monitorExit/tryMonitorEnter are the least used and are
very
strong candidates for removal.
99% of use-cases are supported by classes in the
java.util.concurrent.locks package.
Within the JDK itself it is only used in one JDK test file
test/java/lang/ProcessBuilder/Basic.java:
while (unsafe.tryMonitorEnter(s)) {
unsafe.monitorExit(s);
Thread.sleep(1);
}
for a test verifying an EOF is received on pending reads and it is
polling
to check when the process builder acquires the lock before
destroying the
process, presumably to avoid some sort of race condition that
occasionally
causes the test to fail.
I believe this test as been through a number of rounds, i stared at
things
for a bit, but cannot quickly work out a replacement; i lack the
knowledge
on this area.
Outside of the JDK i can only find one usage of monitorExit/Enter
(according to grep code) in JBoss modules, and i believe this is
only used
on Java 1.6 to work around some limitations in class loading that
were
fixed in 1.7.
Given such very limited use i propose to remove these methods after
having
worked out a fix for ProcessBuilder/Basic.java test.
Paul.
Re: Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods
On 02/27/2014 10:58 AM, David Holmes wrote:
On 27/02/2014 6:38 PM, Peter Levart wrote:
On 02/27/2014 08:41 AM, David Holmes wrote:
On 27/02/2014 5:30 PM, Peter Levart wrote:
On 02/27/2014 08:29 AM, Peter Levart wrote:
On 02/26/2014 09:54 PM, Martin Buchholz wrote:
I don't recall having added this particular wart
to test/java/lang/ProcessBuilder/Basic.java, and history suggests
that
others did that.
It does seem that being able to tell whether a java object
monitor is
currently locked is useful for debugging and monitoring - there
should be a
way to do that.
Thread.holdsLock(Object) ?
Ah, you meant to query from some other thread, right?
Right - that is the usage in Basic.java but I'm still scratching my
head trying to understand what finding the BufferedInputStream locked
is supposed to signify.
Here's the relevant code block:
while (unsafe.tryMonitorEnter(s)) {
unsafe.monitorExit(s);
Thread.sleep(1);
}
This code block waits until it finds that some thread has acquired the
lock on 's' for at least 1 ms (or else it can miss it and will wait
further) ...
Since this block is executed for s instanceof BufferedInputStream, what
this block does, is wait until one of synchronized methods on
BufferedInputStream is called. The thread that does that, calls read()
or read(byte[]) and blocks while holding a lock, since the child process
that is spawned and whose stdout the thread is trying to read in this
situation does a 10 minute sleep.
Ah I see - thanks for the analysis. So this is a classic 'barrier'
waiting for the other thread to have reached the synchronized method.
I think the following could be used instead for the same purpose:
while (!isLocked(s, 100L)) {
Thread.sleep(1);
}
// using the following utility method:
static boolean isLocked(final Object monitor, long millis) throws
InterruptedException {
Thread t = new Thread() {
@Override
public void run() {
synchronized (monitor) { }
}
};
t.start();
t.join(millis);
return t.isAlive();
}
Functionally yes this will only return true if the object is locked
for 100ms. But it is potentially going to generate a lot of temporary
threads. And in theory at least the JIT can optimize the run method
into nothing.
Ok, then what about the following:
static boolean isLocked(final Object monitor, final long millis)
throws InterruptedException {
return new Thread() {
volatile boolean unlocked;
@Override
public void run() {
synchronized (monitor) { unlocked = true; }
}
boolean isLocked() throws InterruptedException {
start();
join(millis);
return !unlocked;
}
}.isLocked();
}
// and usage it like:
while (!isLocked(s, 100L)) {
Thread.sleep(100L);
}
This will allocate about 10 Thread objects per second until the other
thread finally reaches the synchronized read() method in
BufferedInputStream...
Regards, Peter
Still it is a cute trick :)
David
Regards, Peter
David
-
Peter
Regards, Peter
On Wed, Feb 26, 2014 at 7:12 AM, Paul Sandoz
paul.san...@oracle.com
wrote:
Hi,
Out of all the methods on Unsafe i think the
monitorEnter/monitorExit/tryMonitorEnter are the least used and are
very
strong candidates for removal.
99% of use-cases are supported by classes in the
java.util.concurrent.locks package.
Within the JDK itself it is only used in one JDK test file
test/java/lang/ProcessBuilder/Basic.java:
while (unsafe.tryMonitorEnter(s)) {
unsafe.monitorExit(s);
Thread.sleep(1);
}
for a test verifying an EOF is received on pending reads and it is
polling
to check when the process builder acquires the lock before
destroying the
process, presumably to avoid some sort of race condition that
occasionally
causes the test to fail.
I believe this test as been through a number of rounds, i stared
at public static boolean isLocked(final Object monitor,
final long millis) throws InterruptedException {
return new Thread() {
volatile boolean unlocked;
@Override
public void run() {
synchronized (monitor) { unlocked = true; }
}
boolean isLocked() throws InterruptedException {
start();
join(millis);
return !unlocked;
}
}.isLocked();
}
things
for a bit, but cannot quickly work out a replacement; i lack the
knowledge
on this area.
Outside of the JDK i can only find one usage of monitorExit/Enter
(according to grep code
Re: JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger
On 02/27/2014 09:22 AM, Paul Sandoz wrote: On Feb 27, 2014, at 12:51 AM, Brian Burkhalter brian.burkhal...@oracle.com wrote: On Feb 26, 2014, at 1:53 PM, Paul Sandoz wrote: Thanks for the suggestion, Paul. Assuming it is correct, in what way would this be a better approach? (I apologize for being obtuse.) IMHO it is a simpler solution. It's a cache line that requires updating not the cache line*s*, as the current approach can potentially knock out cache lines for other radixes. Potentially yes, but not very likely, since the whole powerCache reference is re-read again *after* the computation of cacheLine and then replaced with a modified clone immediately after that. It should be noted what Alan Eliasen already pointed-out in a discussion back in June 2013: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/018375.html ...so I think what could be improved in current implementation is the potential and more likely useless computation of the same values for the same radix by multiple concurrent threads. Back then I presented an alternative solution which solves that: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/018457.html ...and as can be seen, does not use volatile read/write to publish BigIntegers - that's why it is important that internal implementation of such immutable classes like BigInteger is tolerant to unsafe publication... Regards, Peter I forgot to mention a slightly more complex variation is to CAS the cache line instead of a lazy set, which can ensure a line does not knock another containing a higher exponent. Paul.
Re: rationale for swallowing exceptions in Class#getEnumConstantsShared
On 02/28/2014 04:56 PM, Jochen Theodorou wrote:
Hi,
we stumbled here about a strange error involving enums generated from
our compiler and that did make me see getEnumConstantsShared() in
java.lang.Class.
Now the implementation more or less says, that if calling values()
fails in any way, return null and ignore the exception. But why is the
exception ignored? It could contain valuable information about what
did go wrong
Hi Jochen,
I agree that the exception should be thrown (possibly an unchecked
IllegalArgumentException or something like that with the cause from
reflective operation). If Class.isEnum() returns true then the class
should be considered an enum and if it is not conformant to
specification (which may cause one of those exceptions) then this should
be reported as exception and not as null return. The
getEnumConstantsShared() is also used as implementation for public method:
/**
* Returns the elements of this enum class or *null if this**
** * Class object does not represent an enum type*.
*
* @return an array containing the values comprising the enum class
* represented by this Class object in the order they're
* declared, or null if this Class object does not
* represent an enum type
* @since 1.5
*/
public T[] getEnumConstants() {
T[] values = getEnumConstantsShared();
return (values != null) ? values.clone() : null;
}
The behaviour of above method is not consistent with:
/**
* Returns *true if and only if this class was declared as an enum*
in the
* source code.
*
* @return true if and only if this class was declared as an enum
in the
* source code
* @since 1.5
*/
public boolean isEnum() {
// An enum must both directly extend java.lang.Enum and have
// the ENUM bit set; classes for specialized enum constants
// don't do the former.
return (this.getModifiers() ENUM) != 0
this.getSuperclass() == java.lang.Enum.class;
}
...which can return true at the same time as getEnumConstants() returns
null for the same class...
I don't believe there's any code out there that relies on this faulty
behaviour of getEnumConstants() public method. The other use of
getEnumConstantsShared() is to support the Enum.valueOf(Class, String)
method which already throws IllegalArgumentException if the 1st
parameter does not represent an enum class, so there's no compatibility
concerns about that use.
What do experts say?
Regards, Peter
In my code for example, something obviously goes wrong... sometimes...
depending on moon-phases or something. and I have not the slightest
idea what.
bye Jochen
Re: Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods
On 03/04/2014 05:09 AM, David Holmes wrote:
On 4/03/2014 1:45 PM, David Holmes wrote:
On 3/03/2014 10:56 PM, David M. Lloyd wrote:
Yes, that would necessarily be the contract of a Monitors class,
just as
it is part of the contract of Lock today. If your argument is that it
shouldn't be allowed because it might be used wrong, we might as well
just delete most of the JDK, ReentrantLock included, since it suffers
from the exact same potential problem. The difference is that monitors
have a simple API in the form of synchronized that people were in the
past and would continue to be free (and recommended) to use.
We should not introduce anything that allows something that was
guaranteed to be safe by the language, to become unsafe. So I can
support a call for tryMonitorEnter, but not for explicit enter/exit
actions.
Which of course is not possible at the API level - Doh! Sorry.
Unless the API is made safe by packing it into a lambda-consuming
method. For example:
public static boolean trySynchronized(Runnable action);
Regards, Peter
David
David
-
I would not call recommending monitors to be a premature
optimization.
The memory overhead of ReentrantLock is far too large for many use
cases, especially when it comes to fine-grained locking of large data
sets. In fact I would *only* recommend Lock if your need for the extra
behavior it provides (i.e., multiple conditions, tryLock, timed
tryLock,
and/or interruptible locking) or for the extra behavior provided by
ReentrantLock itself (i.e., lock status introspection) outweighs the
cost of its memory consumption, which so far has happened zero times in
any of the many frameworks I maintain.
On the other hand, adding some of the missing functionality to a
Monitors class would be pretty handy, especially if doing so doesn't
create a large divergence from the existing implementation. Given that
tryMonitorEnter already exists, implementing that method on a Monitors
class seems fairly trivial, which already takes care of one out of the
four special use cases for Locks. Interruptible locking would be my
first choice for a number two, if this idea were actually officially on
the table. Finally, an isMonitorHeld() method would tie in another
branch of the discussion nicely and seems trivially possible as well.
Any other information would be a purely
It's worth noting that it's already possible for user code to implement
the basic functionality using JNI methods, though in the interest of
full disclosure it's worth noting that the enter/exit monitor JNI
functions are forbidden by spec to interoperate with the corresponding
bytecodes.
FWIW using the word monitor instead of lock may help avoid
confusion
and scare off those pesky undergrads.
On 03/02/2014 12:48 PM, Dr Heinz M. Kabutz wrote:
Hi Dave,
lighter is subject to the HotSpot Profiler doing the optimizations
that you expect. Problem is, once you begin manually locking /
unlocking monitors, the byte codes are not recognizable as standard
synchronized(monitor) {} code, thus the optimizations will also not be
as good (or completely absent). At least that has been my experience
when I tried it out a few years ago. So the argument that
synchronized
is faster than ReentrantLock, besides this being IMHO a premature
optimization, cannot be directly applied to when we manually do use
the
monitorEnter/monitorExit methods.
The reason why I personally think they are more dangerous than
ReentrantLock is that we are used to synchronized() {} taking care of
the unlocking for us. If we see a thread that is in the BLOCKED state
(which only happens with synchronized) then we would typically assume
that another thread is /currently /holding the monitor.
Take the following example code:
import sun.misc.*;
import java.lang.reflect.*;
public class MonitorMystery {
public static Unsafe getUnsafe() {
try {
for (Field field : Unsafe.class.getDeclaredFields()) {
if (Modifier.isStatic(field.getModifiers())) {
if (field.getType() == Unsafe.class) {
field.setAccessible(true);
return (Unsafe) field.get(null);
}
}
}
throw new IllegalStateException(Unsafe field not found);
} catch (Exception e) {
throw new IllegalStateException(
Could not initialize unsafe, e);
}
}
public static void main(String... args) throws
InterruptedException {
Thread t = new Thread() {
public void run() {
getUnsafe().monitorEnter(MonitorMystery.class);
}
};
t.start();
Thread.sleep(100);
System.out.println(Trying to synchronize);
synchronized (MonitorMystery.class) {
System.out.println(Managed to synchronized);
}
}
}
We now see that we never manage to synchronize in the main thread
and in
fact if you do a thread dump you will see a deadlock involving only a
single thread :-)
This is why it is so important to use the correct
Re: Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods
On 03/04/2014 08:01 AM, Peter Levart wrote:
On 03/04/2014 05:09 AM, David Holmes wrote:
On 4/03/2014 1:45 PM, David Holmes wrote:
On 3/03/2014 10:56 PM, David M. Lloyd wrote:
Yes, that would necessarily be the contract of a Monitors class,
just as
it is part of the contract of Lock today. If your argument is that it
shouldn't be allowed because it might be used wrong, we might as well
just delete most of the JDK, ReentrantLock included, since it suffers
from the exact same potential problem. The difference is that
monitors
have a simple API in the form of synchronized that people were in the
past and would continue to be free (and recommended) to use.
We should not introduce anything that allows something that was
guaranteed to be safe by the language, to become unsafe. So I can
support a call for tryMonitorEnter, but not for explicit enter/exit
actions.
Which of course is not possible at the API level - Doh! Sorry.
Unless the API is made safe by packing it into a lambda-consuming
method. For example:
public static boolean trySynchronized(Runnable action);
Sorry, forgot the monitor:
public static boolean trySynchronized(Object monitor, Runnable action);
I guess new java.lang.Object instance method is out of question?
Regards, Peter
Regards, Peter
David
David
-
I would not call recommending monitors to be a premature
optimization.
The memory overhead of ReentrantLock is far too large for many use
cases, especially when it comes to fine-grained locking of large data
sets. In fact I would *only* recommend Lock if your need for the
extra
behavior it provides (i.e., multiple conditions, tryLock, timed
tryLock,
and/or interruptible locking) or for the extra behavior provided by
ReentrantLock itself (i.e., lock status introspection) outweighs the
cost of its memory consumption, which so far has happened zero
times in
any of the many frameworks I maintain.
On the other hand, adding some of the missing functionality to a
Monitors class would be pretty handy, especially if doing so doesn't
create a large divergence from the existing implementation. Given that
tryMonitorEnter already exists, implementing that method on a Monitors
class seems fairly trivial, which already takes care of one out of the
four special use cases for Locks. Interruptible locking would be my
first choice for a number two, if this idea were actually
officially on
the table. Finally, an isMonitorHeld() method would tie in another
branch of the discussion nicely and seems trivially possible as well.
Any other information would be a purely
It's worth noting that it's already possible for user code to
implement
the basic functionality using JNI methods, though in the interest of
full disclosure it's worth noting that the enter/exit monitor JNI
functions are forbidden by spec to interoperate with the corresponding
bytecodes.
FWIW using the word monitor instead of lock may help avoid
confusion
and scare off those pesky undergrads.
On 03/02/2014 12:48 PM, Dr Heinz M. Kabutz wrote:
Hi Dave,
lighter is subject to the HotSpot Profiler doing the optimizations
that you expect. Problem is, once you begin manually locking /
unlocking monitors, the byte codes are not recognizable as standard
synchronized(monitor) {} code, thus the optimizations will also
not be
as good (or completely absent). At least that has been my experience
when I tried it out a few years ago. So the argument that
synchronized
is faster than ReentrantLock, besides this being IMHO a premature
optimization, cannot be directly applied to when we manually do
use the
monitorEnter/monitorExit methods.
The reason why I personally think they are more dangerous than
ReentrantLock is that we are used to synchronized() {} taking care of
the unlocking for us. If we see a thread that is in the BLOCKED
state
(which only happens with synchronized) then we would typically assume
that another thread is /currently /holding the monitor.
Take the following example code:
import sun.misc.*;
import java.lang.reflect.*;
public class MonitorMystery {
public static Unsafe getUnsafe() {
try {
for (Field field : Unsafe.class.getDeclaredFields()) {
if (Modifier.isStatic(field.getModifiers())) {
if (field.getType() == Unsafe.class) {
field.setAccessible(true);
return (Unsafe) field.get(null);
}
}
}
throw new IllegalStateException(Unsafe field not found);
} catch (Exception e) {
throw new IllegalStateException(
Could not initialize unsafe, e);
}
}
public static void main(String... args) throws
InterruptedException {
Thread t = new Thread() {
public void run() {
getUnsafe().monitorEnter(MonitorMystery.class);
}
};
t.start();
Thread.sleep(100);
System.out.println(Trying to synchronize);
synchronized (MonitorMystery.class) {
System.out.println(Managed
Re: How to close a Process and having shutdown hooks called ?
On 03/04/2014 02:09 PM, LE PICARD NICOLAS wrote: I didn't know for jdk8 functions destroy and the new one destroyForcibly... And Jonathan was right in his previous answer, I was looking for a solution in Java, I mean a portable one like in Process class, like the destroyGracefully. I am looking to signals because it seems to be a simple way to achieve this portable solution, maybe I'm wrong. It would be useful (as you said) to have a function to kill gracefully through Process class, no ? Nicolas Le Picard Hi Nicolas, So you're spawning a Java sub-process from a Java master process using java.lang.Process, do I understand correctly? Are you in control of the code that represents the child sub-process? Is this code already using System.in (STDIN) stream for anything? If the answers are YES and NO respectively, then you may be able to use the System.in in the sub-process (spawn a special monitoring thread) and Process.getOutputStream() in the master process to establish a one way channel for passing commands in direction: master process - sub-process. One of those commands could be shutdown for example, that would trigger a System.exit() in the sub-process. This way you could make the shut-down hooks in sub-process be executed before exiting... Regards, Peter -Message d'origine- De : Alan Bateman [mailto:alan.bate...@oracle.com] Envoyé : mardi 4 mars 2014 13:12 À : Krystal Mok Cc : LE PICARD NICOLAS; core-libs-dev@openjdk.java.net Objet : Re: How to close a Process and having shutdown hooks called ? On 04/03/2014 11:51, Krystal Mok wrote: Hi Nicolas, Looks like a well discussed question. On Posix systems, SIGTERM should work for you. That's the default signal sent by the 'kill' command on Linux. e.g. please take a look here: http://stackoverflow.com/questions/2541597/how-to-gracefully-handle-th e-sigkill-signal-in-java - Kris I think he's on Windows and is looking for destroy to use something other than TerminateProcess. As background, the destroy method was confusingly specified to kill the sub-process forcibly but it wasn't implemented this way everywhere. On Unix/Linux then the long standing implementation used SIGTERM and so no guarantee that it would cause the sub-process to terminate. This mismatch was examined in JDK 8 (you'll need to go through the archives of this mailing list to see the discussion) and the javadoc updated to make it clear that it is implementation specific as to whether it is done forcibly or not. In addition a new destroyForcibly was added to do the SIGKILL or TerminateProcess for cases where you really want to kill the child. There isn't a corresponding destroyGracefully but clearly this would be useful if were feasible to implement everywhere. -Alan.
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
On 03/04/2014 01:14 AM, Brian Burkhalter wrote: - add AtomicReferenceFieldUpdater-type constant for stringCache initialization Hi Brian, By using volatile read and CAS, there's still a chance that multiple concurrent threads will be invoking the layoutChars(true) concurrently, but you guarantee that only a single instance of String will ever be returned as a result of the toString() method. Is that what you were pursuing? Regards, Peter
Re: JEP 193: Enhanced Volatiles
On 03/05/2014 05:55 PM, Jeroen Frijters wrote: Brian Goetz wrote: I suspect you were expecting this response: we don't add language semantics through annotations. Technically, we're not adding language semantics. The JVM is the one interpreting the annotations. And the JVM is the one implementing the language semantics (together with javac which feeds the JVM with bytecodes). Language semantcis are implemented by the combination of javac and JVM. If you say that this feature does not require any change to javac, you're just saying that you put all the burden on the JVM, but you *are* implementing the language semantics using annotations nevertheless... Regards, Peter I'm not trying to frustrate you; evolving a language with millions of users is really, really hard. And one of the things that makes it hard is recognizing our intrinsic conflicts of interest between what good will this do me and what harm will it do others. I understand, that's why I want to avoid adding language support for this niche/specialist feature. Regards, Jeroen
Re: Review 8035808: Eliminate dependency to GetPropertyAction and other sun.security.action convenient classes
On 03/07/2014 12:31 PM, Alan Bateman wrote:
On 06/03/2014 21:10, Mandy Chung wrote:
Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035808/webrev.00/
This patch converts the use of sun.security.action.GetPropertyAction
tolambda
(PrivilegedActionString) () - System.getProperty(key)
Similarly for GetIntegerAction and GetLongAction.
The sun.security.action.* classes are just convenient classes that
are used not only by the security library but also used by RMI,
management, a couple other components that cause cross-module
dependency that are not absolutely necessary. They can simply be
replaced to call System.getProperty, Integer.getInteger, or
Long.getLong at the callsite.
This looks good to me as it reduces the number of inner classes,
removes a lot of dependencies on sun.* APIs, and updates the code to
use new language features. The only downside appears to the cast but
you can't really get away from that here (at least not without
declaring a PrivilegedAction for each usage).
-Alan.
Maybe, if PrivilegedAction interface could be retrofitted as follows:
public interface PrivilegedExceptionActionT {
T run() throws Exception;
}
public interface PrivilegedActionT extends
PrivilegedExceptionActionT {
@Override
T run();
}
Then in majority of cases where lambda body did not throw a checked
exception, the most-specific rule would choose the
doPrivileged(PrivilegedAction) method automatically... For
checked-exceptional-cases the cast would still be needed.
Regards, Peter
Re: [9] RFR (S): 8036117: MethodHandles.catchException doesn't handle VarargsCollector right (8034120 failed)
Hi,
Excuse my ignorant question: What is the purpose of @LambdaForm.Hidden
annotation?
I suspect it has to do with hiding the call frames in stack traces that
are part of LambdaForm invocation chain. In this case, method:
private static Object[] prepend(Object elem, Object[] array)
in MethodHandleImpl need not be annotated with this annotation, since
it's call frame is not on stack when one of the target methods is
executed. It's just a function used to calculate the argument of the
call. In fact, if prepend() ever throws exception (NPE in case array is
null?), It would be preferable that it's call frame is visible in the
stack trace.
Am I right or am I just talking nonsense?
Regards, Peter
On 03/11/2014 12:12 AM, Vladimir Ivanov wrote:
John, Chris, thanks!
Best regards,
Vladimir Ivanov
On 3/11/14 3:08 AM, Christian Thalinger wrote:
Even better.
On Mar 10, 2014, at 3:21 PM, Vladimir Ivanov
vladimir.x.iva...@oracle.com wrote:
Chris, thanks for the review.
John suggested an elegant way to fix the problem - use asFixedArity.
Updated fix:
http://cr.openjdk.java.net/~vlivanov/8036117/webrev.01/
Best regards,
Vladimir Ivanov
On 3/8/14 4:51 AM, Christian Thalinger wrote:
Seems good to me. I’d like to have another name for this method:
+ private static Object invokeCustom(MethodHandle target,
Object... args) throws Throwable {
On Mar 4, 2014, at 12:00 PM, Vladimir Ivanov
vladimir.x.iva...@oracle.com wrote:
http://cr.openjdk.java.net/~vlivanov/8036117/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8036117
84 lines changed: 74 ins; 3 del; 7 mod
I have to revert a cleanup I did for 8027827.
MethodHandle.invokeWithArguments (and generic invocation) has
unpleasant
peculiarity in behavior when used with VarargsCollector. So,
unfortunately, invokeWithArguments is not an option there.
Looking at the API (excerpts from javadoc [1] [2]), the following
condition doesn't hold in that case:
trailing parameter type of the caller is a reference type
identical
to or assignable to the trailing parameter type of the adapter.
Example:
target.invokeWithArguments((Object[])args)
=
target.invoke((Object)o1,(Object)o2,(Object)o3)
=/
target.invokeExact((Object)o1, (Object)o2, (Object[])o3)
because Object !: Object[].
The fix is to skip unnecessary conversion when invoking a method
handle
and just do a pairwise type conversion.
Testing: failing test case, nashorn w/ experimental features (octane)
Thanks!
Best regards,
Vladimir Ivanov
[1] MethodHandle.invokeWithArguments
Performs a variable arity invocation, ..., as if via an inexact
invoke
from a call site which mentions only the type Object, and whose
arity is
the length of the argument array.
[2] MethodHandle.asVarargsCollector
When called with plain, inexact invoke, if the caller type is the
same
as the adapter, the adapter invokes the target as with invokeExact.
(This is the normal behavior for invoke when types match.)
Otherwise, if the caller and adapter arity are the same, and the
trailing parameter type of the caller is a reference type
identical to
or assignable to the trailing parameter type of the adapter, the
arguments and return values are converted pairwise, as if by
asType on a
fixed arity method handle.
Otherwise, the arities differ, or the adapter's trailing parameter
type
is not assignable from the corresponding caller type. In this
case, the
adapter replaces all trailing arguments from the original trailing
argument position onward, by a new array of type arrayType, whose
elements comprise (in order) the replaced arguments.
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: JDK 9 RFC on 8029770: Improve user friendliness of preferences storage on Unix systems
On 03/10/2014 10:12 PM, Brian Burkhalter wrote: This issue (https://bugs.openjdk.java.net/browse/JDK-8029770) concerns improving preference handling (java.util.prefs.Preferences) on systems other than Mac OS X and Windows, essentially Unix systems other than OS X. It would be good to obtain some comments on the behavior of the prospective changes including whether they are even necessary. For the moment, comments on the behavior are of more interest than those on any particular implementation. For background, on the Unix systems in question, the file system is used to store preferences in a tree with nodes mapping to directories. As node names can contain characters which may cause confusion when used verbatim in directory names, in those cases where such characters are detected, the directory name is derived from the node name by encoding the latter using a non-standard variant of the Base 64 Alphabet (Table 1 in [1]) which is immune to alphabetic character case folding. This can result in some strange and human unreadable directory names in the file system preferences tree. The objective here would be to change this scheme to something more user friendly. There are likely numerous approaches to solving this problem, but I think that first it needs to be determined whether interoperability with earlier Java versions is a requirement and, if so, what the nature of this requirement is. There are probably others, but there are at least the following alternatives (not all of which might be sensible): A) No interoperability: Java 9 and subsequent versions would not share preferences with earlier versions. B) Initialization only: Java 9 preferences would be initialized from the state of pre-9 preferences at the instant of first use. C) Unidirectional: Java 9 would detect changes made by earlier versions. D) Bidirectional: Java 9 and pre-9 preferences would be kept in sync. For options C and D note that only Java 9+ instances could effect this behavior. If for example Java 9 was not run for some time on a particular machine, the pre-9 preferences could significantly diverge from the 9 preferences. These would presumably be brought into sync when a Java 9+ instance next invoked the Preferences APIs. Whichever option were chosen, the changes made could include the definition of a system property which could be set to disable the new preferences handling variant, i.e., to force pre-9 file system preferences behavior. Any comments would be appreciated. Thanks, Brian [1] http://tools.ietf.org/rfc/rfc4648.txt Hi Brian, What is the purpose of changing the scheme? Is it just to be more power-user and developer friendly? To enable users to inspect the preferences with filesystem tools? What about a little command-line and/or gui jprefs tool in the style of regedit or gconftool or similar? Would that be enough to satisfy the goal? Any of the options A...D seem a compatibility nightmare for majority of programs that use preferences. Most users don't ever inspect directories starting with dot '.', so this feature is more or less targeted at developers, right? A tool that is standard part of JRE distribution could do the job of browsing (and perhaps changing of) the preferences without compatibility concerns... Regards, Peter
Re: [9] RFR (S): 8036117: MethodHandles.catchException doesn't handle VarargsCollector right (8034120 failed)
On 03/11/2014 12:59 PM, Vladimir Ivanov wrote:
Peter,
You raised totally valid question.
I marked MethodHandleImpl.prepend with @Hidden annotation because it
is internal implementation detail of JSR292.
You are right that normally a callee can't see it on stack. *But it's
possible to observe it when stack trace is queried from a separate
thread. *
Is this good or bad? It enables tools to see it (for example sampling
profilers, etc...).
Regards, Peter
There's not much value in it in this particular case, but I decided to
reduce possible noise.
Best regards,
Vladimir Ivanov
On 3/11/14 3:35 PM, Peter Levart wrote:
Hi,
Excuse my ignorant question: What is the purpose of @LambdaForm.Hidden
annotation?
I suspect it has to do with hiding the call frames in stack traces that
are part of LambdaForm invocation chain. In this case, method:
private static Object[] prepend(Object elem, Object[] array)
in MethodHandleImpl need not be annotated with this annotation, since
it's call frame is not on stack when one of the target methods is
executed. It's just a function used to calculate the argument of the
call. In fact, if prepend() ever throws exception (NPE in case array is
null?), It would be preferable that it's call frame is visible in the
stack trace.
Am I right or am I just talking nonsense?
Regards, Peter
On 03/11/2014 12:12 AM, Vladimir Ivanov wrote:
John, Chris, thanks!
Best regards,
Vladimir Ivanov
On 3/11/14 3:08 AM, Christian Thalinger wrote:
Even better.
On Mar 10, 2014, at 3:21 PM, Vladimir Ivanov
vladimir.x.iva...@oracle.com wrote:
Chris, thanks for the review.
John suggested an elegant way to fix the problem - use asFixedArity.
Updated fix:
http://cr.openjdk.java.net/~vlivanov/8036117/webrev.01/
Best regards,
Vladimir Ivanov
On 3/8/14 4:51 AM, Christian Thalinger wrote:
Seems good to me. I’d like to have another name for this method:
+ private static Object invokeCustom(MethodHandle target,
Object... args) throws Throwable {
On Mar 4, 2014, at 12:00 PM, Vladimir Ivanov
vladimir.x.iva...@oracle.com wrote:
http://cr.openjdk.java.net/~vlivanov/8036117/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8036117
84 lines changed: 74 ins; 3 del; 7 mod
I have to revert a cleanup I did for 8027827.
MethodHandle.invokeWithArguments (and generic invocation) has
unpleasant
peculiarity in behavior when used with VarargsCollector. So,
unfortunately, invokeWithArguments is not an option there.
Looking at the API (excerpts from javadoc [1] [2]), the following
condition doesn't hold in that case:
trailing parameter type of the caller is a reference type
identical
to or assignable to the trailing parameter type of the adapter.
Example:
target.invokeWithArguments((Object[])args)
=
target.invoke((Object)o1,(Object)o2,(Object)o3)
=/
target.invokeExact((Object)o1, (Object)o2, (Object[])o3)
because Object !: Object[].
The fix is to skip unnecessary conversion when invoking a method
handle
and just do a pairwise type conversion.
Testing: failing test case, nashorn w/ experimental features
(octane)
Thanks!
Best regards,
Vladimir Ivanov
[1] MethodHandle.invokeWithArguments
Performs a variable arity invocation, ..., as if via an inexact
invoke
from a call site which mentions only the type Object, and whose
arity is
the length of the argument array.
[2] MethodHandle.asVarargsCollector
When called with plain, inexact invoke, if the caller type is the
same
as the adapter, the adapter invokes the target as with invokeExact.
(This is the normal behavior for invoke when types match.)
Otherwise, if the caller and adapter arity are the same, and the
trailing parameter type of the caller is a reference type
identical to
or assignable to the trailing parameter type of the adapter, the
arguments and return values are converted pairwise, as if by
asType on a
fixed arity method handle.
Otherwise, the arities differ, or the adapter's trailing parameter
type
is not assignable from the corresponding caller type. In this
case, the
adapter replaces all trailing arguments from the original trailing
argument position onward, by a new array of type arrayType, whose
elements comprise (in order) the replaced arguments.
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm
Re: [9] RFR (S): 8036117: MethodHandles.catchException doesn't handle VarargsCollector right (8034120 failed)
On 03/11/2014 03:05 PM, Vladimir Ivanov wrote:
You raised totally valid question.
I marked MethodHandleImpl.prepend with @Hidden annotation because it
is internal implementation detail of JSR292.
You are right that normally a callee can't see it on stack. *But it's
possible to observe it when stack trace is queried from a separate
thread. *
Is this good or bad? It enables tools to see it (for example sampling
profilers, etc...).
For the purposes of sampling profilers (and other monitoring tools)
@Hidden should be completely ignored. These tools should use
appropriate API to get the data.
I looked into the code and @Hidden has even less effect than I thought
initially. It affects very limited set of cases - only users of
JVM_FillInStackTrace (when filling exception's stack trace,
Thread.dumpStack()). Calls to Thread.getStackTrace() from a separate
thread omit stack trace filtering.
So all you're achieving by annotating prepend() method is that any
exception stack trace, in case it is thrown inside the prepend() method,
will hide where it was thrown from. In case all LambdaForm frames
leading to the prepend() method were also hidden, the exception would
appear to be thrown from the invocation of the MH...
Regards, Peter
Best regards,
Vladimir Ivanov
Regards, Peter
There's not much value in it in this particular case, but I decided to
reduce possible noise.
Best regards,
Vladimir Ivanov
On 3/11/14 3:35 PM, Peter Levart wrote:
Hi,
Excuse my ignorant question: What is the purpose of @LambdaForm.Hidden
annotation?
I suspect it has to do with hiding the call frames in stack traces
that
are part of LambdaForm invocation chain. In this case, method:
private static Object[] prepend(Object elem, Object[] array)
in MethodHandleImpl need not be annotated with this annotation, since
it's call frame is not on stack when one of the target methods is
executed. It's just a function used to calculate the argument of the
call. In fact, if prepend() ever throws exception (NPE in case
array is
null?), It would be preferable that it's call frame is visible in the
stack trace.
Am I right or am I just talking nonsense?
Regards, Peter
On 03/11/2014 12:12 AM, Vladimir Ivanov wrote:
John, Chris, thanks!
Best regards,
Vladimir Ivanov
On 3/11/14 3:08 AM, Christian Thalinger wrote:
Even better.
On Mar 10, 2014, at 3:21 PM, Vladimir Ivanov
vladimir.x.iva...@oracle.com wrote:
Chris, thanks for the review.
John suggested an elegant way to fix the problem - use
asFixedArity.
Updated fix:
http://cr.openjdk.java.net/~vlivanov/8036117/webrev.01/
Best regards,
Vladimir Ivanov
On 3/8/14 4:51 AM, Christian Thalinger wrote:
Seems good to me. I’d like to have another name for this method:
+ private static Object invokeCustom(MethodHandle target,
Object... args) throws Throwable {
On Mar 4, 2014, at 12:00 PM, Vladimir Ivanov
vladimir.x.iva...@oracle.com wrote:
http://cr.openjdk.java.net/~vlivanov/8036117/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8036117
84 lines changed: 74 ins; 3 del; 7 mod
I have to revert a cleanup I did for 8027827.
MethodHandle.invokeWithArguments (and generic invocation) has
unpleasant
peculiarity in behavior when used with VarargsCollector. So,
unfortunately, invokeWithArguments is not an option there.
Looking at the API (excerpts from javadoc [1] [2]), the following
condition doesn't hold in that case:
trailing parameter type of the caller is a reference type
identical
to or assignable to the trailing parameter type of the adapter.
Example:
target.invokeWithArguments((Object[])args)
=
target.invoke((Object)o1,(Object)o2,(Object)o3)
=/
target.invokeExact((Object)o1, (Object)o2, (Object[])o3)
because Object !: Object[].
The fix is to skip unnecessary conversion when invoking a method
handle
and just do a pairwise type conversion.
Testing: failing test case, nashorn w/ experimental features
(octane)
Thanks!
Best regards,
Vladimir Ivanov
[1] MethodHandle.invokeWithArguments
Performs a variable arity invocation, ..., as if via an inexact
invoke
from a call site which mentions only the type Object, and whose
arity is
the length of the argument array.
[2] MethodHandle.asVarargsCollector
When called with plain, inexact invoke, if the caller type is
the
same
as the adapter, the adapter invokes the target as with
invokeExact.
(This is the normal behavior for invoke when types match.)
Otherwise, if the caller and adapter arity are the same, and the
trailing parameter type of the caller is a reference type
identical to
or assignable to the trailing parameter type of the adapter, the
arguments and return values are converted pairwise, as if by
asType on a
fixed arity method handle.
Otherwise, the arities differ, or the adapter's trailing
parameter
type
is not assignable from the corresponding caller type. In this
case, the
adapter replaces all trailing arguments from the original
trailing
argument position onward
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
On 03/11/2014 06:07 PM, Mike Duigou wrote:
You are making stringCache volatile - performance penalty on ARM PPC.
It is understood that the volatile is not free. For this purpose it was
believed that the performance cost was a fair trade off to avoid the heap pollution.
Regardless of the decision in this case the method used would seem to be the best caching
pattern available. We do need to establish when it is appropriate to use this solution
and what the tradeoffs are that would make other solutions more appropriate.
Hi,
What would the following cost?
private transient String stringCache;
public String toString() {
String sc = stringCache;
if (sc == null) {
sc = (String) U.getObjectVolatile(this, STRING_CACHE_OFFSET);
if (sc == null) {
sc = layoutChars(true);
if (!U.compareAndSwapObject(this, STRING_CACHE_OFFSET,
null, sc)) {
sc = (String) U.getObjectVolatile(this,
STRING_CACHE_OFFSET);
}
}
}
return sc;
}
private static final Unsafe U = Unsafe.getUnsafe();
private static final long STRING_CACHE_OFFSET;
static {
try {
STRING_CACHE_OFFSET =
U.objectFieldOffset(BigDecimal.class.getDeclaredField(stringCache));
} catch (NoSuchFieldException e) {
throw new Error(e);
}
}
Regards, Peter
