Re: JDK 9 RFR of JDK-8085879: Mark intermittently failing: java/util/Arrays/ParallelPrefix.java
Look good; thanks, -Joe On 6/4/2015 10:52 PM, Amy Lu wrote: This test has been observed to fail intermittently. This fix is to mark it accordingly with keyword 'intermittent'. bug: https://bugs.openjdk.java.net/browse/JDK-8085879 webrev: http://cr.openjdk.java.net/~amlu/8085879/webrev.00/ Thanks, Amy --- old/test/java/util/Arrays/ParallelPrefix.java 2015-06-05 13:44:24.0 +0800 +++ new/test/java/util/Arrays/ParallelPrefix.java 2015-06-05 13:44:24.0 +0800 @@ -26,6 +26,7 @@ * @summary unit test for Arrays.ParallelPrefix(). * @author Tristan Yan * @run testng ParallelPrefix + * @key intermittent
Review request for JDK-8080906 JDK-8080908: Develop tests for JEP 255 Xerces Updates
Hi Joe and all I have been working on the test task of JEP 255 Xerces Updates. Here I would invite you to review the changes for 2 bugs of this task: 1. JDK-8080906 Develop test for Xerces Update: DOM L3 Serializer To verify default LSSerializer is Xalan dom 3 serializer 2. JDK-8080908 Develop test for Xerces Update: XPointer To verify Xerces revision 415823: XERCESJ-1134. It should have been fixed in JDK-6794483, however, the test Bug6794483Test.java need to be revised to really cover this bug. I also added module dependencies for exported API in this path, it's recommended for Jigsaw change, refer to https://wiki.se.oracle.com/display/JPG/@modules+in+JTReg+tests The webrev is at http://cr.openjdk.java.net/~fyuan/8080906_8080908/webrev.00/, your comment will be appreciated. Best Regards Frank
Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Dmitry,
I’d like to propose the following change to get prettier output (more in line
with GC.class_histogram):
diff --git a/src/share/vm/services/diagnosticCommand.cpp
b/src/share/vm/services/diagnosticCommand.cpp
--- a/src/share/vm/services/diagnosticCommand.cpp
+++ b/src/share/vm/services/diagnosticCommand.cpp
@@ -341,7 +341,6 @@
void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
ResourceMark rm;
- output()-print_cr(Unreachable instances awaiting finalization:);
Klass* k = SystemDictionary::resolve_or_null(
vmSymbols::finalizer_histogram_klass(), THREAD);
assert(k != NULL, FinalizerHistogram class is not accessible);
@@ -375,12 +374,15 @@
assert(count_res != NULL name_res != NULL, Unexpected layout of
FinalizerHistogramEntry);
+ output()-print_cr(Unreachable instances waiting for finalization);
+ output()-print_cr(#instances class name);
+ output()-print_cr(---);
for (int i = 0; i result_oop-length(); ++i) {
oop element_oop = result_oop-obj_at(i);
oop str_oop = element_oop-obj_field(name_fd.offset());
char *name = java_lang_String::as_utf8_string(str_oop);
int count = element_oop-int_field(count_fd.offset());
-output()-print_cr(Class %s - %d, name, count);
+output()-print_cr(%10d %s, count, name);
}
}
A couple of nits:
diagnosticCommand.cpp:359 - extra space after =
diagnosticCommand.cpp:361 - spelling: finlalization - finalization
FinalizerInfoTest.java:69: - spelling: intialized - initialized
FinalizerInfoTest.java:71 - I’d like to see the ; on a new line
Thanks,
/Staffan
On 5 jun 2015, at 00:20, Mandy Chung mandy.ch...@oracle.com wrote:
On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff dmitry.samers...@oracle.com
wrote:
Mandy,
Webrev updated in-place (press shift-reload).
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
getFinalizerHistogram() now returns Entry[]
@library and @build removed from the test.
Looks fine.
Thanks
Mandy
-Dmitry
On 2015-06-04 16:56, Mandy Chung wrote:
On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff dmitry.samers...@oracle.com
wrote:
Mandy,
Thank you for the review.
Updated webrev is:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
Thanks for the update and the test.
addressed comments, added a unit test to JDK workspace.
This test does not need @library and @build, right?
On 2015-06-03 21:36, Mandy Chung wrote:
Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
The VM knows the returned type anyway.
[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]
You already add several symbols - why is it an issue for another one? Or
if adding vm symbols is a concern, you should revert to String and int[]
that you decide not to. The decision should apply consistently (use
String and int, or new Java type).
Perhaps the getFinalizerHistogram method should be renamed
e.g. dump?
The line in vmSymbols looks like:
template(
get_finalizer_histogram_name, getFinalizerHistogram)
I would prefer to keep method name specific enough to be able to
find it by grep in jdk code.
Grep in jdk code is easy as you have a new class :)
Mandy
(other comments are addressed)
-Dmitry
On 2015-06-03 21:36, Mandy Chung wrote:
On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
Updated webrev:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13
I reviewed the jdk change. The version looks okay and some comments:
ReferenceQueue.java - you should eliminate the use of rawtype:
84 Reference rn = r.next;
Change Reference to Reference? extends T
done.
The forEach method - eliminate the use of raw type and
move @SuppressWarning to the field variable in line 182:
@SuppressWarnings(unchecked)
Reference? extends T rn = r.next;
done.
FinalizerHistogram.java
Copyright year needs update.
done.
I personally think the VM code would be much simpler if you stay with
alternative entry of String and int than dealing with a
FinalizerHistogramEntry private class. It's okay as FinalizerHistogram
class is separated.
The comment in line 35 is suitable to move to the class description and
this is the suggestion.
// This FinalizerHistogram class is for GC.finalizer_info diagnostic
command support.
// It is invoked by the VM.
done.
Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
knows the returned type anyway.
[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]
It's an inner class of
FinalizerHistogram and it can simply be named as Entry. I suggest to
have Entry::increment method to replace .instanceCount++.
done.
The tests are in
Re: RFR JDK-8054304: Clarify treatment of bounds in j.l.r.Annotated{WildcardType, TypeVariable}
Hi Srikanth, I would mention that Object is not annotated in the following statements: - AnnotatedWildcardType.getAnnotatedUpperBounds(): Note that if no upper bound is explicitly declared, the upper bound is Object. - AnnotatedTypeVariable.getAnnotatedBounds(): Note that if no bound is explicitly declared, the bound is Object. - TypeVariable.getAnnotatedBounds(): Note that if no upper bound is explicitly declared, the upper bound is Object. Thank you, Elena 04.06.2015 13:16, Srikanth wrote: Hello, Please review this fix for https://bugs.openjdk.java.net/browse/JDK-8054304 (twin for conformance test failures: https://bugs.openjdk.java.net/browse/JDK-8058595) JBS: https://bugs.openjdk.java.net/browse/JDK-8054304 Webrev: http://cr.openjdk.java.net/~sadayapalam/JDK-8054304/webrev.00/ CCC: http://ccc.us.oracle.com/8054304 While as outlined in https://bugs.openjdk.java.net/browse/JDK-8054304, the javadoc of several methods is adjusted, only in one case viz AnnotatedWildcardType.getAnnotatedUpperBounds() there is a behaviour change. All other cases involve javadoc change that explicitly spell out current correct behaviour. The patch passes Tier 1 Tier 2 tests for JDK (i.e identical results with and without) Our plan is to fix this for JDK9 and backport to 8u60. I don't know at the moment if there will be objections for the backport from a process p.o.v as this calls for a specification change. Thanks! Srikanth.
[9] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: GC overhead limit exceeded
Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8068416 Webrev is http://cr.openjdk.java.net/~kshefov/8068416/webrev.01/ Test failure is caused by JDK-8078602 [1]. Suggestion is to exclude the test until [1] is fixed. [1] https://bugs.openjdk.java.net/browse/JDK-8078602 Thanks -Konstantin
Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Staffan,
Thank you for review!
Done. Webrev updated in-place (press shift-reload).
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
-Dmitry
On 2015-06-05 11:20, Staffan Larsen wrote:
Dmitry,
I’d like to propose the following change to get prettier output (more in line
with GC.class_histogram):
diff --git a/src/share/vm/services/diagnosticCommand.cpp
b/src/share/vm/services/diagnosticCommand.cpp
--- a/src/share/vm/services/diagnosticCommand.cpp
+++ b/src/share/vm/services/diagnosticCommand.cpp
@@ -341,7 +341,6 @@
void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
ResourceMark rm;
- output()-print_cr(Unreachable instances awaiting finalization:);
Klass* k = SystemDictionary::resolve_or_null(
vmSymbols::finalizer_histogram_klass(), THREAD);
assert(k != NULL, FinalizerHistogram class is not accessible);
@@ -375,12 +374,15 @@
assert(count_res != NULL name_res != NULL, Unexpected layout of
FinalizerHistogramEntry);
+ output()-print_cr(Unreachable instances waiting for finalization);
+ output()-print_cr(#instances class name);
+ output()-print_cr(---);
for (int i = 0; i result_oop-length(); ++i) {
oop element_oop = result_oop-obj_at(i);
oop str_oop = element_oop-obj_field(name_fd.offset());
char *name = java_lang_String::as_utf8_string(str_oop);
int count = element_oop-int_field(count_fd.offset());
-output()-print_cr(Class %s - %d, name, count);
+output()-print_cr(%10d %s, count, name);
}
}
A couple of nits:
diagnosticCommand.cpp:359 - extra space after =
diagnosticCommand.cpp:361 - spelling: finlalization - finalization
FinalizerInfoTest.java:69: - spelling: intialized - initialized
FinalizerInfoTest.java:71 - I’d like to see the ; on a new line
Thanks,
/Staffan
On 5 jun 2015, at 00:20, Mandy Chung mandy.ch...@oracle.com wrote:
On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff dmitry.samers...@oracle.com
wrote:
Mandy,
Webrev updated in-place (press shift-reload).
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
getFinalizerHistogram() now returns Entry[]
@library and @build removed from the test.
Looks fine.
Thanks
Mandy
-Dmitry
On 2015-06-04 16:56, Mandy Chung wrote:
On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff
dmitry.samers...@oracle.com wrote:
Mandy,
Thank you for the review.
Updated webrev is:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
Thanks for the update and the test.
addressed comments, added a unit test to JDK workspace.
This test does not need @library and @build, right?
On 2015-06-03 21:36, Mandy Chung wrote:
Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
The VM knows the returned type anyway.
[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]
You already add several symbols - why is it an issue for another one? Or
if adding vm symbols is a concern, you should revert to String and int[]
that you decide not to. The decision should apply consistently (use
String and int, or new Java type).
Perhaps the getFinalizerHistogram method should be renamed
e.g. dump?
The line in vmSymbols looks like:
template(
get_finalizer_histogram_name, getFinalizerHistogram)
I would prefer to keep method name specific enough to be able to
find it by grep in jdk code.
Grep in jdk code is easy as you have a new class :)
Mandy
(other comments are addressed)
-Dmitry
On 2015-06-03 21:36, Mandy Chung wrote:
On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
Updated webrev:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13
I reviewed the jdk change. The version looks okay and some comments:
ReferenceQueue.java - you should eliminate the use of rawtype:
84 Reference rn = r.next;
Change Reference to Reference? extends T
done.
The forEach method - eliminate the use of raw type and
move @SuppressWarning to the field variable in line 182:
@SuppressWarnings(unchecked)
Reference? extends T rn = r.next;
done.
FinalizerHistogram.java
Copyright year needs update.
done.
I personally think the VM code would be much simpler if you stay with
alternative entry of String and int than dealing with a
FinalizerHistogramEntry private class. It's okay as FinalizerHistogram
class is separated.
The comment in line 35 is suitable to move to the class description and
this is the suggestion.
// This FinalizerHistogram class is for GC.finalizer_info diagnostic
command support.
// It is invoked by the VM.
done.
Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
knows the returned type anyway.
[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]
It's an inner
Re: [8u-dev] Review request : JDK-8062904: TEST_BUG: Tests java/lang/invoke/LFCaching fail when run with -Xcomp option
Konstantin, The test has the code cache overflow failure only when run with -Xcomp, all other failures has been fixed by https://bugs.openjdk.java.net/browse/JDK-8058733 So my suggestion is either to exclude this test when run with -Xcomp or (better) to reduce iteration number to 1 when -Xcomp, so that no code cache overflow in this case. I'd like to get an answer to my previous question: How reliable the test is if it ignores NoSuchMethodException VirtualMachineError? Are there other manifestations of the problem? Most notably, have you seen VM crashes w/ -Xcomp running that test? Best regards, Vladimir Ivanov -Konstantin Best regards, Vladimir Ivanov -Konstantin On 29.05.2015 14:49, Vladimir Ivanov wrote: What do you mean by ignore code cache overflow? Do you mean I should fix the test to ignore these errors or should I leave this test unfixed because it is a product related issue? The former. How reliable the test is if it ignores NoSuchMethodException VirtualMachineError? Are there other manifestations of the problem? Best regards, Vladimir Ivanov On 28.05.2015 21:22, Vladimir Ivanov wrote: Got it, thanks. Can we ignore errors caused by code cache overflow for now? Best regards, Vladimir Ivanov On 5/28/15 12:03 PM, Konstantin Shefov wrote: Vladimir, This fix is not for timeout issue, this fix is for java.lang.VirtualMachineError: out of space in CodeCache for adapters. Timeout issue is other bug and should be filed separately. I do not know why SQE added RULES with timeout to this bug. By the way, if -Xcomp is set on JDK 8u, test works if not more than one iteration is allowed. The same thing was for JDK 9 until JDK-8046809 had been fixed. -Konstantin On 27.05.2015 19:54, Vladimir Ivanov wrote: Have you tried to reduce iteration granularity? Probably, checking execution duration on every test case is more robust. Best regards, Vladimir Ivanov On 5/27/15 5:50 PM, Konstantin Shefov wrote: Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8062904 Webrev is http://cr.openjdk.java.net/~kshefov/8062904/webrev.01/ Test fails only against JDK 8u and passes against JDK 9. Thanks -Konstantin
Re: [9] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: GC overhead limit exceeded
Vladimir Thanks for reviewing Here is corrected fix: http://cr.openjdk.java.net/~kshefov/8068416/webrev.02 -Konstantin On 06/05/2015 01:00 PM, Vladimir Ivanov wrote: + * @ignore until 8078602 is fixed The standard way to exclude a test is to mention bug ID. Use the following: @ignore 8078602 Otherwise, looks good. Best regards, Vladimir Ivanov On 6/5/15 12:48 PM, Konstantin Shefov wrote: Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8068416 Webrev is http://cr.openjdk.java.net/~kshefov/8068416/webrev.01/ Test failure is caused by JDK-8078602 [1]. Suggestion is to exclude the test until [1] is fixed. [1] https://bugs.openjdk.java.net/browse/JDK-8078602 Thanks -Konstantin
[8u60] approval request for JDK-8068416: LFGarbageCollectedTest.java fails with OOME: GC overhead limit exceeded
Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8068416 jdk9 review thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033998.html jdk8u-dev webrev: http://cr.openjdk.java.net/~kshefov/8068416/webrev.02 Thanks, -Konstantin
Re: RFR 8072773 (fs) Files.lines needs a better splitting implementation for stream source
On Jun 5, 2015, at 12:57 AM, Stefan Zobel splitera...@gmail.com wrote: With respect to the 'stream test library not closing streams' bug: would you mind to create a separate bugtracker issue for that, or do you think it's too special to carry that weight? Not too special just more more work than necessary to separate them out (since the file lines test is in effect also serving as test for the test library changes). Paul.
Re: [8u60] approval request for JDK-8068416: LFGarbageCollectedTest.java fails with OOME: GC overhead limit exceeded
Approved. Regards, Sean. On 05/06/15 11:30, Konstantin Shefov wrote: Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8068416 jdk9 review thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033998.html jdk8u-dev webrev: http://cr.openjdk.java.net/~kshefov/8068416/webrev.02 Thanks, -Konstantin
Re: [9] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: GC overhead limit exceeded
+ * @ignore until 8078602 is fixed The standard way to exclude a test is to mention bug ID. Use the following: @ignore 8078602 Otherwise, looks good. Best regards, Vladimir Ivanov On 6/5/15 12:48 PM, Konstantin Shefov wrote: Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8068416 Webrev is http://cr.openjdk.java.net/~kshefov/8068416/webrev.01/ Test failure is caused by JDK-8078602 [1]. Suggestion is to exclude the test until [1] is fixed. [1] https://bugs.openjdk.java.net/browse/JDK-8078602 Thanks -Konstantin
Re: [9] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: GC overhead limit exceeded
Good. Best regards, Vladimir Ivanov On 6/5/15 1:05 PM, Konstantin Shefov wrote: Vladimir Thanks for reviewing Here is corrected fix: http://cr.openjdk.java.net/~kshefov/8068416/webrev.02 -Konstantin On 06/05/2015 01:00 PM, Vladimir Ivanov wrote: + * @ignore until 8078602 is fixed The standard way to exclude a test is to mention bug ID. Use the following: @ignore 8078602 Otherwise, looks good. Best regards, Vladimir Ivanov On 6/5/15 12:48 PM, Konstantin Shefov wrote: Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8068416 Webrev is http://cr.openjdk.java.net/~kshefov/8068416/webrev.01/ Test failure is caused by JDK-8078602 [1]. Suggestion is to exclude the test until [1] is fixed. [1] https://bugs.openjdk.java.net/browse/JDK-8078602 Thanks -Konstantin
RFR [9] 8081517: minor cleanup for docs
Hello, Could you please review the fix http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8081517 Just some cleanup for docs. Thanks, Alexander
Re: Patch to improve primitives Array.sort()
On May 26, 2015, at 11:54 AM, Paul Sandoz paul.san...@oracle.com wrote: Here is an updated webrev: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8080945-nearly-sorted-primitives/webrev/ Updated with some contributed JMH benchmarks located in the test area. To be moved when there is a better location (i really want to avoid a repeat experience of trying to find benchmarks in this area). I plan to push either today or Monday. Paul.
Re: RFR [9] 8081517: minor cleanup for docs
Hi Alxander
Overall. looks OK. A couple of things I thought could be changed;
In FTPURLConnection, I probably would surround the code in an {@code} tag
In VmIdentifier, I do not see think we still need p in the LI tags
Best
Lance
On Jun 5, 2015, at 8:51 AM, alexander stepanov
alexander.v.stepa...@oracle.com wrote:
Hello,
Could you please review the fix
http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-8081517
Just some cleanup for docs.
Thanks,
Alexander
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: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo
Thanks - this version looks good to me.
/Staffan
On 5 jun 2015, at 13:00, Dmitry Samersoff dmitry.samers...@oracle.com wrote:
Staffan,
Thank you for review!
Done. Webrev updated in-place (press shift-reload).
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
-Dmitry
On 2015-06-05 11:20, Staffan Larsen wrote:
Dmitry,
I’d like to propose the following change to get prettier output (more in
line with GC.class_histogram):
diff --git a/src/share/vm/services/diagnosticCommand.cpp
b/src/share/vm/services/diagnosticCommand.cpp
--- a/src/share/vm/services/diagnosticCommand.cpp
+++ b/src/share/vm/services/diagnosticCommand.cpp
@@ -341,7 +341,6 @@
void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
ResourceMark rm;
- output()-print_cr(Unreachable instances awaiting finalization:);
Klass* k = SystemDictionary::resolve_or_null(
vmSymbols::finalizer_histogram_klass(), THREAD);
assert(k != NULL, FinalizerHistogram class is not accessible);
@@ -375,12 +374,15 @@
assert(count_res != NULL name_res != NULL, Unexpected layout of
FinalizerHistogramEntry);
+ output()-print_cr(Unreachable instances waiting for finalization);
+ output()-print_cr(#instances class name);
+ output()-print_cr(---);
for (int i = 0; i result_oop-length(); ++i) {
oop element_oop = result_oop-obj_at(i);
oop str_oop = element_oop-obj_field(name_fd.offset());
char *name = java_lang_String::as_utf8_string(str_oop);
int count = element_oop-int_field(count_fd.offset());
-output()-print_cr(Class %s - %d, name, count);
+output()-print_cr(%10d %s, count, name);
}
}
A couple of nits:
diagnosticCommand.cpp:359 - extra space after =
diagnosticCommand.cpp:361 - spelling: finlalization - finalization
FinalizerInfoTest.java:69: - spelling: intialized - initialized
FinalizerInfoTest.java:71 - I’d like to see the ; on a new line
Thanks,
/Staffan
On 5 jun 2015, at 00:20, Mandy Chung mandy.ch...@oracle.com wrote:
On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff dmitry.samers...@oracle.com
wrote:
Mandy,
Webrev updated in-place (press shift-reload).
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
getFinalizerHistogram() now returns Entry[]
@library and @build removed from the test.
Looks fine.
Thanks
Mandy
-Dmitry
On 2015-06-04 16:56, Mandy Chung wrote:
On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff
dmitry.samers...@oracle.com wrote:
Mandy,
Thank you for the review.
Updated webrev is:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14
Thanks for the update and the test.
addressed comments, added a unit test to JDK workspace.
This test does not need @library and @build, right?
On 2015-06-03 21:36, Mandy Chung wrote:
Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
The VM knows the returned type anyway.
[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]
You already add several symbols - why is it an issue for another one? Or
if adding vm symbols is a concern, you should revert to String and int[]
that you decide not to. The decision should apply consistently (use
String and int, or new Java type).
Perhaps the getFinalizerHistogram method should be renamed
e.g. dump?
The line in vmSymbols looks like:
template(
get_finalizer_histogram_name, getFinalizerHistogram)
I would prefer to keep method name specific enough to be able to
find it by grep in jdk code.
Grep in jdk code is easy as you have a new class :)
Mandy
(other comments are addressed)
-Dmitry
On 2015-06-03 21:36, Mandy Chung wrote:
On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
Updated webrev:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13
I reviewed the jdk change. The version looks okay and some comments:
ReferenceQueue.java - you should eliminate the use of rawtype:
84 Reference rn = r.next;
Change Reference to Reference? extends T
done.
The forEach method - eliminate the use of raw type and
move @SuppressWarning to the field variable in line 182:
@SuppressWarnings(unchecked)
Reference? extends T rn = r.next;
done.
FinalizerHistogram.java
Copyright year needs update.
done.
I personally think the VM code would be much simpler if you stay with
alternative entry of String and int than dealing with a
FinalizerHistogramEntry private class. It's okay as FinalizerHistogram
class is separated.
The comment in line 35 is suitable to move to the class description and
this is the suggestion.
// This FinalizerHistogram class is for GC.finalizer_info diagnostic
command support.
// It is invoked by the VM.
done.
Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
knows the
RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)
This review request covers the main part of the work for JEP-223, the new version string format [1]. Basically, we'll call this release Java 9, instead of Java 1.9.0. This patch is a folding of all work that has been done so far in the branch JEP-223-branch in jdk9/sandbox. As you can see, it mostly covers build changes, with some code changes in hotspot, jdk, nashorn and langtools that either are corresponding changes in the product code due to the compiler define flags changing from the build, or follow-up changes to handle the new format. The JEP-223 work is not finished by this patch. In fact, there are known issues remaining even after this patch, typically by code that reads the java.version property and tries to parse it. However, this patch is not directly destined for jdk9/dev, but will go into the special verona/stage forest. As for all patches destined for verona/stage it will be code reviewed as if going to jdk9/dev. Once in verona/stage it will bide its time, and it will be complemented with follow-up patches to address remaining issues. When all such issues are resolved and JEP-223 is fully implemented, all changes will be pushed at once (without further code reviews) into jdk9/dev. This patch has been contributed by Magnus Ihse Bursie, Kumar Srinivasan and Alejandro Murillo. Bug: https://bugs.openjdk.java.net/browse/JDK-8085822 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.01 [1] http://openjdk.java.net/jeps/223
Re: RFR [9] 8081517: minor cleanup for docs
Hi Alexander,
On code examples, please use the template:
pre{@code
}/pre
(not just pre... /pre)
See TraceClassVisitor.java: 92+
andFtpURLConnection.java:68
I would use @code instead of @literal in HttpAuthenticator.java:59; it
will make the protocol stand out.
Also in PlatformLogger.java:74+
2)
If indentation is in the scope of your cleanup...
In JdbcRowSetImpl.java, the indentation of continued lines after @return
or @param tags
isn't correct; the continued line should be indented.
And there are a some very long lines that should be wrapped;
See +5637... If the SQL AS clause was not...
+5784 the indentation needs to be fixed.
+5840 also
+5988
+6265 and after...
in many cases the lines that follow of /* do not align the * .
CachedRowSetWriter.java:
- many method comment blocks are not aligned properly with the method of
field that follows.
For example,
/**
* The codeConnection/code object that this writer will use to make a
* connection to the data source to which it will write data.
*
*/
private transient Connection con;
3)
CachedRowSetWriter.java:
+1056: The tag @ param should be tighted up to @param
Thanks, Roger
On 6/5/2015 8:51 AM, alexander stepanov wrote:
Hello,
Could you please review the fix
http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-8081517
Just some cleanup for docs.
Thanks,
Alexander
Re: RFR [9] 8081517: minor cleanup for docs
Hi Roger On Jun 5, 2015, at 10:08 AM, Roger Riggs roger.ri...@oracle.com wrote: 2) If indentation is in the scope of your cleanup… Agree with you and I did not comment on this as I felt it this should be done separately as there is a fair amount that can be done in these classes for clean up. Best lance In JdbcRowSetImpl.java, the indentation of continued lines after @return or @param tags isn't correct; the continued line should be indented. And there are a some very long lines that should be wrapped; See +5637... If the SQL AS clause was not... +5784 the indentation needs to be fixed. +5840 also +5988 +6265 and after... in many cases the lines that follow of /* do not align the * . CachedRowSetWriter.java: - many method comment blocks are not aligned properly with the method of field that follows. For example, /** * The codeConnection/code object that this writer will use to make a * connection to the data source to which it will write data. * */ private transient Connection con; 3) CachedRowSetWriter.java: +1056: The tag @ param should be tighted up to @param Thanks, Roger On 6/5/2015 8:51 AM, alexander stepanov wrote: Hello, Could you please review the fix http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8081517 Just some cleanup for docs. Thanks, Alexander 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: RFR: 7130985: Four helper classes missing in Sun JDK
Added some cleanup code around the BufferedReaders the leftover test files: http://cr.openjdk.java.net/~robm/7130985/webrev.02/ -Rob On 03/06/15 16:20, Rob McKenna wrote: Meant to get this sorted a while back. There was a thread on this last year ( http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-July/027779.html ) A test has been added since then: http://cr.openjdk.java.net/~robm/7130985/webrev.corba/ http://cr.openjdk.java.net/~robm/7130985/webrev.j2se/ -Rob
[8u60] approval request for JDK-8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable
Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8062198 jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4162bcc663dc Thanks, Maxim
Re: Review request for JDK-8080906 JDK-8080908: Develop tests for JEP 255 Xerces Updates
Yes, the tests look good to me too :-) I'd suggest a couple of things: Bug6794483Test: add 8080908 to the bug tag, and year 2015, to the header. I added a link to JDK-6794483 from JDK-8080908. LSSerializerTest: was missing a bug tag. I believe the original test was for 6439439, so add: @bug 6439439, 8080906 to the test class and @bug 8080906 to test testDefaultLSSerializer. This test would have been helpful in detecting the failure of loading LSSerializerImpl in a Jigsaw build. You may add a note to the test to indicate that it would fail in a Jigsaw build, and that the issue will be fixed in JDK-8080266. The changeset will be split into two (JDK-8080906 JDK-8080908) when check in. Thanks, Joe On 6/5/2015 4:29 AM, Lance Andersen wrote: Hi Frank, These seem OK. I am sure Joe will also review when he wakes up in his part of the world :-) Best Lance On Jun 5, 2015, at 3:45 AM, Frank Yuan frank.y...@oracle.com mailto:frank.y...@oracle.com wrote: Hi Joe and all I have been working on the test task of JEP 255 Xerces Updates. Here I would invite you to review the changes for 2 bugs of this task: 1. JDK-8080906 Develop test for Xerces Update: DOM L3 Serializer To verify default LSSerializer is Xalan dom 3 serializer 2. JDK-8080908 Develop test for Xerces Update: XPointer To verify Xerces revision 415823: XERCESJ-1134. It should have been fixed in JDK-6794483, however, the test Bug6794483Test.java need to be revised to really cover this bug. I also added module dependencies for exported API in this path, it's recommended for Jigsaw change, refer to https://wiki.se.oracle.com/display/JPG/@modules+in+JTReg+tests The webrev is at http://cr.openjdk.java.net/~fyuan/8080906_8080908/webrev.00/, your comment will be appreciated. Best Regards Frank http://oracle.com/us/design/oracle-email-sig-198324.gif http://oracle.com/us/design/oracle-email-sig-198324.gifhttp://oracle.com/us/design/oracle-email-sig-198324.gif http://oracle.com/us/design/oracle-email-sig-198324.gifLance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com mailto:lance.ander...@oracle.com
Re: [8u-dev] Review request : JDK-8062904: TEST_BUG: Tests java/lang/invoke/LFCaching fail when run with -Xcomp option
Konstantin, I'd like to get an answer to my previous question: How reliable the test is if it ignores NoSuchMethodException VirtualMachineError? Are there other manifestations of the problem? Most notably, have you seen VM crashes w/ -Xcomp running that test? I have seen no crashes with -Xcomp in our tests results base, only NoSuchMethodException and VirtualMachineError. Good. Thanks for the info. This failure does not occur with JDK 9 on -Xcomp, I used the same random seeds (-Dseed), and the problem exists only with JDK 8u, not JDK 9. Because there is no failure with JDK 9 I can suppose there could be a product issue in JDK 8u. That's strange. There should be no difference in MethodHandle stub management between 8u 9 now (once allocated they stay forever). 9 has segmented code cache feature and it can lead to observable differences in behavior. How many iterations does the test perform on 8u 9? What if you increase the number? Also, I'd suggest you to monitor code cache usage during the run and when the test finishes. Best regards, Vladimir Ivanov
Re: RFR [9] 8081517: minor cleanup for docs
Hi Alexander On Jun 5, 2015, at 1:33 PM, alexander stepanov alexander.v.stepa...@oracle.com wrote: Hello Lance, Roger, Thank you for the notes, please see the updated webrev: http://cr.openjdk.java.net/~avstepan/8081517/webrev.01/index.html (files changed: FtpURLConnection.java, VmIdentifier.java, TraceClassVisitor.java, HttpAuthenticator.java, PlatformLogger.java and JdbcRowSetImpl.java - some indents were fixed) Looks OK to me. Thank you for making the additional updates SQL AS clause that's probably used for the SQL AS keyword. In such a case the upper case may be suitable. Yes should be upper case Best Lance Regards, Alexander On 05.06.2015 17:12, Lance Andersen wrote: Hi Roger On Jun 5, 2015, at 10:08 AM, Roger Riggs roger.ri...@oracle.com wrote: 2) If indentation is in the scope of your cleanup… Agree with you and I did not comment on this as I felt it this should be done separately as there is a fair amount that can be done in these classes for clean up. Best lance In JdbcRowSetImpl.java, the indentation of continued lines after @return or @param tags isn't correct; the continued line should be indented. And there are a some very long lines that should be wrapped; See +5637... If the SQL AS clause was not... +5784 the indentation needs to be fixed. +5840 also +5988 +6265 and after... in many cases the lines that follow of /* do not align the * . CachedRowSetWriter.java: - many method comment blocks are not aligned properly with the method of field that follows. For example, /** * The codeConnection/code object that this writer will use to make a * connection to the data source to which it will write data. * */ private transient Connection con; 3) CachedRowSetWriter.java: +1056: The tag @ param should be tighted up to @param Thanks, Roger On 6/5/2015 8:51 AM, alexander stepanov wrote: Hello, Could you please review the fix http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8081517 Just some cleanup for docs. Thanks, Alexander 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: RFR [9] 8081517: minor cleanup for docs
Hi Alexander, Looks good, thanks for the updates. Roger On 6/5/2015 1:33 PM, alexander stepanov wrote: Hello Lance, Roger, Thank you for the notes, please see the updated webrev: http://cr.openjdk.java.net/~avstepan/8081517/webrev.01/index.html (files changed: FtpURLConnection.java, VmIdentifier.java, TraceClassVisitor.java, HttpAuthenticator.java, PlatformLogger.java and JdbcRowSetImpl.java - some indents were fixed) SQL AS clause that's probably used for the SQL AS keyword. In such a case the upper case may be suitable. Regards, Alexander On 05.06.2015 17:12, Lance Andersen wrote: Hi Roger On Jun 5, 2015, at 10:08 AM, Roger Riggs roger.ri...@oracle.com wrote: 2) If indentation is in the scope of your cleanup… Agree with you and I did not comment on this as I felt it this should be done separately as there is a fair amount that can be done in these classes for clean up. Best lance In JdbcRowSetImpl.java, the indentation of continued lines after @return or @param tags isn't correct; the continued line should be indented. And there are a some very long lines that should be wrapped; See +5637... If the SQL AS clause was not... +5784 the indentation needs to be fixed. +5840 also +5988 +6265 and after... in many cases the lines that follow of /* do not align the * . CachedRowSetWriter.java: - many method comment blocks are not aligned properly with the method of field that follows. For example, /** * The codeConnection/code object that this writer will use to make a * connection to the data source to which it will write data. * */ private transient Connection con; 3) CachedRowSetWriter.java: +1056: The tag @ param should be tighted up to @param Thanks, Roger On 6/5/2015 8:51 AM, alexander stepanov wrote: Hello, Could you please review the fix http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8081517 Just some cleanup for docs. Thanks, Alexander 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: RFR [9] 8081517: minor cleanup for docs
Hello Lance, Roger, Thank you for the notes, please see the updated webrev: http://cr.openjdk.java.net/~avstepan/8081517/webrev.01/index.html (files changed: FtpURLConnection.java, VmIdentifier.java, TraceClassVisitor.java, HttpAuthenticator.java, PlatformLogger.java and JdbcRowSetImpl.java - some indents were fixed) SQL AS clause that's probably used for the SQL AS keyword. In such a case the upper case may be suitable. Regards, Alexander On 05.06.2015 17:12, Lance Andersen wrote: Hi Roger On Jun 5, 2015, at 10:08 AM, Roger Riggs roger.ri...@oracle.com wrote: 2) If indentation is in the scope of your cleanup… Agree with you and I did not comment on this as I felt it this should be done separately as there is a fair amount that can be done in these classes for clean up. Best lance In JdbcRowSetImpl.java, the indentation of continued lines after @return or @param tags isn't correct; the continued line should be indented. And there are a some very long lines that should be wrapped; See +5637... If the SQL AS clause was not... +5784 the indentation needs to be fixed. +5840 also +5988 +6265 and after... in many cases the lines that follow of /* do not align the * . CachedRowSetWriter.java: - many method comment blocks are not aligned properly with the method of field that follows. For example, /** * The codeConnection/code object that this writer will use to make a * connection to the data source to which it will write data. * */ private transient Connection con; 3) CachedRowSetWriter.java: +1056: The tag @ param should be tighted up to @param Thanks, Roger On 6/5/2015 8:51 AM, alexander stepanov wrote: Hello, Could you please review the fix http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8081517 Just some cleanup for docs. Thanks, Alexander 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: RFR 8080640: Reduce copying when reading JAR/ZIP entries
Hi Sherman, I have a new webrev which reverts that part, http://cr.openjdk.java.net/~sfriberg/JDK-8080640/webrev.2/ Summary of changes Reduce lock region in ZipFile.getInputstream Add private ZipFile.getBytes that can be used in select places in the JDK where all bytes will be read Could you sponsor this change once it has been reviewed? Thanks, Staffan On 06/03/2015 10:45 AM, Xueming Shen wrote: Staffan, I'm not convinced that the benefit here is significant enough to change the getInputStream() to return a ByteArrayInputStream, given this can be easily achieved by wrapping the returned byte[] from getBytes(ZipEntry) at user's site. I would suggest to file a separate rfe on this disagreement and move on with the agreed getBytes() for now. Thanks, -Sherman On 06/02/2015 10:27 AM, Staffan Friberg wrote: On 05/22/2015 01:15 PM, Staffan Friberg wrote: On 05/22/2015 11:51 AM, Xueming Shen wrote: On 05/22/2015 11:41 AM, Staffan Friberg wrote: On 05/21/2015 11:00 AM, Staffan Friberg wrote: On 05/21/2015 09:48 AM, Staffan Friberg wrote: On 05/20/2015 10:57 AM, Xueming Shen wrote: On 05/18/2015 06:44 PM, Staffan Friberg wrote: Hi, Wanted to get reviews and feedback on this performance improvement for reading from JAR/ZIP files during classloading by reducing unnecessary copying and reading the entry in one go instead of in small portions. This shows a significant improvement when reading a single entry and for a large application with 10k classes and 500+ JAR files it improved the startup time by 4%. For more details on the background and performance results please see the RFE entry. RFE - https://bugs.openjdk.java.net/browse/JDK-8080640 WEBREV - http://cr.openjdk.java.net/~sfriberg/JDK-8080640/webrev.0 Cheers, Staffan Hi Staffan, If I did not miss something here, from your use scenario it appears to me the only thing you really need here to help boost your performance is byte[] ZipFile.getAllBytes(ZipEntry ze); You are allocating a byte[] at use side and wrapping it with a ByteBuffer if the size is small enough, otherwise, you letting the ZipFile to allocate a big enough one for you. It does not look like you can re-use that byte[] (has to be wrapped by the ByteArrayInputStream and return), why do you need two different methods here? The logic would be much easier to simply let the ZipFile to allocate the needed buffer with appropriate size, fill the bytes and return, with a OOME if the entry size is bigger than 2g. The only thing we use from the input ze is its name, get the size/csize from the jzentry, I don't think jzentry.csize/size can be unknown, they are from the cen table. If the real/final use of the bytes is to wrap it with a ByteArrayInputStream,why bother using ByteBuffer here? Shouldn't a direct byte[] with exactly the size of the entry server better. -Sherman Hi Sherman, Thanks for the comments. I agree, was starting out with bytebuffer because I was hoping to be able to cache things where the buffer was being used, but since the buffer is past along further I couldn't figure out a clean way to do it. Will rewrite it to simply just return a buffer, and only wrap it in the Resource class getByteBuffer. What would be your thought on updating the ZipFile.getInputStream to return ByteArrayInputStream for small entries? Currently I do that work outside in two places and moving it would potentially speed up others reading small entries as well. Thanks, Staffan Just realized that my use of ByteArrayInputStream would miss Jar verification if enabled so the way to go hear would be to add it if possible to the ZipFile.getInputStream. //Staffan Hi, Here is an updated webrev which uses a byte[] directly and also uses ByteArrayInputStream in ZipFile for small entries below 128k. I'm not sure about the benefit of doing the ByteArrayInputStream in ZipFile.getInputStream. It has the consequence of changing the expected behavior of getInputStream() (instead of return an input stream waiting for reading, it now reads all bytes in advance), something we might not want to do in a performance tuning. Though it might be reasonable to guess everyone get an input stream is to read all bytes from it later. -Sherman http://cr.openjdk.java.net/~sfriberg/JDK-8080640/webrev.1 //Staffan Agree that it will change the behavior slightly, but as you said it is probably expected that some one will read the stream eventually. We could reduce the size further if that makes a difference, if the size is below 65k we would not use more memory than the buffer allocated for the InflaterStream today. The total allocation would be slightly larger for deflated entries as we would allocate a byte[] for the compressed bytes, but it would be GC:able and not kept alive. So from a memory perspective the difference is very limited. //Staffan Hi, Bumping this thread to get some more comments on the concern about changing
Re: [8u60] approval request for JDK-8059411: RowSetWarning does not correctly chain warnings
Approved. -Rob On 05/06/15 16:45, Maxim Soloviev wrote: Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8059411 jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4110a7627857 Thanks, Maxim
[8u60] approval request for JDK-8066188: BaseRowSet returns the wrong default value for escape processing
Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8066188 jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/f619341171c0 Thanks, Maxim
Re: [8u60] approval request for JDK-8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable
Approved. -Rob On 05/06/15 16:39, Maxim Soloviev wrote: Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8062198 jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4162bcc663dc Thanks, Maxim
[8u60] approval request for JDK-8059411: RowSetWarning does not correctly chain warnings
Please approve direct backport. bug: https://bugs.openjdk.java.net/browse/JDK-8059411 jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4110a7627857 Thanks, Maxim
Re: Review request for JDK-8080906 JDK-8080908: Develop tests for JEP 255 Xerces Updates
Hi Frank, These seem OK. I am sure Joe will also review when he wakes up in his part of the world :-) Best Lance On Jun 5, 2015, at 3:45 AM, Frank Yuan frank.y...@oracle.com wrote: Hi Joe and all I have been working on the test task of JEP 255 Xerces Updates. Here I would invite you to review the changes for 2 bugs of this task: 1. JDK-8080906 Develop test for Xerces Update: DOM L3 Serializer To verify default LSSerializer is Xalan dom 3 serializer 2. JDK-8080908 Develop test for Xerces Update: XPointer To verify Xerces revision 415823: XERCESJ-1134. It should have been fixed in JDK-6794483, however, the test Bug6794483Test.java need to be revised to really cover this bug. I also added module dependencies for exported API in this path, it's recommended for Jigsaw change, refer to https://wiki.se.oracle.com/display/JPG/@modules+in+JTReg+tests The webrev is at http://cr.openjdk.java.net/~fyuan/8080906_8080908/webrev.00/, your comment will be appreciated. Best Regards Frank 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: RFR [9] 8081517: minor cleanup for docs
ok, as Lance mentioned, that can be deferred.
Roger
On 6/5/2015 10:59 AM, alexander stepanov wrote:
indentation is in the scope of your cleanup
No, I didn't check the indents, lengths of lines etc. (sorry). The
intention was just to fix some tidy warnings (like empty p tags) and
javadoc errors like invalid parameter names, invalid tags etc. Tags
code/code were replaced with {@code} in couple of files just
because of some connected warnings like unclosed tags etc.
Regards,
Alexander
On 05.06.2015 17:12, Lance Andersen wrote:
Hi Roger
On Jun 5, 2015, at 10:08 AM, Roger Riggs roger.ri...@oracle.com wrote:
2)
If indentation is in the scope of your cleanup…
Agree with you and I did not comment on this as I felt it this should
be done separately as there is a fair amount that can be done in
these classes for clean up.
Best
lance
In JdbcRowSetImpl.java, the indentation of continued lines after
@return or @param tags
isn't correct; the continued line should be indented.
And there are a some very long lines that should be wrapped;
See +5637... If the SQL AS clause was not...
+5784 the indentation needs to be fixed.
+5840 also
+5988
+6265 and after...
in many cases the lines that follow of /* do not align the * .
CachedRowSetWriter.java:
- many method comment blocks are not aligned properly with the
method of field that follows.
For example,
/**
* The codeConnection/code object that this writer will use to
make a
* connection to the data source to which it will write data.
*
*/
private transient Connection con;
3)
CachedRowSetWriter.java:
+1056: The tag @ param should be tighted up to @param
Thanks, Roger
On 6/5/2015 8:51 AM, alexander stepanov wrote:
Hello,
Could you please review the fix
http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-8081517
Just some cleanup for docs.
Thanks,
Alexander
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Another take on Finalization
Hi. I have had an interest in the topic of finalization ever since it caused me to abandon the G1 collector 3 or 4 years ago. I’ve recently implemented a fix for my interpretation of the problem, which might be very different from the discussion currently ongoing in the thread entitled JEP 132: More-prompt finalization”. My problem was that finalization was not being run at all with the G1 collector. Not at all. That would have been fine with me because none of the existing objects in the Finalizer queue actually needed the service anymore: the files, sockets, streams, etc. had all been properly closed by my application, otherwise the server would have long since failed completely. However, those objects started to accumulate in the VM and eventually (8 hours later) brought the server down. Which brings me to a few points: Finalization as conceived in the early JDKs was a bad idea. To make matters worse, the way we then made use of it in those early days was A REALLY REALLY bad idea. None of this mattered in those days because the GC ran often and quickly and finalization occurred during every GC cycle. There may be situations where finalization as a feature actually matters, but in the intervening years the JDK has added new technologies that provide a way to accomplish finalization on your own, in your own code. A few helper classes and it might even be easy when it’s necessary, which is hopefully almost never. Many of the uses of finalize() in the JDK today are bad and should be deleted. My fix, BTW, was to use a back door (that I added to SharedSecrets) in all the JDK classes that had a finalize() method, so that when a resource is properly closed, by calling the close() method for example, the back door would remove the Finalizer for the specified object from the linked list of Finalizer objects, thus removing it from the finalization equation altogether. I implemented this, and then the various tests of creating a huge number of objects with a finalize() method ran quickly and flawlessly with no horrific GCs or even a growing memory pool. The main problem with my solution was that there was this nasty SharedSecrets back door, so it has been rejected and probably rightly so. However, it proved a point. But now I am wondering why the actual right thing to do is not simply this: Remove the finalize() method from all the worst offenders in the JDK. I cannot remember all the places I patched when I implemented my fix, but the majority of them were pieces of code that absolutely had a close() method. If you don’t close objects when you’re done with them, your program PROBABLY SHOULD BE BROKEN. But even if you do not accept that, for all practical purposes, the program IS broken today because finalization is absolutely NOT run in a timely enough fashion. BTW - I never understood why CMS and other GC’s had absolutely no problem running finalization in a very timely fashion while the G1 collector just never seemed to get around to it. My interpretation of that fact has always led me to believe that it’s not a throughput issue with the finalization thread (not in real world examples, anyway) but rather a GC implementation that didn’t feel the need to be thorough enough to make sure something is ready to be finalized. I mean, when the G1 collector was forced to run a full collection (a death sentence on a 15Gb heap but it did occur) all the finalizable objects were found AND finalized immediately, all 15 or 20 million of them. So in summary: (1) The problem with finalization is that people use it. And more importantly, that the JDK is filled with inappropriate uses of it. (2) The main solution is probably just to delete the inappropriate uses in the JDK. But if that’s not OK, then some sort of patch like what I did which allows the JDK classes to unregister the Finalizer’s when they are no longer needed, i.e., when the object knows that it has cleaned itself up. I am curious to hear your thoughts. JP
Re: Another take on Finalization
Am Fri, 5 Jun 2015 22:11:08 +0100 schrieb Jonathan Payne core-l...@jpayne.net: My problem was that finalization was not being run at all with the G1 collector. The problem that an object is not detected as unreachable and not enqueued into the finalizer queue is not only a G1 problem. When an application has very short transactions and no objects get promoted then G1 and CMS (and PrallelOld) trigger very seldom an old GC, so this leads to a long time that finalizeable objects do not get cleaned up. Not sure if G1 is especially bad in that regards. I think it might be related to the bug that it never responded to the DGC cleanup unlike other GCs? Something similar to calling System.gc() every hour but with less negative effects would be a helpful tool for those situations. This is also a problem for other memory pools (class loaders, code cache, constant pool, direct buffers) as well. So having a slow background cleanup is a good thing in all cases. My fix, BTW, was to use a back door (that I added to SharedSecrets) in all the JDK classes that had a finalize() method, so that when a resource is properly closed, by calling the close() method for example, the back door would remove the Finalizer for the specified object from the linked list of Finalizer objects I think it would be good to offer such an API to early remove an object from the reference queue. Gruss Bernd
Re: RFR 8071597 Add Stream dropWhile and takeWhile operations
On 6/3/15 9:21 AM, Paul Sandoz wrote: I had prepared an alternative rendition stashed away just in case this came up :-) I still want to retain a punchy short first paragraph. What do you think about the following? /** - * Returns a stream consisting of the longest prefix of elements taken from - * this stream that match the given predicate. + * Returns a stream consisting of elements taken from this stream that match + * the given predicate. Well, I'm sorry to say, this reminds me of that old tech support joke about the answer that is technically true but completely useless. :-) It's not incorrect, of course, but it's almost indistinguishable from the first sentence of Stream.filter(). People who know what takeWhile means will raise an eyebrow at this, go read the rest of the doc, and go ohhh, yeah there's this complication with unordered streams. People who are trying to learn the system will go Huh? I agree on the need for a punchy first sentence since that's what goes into the Method Summary. The difficulty is making something sufficiently general that it covers all the cases, while not making it so vague as to be useless. Sorry, but I don't have any better suggestions at the moment. I might be able to come up with something in the next few days... but don't let this hold you up if you need to press forward. s'marks
Re: RFR [9] 8081517: minor cleanup for docs
indentation is in the scope of your cleanup
No, I didn't check the indents, lengths of lines etc. (sorry). The
intention was just to fix some tidy warnings (like empty p tags) and
javadoc errors like invalid parameter names, invalid tags etc. Tags
code/code were replaced with {@code} in couple of files just because
of some connected warnings like unclosed tags etc.
Regards,
Alexander
On 05.06.2015 17:12, Lance Andersen wrote:
Hi Roger
On Jun 5, 2015, at 10:08 AM, Roger Riggs roger.ri...@oracle.com wrote:
2)
If indentation is in the scope of your cleanup…
Agree with you and I did not comment on this as I felt it this should be done
separately as there is a fair amount that can be done in these classes for
clean up.
Best
lance
In JdbcRowSetImpl.java, the indentation of continued lines after @return or
@param tags
isn't correct; the continued line should be indented.
And there are a some very long lines that should be wrapped;
See +5637... If the SQL AS clause was not...
+5784 the indentation needs to be fixed.
+5840 also
+5988
+6265 and after...
in many cases the lines that follow of /* do not align the * .
CachedRowSetWriter.java:
- many method comment blocks are not aligned properly with the method of field
that follows.
For example,
/**
* The codeConnection/code object that this writer will use to make a
* connection to the data source to which it will write data.
*
*/
private transient Connection con;
3)
CachedRowSetWriter.java:
+1056: The tag @ param should be tighted up to @param
Thanks, Roger
On 6/5/2015 8:51 AM, alexander stepanov wrote:
Hello,
Could you please review the fix
http://cr.openjdk.java.net/~avstepan/8081517/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-8081517
Just some cleanup for docs.
Thanks,
Alexander
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
