August 2014 - core-libs-dev

RFR(S): 8055004: Reduce allocation overhead in java.time.Period/Duration parse methods
by Claes Redestad 13 Aug '14

13 Aug '14

Hi, can I have a review of this performance improvement to java.time.Period#parse and java.time.Duration#parse? bug: https://bugs.openjdk.java.net/browse/JDK-8055004 webrev: http://cr.openjdk.java.net/~redestad/8055004/webrev.0 The patch avoids String allocation through matcher.group(n) calls, utilizes recently added capability of parsing CharSequence offsets and improves on the rather awkward way fractions of a second was aligned to nanosecond precision in java.time.Duration. Microbenchmarks show a 1.2-1.5x throughput improvement in both cases. Thanks! /Claes

2 2

RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, final round
by Ioi Lam 13 Aug '14

13 Aug '14

Hi, Thank you all for the reviews! I have prepared a final version that has incorporated all the comments. http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-vfinal/ - Ioi On 8/9/14, 1:02 AM, Ioi Lam wrote: > Hi, > > Thanks a lot to everyone for the very useful comments. I have updated > the webrev > > Just the delta from the previous round of review: > > http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3_delta_from_v2/ > > All the changes: > > http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/ > > Thanks > - Ioi > > On 7/28/14, 4:09 PM, Ioi Lam wrote: >> Hi Folks, >> >> Please review the following clean up and refactoring of the CDS code, >> for JDK9 >> >> http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v2/ >> https://bugs.openjdk.java.net/browse/JDK-8046070 >> >> Summary of fix: >> >> Clean up and refactor the Class Data Sharing (CDS) code, including: >> >> + Improve archive integrity checking >> + Support bytecode verification during archive dumping time >> + Allow the user to configure the CDS class list and archive file. >> + Allow future extension of the CDS class loading mechanism. >> >> Tests: >> >> JPRT >> UTE (vm.runtime.testlist, vm.quick.testlist, >> vm.parallel_class_loading.testlist) >> Various adhoc SQE tests on Aurora >> JCK >> >> Thanks >> - Ioi >

2 1

RFR [8051382] Optimize java.lang.reflect.Modifier.toString()
by Ivan Gerasimov 13 Aug '14

13 Aug '14

Hello! StringBuilder can be replaced with StringJoiner, which will make the code shorter and, probably, more efficient. The existing regtest (java/lang/reflect/Modifier/toStringTest.java) can be used to make sure nothing is broken. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8051382 WEBREV: http://cr.openjdk.java.net/~igerasim/8051382/0/webrev/ Sincerely yours, Ivan

5 13

Does this look like a race?
by Andrew Haley 13 Aug '14

13 Aug '14

