March 2020 - core-libs-dev

Review Request: JDK-8240242: improve the javadoc for Lookup::dropLookupModes w.r.t. dropping UNCONDITIONAL
by Mandy Chung 10 Mar '20

10 Mar '20

Hi Chris, Below is the revised patch per your suggestion. I made some minor fixes in the method name shown in the "Access modes" section as well. Mandy diff --git a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java --- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java @@ -740,11 +740,11 @@ * The table below shows the access modes of a {@code Lookup} produced by * any of the following factory or transformation methods: * <ul> - * <li>{@link #lookup() MethodHandles.lookup()}</li> - * <li>{@link #publicLookup() MethodHandles.publicLookup()}</li> - * <li>{@link #privateLookupIn(Class, Lookup) MethodHandles.privateLookupIn}</li> - * <li>{@link Lookup#in}</li> - * <li>{@link Lookup#dropLookupMode(int)}</li> + * <li>{@link #lookup() MethodHandles::lookup}</li> + * <li>{@link #publicLookup() MethodHandles::publicLookup}</li> + * <li>{@link #privateLookupIn(Class, Lookup) MethodHandles::privateLookupIn}</li> + * <li>{@link Lookup#in Lookup::in}</li> + * <li>{@link Lookup#dropLookupMode(int) Lookup::dropLookupMode}</li> * </ul> * * <table class="striped"> @@ -1524,14 +1524,22 @@ * Creates a lookup on the same lookup class which this lookup object * finds members, but with a lookup mode that has lost the given lookup mode. * The lookup mode to drop is one of {@link #PUBLIC PUBLIC}, {@link #MODULE - * MODULE}, {@link #PACKAGE PACKAGE}, {@link #PROTECTED PROTECTED} or {@link #PRIVATE PRIVATE}. - * {@link #PROTECTED PROTECTED} is always - * dropped and so the resulting lookup mode will never have this access capability. - * When dropping {@code PACKAGE} then the resulting lookup will not have {@code PACKAGE} - * or {@code PRIVATE} access. When dropping {@code MODULE} then the resulting lookup will - * not have {@code MODULE}, {@code PACKAGE}, or {@code PRIVATE} access. If {@code PUBLIC} - * is dropped then the resulting lookup has no access. If {@code UNCONDITIONAL} - * is dropped then the resulting lookup has no access. + * MODULE}, {@link #PACKAGE PACKAGE}, {@link #PROTECTED PROTECTED}, + * {@link #PRIVATE PRIVATE}, or {@link #UNCONDITIONAL UNCONDITIONAL}. + * + * <p> If this lookup is a {@linkplain MethodHandles#publicLookup() public lookup}, + * this lookup has {@code UNCONDITIONAL} mode set and it has no other mode set. + * When dropping {@code UNCONDITIONAL} on a public lookup then the resulting + * lookup has no access. + * + * <p> If this lookup is not a public lookup, then the following applies + * regardless of its {@linkplain #lookupModes() lookup modes}. + * {@link #PROTECTED PROTECTED} is always dropped and so the resulting lookup + * mode will never have this access capability. When dropping {@code PACKAGE} + * then the resulting lookup will not have {@code PACKAGE} or {@code PRIVATE} + * access. When dropping {@code MODULE} then the resulting lookup will not + * have {@code MODULE}, {@code PACKAGE}, or {@code PRIVATE} access. + * When dropping {@code PUBLIC} then the resulting lookup has no access. * * @apiNote * A lookup with {@code PACKAGE} but not {@code PRIVATE} mode can safely

2 1

Re: JDK14 spec query : MethodHandles:dropLookupMode(int)
by Mandy Chung 10 Mar '20

10 Mar '20

I have bcc'ed jdk-dev and add core-libs-dev mailing list where this thread should be discussed. The spec says: "When dropping|PACKAGE|then the resulting lookup will not have|PACKAGE|or|PRIVATE|access.When dropping|MODULE|then the resulting lookup will not have|MODULE|,|PACKAGE|, or|PRIVATE|access. If|PUBLIC|is dropped then the resulting lookup has no access." @throws IAE if modeToDrop is a valid mode. I found the spec is quite clear that there is no condition to require the modeToDrop must exist. Mandy [1] https://bugs.openjdk.java.net/browse/JDK-8226916 On 3/9/20 3:06 AM, Andrew Leonard wrote: > Thank you Mandy for raising the bug, I have one more query please, we > may want to add to the bug that I am not sure about: > *testDropLookupMode*() in > test/jdk/java/lang/invoke/modules/m3/jdk/test/ModuleAccessTest.java: > > public void testDropLookupMode() throws Exception { > Lookup lookup = MethodHandles.privateLookupIn(m5.type1, m4.lookup); > assertTrue((lookup.lookupModes() & MODULE) == 0); <--- MODULE > doesn't exist > ... > Lookup lookup3 = lookup.dropLookupMode(MODULE); > ---> assertTrue(lookup3.lookupModes() == (lookup.lookupModes() & > ~(PROTECTED|PRIVATE|PACKAGE))); > > > Based on the expected result above, does it mean PRIVATE, PACKAGE > should be dropped whether > or not MODULE exists in the access mode (PROTECTED is dropped by > default) ? > If so, the document should be updated to explicitly clarify the > exception case? > > Thanks, > Andrew > > Andrew Leonard > Java Runtimes Development > IBM Hursley > IBM United Kingdom Ltd > internet email: andrew_m_leonard(a)uk.ibm.com > > Unless stated otherwise above: > IBM United Kingdom Limited - Registered in England and Wales with > number 741598. > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

2 3

[15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
by naoto.sato＠oracle.com 09 Mar '20

09 Mar '20

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8239836 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8239836/webrev.00/ It turned out that 'transitionList' is not necessarily a superset of 'standardOffsetTransitionList', as in some occasions where a standard offset change and a savings change happen at the same time (canceling each other), resulting in no wall time transition. This means that the "rules" in the sample code is a valid ZoneRules instance. However, there is an assumption in ZoneRules where no (wall time) transition means the fixed offset zone. With that assumption, the example rule is considered a fixed offset zone which is not correct. So the fix proposed here is not to throw an IAE but to recognize the rule as a valid, non-fixed offset ZoneRules instance. Naoto

4 15

RFR 8232161: Align some one-way conversion in MS950 charset with Windows
by Ichiroh Takiguchi 09 Mar '20

09 Mar '20

Hello. Could you review the fix ? Bug: https://bugs.openjdk.java.net/browse/JDK-8232161 Change: https://cr.openjdk.java.net/~itakiguchi/8232161/webrev.01/ CSR 8233385 [1] was approved. [1] https://bugs.openjdk.java.net/browse/JDK-8233385 Thanks, Ichiroh Takiguchi IBM Japan, Ltd.

2 5

RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format
by Chihiro Ito 09 Mar '20

09 Mar '20

Hi, I tried to correct this problem. Could you review this fix, please? According to the RFC 4122, UUID has a fixed format. I tried to raise an exception if a string was specified that is not suitable for this format. Also, is there anything else I should be aware of with this bug? Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/ JBS : https://bugs.openjdk.java.net/browse/JDK-8216407 Regards, Chihiro

5 8

argfiles parsing broken for argfiles > 4096 bytes
by Robert Stupp 09 Mar '20

09 Mar '20

Hi, I recently came across a bug in the java executable with an argfile that's larger than 4096 bytes, if a 4096-byte-chunk ends in a comment line. One bug happens when the last character of a comment line is the 4096th byte and the trailing newline is the first byte in the next chunk. In that case, nextToken() in src/java.base/share/native/libjli/args.c calls JLI_List_addSubstring (in the if-block at the end of nextToken()) with the contents of the current comment line. The next "valid" option gets appended to that comment line and java errors out. I've added some debugging output to the last if-block and pctx- >state==FIND_NEXT and the the substring being added is the comment line. I've also tried and changed this line http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/jav… to 'if (anchor != nextc && pctx->state == IN_TOKEN)' and the argfile is parsed correctly. Steps to reproduce:1. save the attached my-argfile2. run 'java @my- argfile my.className'3. java errors out with Error: Could not find or load main class # XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX-Dfoo=barCaused by: java.lang.ClassNotFoundException: # XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX-Dfoo=bar With the above change (the attached patch in argfile-parse-bug.txt), argfile parsing works as expected.Also updated the condition in the loop looking for a comment's newline, as it looks inconsistent to the condition in the loop for FIND_NEXT/SKIP_LEAD_WS and the condition of the outer for-loop. Robert

4 7

RFR: 8196334: Optimize UUID#fromString
by Claes Redestad 08 Mar '20

08 Mar '20

Hi all, please review this patch to optimize UUID#fromString. Jon Chambers originally proposed a patch that used a strict parser to get a similar speed-up, but I failed to adapt it in a way that could fall back to the less strict behavior while maintaining a reasonable speed-up in the fast-path case. Sorry, Jon! The patch proposed here was recently contributed by Andriy Plokhotnyuk (OCA signed), and manages to get more than a 3x speed-up on the new fromString microbenchmark, while falling back gently to the current, less strict implementation if ever needed. I've done some light edits, and added a simple microbenchmark. Bug: https://bugs.openjdk.java.net/browse/JDK-8196334 Webrev: http://cr.openjdk.java.net/~redestad/8196334/open.00/ Testing: tier1-3 Thanks! /Claes

4 11

RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder
by Roger Riggs 06 Mar '20

06 Mar '20

Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced. Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced. A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/ Issue: https://bugs.openjdk.java.net/browse/JDK-8239893 Thanks, Roger

3 6

Re: RFR(S): 8232081: Try to link all classes during dynamic CDS dump
by David Holmes 06 Mar '20

06 Mar '20

Hi Calvin, Adding core-libs-dev as you are messing with their code :) On 27/02/2020 1:19 pm, Calvin Cheung wrote: > JBS: https://bugs.openjdk.java.net/browse/JDK-8232081 > webrev: http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/ > > The proposed changeset for this RFE adds a JVM_LinkClassesForCDS() > function to be called from java/lang/Shutdown to notify the JVM to link > the classes loaded by the builtin class loaders. The This would be much less disruptive if this was handled purely on the VM side once we have started shutdown. No need to make any changes to the Java side then, nor jvm.cpp. Thanks, David > MetaspaceShared::link_and_cleanup_shared_classes() has been modified to > handle both static and dynamic CDS dump. For dynamic CDS dump, only > classes loaded by the builtin class loaders will be linked. Local > performance testing using javac on HelloWorld.java shows an improvement > of >5%. > > Passed tier1 - 4 tests. > > thanks, > > Calvin >

3 16

RFR(s): 8240189: [TESTBUG] Some cgroup tests are failing after JDK-8231111
by Severin Gehwolf 06 Mar '20

06 Mar '20

Hi, Could somebody please review those test fixes for the Metrics platform tests? Some code paths are apparently not being executed on "common" platforms which didn't make those issues show up earlier :-/ I've missed to update some hard-coded values to account for changes done via JDK-8231111. Matthias Baesken from SAP was kind enough to run this through their testing and no more issues show up after this. Much appreciated, Matthias! Bug: https://bugs.openjdk.java.net/browse/JDK-8240189 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8240189/01/webrev/ Testing: Docker tests on Linux x86_64 cgroups v1 and Linux cgroups v2 (no change). Additional testing done by SAP. All seems to look good. Thoughts? Thanks, Severin

3 5