Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v3]
On Tue, 16 Feb 2021 21:15:58 GMT, Evan Whelan wrote: >> Hi, >> >> Please review this fix for JDK-8252883. This handles the case when an >> AccessDeniedException is being thrown on Windows, due to a delay in deleting >> the lock file it is trying to write to. >> >> This fix passes all testing. >> >> Kind regards, >> Evan > > Evan Whelan has updated the pull request incrementally with one additional > commit since the last revision: > > 8252883: Throw exception if exit code of child process is non-zero test/jdk/java/util/logging/FileHandlerAccessTest.java line 9: > 7: * published by the Free Software Foundation. Oracle designates this > 8: * particular file as subject to the "Classpath" exception as provided > 9: * by Oracle in the LICENSE file that accompanied this code. Tests do not need, and should not have, the CPE. - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
On Tue, 16 Feb 2021 22:27:12 GMT, Mandy Chung wrote: > Update test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java > to use ForceGC from test library which has additional instrumentation to > diagnose problem. In addition, this will get the fix for JDK-8258007. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2597
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Tue, 16 Feb 2021 23:18:55 GMT, Claes Redestad wrote:
>> Modify the `unmodifiable*` methods in `java.util.Collections` to be
>> idempotent. That is, when given an immutable collection from
>> `java.util.ImmutableCollections` or `java.util.Collections`, these methods
>> will return the reference instead of creating a new immutable collection
>> that wraps the existing one.
>
> src/java.base/share/classes/java/util/Collections.java line 1130:
>
>> 1128: public static Set unmodifiableSet(Set s) {
>> 1129: if(s.getClass() == UnmodifiableSet.class ||
>> 1130:s.getClass() == ImmutableCollections.Set12.class ||
>
> These might be problematic: `ImmutableCollections.Set*` differs in behavior
> subtly from `UnmodifiableSet`, e.g., `set.contains(null)` will throw an NPE.
> I'd limit the check and optimization to `UnmodifiableSet` here
No? This unmodifiable set here just delegates call to the backing field `c`, so
all exceptions from `c`'s calls are just delegated, aren't they? The NPE will
still be thrown; it's just that the stack trace will be different (i.e. one
more level of call in the stacktrace)
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Integrated: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods
On Tue, 9 Feb 2021 06:42:26 GMT, Joe Darcy wrote: > A follow-up of sorts to JDK-8257086, this change aims to improve the > discussion of the relationship between Object.equals and compareTo and > compare methods. The not-consistent-with-equals natural ordering of > BigDecimal get more explication too. While updating Object, I added some uses > of apiNote and implSpec, as appropriate. While a signum method did not exist > when Comparable was added, it has existed for many years so I removed > obsolete text that defined a "sgn" function to compute signum. > > Once the changes are agreed to, I'll reflow paragraphs and update the > copyright years. This pull request has now been integrated. Changeset: d547e1a8 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/d547e1a8 Stats: 151 lines in 4 files changed: 52 ins; 16 del; 83 mod 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods Reviewed-by: bpb, smarks, rriggs - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Tue, 16 Feb 2021 21:57:43 GMT, Ian Graves wrote:
> Modify the `unmodifiable*` methods in `java.util.Collections` to be
> idempotent. That is, when given an immutable collection from
> `java.util.ImmutableCollections` or `java.util.Collections`, these methods
> will return the reference instead of creating a new immutable collection that
> wraps the existing one.
src/java.base/share/classes/java/util/Collections.java line 1473:
> 1471: public static Map unmodifiableMap(Map extends V> m) {
> 1472: if(m.getClass() == UnmodifiableMap.class ||
> 1473:m.getClass() == ImmutableCollections.Map1.class ||
(I'm not a reviewer.)
I think this causes a change in behavior to this silly program.
var map1 = Map.of(1, 2);
var map2 = Collections.unmodifiableMap(map1);
try {
System.out.println("map1 returned " + map1.entrySet().contains(null));
} catch (NullPointerException e) {
System.out.println("map1 threw");
}
try {
System.out.println("map2 returned " + map2.entrySet().contains(null));
} catch (NullPointerException e) {
System.out.println("map2 threw");
}
With JDK 15 the output is:
> map1 threw
> map2 returned false
With this change I think the output will be:
> map1 threw
> map2 threw
It seems unlikely that anyone will be bit by this, but it is a change to
behavior and it wasn't called out in the Jira issue, so I felt it was worth
mentioning.
I think it is just this one specific case that changes -- only `Map1`, and only
`entrySet().contains(null)`. Other sub-collections like `keySet()` and
`values()` and `subList(...)` already throw on `contains(null)` for both the
`ImmutableCollections.*` implementation and the `Collections.umodifiable*`
wrapper. `MapN`'s `entrySet().contains(null)` already returns `false` for both.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v8]
> A follow-up of sorts to JDK-8257086, this change aims to improve the > discussion of the relationship between Object.equals and compareTo and > compare methods. The not-consistent-with-equals natural ordering of > BigDecimal get more explication too. While updating Object, I added some uses > of apiNote and implSpec, as appropriate. While a signum method did not exist > when Comparable was added, it has existed for many years so I removed > obsolete text that defined a "sgn" function to compute signum. > > Once the changes are agreed to, I'll reflow paragraphs and update the > copyright years. Joe Darcy has updated the pull request incrementally with two additional commits since the last revision: - Reflow paragraphs. - Update copyright year. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2471/files - new: https://git.openjdk.java.net/jdk/pull/2471/files/e27b6772..51575059 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2471=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2471=06-07 Stats: 92 lines in 4 files changed: 8 ins; 2 del; 82 mod Patch: https://git.openjdk.java.net/jdk/pull/2471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2471/head:pull/2471 PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v6]
On Fri, 12 Feb 2021 23:31:04 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo.
>
> src/java.base/share/classes/java/math/BigDecimal.java line 99:
>
>> 97: * hold. The results of methods like {@link scale} and {@link
>> 98: * unscaledValue} will differ for numerically equal values with
>> 99: * different representations.
>
> While this text is correct and reasonable, it doesn't really explain _why_
> equals() considers the representation. One might assume incorrectly that the
> representation is internal only and doesn't affect computations. Of course it
> does affect computations, which is why I suggested the example. Maybe the
> example doesn't belong right here, in which case it's reasonable for this
> change to proceed. I think it would be good to put an example _somewhere_ of
> non-substitutability of numerical values with different representations.
> Maybe we could handle that separately.
I've filed DK-8261862: "Expand discussion of rationale for BigDecimal
equals/compareTo semantics" as a follow-up bug.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v7]
> A follow-up of sorts to JDK-8257086, this change aims to improve the > discussion of the relationship between Object.equals and compareTo and > compare methods. The not-consistent-with-equals natural ordering of > BigDecimal get more explication too. While updating Object, I added some uses > of apiNote and implSpec, as appropriate. While a signum method did not exist > when Comparable was added, it has existed for many years so I removed > obsolete text that defined a "sgn" function to compute signum. > > Once the changes are agreed to, I'll reflow paragraphs and update the > copyright years. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Fix typo in link. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2471/files - new: https://git.openjdk.java.net/jdk/pull/2471/files/03f01e65..e27b6772 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2471=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2471=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2471/head:pull/2471 PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Fri, 12 Feb 2021 09:18:10 GMT, Philippe Marschall wrote: >> Implement three optimiztations for Reader.read(CharBuffer) >> >> * Add a code path for heap buffers in Reader#read to use the backing array >> instead of allocating a new one. >> * Change the code path for direct buffers in Reader#read to limit the >> intermediate allocation to `TRANSFER_BUFFER_SIZE`. >> * Implement `InputStreamReader#read(CharBuffer)` and delegate to >> `StreamDecoder`. >> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation. > > Philippe Marschall has updated the pull request incrementally with two > additional commits since the last revision: > > - Replace c-style array declarations > - Share work buffer between #skip and #read test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 34: > 32: import org.testng.annotations.Test; > 33: > 34: import java.io.*; It's generally better not to use a wild card. test/jdk/java/io/InputStreamReader/ReadCharBuffer.java line 73: > 71: } > 72: > 73: buffer.clear(); I think `buffer.rewind()` would be more in keeping with the specification verbiage although there will be no practical effect here. Same comment applies below and in the other test `ReadCharBuffer`. - PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer)
On Tue, 5 Jan 2021 00:48:54 GMT, Brian Burkhalter wrote: >> A couple of implementation notes: >> >> Reader#read(CharBuffer) >> >> on-heap case >> >> I introduced a dedicated path for the on-heap case and directly read into >> the backing array. This completely avoids the intermediate allocation and >> copy. Compared to the initial proposal the buffer position is updated. This >> has to be done because we bypass the buffer and directly read into the >> array. This also handles the case that #read returns -1. >> >> I am using a c-style array declaration because the rest of the class uses it. >> >> off-heap case >> >> I limit the intermadiate allocation to TRANSFER_BUFFER_SIZE. In order to >> reduce the total intermediate allocation we now call #read multiple times >> until the buffer is full or EOF is reached. This changes the behavior >> slightly as now possibly more data is read. However this should contribute >> to reduce the overall intermediate allocations. >> >> A lock is acquired to keep the the read atomic. This is consistent with >> #skip which also holds a lock over multiple #read calls. This is >> inconsistent with #transferTo which does not acquire a lock over multiple >> #read calls. >> >> The implementation took inspiration from java.io.InputStream#readNBytes(int). >> >> InputStreamReader#read(CharBuffer) >> >> Since StreamDecoder is a Reader as well we can simply delegate. >> >> StreamDecoder#read(CharBuffer) >> >> Interestingly this was not implemented even though StreamDecoder internally >> works on NIO buffers. >> >> on-heap case >> >> We see a performance improvement and the elimination of all intermediate >> allocation. >> >> StreamDecoder#read(char[], int, int) and InputStreamReader#read(char[], int, >> int) may get a small improvement as we no longer #slice the buffer. >> >> off-heap case >> >> We see the elimination of all intermediate allocation but a performance >> penalty because we hit the slow path in #decodeLoop. There is a trade-off >> here, we could improve the performance to previous levels by introducing >> intermediate allocation again. I don't know how much the users of off-heap >> buffers care about introducing intermediate allocation vs maximum throughput. >> >> I was struggling to come up with microbenchmarks because the built in >> InputStream and Reader implementations are finite but JMH calls the >> benchmark methods repeatably. I ended up implementing custom infinite >> InputStream and Reader implementations. The code can be found there: >> >> https://github.com/marschall/reader-benchmarks/tree/master/src/main/java/com/github/marschall/readerbenchmarks >> >> An Excel with charts can be found here: >> >> https://github.com/marschall/reader-benchmarks/raw/master/src/main/resources/4926314.xlsx >> >> I looked for java.io.Reader#read(CharBuffer) users in the JDK and only found >> java.util.Scanner#readInput(). I wrote a microbenchmark for this but only >> found minuscule improvements due to regex dominating the runtime. >> >> There seem to be no direct Reader tests in the tier1 suite, the initial code >> was wrong because I forgot to update the buffer code position but I only >> found out because some Scanner tests failed. > > I changed the JBS issue summary to match the title of this PR so that > integration blocker is removed. > > How does the off-heap performance of `InputStreamReader.read(CharBuffer)` > compare for the case where only the change to `Reader` is made versus if all > your proposed changes are applied? > > Some kind of specific test should likely be included. > > Note that the more recent copyright year is now 2021. I think the implementation changes here look good. I don't know however whether there is enough coverage in the tests. These should verify that the `Reader`, `CharArrayReader`, and `InputStreamReader` implementations of `read(CharBuffer)` are accurate. If there is already sufficient coverage in the tests in `test/jdk/java/io` then that is good enough and nothing need be added. - PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Tue, 16 Feb 2021 21:57:43 GMT, Ian Graves wrote:
> Modify the `unmodifiable*` methods in `java.util.Collections` to be
> idempotent. That is, when given an immutable collection from
> `java.util.ImmutableCollections` or `java.util.Collections`, these methods
> will return the reference instead of creating a new immutable collection that
> wraps the existing one.
Changes requested by redestad (Reviewer).
src/java.base/share/classes/java/util/Collections.java line 1130:
> 1128: public static Set unmodifiableSet(Set s) {
> 1129: if(s.getClass() == UnmodifiableSet.class ||
> 1130:s.getClass() == ImmutableCollections.Set12.class ||
These might be problematic: `ImmutableCollections.Set*` differs in behavior
subtly from `UnmodifiableSet`, e.g., `set.contains(null)` will throw an NPE.
I'd limit the check and optimization to `UnmodifiableSet` here
src/java.base/share/classes/java/util/Collections.java line 1315:
> 1313: public static List unmodifiableList(List list) {
> 1314: if (list.getClass() == UnmodifiableList.class ||
> list.getClass() == UnmodifiableRandomAccessList.class ||
> 1315: list.getClass() == ImmutableCollections.List12.class ||
Similar issue here
src/java.base/share/classes/java/util/Collections.java line 1473:
> 1471: public static Map unmodifiableMap(Map extends V> m) {
> 1472: if(m.getClass() == UnmodifiableMap.class ||
> 1473:m.getClass() == ImmutableCollections.Map1.class ||
And here.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
RFR: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
Update test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java to use ForceGC from test library which has additional instrumentation to diagnose problem. In addition, this will get the fix for JDK-8258007. - Commit messages: - fix copyright header update - update copyright header - 8261851: update test to use ForceGC from test library Changes: https://git.openjdk.java.net/jdk/pull/2597/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2597=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261851 Stats: 39 lines in 1 file changed: 1 ins; 37 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2597.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2597/head:pull/2597 PR: https://git.openjdk.java.net/jdk/pull/2597
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Tue, 16 Feb 2021 21:57:43 GMT, Ian Graves wrote: > Modify the `unmodifiable*` methods in `java.util.Collections` to be > idempotent. That is, when given an immutable collection from > `java.util.ImmutableCollections` or `java.util.Collections`, these methods > will return the reference instead of creating a new immutable collection that > wraps the existing one. Please add a space after `if` -> `if `. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v3]
On Tue, 16 Feb 2021 17:59:51 GMT, Naoto Sato wrote: >> Please review this doc fix to j.l.Character, which now includes the table of >> the history of supported Unicode versions. A corresponding CSR will be filed >> accordingly. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains two additional commits since > the last revision: > > - Removed empty tag > - 8261621: Delegate Unicode history from JLS to j.l.Character Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2538
RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
Modify the `unmodifiable*` methods in `java.util.Collections` to be idempotent. That is, when given an immutable collection from `java.util.ImmutableCollections` or `java.util.Collections`, these methods will return the reference instead of creating a new immutable collection that wraps the existing one. - Commit messages: - Changing Collections.unmodifiable* to idempotent behavior. Changes: https://git.openjdk.java.net/jdk/pull/2596/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2596=00 Issue: https://bugs.openjdk.java.net/browse/JDK-6323374 Stats: 281 lines in 2 files changed: 281 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2596.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596 PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v3]
> Hi, > > Please review this fix for JDK-8252883. This handles the case when an > AccessDeniedException is being thrown on Windows, due to a delay in deleting > the lock file it is trying to write to. > > This fix passes all testing. > > Kind regards, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8252883: Throw exception if exit code of child process is non-zero - Changes: - all: https://git.openjdk.java.net/jdk/pull/2572/files - new: https://git.openjdk.java.net/jdk/pull/2572/files/e755d323..9472f73c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2572=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2572=01-02 Stats: 9 lines in 1 file changed: 4 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572 PR: https://git.openjdk.java.net/jdk/pull/2572
Re: System.getEnv(String name, String def)
- Mail original -
> De: "Michael Kuhlmann"
> À: "core-libs-dev"
> Envoyé: Mardi 16 Février 2021 13:34:30
> Objet: Re: System.getEnv(String name, String def)
> Hi Rémi,
>
> I don't want to be pedantic, but I see this as an anti-pattern. You
> would create an Optional just to immediately call orElse() on it. That's
> not how Optional should be used. (But you know that.)
>
> It's described in Recipe 12 of this Java Magazine article, for instance:
> https://blogs.oracle.com/javamagazine/12-recipes-for-using-the-optional-class-as-its-meant-to-be-used
yep, you are right.
Optional.ofNullable(...).orElse(...) is not the best pattern in term of
readability.
>
> Best,
> Michael
regards,
Rémi
>
> On 2/15/21 3:09 PM, Remi Forax wrote:
>> Hi Loic,
>> You can use Optional.OfNullable() which is a kind of the general bridge
>> between
>> the nullable world and the non-nullable one.
>>
>>var fooOptional = Optional.ofNullable(System.getenv("FOO"));
>>var fooValue = fooOptional.orElse(defaultValue);
>>
>> regards,
>> Rémi Forax
>>
>> - Mail original -
>>> De: "Loïc MATHIEU"
>>> À: "core-libs-dev"
>>> Envoyé: Lundi 15 Février 2021 14:59:42
>>> Objet: System.getEnv(String name, String def)
>>
>>> Hello,
>>>
>>> I wonder if there has already been some discussion to provide
>>> a System.getEnv(String name, String def) method that allows to return a
>>> default value in case the env variable didn't exist.
>>>
>>> When using system properties instead of env variable, we do have a
>>> System.getProperty(String key, String def) variant.
>>>
>>> Stating the JavaDoc of System.getEnv():
>>> *System properties and environment variables are both conceptually mappings
>>> between names and values*
>>>
>>> So if system properties and environment variables are similar concepts,
>>> they should provide the same functionalities right ?
>>>
>>> This would be very convenient as more and more people rely on
>>> environment variables these days to configure their applications.
>>>
>>> Regards,
>>>
> >> Loïc
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v3]
> This patch exposes a couple of intrinsics used by String to speed up ASCII > checking and byte[] -> char[] inflation, which can be used by latin1 and > ASCII-compatible CharsetDecoders to speed up decoding operations. > > - Fast-path implemented for all standard charsets, with up to 10x performance > improvements in microbenchmarks reading Strings from ByteArrayInputStream. > - Cleanup of StreamDecoder/-Encoder with some minor improvements when > interpreting > - Reworked creation of JavaLangAccess to be safely published for > CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these > encoders are created during System.initPhase1 the current sequence caused the > initialization to became unstable and a few tests were consistently getting > an NPE. > > Testing: tier1-3 Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Copyrights - Review fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/2574/files - new: https://git.openjdk.java.net/jdk/pull/2574/files/b0bf8cfb..367be2a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2574=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2574=01-02 Stats: 15 lines in 10 files changed: 1 ins; 1 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/2574.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2574/head:pull/2574 PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]
On Tue, 16 Feb 2021 19:48:09 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert rem assertions > > src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 48: > >> 46: import jdk.internal.module.ServicesCatalog; >> 47: import jdk.internal.reflect.ConstantPool; >> 48: import jdk.internal.vm.annotation.IntrinsicCandidate; > > Not needed. Fixed > src/java.base/share/classes/java/lang/String.java line 1017: > >> 1015: * @return the number of bytes successfully decoded, at most len >> 1016: */ >> 1017: /* package-private */ > > Some more explanation would be helpful here, as it is accessed from System > for shared secrets. Ok, added a short comment - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]
On Mon, 15 Feb 2021 15:19:01 GMT, Claes Redestad wrote: >> This patch exposes a couple of intrinsics used by String to speed up ASCII >> checking and byte[] -> char[] inflation, which can be used by latin1 and >> ASCII-compatible CharsetDecoders to speed up decoding operations. >> >> - Fast-path implemented for all standard charsets, with up to 10x >> performance improvements in microbenchmarks reading Strings from >> ByteArrayInputStream. >> - Cleanup of StreamDecoder/-Encoder with some minor improvements when >> interpreting >> - Reworked creation of JavaLangAccess to be safely published for >> CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and >> these encoders are created during System.initPhase1 the current sequence >> caused the initialization to became unstable and a few tests were >> consistently getting an NPE. >> >> Testing: tier1-3 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert rem assertions Great improvement! Looks good to me overall. I wondered about the changes in JLA as Alan already mentioned. src/java.base/share/classes/java/lang/String.java line 1017: > 1015: * @return the number of bytes successfully decoded, at most len > 1016: */ > 1017: /* package-private */ Some more explanation would be helpful here, as it is accessed from System for shared secrets. src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 48: > 46: import jdk.internal.module.ServicesCatalog; > 47: import jdk.internal.reflect.ConstantPool; > 48: import jdk.internal.vm.annotation.IntrinsicCandidate; Not needed. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy [v12]
> 8080272 Refactor I/O stream copying to use > InputStream.transferTo/readAllBytes and Files.copy Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8080272: Refactor I/O stream copying to use java.io.InputStream.transferTo cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/1853/files - new: https://git.openjdk.java.net/jdk/pull/1853/files/6e71e961..1b30471d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1853=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1853=10-11 Stats: 7 lines in 2 files changed: 0 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1853.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1853/head:pull/1853 PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy [v9]
On Tue, 9 Feb 2021 11:40:09 GMT, Philippe Marschall
wrote:
>> Andrey Turbanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8080272: Refactor I/O stream copying to use java.io.InputStream.transferTo
>> fix review comments
>
> src/java.base/share/classes/java/util/jar/JarInputStream.java line 93:
>
>> 91: if (e != null &&
>> JarFile.MANIFEST_NAME.equalsIgnoreCase(e.getName())) {
>> 92: man = new Manifest();
>> 93: byte[] bytes = new BufferedInputStream(this).readAllBytes();
>
> I wonder if it makes sense to avoid reading the entire manifest into a byte[]
> when we don't need to verify the JAR. This may help avoiding some
> intermediate allocation and copying. This make be noticeable for some of the
> larger manifests (Java EE, OSGi, ...). A possible implementation may look
> like this
> https://github.com/marschall/jdk/commit/c50880ffb18607077c4da3456b27957d1df8edb7.
>
> In either case since we're calling #readAllBytes I don't see why we are
> wrapping in a BufferedInputStream rather than calling #readAllBytes directly.
Usage of `BufferedInputStream` removed
-
PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy [v9]
On Mon, 8 Feb 2021 21:18:44 GMT, Philippe Marschall wrote: >> Andrey Turbanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8080272: Refactor I/O stream copying to use java.io.InputStream.transferTo >> fix review comments > > src/java.base/share/classes/sun/security/provider/certpath/X509CertPath.java > line 230: > >> 228: // Copy the entire input stream into an InputStream >> that does >> 229: // support mark >> 230: is = new ByteArrayInputStream(is.readAllBytes()); > > I don't understand why the check for #markSupported is done there. The > InputStream constructor of PKCS7 creates a DataInputStream on the InputStream > only to then call #readFully. I can't find a place where a call to #mark > happens. Since the InputStream constructor reads all bytes anyway I wonder > whether we could remove this if and unconditionally do: > > PKCS7 pkcs7 = new PKCS7(is.readAllBytes()); Good idea. Will improve. By the way, code in `sun.security.pkcs.PKCS7#PKCS7(java.io.InputStream)` looks suspicious: it reads only `InputStream.available()` bytes, which doesn't make much sense to me. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v3]
> Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Removed empty tag - 8261621: Delegate Unicode history from JLS to j.l.Character - Changes: https://git.openjdk.java.net/jdk/pull/2538/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2538=02 Stats: 36 lines in 1 file changed: 34 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2538.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2538/head:pull/2538 PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: JDK-8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
On Sat, 13 Feb 2021 00:16:50 GMT, Joe Wang wrote: > Adds a property similar to 'isStandalone' in JDK-8249867. > > Please note that the test received an auto-format. The material changes were > the two tests marked with bug id 8260858 and related data and methods. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2559
Re: RFR: JDK-8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
On Tue, 16 Feb 2021 17:23:35 GMT, Joe Wang wrote: >> test/jaxp/javax/xml/jaxp/unittest/common/prettyprint/PrettyPrintTest.java >> line 107: >> >>> 105: + "\n" >>> 106: + ""; >>> 107: >> >> Could be better to use text blocks? > > In general, we're keeping it at source level 8 to facilitate backports. As a > result, only a few projects so far have been done using more advanced > features. Makes sense. Thanks for the explanation. - PR: https://git.openjdk.java.net/jdk/pull/2559
Re: RFR: JDK-8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
On Sun, 14 Feb 2021 21:27:49 GMT, Naoto Sato wrote: >> Adds a property similar to 'isStandalone' in JDK-8249867. >> >> Please note that the test received an auto-format. The material changes were >> the two tests marked with bug id 8260858 and related data and methods. > > test/jaxp/javax/xml/jaxp/unittest/common/prettyprint/PrettyPrintTest.java > line 107: > >> 105: + "\n" >> 106: + ""; >> 107: > > Could be better to use text blocks? In general, we're keeping it at source level 8 to facilitate backports. As a result, only a few projects so far have been done using more advanced features. - PR: https://git.openjdk.java.net/jdk/pull/2559
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy
On Sun, 20 Dec 2020 17:05:21 GMT, Andrey Turbanov wrote: > 8080272 Refactor I/O stream copying to use > InputStream.transferTo/readAllBytes and Files.copy Hi @turbanoff, I'm happy to sponsor but I see two comments by @marschall - have they been addressed? https://github.com/openjdk/jdk/pull/1853#discussion_r572815422 https://github.com/openjdk/jdk/pull/1853#discussion_r572380746 - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]
On Tue, 16 Feb 2021 11:48:05 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8252883: Doc cleanup, code formatting and throwing exceptions instead of >> catching > > test/jdk/java/util/logging/FileHandlerAccessTest.java line 96: > >> 94: System.out.println(name + "\t|" + line); >> 95: } >> 96: childProcess.waitFor(); > > Should you be checking the status returned by `waitFor` and fail the test if > it's not `0`? I'll modify the code to add this behaviour. Thanks for the suggestion :) - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]
On Tue, 16 Feb 2021 11:33:08 GMT, Daniel Fuchs wrote:
>> Evan Whelan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8252883: Doc cleanup, code formatting and throwing exceptions instead of
>> catching
>
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 75:
>
>> 73: handler.close();
>> 74: } catch (Exception e) {
>> 75: throw new RuntimeException(e);
>
> Will throwing an exception in a thread (if happening in the Thread created at
> line 60) cause jtreg to fail? Or will the exception simply be discarded?
The exception will in fact make the jtreg test fail. I verified this by
creating a dummy test case and throwing an exception within a thread as you
suggested.
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 98:
>
>> 96: childProcess.waitFor();
>> 97: } catch (Exception e) {
>> 98: throw new RuntimeException(e);
>
> Same holds there as well. Maybe you could make an experiment with a dummy
> test to see whether the exception will make the test fail.
Please see above comment. Verified that an exception thrown within a thread
causes test to fail
-
PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v6]
On Fri, 12 Feb 2021 22:52:06 GMT, Joe Darcy wrote: >> A follow-up of sorts to JDK-8257086, this change aims to improve the >> discussion of the relationship between Object.equals and compareTo and >> compare methods. The not-consistent-with-equals natural ordering of >> BigDecimal get more explication too. While updating Object, I added some >> uses of apiNote and implSpec, as appropriate. While a signum method did not >> exist when Comparable was added, it has existed for many years so I removed >> obsolete text that defined a "sgn" function to compute signum. >> >> Once the changes are agreed to, I'll reflow paragraphs and update the >> copyright years. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Tue, 16 Feb 2021 15:54:08 GMT, Rémi Forax wrote: >> The interface method is a default method, so not technically an override. > > It's not an override but @Override has a broader semantics than just > overriding an existing method. > Given that it helps to understand that this method is part of a larger > algorithm, i think this method should be tagged with @Override Okay - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Tue, 16 Feb 2021 14:03:56 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
>> 1548:
>>
>>> 1546: * @return a stream of (pseudo)randomly chosen {@code int}
>>> values
>>> 1547: */
>>> 1548:
>>
>> Is `@Override` intentionally omitted?
>
> The interface method is a default method, so not technically an override.
It's not an override but @Override has a broader semantics than just overriding
an existing method.
Given that it helps to understand that this method is part of a larger
algorithm, i think this method should be tagged with @Override
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
On Tue, 16 Feb 2021 15:05:33 GMT, Alan Bateman wrote: > The reformatting changes to StreamEncoder/StreamDecoder make it hard to see > if there are any actual changes. Is there anything we should look at? It's > okay to include this cleanup, I can't tell what happened with this source > file. > Apart from a few things being made final that weren't, and a flattening of a back-to-back Charset lookup in one of the constructors its mostly formatting cleanups. > I want to study the change System.setJavaLangAccess a bit further to > understand why this is needed (I know there is a awkward bootstrapping issue > here). Right, I'm not exactly sure why the more limited changes I attempted in 5f4e87f failed. In that change I simply changed the initialization order, which made the failing (closed) tests pass locally - but not in our CI. Since this is in initPhase1 there should be no concurrency possible. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
On Tue, 16 Feb 2021 15:05:33 GMT, Alan Bateman wrote: >>> > Is there a reason >>> > `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], >>> > int, int)` wasn't moved to `JavaLangAccess` as well? >>> >>> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders >>> seem very reasonable, which I was thinking of exploring next as a separate >>> RFE. >> >> Maybe I misunderstood. The intrinsified method you point out here pre-dates >> the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in >> StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617 >> >> It might be worthwhile cleaning this up. Not having to route via >> SharedSecrets -> JavaLangAccess does speed things up during >> startup/interpretation, at the cost of some code duplication. > > This looks a really good change and it's great can these decoders get the > benefit of the instrinics work. > > ISO_8869_1.decodeArrayLoop using JLA.inflate looks a bit strange and maybe we > can rename the method in the shared secret to inflatedCopy or just copy. or > just add a comment. The reason is that code outside of java.lang shouldn't > know anything about String's internals and inflation. > > The reformatting changes to StreamEncoder/StreamDecoder make it hard to see > if there are any actual changes. Is there anything we should look at? It's > okay to include this cleanup, I can't tell what happened with this source > file. > > I want to study the change System.setJavaLangAccess a bit further to > understand why this is needed (I know there is a awkward bootstrapping issue > here). > > I expect Naoto will want to review this PR too. I added the `Inflated copy from byte[] to char[], as defined by StringLatin1` comment to the `inflate` method in `JavaLangAccess`, but you're probably right and it would be good to make the name more explicit when exporting it outside of the package internal use. How about `inflateBytesToChars`? - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
On Tue, 16 Feb 2021 13:45:41 GMT, Claes Redestad wrote: >>> Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], >>> int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well? >> >> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem >> very reasonable, which I was thinking of exploring next as a separate RFE. > >> > Is there a reason >> > `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], >> > int, int)` wasn't moved to `JavaLangAccess` as well? >> >> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem >> very reasonable, which I was thinking of exploring next as a separate RFE. > > Maybe I misunderstood. The intrinsified method you point out here pre-dates > the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in > StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617 > > It might be worthwhile cleaning this up. Not having to route via > SharedSecrets -> JavaLangAccess does speed things up during > startup/interpretation, at the cost of some code duplication. This looks a really good change and it's great can these decoders get the benefit of the instrinics work. ISO_8869_1.decodeArrayLoop using JLA.inflate looks a bit strange and maybe we can rename the method in the shared secret to inflatedCopy or just copy. or just add a comment. The reason is that code outside of java.lang shouldn't know anything about String's internals and inflation. The reformatting changes to StreamEncoder/StreamDecoder make it hard to see if there are any actual changes. Is there anything we should look at? It's okay to include this cleanup, I can't tell what happened with this source file. I want to study the change System.setJavaLangAccess a bit further to understand why this is needed (I know there is a awkward bootstrapping issue here). I expect Naoto will want to review this PR too. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Thu, 4 Feb 2021 22:49:23 GMT, Gerard Ziemski wrote:
>> Anton Kozlov has updated the pull request incrementally with six additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>> - Add comments to WX transitions
>>
>>+ minor change of placements
>> - Use macro conditionals instead of empty functions
>> - Add W^X to tests
>> - Do not require known W^X state
>> - Revert w^x in gtests
>
> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 297:
>
>> 295: stub = SharedRuntime::handle_unsafe_access(thread, next_pc);
>> 296: }
>> 297: } else if (sig == SIGILL && nativeInstruction_at(pc)->is_stop()) {
>
> Can we add a comment here describing what this case means?
This was added as part of this commit ( to linux_aarch64) -
https://github.com/openjdk/jdk/commit/339d52600b285eb3bc57d9ff107567d4424efeb1
@gerard-ziemski do we really want to add anything new here ?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Sat, 13 Feb 2021 15:03:53 GMT, Andrey Turbanov
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added table of available algorithms.
>
> src/java.base/share/classes/java/util/Random.java line 29:
>
>> 27:
>> 28: import java.io.*;
>> 29: import java.math.BigInteger;
>
> This import is not actually used
good
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 115:
>
>> 113: static final String BAD_BOUND = "bound must be positive";
>> 114: static final String BAD_FLOATING_BOUND = "bound must be finite and
>> positive";
>> 115: static final String BAD_RANGE = "bound must be greater than origin";
>
> Unused fields in Random class now can be cleaned up:
> `java.util.Random#BadRange`, `java.util.Random#BadSize`
Good
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 149:
>
>> 147: */
>> 148: public static void checkJumpDistance(double distance) {
>> 149: if (!(distance > 0.0 && distance < Float.POSITIVE_INFINITY
>
> I wounder if this usage of `Float.POSITIVE_INFINITY` intentional here? Looks
> a bit suspicious for comparison with `double` argument
Turns out the method is no longer used.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 1548:
>
>> 1546: * @return a stream of (pseudo)randomly chosen {@code int}
>> values
>> 1547: */
>> 1548:
>
> Is `@Override` intentionally omitted?
The interface method is a default method, so not technically an override.
> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
> 1943:
>
>> 1941:
>> 1942: // IllegalArgumentException messages
>> 1943: static final String BadLogDistance = "logDistance must be
>> non-negative";
>
> seems unused
Good.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
On Mon, 15 Feb 2021 20:34:53 GMT, Claes Redestad wrote: > > Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], > > int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well? > > Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem > very reasonable, which I was thinking of exploring next as a separate RFE. Maybe I misunderstood. The intrinsified method you point out here pre-dates the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617 It might be worthwhile cleaning this up. Not having to route via SharedSecrets -> JavaLangAccess does speed things up during startup/interpretation, at the cost of some code duplication. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702 [v3]
> After the fix for JDK-8253702, the test java/lang/System/OsVersionTest.java > still fails on BigSur versions that have a patch version (> 1), e.g. on macOS > Big Sur 11.2.1, and where the JDK was built with xcode < 12. > > java.lang.Error: 11.2 != 11.2.1 > > This is a proposal to relax the test and throw a SkippedException in such > cases. Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/2576/files - new: https://git.openjdk.java.net/jdk/pull/2576/files/f06114c9..9fe46d7b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2576=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2576=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2576.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2576/head:pull/2576 PR: https://git.openjdk.java.net/jdk/pull/2576
Integrated: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702
On Mon, 15 Feb 2021 13:27:36 GMT, Christoph Langer wrote: > After the fix for JDK-8253702, the test java/lang/System/OsVersionTest.java > still fails on BigSur versions that have a patch version (> 1), e.g. on macOS > Big Sur 11.2.1, and where the JDK was built with xcode < 12. > > java.lang.Error: 11.2 != 11.2.1 > > This is a proposal to relax the test and throw a SkippedException in such > cases. This pull request has now been integrated. Changeset: 8ba390d1 Author:Christoph Langer URL: https://git.openjdk.java.net/jdk/commit/8ba390d1 Stats: 10 lines in 1 file changed: 7 ins; 0 del; 3 mod 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702 Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/2576
Re: System.getEnv(String name, String def)
Hi Rémi,
I don't want to be pedantic, but I see this as an anti-pattern. You
would create an Optional just to immediately call orElse() on it. That's
not how Optional should be used. (But you know that.)
It's described in Recipe 12 of this Java Magazine article, for instance:
https://blogs.oracle.com/javamagazine/12-recipes-for-using-the-optional-class-as-its-meant-to-be-used
Best,
Michael
On 2/15/21 3:09 PM, Remi Forax wrote:
Hi Loic,
You can use Optional.OfNullable() which is a kind of the general bridge between
the nullable world and the non-nullable one.
var fooOptional = Optional.ofNullable(System.getenv("FOO"));
var fooValue = fooOptional.orElse(defaultValue);
regards,
Rémi Forax
- Mail original -
De: "Loïc MATHIEU"
À: "core-libs-dev"
Envoyé: Lundi 15 Février 2021 14:59:42
Objet: System.getEnv(String name, String def)
Hello,
I wonder if there has already been some discussion to provide
a System.getEnv(String name, String def) method that allows to return a
default value in case the env variable didn't exist.
When using system properties instead of env variable, we do have a
System.getProperty(String key, String def) variant.
Stating the JavaDoc of System.getEnv():
*System properties and environment variables are both conceptually mappings
between names and values*
So if system properties and environment variables are similar concepts,
they should provide the same functionalities right ?
This would be very convenient as more and more people rely on
environment variables these days to configure their applications.
Regards,
Loïc
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]
On Tue, 16 Feb 2021 11:37:05 GMT, Evan Whelan wrote:
>> Hi,
>>
>> Please review this fix for JDK-8252883. This handles the case when an
>> AccessDeniedException is being thrown on Windows, due to a delay in deleting
>> the lock file it is trying to write to.
>>
>> This fix passes all testing.
>>
>> Kind regards,
>> Evan
>
> Evan Whelan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8252883: Doc cleanup, code formatting and throwing exceptions instead of
> catching
Thanks for taking on the changes. Still some comments...
test/jdk/java/util/logging/FileHandlerAccessTest.java line 75:
> 73: handler.close();
> 74: } catch (Exception e) {
> 75: throw new RuntimeException(e);
Will throwing an exception in a thread (if happening in the Thread created at
line 60) cause jtreg to fail? Or will the exception simply be discarded?
test/jdk/java/util/logging/FileHandlerAccessTest.java line 98:
> 96: childProcess.waitFor();
> 97: } catch (Exception e) {
> 98: throw new RuntimeException(e);
Same holds there as well. Maybe you could make an experiment with a dummy test
to see whether the exception will make the test fail.
test/jdk/java/util/logging/FileHandlerAccessTest.java line 96:
> 94: System.out.println(name + "\t|" + line);
> 95: }
> 96: childProcess.waitFor();
Should you be checking the status returned by `waitFor` and fail the test if
it's not `0`?
-
PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]
On Tue, 16 Feb 2021 11:33:08 GMT, Evan Whelan wrote: >> Hi, >> >> Please review this fix for JDK-8252883. This handles the case when an >> AccessDeniedException is being thrown on Windows, due to a delay in deleting >> the lock file it is trying to write to. >> >> This fix passes all testing. >> >> Kind regards, >> Evan > > Evan Whelan has updated the pull request incrementally with one additional > commit since the last revision: > > 8252883: Doc cleanup, code formatting and throwing exceptions instead of > catching Daniel's suggestions have been integrated to the patch - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]
> Hi, > > Please review this fix for JDK-8252883. This handles the case when an > AccessDeniedException is being thrown on Windows, due to a delay in deleting > the lock file it is trying to write to. > > This fix passes all testing. > > Kind regards, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8252883: Doc cleanup, code formatting and throwing exceptions instead of catching - Changes: - all: https://git.openjdk.java.net/jdk/pull/2572/files - new: https://git.openjdk.java.net/jdk/pull/2572/files/045d725f..e755d323 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2572=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2572=00-01 Stats: 36 lines in 2 files changed: 13 ins; 5 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/2572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572 PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v2]
On Mon, 15 Feb 2021 12:55:46 GMT, Daniel Fuchs wrote:
>> Evan Whelan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8252883: Doc cleanup, code formatting and throwing exceptions instead of
>> catching
>
> src/java.logging/share/classes/java/util/logging/FileHandler.java line 39:
>
>> 37: import java.nio.channels.FileChannel;
>> 38: import java.nio.channels.OverlappingFileLockException;
>> 39: import java.nio.file.*;
>
> We avoid using the wildcard with imports in the JDK sources. Could you revert
> that change?
Yes, reverted thanks
> src/java.logging/share/classes/java/util/logging/FileHandler.java line 512:
>
>> 510: // Try again. If it doesn't work, then this will
>> 511: // eventually ensure that we increment "unique"
>> and
>> 512: // use another file name.
>
> I believe this would be more clear if the comment was put inside the if
> statement, just before continue.
> It might be good to add some words about what might be causing the
> AccessDeniedException as well...
> Maybe something like:
>
> } catch (AccessDeniedException ade) {
> // This can be either a temporary, or a more
> permanent issue.
> // The lock file might be still pending deletion from
> a previous run
> // (temporary), or the parent directory might not be
> accessible, etc...
> // If we can write to the current directory, and this
> is a regular file,
> // let's try again.
> if (Files.isRegularFile(lockFilePath,
> LinkOption.NOFOLLOW_LINKS)
> && isParentWritable(lockFilePath)) {
> // Try again. If it doesn't work, then this will
> // eventually ensure that we increment "unique"
> and
> // use another file name.
> continue;
> } else {
> throw ade; // no need to retry
> } ...
I agree. I've updated the comments
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 47:
>
>> 45: if (!(args.length == 2 || args.length == 1)) {
>> 46: System.out.println("Usage error: expects java
>> FileHandlerAccessTest [process/thread] ");
>> 47: System.exit(1);
>
> We usually avoid `System.exit()` in tests. `return` should be enough.
Changed to `return`
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 75:
>
>> 73: }
>> 74: catch(Exception e) {
>> 75: e.printStackTrace();
>
> If you only print exceptions, when does the test fail?
Thanks for catching this.
I've updated the test to throw the exceptions rather than printing stack trace.
This was a modified version of the test for debugging purposes.
It's worth noting that it's not possible to write a test which can
deterministically prove/ verify the behaviour (and related fix) of the parent
bug as this involves the situation where multiple threads conflict on a Windows
machine.
The test attached is a best-effort and was tried around 40 times.
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 98:
>
>> 96: childProcess.waitFor();
>> 97: }
>> 98: catch(Exception e) {
>
> space missing after `catch`
Fixed
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 99:
>
>> 97: }
>> 98: catch(Exception e) {
>> 99: e.printStackTrace();
>
> Same remark here: should this make the test fail?
Thanks Daniel, please see my above reply
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 101:
>
>> 99: e.printStackTrace();
>> 100: }
>> 101: finally {
>
> I would prefer if `catch` and `finally` where on the same line than the
> preceding closing brace:
>
> try {
> ...
> } catch (...) {
> ...
> } finally {
> ...
> }
Fixed
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 106:
>
>> 104: }
>> 105: if (bufferedReader != null){
>> 106: try{
>
> space missing after `try`
Fixed
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 109:
>
>> 107: bufferedReader.close();
>> 108: }
>> 109: catch(Exception ignored){}
>
> space missing after `catch`
Fixed
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 113:
>
>> 111: }
>> 112: }
>> 113: }
>
> Please add a newline at the end of the file.
Done
-
PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
On Fri, 12 Feb 2021 11:02:36 GMT, Christoph Langer wrote: >> JDK-8261422: Adjust problematic String.format calls in >> jdk/internal/util/Preconditions.java outOfBoundsMessage > > As we're potentially formatting any "Number" type here, theoretically floats > could be passed which would result in an Exception. In fact, only decimal > types are passed by the callers so this should not happen. Unfortunately > there's no super type specifying decimal numbers. > > And as we're not doing some fancy formatting but just %d, I think replacing > this with %s should be ok. I see this change been integrated but there is further work required on the test coverage. Are you planing to do code coverage and create a follow-on issue to add more tests? - PR: https://git.openjdk.java.net/jdk/pull/2483
Integrated: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken wrote: > JDK-8261422: Adjust problematic String.format calls in > jdk/internal/util/Preconditions.java outOfBoundsMessage This pull request has now been integrated. Changeset: 219b115e Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/219b115e Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod 8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage Reviewed-by: clanger - PR: https://git.openjdk.java.net/jdk/pull/2483
