Re: javax.sql.rowset.serial.SerialBlob doesn't support free and getBinaryStream
Hi Lance, Is there any plan for this issue, if any could you update to me? Thanks a lot! On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote: Hi Deven, I will address the needed updates a bit later. Thank you for your input Best Lance On Oct 29, 2012, at 3:51 AM, Deven You wrote: Hi Alan, The Java Spec does not mention the thread safe for JDBC API. But I see the other code in SerialBlob/SerialClob have not consider it. I think use buff == null to replace isFree is a good idea because it also avoid the problem for the condition buf == null isFree == false so we won't need create a readObject method. Thanks for your suggestion for isFree, I will correct it later. Lance: How about your suggestion? Since you mentioned you will develop the implementation yourself. I use my implementation mainly for the test cases. But you may also take a look my implementation. Thanks a lot! On 09/21/2012 04:37 PM, Alan Bateman wrote: On 21/09/2012 04:21, Deven You wrote: Hi Lance, I am very busy with other work so I can't work with the SerialBlob/SerialClob item for long time. I am very happy to refine the current test case and create new tests for SerialClob. I have create a new webre[1] for this task, please review it. [1] http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/ http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/ PS: If the isFree is not transient, I want to know how we add this field to the javadoc serialized form? I don't know very much about the rowset API and I can't see anything to specify whether it is meant to be safe for use by concurrent threads. There are clearly lots of issues here and implementing free introduces a lot more, especially with the possibility of an asynchronous free or more than one thread calling free at around the same time. Have you considered buf == null to mean that the resources are freed? That might avoid needing to change the serialized form. Also as these types are serializable it means you have to consider the case where you deserialize to buf == null isFree == false for example. On that point, it looks to me that this code needs a readObject anyway (for several reasons). A small point is that isFree is a odd name for a method that doesn't return a boolean. If the patch goes ahead then I think it needs a better name, ensureNotFree or requireNotFree or something like that. -Alan. Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
hg: jdk8/tl/jdk: 8003898: X11 toolkit can be chosen as the default toolkit
Changeset: ee6e5b7d5d55 Author:uta Date: 2012-11-23 13:07 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ee6e5b7d5d55 8003898: X11 toolkit can be chosen as the default toolkit Summary: XToolkit is not selected for any values of system-wide environment variables (ex. DISPLAY). Reviewed-by: anthony, art ! src/solaris/native/java/lang/java_props_macosx.c
Re: hg: jdk8/tl/jdk: 8003898: X11 toolkit can be chosen as the default toolkit
On 23/11/2012 09:09, alexey.ut...@oracle.com wrote: Changeset: ee6e5b7d5d55 Author:uta Date: 2012-11-23 13:07 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ee6e5b7d5d55 8003898: X11 toolkit can be chosen as the default toolkit Summary: XToolkit is not selected for any values of system-wide environment variables (ex. DISPLAY). Reviewed-by: anthony, art ! src/solaris/native/java/lang/java_props_macosx.c Thank you, it's great to have this fix in jdk8/tl, it means we should now finally be able to un-exclude all those tests that you proposed in your earlier webrev. -Alan
Review request: JDK-7162111 TEST_BUG: change tests run in headless mode [macosx] (open part - ver. 3)
The sub-task JDK-8003898 (X11 toolkit can be chosen as the default toolkit) was resolved. New version that takes it into account (changes in TraceJFrame.java): http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7162111/webrev.03/ On 22.11.2012 13:57, Alan Bateman wrote: On 22/11/2012 07:38, Alexey Utkin wrote: Bug description: https://jbs.oracle.com/bugs/browse/JDK-7162111 Here is the suggested fix that contains the changes in JFrameCreateTime.java: http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7162111/webrev.02 The changes to test/java/io/Serializable/resolveClass/* looks fine for me. On JFrameCreateTime.java then I wonder if the headless check should be in TraceJFrame instead. I suspect this because JFrameCreateTime is the target app that is traced. Either way, I'm curious why Throwable is caught I looked at GraphicsEnvironment.getLocalGraphicsEnvironment() and isHeadlessInstance() and neither is specified to throw anything, I'm just curious what we are catching and recovering from, an Error perhaps? Done. -Alan.
Re: Review request: JDK-7162111 TEST_BUG: change tests run in headless mode [macosx] (open part - ver. 3)
On 23/11/2012 10:40, Alexey Utkin wrote: The sub-task JDK-8003898 (X11 toolkit can be chosen as the default toolkit) was resolved. New version that takes it into account (changes in TraceJFrame.java): http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7162111/webrev.03/ This looks good to me. Do you mind leaving in the jdk_io header in ProblemList? It just makes it obvious to people where to list tests as we've already tried to group them by area (no need to generate a new webrev for this of course). -Alan
Re: bottleneck by java.lang.Class.getAnnotations() - a better patch
Hi David, Alan, Alexander and others, Here's a refinement of a patch I proposed in this thread a couple of weeks ago: http://dl.dropbox.com/u/101777488/jdk8-hacks/JEP-149/webrev.02/index.html The changed sources can be browsed here: https://github.com/plevart/jdk8-hacks/tree/annotation-arrays/src/java/lang The main improvement is the replacement of a filed: MapClass? extends Annotation, Annotation declaredAnnotations; with plain: Annotation[] declaredAnnotations; ...in the Class.VolatileData structure. It can be seen from the public API that caching a HashMap of declared annotations is never needed since the only public method (getDeclaredAnnotations()) returns an array. Therefore it is much more efficient to cache the array and only clone it on every public method invocation. Unfortunately it is not so with annotations field. There is a public method (getAnnotation(Class? extends Annotation)) that has O(1) time complexity because of keeping a HashMap in the cache. I did an experiment with keeping annotations (declared + inherited) in an array, sorted by the hashCode of annotationType and the lookup then was a binary search by hashCode of annotationType followed by linear search of the equal hashCode neighborhood. This has O(log N) time complexity, which is not so bad, but the constant factor was pretty high because invoking Annotation.annotationType() method goes through dynamic Proxy to an InvocationHandler... Speaking of that, I think there's plenty of opportunity for JEP-149 related optimization in the field of annotations. For example, generating a special class for every annotationType that would hold it's own annotation data and answer to method requests directly would give a large time and space gain. It can also be noticed that there's a discrepancy of how lookup is implemented for example: - lookup of Annotation by annotationType on a class is a HashMap.get() which has O(1) time complexity - lookup of a of Field by fieldName on a class is a linear search in an array which has O(N) time complexity - similar for Method by methodName and argumentTypes - O(N) The time complexity improvement for Field and Method lookup could be performed by keeping some cached arrays pre-sorted by the name of Field or Method and employing a binary search to locate or almost-locate the object. Since public API explicitly states that methods returning arrays of Fields and Methods do not specify any order, pre-sorting the arrays would not break the contract. Regards, Peter On 11/07/2012 11:39 AM, Peter Levart wrote: On 11/07/2012 03:10 AM, David Holmes wrote: Hi Peter, The movement of the reflection caches to a helper object is exactly what I had previously proposed here (some differences in the details of course): http://cr.openjdk.java.net/~dholmes/JEP-149/webrev/ and discussed here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009749.html but this did not touch the annotations fields. David Hi David, Thanks for the pointer. There is a discussion between Brian and you (to quote some of it): On 5/04/2012 1:28 PM, Brian Goetz wrote: / Reducing the number of SoftReferences in ReflectionHelper also seems an // attractive target for memory reduction. Rather than eight soft // references (eight extra objects), maintaining a SoftRef to the entire // RH, or at least to the part of the RH that is currently SR'ed if the two // non-SR'ed fields can't be recomputed, would save you a whole pile of // objects per class (and might also reduce pressure on GC, having 8x fewer // SRs to process.) / I'd have to consider the intended semantics of these soft references before considering any change here. It would hard to predict how this might impact runtime performance if we have coarser-grained soft references. The current changes should be semantically null. / Finally, you may be able save an extra field per Class by storing the // ReflectionHelper in a ClassValue on Java SE 8, rather than a field. / ClassValue is something I'm keeping an eye on, but an entry in ClassValue is going to consume more dynamic memory than a single direct field. Thanks, David ...the 8 SoftReferences refer to arrays which are never hard referenced by the outside world (arrays are always defensively copied), so it's reasonable to expect that all SoftReferences would be cleared at the same time anyway. And if 8 SoftReferences are replaced with 1, the worst case scenario (to quote Hinkmond Wong): Hi Brian, One of the issues we have in the Java Embedded group (as David points out in his summary), is that while on paper the theoretical max savings seems great (as you point out also), in practice as David points out in his note, this might be a wash if there are a lot more reflection using classes vs. non-reflection using classes in typical real-world applications, not the low or zero reflection using class ratio that happens in the theoretical best case.
Re: Request for review: 7173494: some jdk tests are not run in test/Makefile
On 22/11/2012 16:36, Rob McKenna wrote: Hi folks, Looking to backport these changes to the test makefiles to jdk7. As per Alans original mail: This one is a small clean-up of the test targets defined in jdk/test/Makefile. The union of the tests executed by each of the make targets should be the entire test suite but this isn't so, there are small number of tests that aren't run. I've renamed jdk_misc to jdk_other (the original name is confusing because of sun.misc) and expanded it to run additional areas that have a small number of tests. If more tests are added to these areas over time then it may make sense to add new targets in the future. make jdk_jmx now runs the JMX tests as it was confusing to have those tests split between management1 and management2. I've also renamed jdk_tools1 to jdk_jdi to make it clear that this is the JDI tests rather than tools. When Kelly originally set this up he split the NIO tests into 3 batches, I don't think this is necessary any longer (the really slow tests have been long been dialed down or changed to run much faster). The tl folder refers to the forest root, the jdk folder refers to the jdk repo. http://cr.openjdk.java.net/~robm/7173494/ ( 8 changesets: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3b79177ebfef http://hg.openjdk.java.net/jdk8/tl/rev/4bde5640fb36 ) -Rob This looks okay to me, at least I don't see anything obviously different to the changes that we have in 8. I assume you will test these changes well before pushing them as it's very easy to get the configuration wrong. -Alan
Re: Review request: JDK-7162111 TEST_BUG: change tests run in headless mode [macosx] (open part - ver. 3)
On 23.11.2012 14:58, Alan Bateman wrote: On 23/11/2012 10:40, Alexey Utkin wrote: The sub-task JDK-8003898 (X11 toolkit can be chosen as the default toolkit) was resolved. New version that takes it into account (changes in TraceJFrame.java): http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7162111/webrev.03/ This looks good to me. Do you mind leaving in the jdk_io header in ProblemList? It just makes it obvious to people where to list tests as we've already tried to group them by area (no need to generate a new webrev for this of course). I do not know the policy for such a case. If you like to remove the jdk_io header in ProblemList, I will. -uta -Alan
Re: Review request: JDK-7162111 TEST_BUG: change tests run in headless mode [macosx] (open part - ver. 3)
On 23/11/2012 11:22, Alexey Utkin wrote: On 23.11.2012 14:58, Alan Bateman wrote: On 23/11/2012 10:40, Alexey Utkin wrote: The sub-task JDK-8003898 (X11 toolkit can be chosen as the default toolkit) was resolved. New version that takes it into account (changes in TraceJFrame.java): http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7162111/webrev.03/ This looks good to me. Do you mind leaving in the jdk_io header in ProblemList? It just makes it obvious to people where to list tests as we've already tried to group them by area (no need to generate a new webrev for this of course). I do not know the policy for such a case. If you like to remove the jdk_io header in ProblemList, I will. I would prefer if you left it in the file (you'll see other examples in there where we leave a header as a placeholder so that folks know where to list the tests). -Alan
Re: Review request: JDK-7162111 TEST_BUG: change tests run in headless mode [macosx] (open part - ver. 3)
On 23.11.2012 15:24, Alan Bateman wrote: On 23/11/2012 11:22, Alexey Utkin wrote: On 23.11.2012 14:58, Alan Bateman wrote: On 23/11/2012 10:40, Alexey Utkin wrote: The sub-task JDK-8003898 (X11 toolkit can be chosen as the default toolkit) was resolved. New version that takes it into account (changes in TraceJFrame.java): http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7162111/webrev.03/ This looks good to me. Do you mind leaving in the jdk_io header in ProblemList? It just makes it obvious to people where to list tests as we've already tried to group them by area (no need to generate a new webrev for this of course). I do not know the policy for such a case. If you like to remove the jdk_io header in ProblemList, I will. I would prefer if you left it in the file (you'll see other examples in there where we leave a header as a placeholder so that folks know where to list the tests). I mean: # jdk_io -# 7162111 - these tests need to be updated to run headless -java/io/Serializable/resolveClass/deserializeButton/run.sh macosx-all -java/io/Serializable/serialver/classpath/run.sh macosx-all -java/io/Serializable/serialver/nested/run.shmacosx-all - -Alan
Re: Request for Review: 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion
On 22/11/2012 21:27, Rob McKenna wrote: Hi folks, Looking for a review for the webrev below, which also resolves: 7175692: (process) Process.exec should use posix_spawn [macosx] For additional context and a brief description it would be well worth looking at the following thread started by Michael McMahon, who did the brunt of the work for this fix: http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/thread.html#1644 Basically the fix aims to swap fork for posix_spawn as the default process launch mechanism on Solaris and Mac OSX in order to avoid swap exhaustion issues encountered with fork()/exec(). It also offers a flag (java.lang.useFork) to allow a user to revert to the old behaviour. I'm having trouble seeing the wood for the trees at this point so I'm anticipating plenty of feedback. In particular I'd appreciate some discussion on: - The binary launcher name property name may need some work. A more general property (java.lang.launchMechanism) to allow a user to specify a particular call was mooted too. It may be more future proof and I'm completely open to that. (e.g. launchMechanism=spawn|fork|vfork|clone - we would obviously ignore inapplicable values on a per-platform basis) - I'd like a more robust way of checking that someone isn't trying to use jprochelper outside of the context for which it is meant. - The decision around which call to use getting moved to the java level and away from the native preprocessor. The webrev is at: http://cr.openjdk.java.net/~robm/5049299/webrev.01/ http://cr.openjdk.java.net/%7Erobm/5049299/webrev.01/ It's great to get this one moving again, we should have helped Michael more to get this over the line on the first outing. This one will require very careful review, I don't have cycles this week, I hope others do. For now I think that naming the trampoline jprochelper or jspawnhelper is okay. To make it more robust then I'd probably prepend a magic number or some token. Also I'd probably fstat stdin and uses S_FIFO to check the mode. As the property is implementation specific then I think something like jdk.lang.process.useFork is a better start. It would be nice not to require it although I agree we have to walk carefully as this area has a tendency to bite back. I don't think you need to make it any more configurable than that. If this is successful then I think we should look at updating the hotspot code too as it has the same issue with VM options such as OnError. -Alan.
Re: hg: jdk8/tl/jdk: 6924259: Remove offset and count fields from java.lang.String
I just realized that I also use String.trim() a lot, and have always assumed it's very cheap; that's no longer the case. JDK itself contains many usages of trim(); even a lot of str.substring().trim() Just saying. On Wed, Nov 14, 2012 at 9:14 AM, Zhong Yu zhong.j...@gmail.com wrote: Changing String.substring() from O(1) to O(n) is a big deal; we may say it breaks compatibility. Any code that intends to work across JDK versions before and after 7u6 cannot use the method, since its behavior is so different in different versions. Any deployment that upgrades JDK to 7u6 and later needs to review all its usages of substring(). That's a ton of work. A quick workaround might be to refactor all substring() usages to some `old_substring()` method that preserves O(1). Unfortunately `old_substring()` cannot exist, so there's no quick workaround possible. The memory leak problem of the old substring() method is well-known among Java programmers, it's not really a big problem today. For the uninitiated, they might expect that substring() is leak-free; but they might also expect that substring() is O(1). There's no apparent reason why favoring one is better than favoring the other. In the old implementation, there's a workaround to achieve leak-free, by `new String(String)`. In the new implementation, there is no workaround to achieve O(1), unless the developer uses something other than String. It is basically impossible to change String to another type if it's part of a method signature. With all these problems, please reconsider this change and see if you should roll it back. Thanks. Zhong Yu
Re: javax.sql.rowset.serial.SerialBlob doesn't support free and getBinaryStream
It is on my list. to update the javadocs I need a ccc which I have not done yet and is needed as part of this change On Nov 23, 2012, at 3:07 AM, Deven You wrote: Hi Lance, Is there any plan for this issue, if any could you update to me? Thanks a lot! On 10/29/2012 06:39 PM, Lance Andersen - Oracle wrote: Hi Deven, I will address the needed updates a bit later. Thank you for your input Best Lance On Oct 29, 2012, at 3:51 AM, Deven You wrote: Hi Alan, The Java Spec does not mention the thread safe for JDBC API. But I see the other code in SerialBlob/SerialClob have not consider it. I think use buff == null to replace isFree is a good idea because it also avoid the problem for the condition buf == null isFree == false so we won't need create a readObject method. Thanks for your suggestion for isFree, I will correct it later. Lance: How about your suggestion? Since you mentioned you will develop the implementation yourself. I use my implementation mainly for the test cases. But you may also take a look my implementation. Thanks a lot! On 09/21/2012 04:37 PM, Alan Bateman wrote: On 21/09/2012 04:21, Deven You wrote: Hi Lance, I am very busy with other work so I can't work with the SerialBlob/SerialClob item for long time. I am very happy to refine the current test case and create new tests for SerialClob. I have create a new webre[1] for this task, please review it. [1] http://cr.openjdk.java.net/~youdwei/OJDK-576/webrev.01/ http://cr.openjdk.java.net/%7Eyoudwei/OJDK-576/webrev.01/ PS: If the isFree is not transient, I want to know how we add this field to the javadoc serialized form? I don't know very much about the rowset API and I can't see anything to specify whether it is meant to be safe for use by concurrent threads. There are clearly lots of issues here and implementing free introduces a lot more, especially with the possibility of an asynchronous free or more than one thread calling free at around the same time. Have you considered buf == null to mean that the resources are freed? That might avoid needing to change the serialized form. Also as these types are serializable it means you have to consider the case where you deserialize to buf == null isFree == false for example. On that point, it looks to me that this code needs a readObject anyway (for several reasons). A small point is that isFree is a odd name for a method that doesn't return a boolean. If the patch goes ahead then I think it needs a better name, ensureNotFree or requireNotFree or something like that. -Alan. Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: bottleneck by java.lang.Class.getAnnotations() - a better patch
Hi again, I have found an inconsistency in the last patch I posted. Specifically the Method.getAnnotation(Class) and Constructor.getAnnotation(Class) did not delegate to the root instance as .getAnnotations() did. I corrected that in new revision of the patch: http://dl.dropbox.com/u/101777488/jdk8-hacks/JEP-149/webrev.03/index.html I also modified the benchmarks somewhat so that now they exercise the .getAnnotation(Class) methods on various objects instead of bulk getAnnotations() methods: https://raw.github.com/plevart/jdk8-hacks/annotation-arrays/src/test/ReflectionTest.java I got results for 2 different machines: Desktop PC (one socket i7-2600K, 4 cores), Linux 3.6 64bit: https://raw.github.com/plevart/jdk8-hacks/annotation-arrays/benchmark_results_i7-2600K.txt and: Sun Blade T6320 (one socket T2, 8 cores), Solaris 10 64bit: https://raw.github.com/plevart/jdk8-hacks/annotation-arrays/benchmark_results_T2.txt On the T2, the concurrency bottleneck is even more visible. Regards, Peter On 11/23/2012 12:08 PM, Peter Levart wrote: Hi David, Alan, Alexander and others, Here's a refinement of a patch I proposed in this thread a couple of weeks ago: http://dl.dropbox.com/u/101777488/jdk8-hacks/JEP-149/webrev.02/index.html The changed sources can be browsed here: https://github.com/plevart/jdk8-hacks/tree/annotation-arrays/src/java/lang The main improvement is the replacement of a filed: MapClass? extends Annotation, Annotation declaredAnnotations; with plain: Annotation[] declaredAnnotations; ...in the Class.VolatileData structure. It can be seen from the public API that caching a HashMap of declared annotations is never needed since the only public method (getDeclaredAnnotations()) returns an array. Therefore it is much more efficient to cache the array and only clone it on every public method invocation. Unfortunately it is not so with annotations field. There is a public method (getAnnotation(Class? extends Annotation)) that has O(1) time complexity because of keeping a HashMap in the cache. I did an experiment with keeping annotations (declared + inherited) in an array, sorted by the hashCode of annotationType and the lookup then was a binary search by hashCode of annotationType followed by linear search of the equal hashCode neighborhood. This has O(log N) time complexity, which is not so bad, but the constant factor was pretty high because invoking Annotation.annotationType() method goes through dynamic Proxy to an InvocationHandler... Speaking of that, I think there's plenty of opportunity for JEP-149 related optimization in the field of annotations. For example, generating a special class for every annotationType that would hold it's own annotation data and answer to method requests directly would give a large time and space gain. It can also be noticed that there's a discrepancy of how lookup is implemented for example: - lookup of Annotation by annotationType on a class is a HashMap.get() which has O(1) time complexity - lookup of a of Field by fieldName on a class is a linear search in an array which has O(N) time complexity - similar for Method by methodName and argumentTypes - O(N) The time complexity improvement for Field and Method lookup could be performed by keeping some cached arrays pre-sorted by the name of Field or Method and employing a binary search to locate or almost-locate the object. Since public API explicitly states that methods returning arrays of Fields and Methods do not specify any order, pre-sorting the arrays would not break the contract. Regards, Peter On 11/07/2012 11:39 AM, Peter Levart wrote: On 11/07/2012 03:10 AM, David Holmes wrote: Hi Peter, The movement of the reflection caches to a helper object is exactly what I had previously proposed here (some differences in the details of course): http://cr.openjdk.java.net/~dholmes/JEP-149/webrev/ and discussed here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009749.html but this did not touch the annotations fields. David Hi David, Thanks for the pointer. There is a discussion between Brian and you (to quote some of it): On 5/04/2012 1:28 PM, Brian Goetz wrote: / Reducing the number of SoftReferences in ReflectionHelper also seems an // attractive target for memory reduction. Rather than eight soft // references (eight extra objects), maintaining a SoftRef to the entire // RH, or at least to the part of the RH that is currently SR'ed if the two // non-SR'ed fields can't be recomputed, would save you a whole pile of // objects per class (and might also reduce pressure on GC, having 8x fewer // SRs to process.) / I'd have to consider the intended semantics of these soft references before considering any change here. It would hard to predict how this might impact runtime performance if we have coarser-grained soft references. The current changes should be semantically null. / Finally, you may be able save an