(Please forgive me if this should go to the AWT list or somesuch. It seems to me like this is not really a graphics-related issue, but one of Java concurrency.) This is in JDK9, sun.font.GlyphList. There is a non-volatile boolean inUse, and it is not always written in a synchronized block. It is used to allow exclusive access to a singleton instance. It seems to me that, at a minimum, inUse should be volatile, or e.g. strikelist might be overwritten to null after some other thread has started using this GlyphList. Do you agree? Thanks, Andrew. /* This scheme creates a singleton GlyphList which is checked out * for use. Callers who find its checked out create one that after use * is discarded. This means that in a MT-rendering environment, * there's no need to synchronise except for that one instance. * Fewer threads will then need to synchronise, perhaps helping * throughput on a MP system. If for some reason the reusable * GlyphList is checked out for a long time (or never returned?) then * we would end up always creating new ones. That situation should not * occur and if it did, it would just lead to some extra garbage being * created. private static GlyphList reusableGL = new GlyphList(); private static boolean inUse; ... public static GlyphList getInstance() { /* The following heuristic is that if the reusable instance is * in use, it probably still will be in a micro-second, so avoid * synchronising on the class and just allocate a new instance. * The cost is one extra boolean test for the normal case, and some * small number of cases where we allocate an extra object when * in fact the reusable one would be freed very soon. */ if (inUse) { return new GlyphList(); } else { synchronized(GlyphList.class) { if (inUse) { return new GlyphList(); } else { inUse = true; return reusableGL; } } } } ... /* There's a reference equality test overhead here, but it allows us * to avoid synchronizing for GL's that will just be GC'd. This * helps MP throughput. */ public void dispose() { if (this == reusableGL) { if (graybits != null && graybits.length > MAXGRAYLENGTH) { graybits = null; } usePositions = false; strikelist = null; // remove reference to the strike list inUse = false; } }

3 3

JDK RFR of 8054720: Modifications of I/O methods for instrumentation purposes
by Brian Burkhalter 12 Aug '14

12 Aug '14

JDK 9 reviewers: I would like to request the review of this patch to address the indicated issue: Issue: https://bugs.openjdk.java.net/browse/JDK-8054720 Patch: http://cr.openjdk.java.net/~bpb/8054720/ This changes passes the jdk_io and jdk_net tests on all platforms. Thanks, Brian

3 5

Re: RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #3
by David Holmes 12 Aug '14

12 Aug '14

Hi Ioi, We seem to have lost core-libs-dev so I added them back. A couple of minor follow ups. On 9/08/2014 6:02 PM, Ioi Lam wrote: > Hi, > > Thanks a lot to everyone for the very useful comments. I have updated > the webrev > > Just the delta from the previous round of review: > > http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3_delta_from_v2/ JDK changes: URLClassLoader.java: Doesn't this Note + * + * NOTE: the logic used here has been duplicated in the VM native code + * (search for invocation of definePackageInternal in the HotSpot sources). + * If this is changed, the VM code also need to be modified. belong on definePackageInternal, not defineClass ?? --- hotspot changes: hotspot/src/share/vm/classfile/classLoader.cpp The scoping of the ResourceMark doesn't look right: 592 // Iterate over class path entries 593 for (int start = 0; start < len; start = end) { 594 while (class_path[end] && class_path[end] != os::path_separator()[0]) { 595 end++; 596 } 597 EXCEPTION_MARK; 598 ResourceMark rm(THREAD); 599 char* path = NEW_RESOURCE_ARRAY(char, end-start+1); 600 strncpy(path, &class_path[start], end-start); 601 path[end-start] = '\0'; 602 update_class_path_entry_list(path, false); 603 #if INCLUDE_CDS 604 if (DumpSharedSpaces) { 605 check_shared_classpath(path); 606 } 607 #endif 608 while (class_path[end] == os::path_separator()[0]) { 609 end++; 610 } 611 } Doesn't the RESOURCE_ARRAY need to be freed within the ResourceMark block? --- src/share/vm/runtime/arguments.cpp 3340 // This causes problems with CDS, which requires that all directories specified in the classpath 3341 // must be empty. Should that be "must not be empty"? Or did you mean directory names? --- src/share/vm/runtime/javaCalls.cpp + // may cause undesriable side-effect in the class metadata. Typo: undesriable; also side-effects --- David ----- > All the changes: > > http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v3/ > > Thanks > - Ioi > > On 7/28/14, 4:09 PM, Ioi Lam wrote: >> Hi Folks, >> >> Please review the following clean up and refactoring of the CDS code, >> for JDK9 >> >> http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v2/ >> https://bugs.openjdk.java.net/browse/JDK-8046070 >> >> Summary of fix: >> >> Clean up and refactor the Class Data Sharing (CDS) code, including: >> >> + Improve archive integrity checking >> + Support bytecode verification during archive dumping time >> + Allow the user to configure the CDS class list and archive file. >> + Allow future extension of the CDS class loading mechanism. >> >> Tests: >> >> JPRT >> UTE (vm.runtime.testlist, vm.quick.testlist, >> vm.parallel_class_loading.testlist) >> Various adhoc SQE tests on Aurora >> JCK >> >> Thanks >> - Ioi >

4 9

RFR JDK-8054857: Fix typos in java.lang.** packages
by Pavel Rappo 11 Aug '14

11 Aug '14

Hi everyone, Could you please review my change for JDK-8054857? http://cr.openjdk.java.net/~prappo/8054857/webrev.00/ It's just a bunch of misspellings and typos in comments and javadoc. Thanks -Pavel

3 2

RFR: JDK-8038861 [javadoc] broken links in org/omg/CORBA/FloatSeqHelper.html
by Mark Sheppard 11 Aug '14

11 Aug '14

Hi please oblige and review the following change http://cr.openjdk.java.net/~msheppar/8038861/webrev/ which addresses the issue raised in https://bugs.openjdk.java.net/browse/JDK-8038861 HREFs in FloatSeqHelper.java java doc are incorrect, and fix changes cgi.omg.org to www.omg.org regards Mark

2 1

[8u40] RFR(M): 8050114, 8041972, 8006627
by Claes Redestad 11 Aug '14

11 Aug '14

Hi, I want to request backports of 8050114 [1], 8041972 [2] and 8006627[3] to 8u-dev. The patches apply cleanly when pushed in the sequence listed. [1] Expose Integer/Long formatUnsigned methods internally - jdk9 patch appliescleanly to jdk8u-dev https://bugs.openjdk.java.net/browse/JDK-8050114 http://cr.openjdk.java.net/~redestad/8050114/webrev.2 http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/7f056c2a6c54 [2] Add improved parse methods for Long/Integer - public API methods made package private and exposed via JavaLangAccess - rolled typos fixes found and reported under JDK-8054828 into this patch https://bugs.openjdk.java.net/browse/JDK-8041972 http://cr.openjdk.java.net/~redestad/8041972/backport.2 http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/127da56ff224 [3] UUID to/from String performance should be improved by reducing object allocations - non-public parseLong method now accessed via JavaLangAccess https://bugs.openjdk.java.net/browse/JDK-8006627 http://cr.openjdk.java.net/~redestad/8006627/backport.0 http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/3b298c230549 Thanks! /Claes

1 2

RFR [8054841] : (process) ProcessBuilder leaks native memory
by Ivan Gerasimov 11 Aug '14

11 Aug '14

Hello! There seems to be a small native memory leak in the ProcessBuilder's code. Would you please help review the trivial fix for that? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8054841 WEBREV: http://cr.openjdk.java.net/~igerasim/8054841/0/webrev/ Sincerely yours, Ivan

3 3