RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods
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. - Commit messages: - Merge branch 'master' into 8261123 - Initial changes for JDK-8261123. Changes: https://git.openjdk.java.net/jdk/pull/2471/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2471&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261123 Stats: 80 lines in 4 files changed: 38 ins; 14 del; 28 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: 8223188: Consider rewriting in C++ or moving to ".c" files
On Mon, 8 Feb 2021 22:26:22 GMT, Alexey Semenyuk wrote: > Remove needless `#ifdef __cplusplus` from .cpp sources The Bug Synopsis of 'Consider rewriting in C++ or moving to ".c" files' is unclear. I would request that the Bug Synopsis to be changed to "removed unnecessary #ifdef __cplusplus" before integrating this PR. - Changes requested by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2466
Re: RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
On Mon, 8 Feb 2021 21:42:36 GMT, Naoto Sato wrote: > Please review this simple test case fix. By sampling locales to test, it > reduces the possibility of the time out. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2465
Re: RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
On Tue, 9 Feb 2021 00:04:44 GMT, Naoto Sato wrote: >> The fix looks ok to me. Just an interesting note that this test took such a >> long time to run. Given the amount of locales, limit(30) would reduce the >> time to approximately 1/5 of the time, that would still be about 100,000ms >> according to the bug report. That feels pretty long for a single test case >> that, outside of the test environment, takes only 1s (my local test took 5s >> without limit(30)). >> >> Also, there are a bunch of other tests that also run through all of the >> available locales, I wonder if they also take a long time to run. > > As of JDK15, there are more than 800 locales, so reducing the samples down to > 30 makes it less than 4% of the original (~20,000ms on that machine). But I > agree that the test took too long than my environment too. Could be a very > slow machine. > As to other tests calling available locales, the reason this test took a > long time is that the loop multiplies with the number of time zones which is > also huge (600+), so it would be less likely that other locale tests cause > timeouts. I see. I didn't know locales had gone from 160 to 800! All the more reason to set a limit. Rerun with jdk 16, limit(30) does make it a breeze comparing to that without (which made my machine start to spin). 20,000ms is reasonable on a slower machine with a debug build. - PR: https://git.openjdk.java.net/jdk/pull/2465
Re: RFR: 8261306: ServiceLoader documentation has malformed Unicode escape
On Tue, 9 Feb 2021 00:11:52 GMT, Brian Burkhalter wrote: > Please review this really small correction to the class level documentation > of `java.util.ServiceLoader`. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2468
Re: RFR: 8223188: Consider rewriting in C++ or moving to ".c" files
On Mon, 8 Feb 2021 22:26:22 GMT, Alexey Semenyuk wrote: > Remove needless `#ifdef __cplusplus` from .cpp sources Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2466
Re: RFR: 8261306: ServiceLoader documentation has malformed Unicode escape
On Tue, 9 Feb 2021 00:11:52 GMT, Brian Burkhalter wrote: > Please review this really small correction to the class level documentation > of `java.util.ServiceLoader`. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2468
Re: RFR: 8261306: ServiceLoader documentation has malformed Unicode escape
On Tue, 9 Feb 2021 00:11:52 GMT, Brian Burkhalter wrote: > Please review this really small correction to the class level documentation > of `java.util.ServiceLoader`. I think this is fine. You could also probably use &num given this is HTML I believe - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2468
Re: RFR: 8261298: LinuxPackage.c, getJvmLauncherLibPath RPM->DEB typo
On Mon, 8 Feb 2021 08:49:17 GMT, Aleksey Shipilev wrote:
> SonarCloud instance reports as new warning after JDK-8254702:
>
> This branch can not be reached because the condition duplicates a previous
> condition in the same sequence of "if/else if" statements.
>
> char* getJvmLauncherLibPath(void) {
>...
> if (PACKAGE_TYPE_RPM == pkg->type) {
> pkgQueryCmd = "rpm -ql '%s' 2>/dev/null";
> } else if (PACKAGE_TYPE_RPM == pkg->type) { <--- here
> pkgQueryCmd = "dpkg -L '%s' 2>/dev/null";
>
> Seems like an obvious typo.
>
> Additional tests:
> - [x] Linux x86_64 (Ubuntu) `tools/jpackage`
Marked as reviewed by almatvee (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/2452
RFR: 8261306: ServiceLoader documentation has malformed Unicode escape
Please review this really small correction to the class level documentation of `java.util.ServiceLoader`. - Commit messages: - 8261306: ServiceLoader documentation has malformed Unicode escape Changes: https://git.openjdk.java.net/jdk/pull/2468/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2468&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261306 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2468.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2468/head:pull/2468 PR: https://git.openjdk.java.net/jdk/pull/2468
Re: RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks [v2]
On Mon, 8 Feb 2021 15:25:02 GMT, Aleksey Shipilev wrote:
>> After JDK-8254702, SonarCloud instance complains about blocks like these:
>> "Change this loop body so that it can be executed more than once."
>>
>> int initJvmlLauncherData(JvmlLauncherData* ptr) const {
>> // Store path to JLI library just behind JvmlLauncherData header.
>> char* curPtr = reinterpret_cast(ptr + 1);
>> do {
>> const size_t count = sizeof(char)
>> * (jliLibPath.size() + 1 /* trailing zero */);
>> if (ptr) {
>> std::memcpy(curPtr, jliLibPath.c_str(), count);
>> ptr->jliLibPath = curPtr;
>> }
>> curPtr += count;
>> } while (false); // < here
>>
>> There is no sense in having `while(false)` here, where the syntactic `{}`
>> block would do. There are also other uses in the jpackage code that employ
>> `while(0)` for this, and then they also trigger the inspection about the
>> implicit conversion of zero int to boolean.
>>
>> Additional testing:
>> - [x] Linux x86_64 (Ubuntu) tools/jpackage
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> More cleanup in tstrings.cpp
Marked as reviewed by almatvee (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
On Mon, 8 Feb 2021 23:42:04 GMT, Joe Wang wrote: >> Marked as reviewed by lancea (Reviewer). > > The fix looks ok to me. Just an interesting note that this test took such a > long time to run. Given the amount of locales, limit(30) would reduce the > time to approximately 1/5 of the time, that would still be about 100,000ms > according to the bug report. That feels pretty long for a single test case > that, outside of the test environment, takes only 1s (my local test took 5s > without limit(30)). > > Also, there are a bunch of other tests that also run through all of the > available locales, I wonder if they also take a long time to run. As of JDK15, there are more than 800 locales, so reducing the samples down to 30 makes it less than 4% of the original (~20,000ms on that machine). But I agree that the test took too long than my environment too. Could be a very slow machine. As to other tests calling available locales, the reason this test took a long time is that the loop multiplies with the number of time zones which is also huge (600+), so it would be less likely that other locale tests cause timeouts. - PR: https://git.openjdk.java.net/jdk/pull/2465
Re: RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
On Mon, 8 Feb 2021 21:52:45 GMT, Lance Andersen wrote: >> Please review this simple test case fix. By sampling locales to test, it >> reduces the possibility of the time out. > > Marked as reviewed by lancea (Reviewer). The fix looks ok to me. Just an interesting note that this test took such a long time to run. Given the amount of locales, limit(30) would reduce the time to approximately 1/5 of the time, that would still be about 100,000ms according to the bug report. That feels pretty long for a single test case that, outside of the test environment, takes only 1s (my local test took 5s without limit(30)). Also, there are a bunch of other tests that also run through all of the available locales, I wonder if they also take a long time to run. - PR: https://git.openjdk.java.net/jdk/pull/2465
Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream
On Mon, 8 Feb 2021 22:58:13 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/FilterReader.java line 81:
>>
>>> 79: * {@inheritDoc}
>>> 80: *
>>> 81: * @throws IllegalArgumentException If {@code n} is negative
>>> and the
>>
>> Does this have to be different from the `Reader.skip()`'s description? Since
>> the contained reader implements `Reader` (throws IAE as a contract), that
>> condition after `and` is always true?
>
> This came from some `Reader`s, e.g., `CharArrayReader`, `StringReader`,
> overriding `skip()` _not_ to throw an IAE. But at the specification level
> perhaps this should not be recognized.
Makes sense. Then I would let CSR decide whether to include the description or
not.
-
PR: https://git.openjdk.java.net/jdk/pull/2274
Re: RFR: 8223188: Consider rewriting in C++ or moving to ".c" files
On Mon, 8 Feb 2021 22:26:22 GMT, Alexey Semenyuk wrote: > Remove needless `#ifdef __cplusplus` from .cpp sources Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2466
Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream
On Mon, 8 Feb 2021 22:51:43 GMT, Naoto Sato wrote:
>> Please review this clarification of the specification of the method
>> `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the
>> behavior of the method is made clear for the case when the `Reader` is
>> already at the end of its stream when the method is invoked. A corresponding
>> CSR will be filed. Also, the change includes an update to an existing test
>> in order to verify that the specification change reflects actual behavior.
>
> src/java.base/share/classes/java/io/FilterReader.java line 81:
>
>> 79: * {@inheritDoc}
>> 80: *
>> 81: * @throws IllegalArgumentException If {@code n} is negative
>> and the
>
> Does this have to be different from the `Reader.skip()`'s description? Since
> the contained reader implements `Reader` (throws IAE as a contract), that
> condition after `and` is always true?
This came from some `Reader`s, e.g., `CharArrayReader`, `StringReader`,
overriding `skip()` _not_ to throw an IAE. But at the specification level
perhaps this should not be recognized.
-
PR: https://git.openjdk.java.net/jdk/pull/2274
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v7]
> Could I get the following change reviewed please? It fixes a problem (in > JEP380) on Windows where some file operations on Unix domain sockets were not > working and led to the feature being disabled on Windows 2019 Server in JDK > 16. So, the fix re-enables the feature on all versions of Windows that > support it. > > The test checks all the file APIs that were affected by the problem. The > change touches the code that handles symbolic links in Windows (since they > are implemented as NTFS reparse points, like Unix sockets), but I didn't add > any specific testing in this area, as I assume the existing unit tests for > NIO symbolic links should cover that. If I should add more tests here, then I > can do that. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: update - Changes: - all: https://git.openjdk.java.net/jdk/pull/2424/files - new: https://git.openjdk.java.net/jdk/pull/2424/files/c1fd3565..92031fbc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=05-06 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2424.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424 PR: https://git.openjdk.java.net/jdk/pull/2424
Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream
On Thu, 28 Jan 2021 02:22:55 GMT, Brian Burkhalter wrote:
> Please review this clarification of the specification of the method
> `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the
> behavior of the method is made clear for the case when the `Reader` is
> already at the end of its stream when the method is invoked. A corresponding
> CSR will be filed. Also, the change includes an update to an existing test in
> order to verify that the specification change reflects actual behavior.
src/java.base/share/classes/java/io/FilterReader.java line 81:
> 79: * {@inheritDoc}
> 80: *
> 81: * @throws IllegalArgumentException If {@code n} is negative and
> the
Does this have to be different from the `Reader.skip()`'s description? Since
the contained reader implements `Reader` (throws IAE as a contract), that
condition after `and` is always true?
-
PR: https://git.openjdk.java.net/jdk/pull/2274
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v6]
> Could I get the following change reviewed please? It fixes a problem (in > JEP380) on Windows where some file operations on Unix domain sockets were not > working and led to the feature being disabled on Windows 2019 Server in JDK > 16. So, the fix re-enables the feature on all versions of Windows that > support it. > > The test checks all the file APIs that were affected by the problem. The > change touches the code that handles symbolic links in Windows (since they > are implemented as NTFS reparse points, like Unix sockets), but I didn't add > any specific testing in this area, as I assume the existing unit tests for > NIO symbolic links should cover that. If I should add more tests here, then I > can do that. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: fix mistake in last push - Changes: - all: https://git.openjdk.java.net/jdk/pull/2424/files - new: https://git.openjdk.java.net/jdk/pull/2424/files/f644e939..c1fd3565 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=04-05 Stats: 5 lines in 1 file changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2424.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424 PR: https://git.openjdk.java.net/jdk/pull/2424
RFR: 8223188: Consider rewriting in C++ or moving to ".c" files
Remove needless `#ifdef __cplusplus` from .cpp sources - Commit messages: - 8223188: Consider rewriting in C++ or moving to ".c" files Changes: https://git.openjdk.java.net/jdk/pull/2466/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2466&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8223188 Stats: 10 lines in 2 files changed: 0 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2466.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2466/head:pull/2466 PR: https://git.openjdk.java.net/jdk/pull/2466
Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream
On Thu, 28 Jan 2021 02:22:55 GMT, Brian Burkhalter wrote: > Please review this clarification of the specification of the method > `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the > behavior of the method is made clear for the case when the `Reader` is > already at the end of its stream when the method is invoked. A corresponding > CSR will be filed. Also, the change includes an update to an existing test in > order to verify that the specification change reflects actual behavior. ping - PR: https://git.openjdk.java.net/jdk/pull/2274
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]
On Mon, 8 Feb 2021 21:59:53 GMT, Michael McMahon wrote:
>> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 862:
>>
>>> 860: * and a handle to the socket file if it is.
>>> 861: */
>>> 862: private long openSocketForReadAttributeAccess() {
>>
>> The methods here throw WindowsException when they fail and might be better
>> to keep it consistent and throw WindowsException rather than returning
>> INVALID_HANDLE_VALUE.
>
> Okay
Actually, there's a mistake in the current latest version (04). Should have
tested it before pushing. Will push update shortly.
-
PR: https://git.openjdk.java.net/jdk/pull/2424
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]
On Mon, 8 Feb 2021 16:50:02 GMT, Alan Bateman wrote:
>> Michael McMahon 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 11 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into 8252971-socket-attributes
>> - add specific check for unix domain socket
>> - Merge branch 'master' into 8252971-socket-attributes
>> - update
>> - update
>> - update
>> - update
>> - update after Alan's first review
>> - Merge branch 'master' into 8252971-socket-attributes
>> - test update
>> - ... and 1 more:
>> https://git.openjdk.java.net/jdk/compare/4e1be4ef...746b4762
>
> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 862:
>
>> 860: * and a handle to the socket file if it is.
>> 861: */
>> 862: private long openSocketForReadAttributeAccess() {
>
> The methods here throw WindowsException when they fail and might be better to
> keep it consistent and throw WindowsException rather than returning
> INVALID_HANDLE_VALUE.
Okay
-
PR: https://git.openjdk.java.net/jdk/pull/2424
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v5]
> Could I get the following change reviewed please? It fixes a problem (in > JEP380) on Windows where some file operations on Unix domain sockets were not > working and led to the feature being disabled on Windows 2019 Server in JDK > 16. So, the fix re-enables the feature on all versions of Windows that > support it. > > The test checks all the file APIs that were affected by the problem. The > change touches the code that handles symbolic links in Windows (since they > are implemented as NTFS reparse points, like Unix sockets), but I didn't add > any specific testing in this area, as I assume the existing unit tests for > NIO symbolic links should cover that. If I should add more tests here, then I > can do that. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: update - Changes: - all: https://git.openjdk.java.net/jdk/pull/2424/files - new: https://git.openjdk.java.net/jdk/pull/2424/files/746b4762..f644e939 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=03-04 Stats: 29 lines in 2 files changed: 2 ins; 16 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/2424.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424 PR: https://git.openjdk.java.net/jdk/pull/2424
Re: RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
On Mon, 8 Feb 2021 21:42:36 GMT, Naoto Sato wrote: > Please review this simple test case fix. By sampling locales to test, it > reduces the possibility of the time out. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2465
Re: RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
On Mon, 8 Feb 2021 21:42:36 GMT, Naoto Sato wrote: > Please review this simple test case fix. By sampling locales to test, it > reduces the possibility of the time out. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2465
Integrated: 8240632: Note differences between IEEE 754-2019 math lib special cases and java.lang.Math
On Thu, 4 Feb 2021 02:08:12 GMT, Joe Darcy wrote: > Recent revisions of the IEEE 754 floating-point standard have added guidance > on how typical math library methods (sin, cos, tan, etc.) should behave in > terms of their general quality of implementation as well as on special values. > > Other than the pow methods, for the recommended operations listed by IEEE 754 > that the Java math library already includes, the special cases that are > specified by Java are the same as those specified by IEEE 754, except for the > pow method. IEEE 754 calls out some special cases not explicitly listed in > the Java specs. This changeset adds those special cases to the spec and adds > tests of the the special cases if not already present. > > If method "Foo" already had a regression test, new cases were added it it. > Otherwise, a new test was added to cover the special cases of several methods. > > There is no intention at the moment to change the behavior of pow to align > with IEEE 754. This pull request has now been integrated. Changeset: 2fd8ed02 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/2fd8ed02 Stats: 264 lines in 5 files changed: 243 ins; 0 del; 21 mod 8240632: Note differences between IEEE 754-2019 math lib special cases and java.lang.Math Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/2395
Re: RFR: 8240632: Note differences between IEEE 754-2019 math lib special cases and java.lang.Math [v3]
On Sun, 7 Feb 2021 00:45:03 GMT, Joe Darcy wrote: >> Recent revisions of the IEEE 754 floating-point standard have added guidance >> on how typical math library methods (sin, cos, tan, etc.) should behave in >> terms of their general quality of implementation as well as on special >> values. >> >> Other than the pow methods, for the recommended operations listed by IEEE >> 754 that the Java math library already includes, the special cases that are >> specified by Java are the same as those specified by IEEE 754, except for >> the pow method. IEEE 754 calls out some special cases not explicitly listed >> in the Java specs. This changeset adds those special cases to the spec and >> adds tests of the the special cases if not already present. >> >> If method "Foo" already had a regression test, new cases were added it it. >> Otherwise, a new test was added to cover the special cases of several >> methods. >> >> There is no intention at the moment to change the behavior of pow to align >> with IEEE 754. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review comments. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2395
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]
On Mon, 8 Feb 2021 16:48:44 GMT, Alan Bateman wrote: >> Michael McMahon 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 11 additional >> commits since the last revision: >> >> - Merge branch 'master' into 8252971-socket-attributes >> - add specific check for unix domain socket >> - Merge branch 'master' into 8252971-socket-attributes >> - update >> - update >> - update >> - update >> - update after Alan's first review >> - Merge branch 'master' into 8252971-socket-attributes >> - test update >> - ... and 1 more: >> https://git.openjdk.java.net/jdk/compare/6a98ca22...746b4762 > > src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java line > 346: > >> 344: >> Set.of(WindowsChannelFactory.OPEN_REPARSE_POINT), >> 345: 0L); >> 346: fc.close(); > > The new version looks a bit strange. As I read it, the attempt to open the > file fails with ERROR_CANT_ACCESS_FILE so you then check test if the file is > a socket file. That succeeds so we should be done. What is the reason for > opening the file again? I added the test of the socket reparse tag to be 100% sure that we weren't returning false positives for other cases of ERROR_CANT_ACCESS_FILE. But, otherwise, I thought I had to open the file the same way as before to be sure that it is readable. If checking the reparse tag is sufficient then great, that simplifies the change. - PR: https://git.openjdk.java.net/jdk/pull/2424
RFR: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
Please review this simple test case fix. By sampling locales to test, it reduces the possibility of the time out. - Commit messages: - 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out Changes: https://git.openjdk.java.net/jdk/pull/2465/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2465&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261279 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2465.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2465/head:pull/2465 PR: https://git.openjdk.java.net/jdk/pull/2465
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v5]
On Tue, 26 Jan 2021 18:22:02 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 one > additional commit since the last revision: > > Limit amount read to avoid BufferOverflowException > > - limit the amount read > - add tests src/java.base/share/classes/java/io/Reader.java line 194: > 192: nread = this.read(cbuf, off, len); > 193: if (nread > 0) > 194: target.position(target.position() + nread); As `target` is mutable, I think you would do better to change lines 189-194 to something like: char cbuf[] = target.array(); int pos = target.position(); int rem = target.limit() - pos; if (rem <= 0) return -1; int off = target.arrayOffset() + pos; nread = this.read(cbuf, off, rem); if (nread > 0) target.position(pos + nread); - PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v3]
On Tue, 5 Jan 2021 17:44:20 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 one
> additional commit since the last revision:
>
> Update copyright years
src/java.base/share/classes/java/io/Reader.java line 198:
> 196: } else {
> 197: int remaining = target.remaining();
> 198: char cbuf[] = new char[Math.min(remaining,
> TRANSFER_BUFFER_SIZE)];
As `cbuf` for the off-heap case is used in a synchronized block, is there the
opportunity for some sort of cached array here and would it help?
-
PR: https://git.openjdk.java.net/jdk/pull/1915
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy [v9]
On Mon, 8 Feb 2021 20:58:01 GMT, Andrey Turbanov wrote: >> 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 > 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()); - 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]
> 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 fix review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/1853/files - new: https://git.openjdk.java.net/jdk/pull/1853/files/94e99817..6a8a3ae6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1853&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1853&range=07-08 Stats: 29 lines in 10 files changed: 16 ins; 0 del; 13 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 [v7]
On Mon, 8 Feb 2021 16:19:17 GMT, Julia Boes wrote: >> Andrey Turbanov has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. > > src/java.base/share/classes/sun/security/provider/certpath/X509CertPath.java > line 29: > >> 27: >> 28: import java.io.ByteArrayInputStream; >> 29: import java.io.IOException; > > The copyright year needs to be updated for this file and changed to 2021 in > the other files where applicable. done - 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 [v8]
On Mon, 8 Feb 2021 14:38:52 GMT, Weijun Wang 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
>> revert changes in Apache Santuario
>
> src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/Utils.java
> line 49:
>
>> 47: throws IOException
>> 48: {
>> 49: return is.readAllBytes();
>
> This is also from Apache Santuario. It's better to keep it unchanged.
reverted
-
PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo [v8]
On Mon, 8 Feb 2021 14:01:34 GMT, Alan Bateman 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 >> revert changes in Apache Santuario > > src/java.management/share/classes/javax/management/loading/MLet.java line > 1147: > >> 1145: .toFile(); >> 1146: file.deleteOnExit(); >> 1147: Files.copy(is, file.toPath()); > > One thing to check here is the existing behavior when the file already exists > (as Files.copy will fail if the file exists, need to specify REPLACE_EXISTING > to get the semantics of the existing code). > > The code that follows checks if the file exists, this will always be true > when Files.copy succeeds. fixed - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v5]
> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java > test. Ivan Šipka has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - 8166026: Refactor java/lang shell tests to java - 8166026: Refactor java/lang shell tests to java - 8166026: Refactor java/lang shell tests to java - 8166026: Refactor java/lang shell tests to java - Changes: https://git.openjdk.java.net/jdk/pull/1577/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=04 Stats: 444 lines in 6 files changed: 139 ins; 305 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1577.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577 PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]
On Sun, 7 Feb 2021 15:45:00 GMT, Peter Levart wrote: >> Attila Szegedi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Rename ClassLoaderRelation to RetentionDirection; it is better >> self-documenting this way. > > I think this looks good Attila. > @plevart would you be then be okay with approving this PR? Also, @hns or > @sundararajana can I maybe get a review from either of you? @szegedi I've looked at the BiClassValue code and it looks good to me. If that's ok for you I'll review the rest of the PR tomorrow morning. - PR: https://git.openjdk.java.net/jdk/pull/1918
Re: RFR: 8261254: Initialize charset mapping data lazily
On Mon, 8 Feb 2021 17:40:00 GMT, Naoto Sato wrote: >> This patch refactor JDK internal charsets to initialize charset mapping data >> lazily when needed via holder classes. This means both a startup improvement >> in some cases, and possible throughput improvements for all DoubleByte-based >> Charsets. >> >> Testing: tier1-3 > > Looks good to me. I initially thought of the same impression as Alan's, i.e., > enumerating all charsets is not that common (in fact, same applies to Locale, > and I once explored usages and it ended all up in test cases), but if real > apps are using it, this is the right way to do so. Thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/2449
Integrated: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote: > This patch refactor JDK internal charsets to initialize charset mapping data > lazily when needed via holder classes. This means both a startup improvement > in some cases, and possible throughput improvements for all DoubleByte-based > Charsets. > > Testing: tier1-3 This pull request has now been integrated. Changeset: 92c6e6df Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/92c6e6df Stats: 274 lines in 21 files changed: 24 ins; 59 del; 191 mod 8261254: Initialize charset mapping data lazily Reviewed-by: alanb, jkuhn, naoto - PR: https://git.openjdk.java.net/jdk/pull/2449
Integrated: 8259074: regex benchmarks and tests
On Tue, 5 Jan 2021 03:15:56 GMT, Martin Buchholz wrote: > 8259074: regex benchmarks and tests This pull request has now been integrated. Changeset: 351d7888 Author:Martin Buchholz URL: https://git.openjdk.java.net/jdk/commit/351d7888 Stats: 483 lines in 5 files changed: 473 ins; 7 del; 3 mod 8259074: regex benchmarks and tests Reviewed-by: redestad - PR: https://git.openjdk.java.net/jdk/pull/1940
Re: RFR: 8259074: regex benchmarks and tests [v5]
On Tue, 2 Feb 2021 13:10:29 GMT, Claes Redestad wrote: >> Martin Buchholz has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fix imports > > Marked as reviewed by redestad (Reviewer). @cl4es Your comment sent me back to jmh re-education camp. I never looked at warmup iterations numbers before, but I now see they can be very useful for eyeballing how long warmup takes. Very JVM-dependent of course. In practice I will stick with my rule of thumb that 10 seconds of warmup is good enough for any simple program, and that happens to be the JMH warmup time default. OTOH I think more than one warmup iteration is overkill for most benchmarks. - PR: https://git.openjdk.java.net/jdk/pull/1940
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote: > This patch refactor JDK internal charsets to initialize charset mapping data > lazily when needed via holder classes. This means both a startup improvement > in some cases, and possible throughput improvements for all DoubleByte-based > Charsets. > > Testing: tier1-3 Looks good to me. I initially thought of the same impression as Alan's, i.e., enumerating all charsets is not that common (in fact, same applies to Locale, and I once explored usages and it ended all up in test cases), but if real apps are using it, this is the right way to do so. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2449
Re: JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle
I now implemented a prototype for JDK-6824466: [1] The goal I set here was to remove the old AccessorGenerator. The good news: - It works - all tier1 tests pass, except one that requires the old implementation. - A lot of code has been removed. And even more could be removed. The bad news: - I hit a performance regression [2]. The JMH tests were written by Peter Levart [3]. As much as I don't like the old AccessorGenerator, I have to say that it is very optimized. If you have any ideas on how the performance could be improved, or how I could find that out where the performance drops, let me know. - Johannes PS.: I played around with adding some @Stable and @ForceInline annotations, and got a big performance improvement for the const cases (~9 ns/op). But that came with other performance regressions for the non-const cases. [1]: https://github.com/openjdk/jdk/pull/2457 [2]: https://gist.github.com/DasBrain/09b37510955b14f0ad82313df8d8de37#gistcomment-3623292 [3]: http://cr.openjdk.java.net/~plevart/jdk-dev/6824466_MHReflectionAccessors/
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]
On Sun, 7 Feb 2021 20:16:08 GMT, Michael McMahon wrote:
>> Could I get the following change reviewed please? It fixes a problem (in
>> JEP380) on Windows where some file operations on Unix domain sockets were
>> not working and led to the feature being disabled on Windows 2019 Server in
>> JDK 16. So, the fix re-enables the feature on all versions of Windows that
>> support it.
>>
>> The test checks all the file APIs that were affected by the problem. The
>> change touches the code that handles symbolic links in Windows (since they
>> are implemented as NTFS reparse points, like Unix sockets), but I didn't add
>> any specific testing in this area, as I assume the existing unit tests for
>> NIO symbolic links should cover that. If I should add more tests here, then
>> I can do that.
>>
>> Thanks,
>> Michael
>
> Michael McMahon 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 11 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8252971-socket-attributes
> - add specific check for unix domain socket
> - Merge branch 'master' into 8252971-socket-attributes
> - update
> - update
> - update
> - update
> - update after Alan's first review
> - Merge branch 'master' into 8252971-socket-attributes
> - test update
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/77ac60d6...746b4762
src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java line
346:
> 344:
> Set.of(WindowsChannelFactory.OPEN_REPARSE_POINT),
> 345: 0L);
> 346: fc.close();
The new version looks a bit strange. As I read it, the attempt to open the file
fails with ERROR_CANT_ACCESS_FILE so you then check test if the file is a
socket file. That succeeds so we should be done. What is the reason for opening
the file again?
src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 862:
> 860: * and a handle to the socket file if it is.
> 861: */
> 862: private long openSocketForReadAttributeAccess() {
The methods here throw WindowsException when they fail and might be better to
keep it consistent and throw WindowsException rather than returning
INVALID_HANDLE_VALUE.
-
PR: https://git.openjdk.java.net/jdk/pull/2424
Re: RFR: JDK-8261237: isClassPathAttributePresent use try with resources with JarFile
On Mon, 8 Feb 2021 14:25:45 GMT, Christoph Langer wrote:
>> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 87:
>>
>>> 85: * path is not a JAR file or some other error occurs.
>>> 86: */
>>> 87: public static boolean isClassPathAttributePresent(String path) {
>>
>> Is isClassPathAttributePresent used anywhere? I remember we added VMSupport
>> for use by JVMTI and the attach mechanism but I don't see any usages of this
>> method now.
>
> Seems you're right. My search for "isClassPathAttributePresent" also didn't
> yield anything. So why not remove it altogether...?
Removing it sounds good.
-
PR: https://git.openjdk.java.net/jdk/pull/2429
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo [v8]
On Mon, 8 Feb 2021 14:40:16 GMT, Weijun Wang 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 >> revert changes in Apache Santuario > > The other security-related code changes look good to me. I've updated the issue summary to better reflect the changes, the PR summary should be renamed accordingly. As mentioned earlier, have you run the tests for the affected areas? Here's some information on how to do that: http://openjdk.java.net/guide/#testing-the-jdk - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks [v2]
On Mon, 8 Feb 2021 15:25:02 GMT, Aleksey Shipilev wrote:
>> After JDK-8254702, SonarCloud instance complains about blocks like these:
>> "Change this loop body so that it can be executed more than once."
>>
>> int initJvmlLauncherData(JvmlLauncherData* ptr) const {
>> // Store path to JLI library just behind JvmlLauncherData header.
>> char* curPtr = reinterpret_cast(ptr + 1);
>> do {
>> const size_t count = sizeof(char)
>> * (jliLibPath.size() + 1 /* trailing zero */);
>> if (ptr) {
>> std::memcpy(curPtr, jliLibPath.c_str(), count);
>> ptr->jliLibPath = curPtr;
>> }
>> curPtr += count;
>> } while (false); // < here
>>
>> There is no sense in having `while(false)` here, where the syntactic `{}`
>> block would do. There are also other uses in the jpackage code that employ
>> `while(0)` for this, and then they also trigger the inspection about the
>> implicit conversion of zero int to boolean.
>>
>> Additional testing:
>> - [x] Linux x86_64 (Ubuntu) tools/jpackage
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> More cleanup in tstrings.cpp
Marked as reviewed by asemenyuk (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: 8261298: LinuxPackage.c, getJvmLauncherLibPath RPM->DEB typo
On Mon, 8 Feb 2021 08:49:17 GMT, Aleksey Shipilev wrote:
> SonarCloud instance reports as new warning after JDK-8254702:
>
> This branch can not be reached because the condition duplicates a previous
> condition in the same sequence of "if/else if" statements.
>
> char* getJvmLauncherLibPath(void) {
>...
> if (PACKAGE_TYPE_RPM == pkg->type) {
> pkgQueryCmd = "rpm -ql '%s' 2>/dev/null";
> } else if (PACKAGE_TYPE_RPM == pkg->type) { <--- here
> pkgQueryCmd = "dpkg -L '%s' 2>/dev/null";
>
> Seems like an obvious typo.
>
> Additional tests:
> - [x] Linux x86_64 (Ubuntu) `tools/jpackage`
Marked as reviewed by asemenyuk (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/2452
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo [v7]
On Mon, 21 Dec 2020 08:19:08 GMT, Andrey Turbanov wrote: >> 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo > > Andrey Turbanov has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. src/java.base/share/classes/sun/security/provider/certpath/X509CertPath.java line 29: > 27: > 28: import java.io.ByteArrayInputStream; > 29: import java.io.IOException; The copyright year needs to be updated for this file and changed to 2021 in the other files where applicable. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks [v2]
On Mon, 8 Feb 2021 15:25:02 GMT, Aleksey Shipilev wrote:
>> After JDK-8254702, SonarCloud instance complains about blocks like these:
>> "Change this loop body so that it can be executed more than once."
>>
>> int initJvmlLauncherData(JvmlLauncherData* ptr) const {
>> // Store path to JLI library just behind JvmlLauncherData header.
>> char* curPtr = reinterpret_cast(ptr + 1);
>> do {
>> const size_t count = sizeof(char)
>> * (jliLibPath.size() + 1 /* trailing zero */);
>> if (ptr) {
>> std::memcpy(curPtr, jliLibPath.c_str(), count);
>> ptr->jliLibPath = curPtr;
>> }
>> curPtr += count;
>> } while (false); // < here
>>
>> There is no sense in having `while(false)` here, where the syntactic `{}`
>> block would do. There are also other uses in the jpackage code that employ
>> `while(0)` for this, and then they also trigger the inspection about the
>> implicit conversion of zero int to boolean.
>>
>> Additional testing:
>> - [x] Linux x86_64 (Ubuntu) tools/jpackage
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> More cleanup in tstrings.cpp
OK - looks good to me.
-
Marked as reviewed by herrick (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks
On Mon, 8 Feb 2021 14:31:32 GMT, Andy Herrick wrote: > There are also instances of "while(0)" (without space) in app.cpp, > tstrings.cpp. and ErrorHandling.h Right. I removed more things in `tstrings.cpp`, but in other places, `while(0)` are in `#define`-s, and that is one place where those make sense: they guard from syntax/semantic errors during macro expansion. - PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks [v2]
> After JDK-8254702, SonarCloud instance complains about blocks like these:
> "Change this loop body so that it can be executed more than once."
>
> int initJvmlLauncherData(JvmlLauncherData* ptr) const {
> // Store path to JLI library just behind JvmlLauncherData header.
> char* curPtr = reinterpret_cast(ptr + 1);
> do {
> const size_t count = sizeof(char)
> * (jliLibPath.size() + 1 /* trailing zero */);
> if (ptr) {
> std::memcpy(curPtr, jliLibPath.c_str(), count);
> ptr->jliLibPath = curPtr;
> }
> curPtr += count;
> } while (false); // < here
>
> There is no sense in having `while(false)` here, where the syntactic `{}`
> block would do. There are also other uses in the jpackage code that employ
> `while(0)` for this, and then they also trigger the inspection about the
> implicit conversion of zero int to boolean.
>
> Additional testing:
> - [x] Linux x86_64 (Ubuntu) tools/jpackage
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
More cleanup in tstrings.cpp
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2454/files
- new: https://git.openjdk.java.net/jdk/pull/2454/files/92873eee..d895c44a
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2454&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2454&range=00-01
Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/2454.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2454/head:pull/2454
PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: 8260621: Avoid memory leak in ImageBufferCache [v2]
On Wed, 3 Feb 2021 01:29:02 GMT, Bo Zhang wrote: >> Previously, `ImageBufferCache` contains a ThreadLocal field which holds >> strong reference to `ImageBufferCache$BufferReference.class`. When loaded >> from `jrt-fs.jar`, this will keep `JrtFileSystemProvider$JrtFsLoader` >> in memory forever and never being GCed. >> >> The fix replace the old `ImageBufferCache$BufferReference` class with >> `WeakReference`, which is verified by provided test. > > Bo Zhang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by jlaskey (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2307
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo [v8]
On Mon, 21 Dec 2020 09:16:11 GMT, Andrey Turbanov
wrote:
>> 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo
>
> 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
> revert changes in Apache Santuario
The other security-related code changes look good to me.
src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/Utils.java line
49:
> 47: throws IOException
> 48: {
> 49: return is.readAllBytes();
This is also from Apache Santuario. It's better to keep it unchanged.
-
PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks
On Mon, 8 Feb 2021 09:17:46 GMT, Aleksey Shipilev wrote:
> After JDK-8254702, SonarCloud instance complains about blocks like these:
> "Change this loop body so that it can be executed more than once."
>
> int initJvmlLauncherData(JvmlLauncherData* ptr) const {
> // Store path to JLI library just behind JvmlLauncherData header.
> char* curPtr = reinterpret_cast(ptr + 1);
> do {
> const size_t count = sizeof(char)
> * (jliLibPath.size() + 1 /* trailing zero */);
> if (ptr) {
> std::memcpy(curPtr, jliLibPath.c_str(), count);
> ptr->jliLibPath = curPtr;
> }
> curPtr += count;
> } while (false); // < here
>
> There is no sense in having `while(false)` here, where the syntactic `{}`
> block would do. There are also other uses in the jpackage code that employ
> `while(0)` for this, and then they also trigger the inspection about the
> implicit conversion of zero int to boolean.
>
> Additional testing:
> - [x] Linux x86_64 (Ubuntu) tools/jpackage
There are also instances of "while(0)" (without space) in app.cpp,
tstrings.cpp. and ErrorHandling.h
-
PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: JDK-8261237: isClassPathAttributePresent use try with resources with JarFile
On Mon, 8 Feb 2021 13:41:17 GMT, Alan Bateman wrote:
>> Hello,
>> Currently in jdk/internal/vm/VMSupport.java , we create a JarFile without a
>> related finally clause or try with resources. That should better be changed.
>> See also the Sonar check result :
>>
>> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM8zf8sPJZZzON5qG&resolved=false&severities=BLOCKER&types=BUG
>>
>> public static boolean isClassPathAttributePresent(String path) {
>> try {
>> Manifest man = (new JarFile(path)).getManifest();
>> Use try-with-resources or close this "JarFile" in a "finally" clause.
>
> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 87:
>
>> 85: * path is not a JAR file or some other error occurs.
>> 86: */
>> 87: public static boolean isClassPathAttributePresent(String path) {
>
> Is isClassPathAttributePresent used anywhere? I remember we added VMSupport
> for use by JVMTI and the attach mechanism but I don't see any usages of this
> method now.
Seems you're right. My search for "isClassPathAttributePresent" also didn't
yield anything. So why not remove it altogether...?
-
PR: https://git.openjdk.java.net/jdk/pull/2429
Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo [v8]
On Mon, 21 Dec 2020 09:16:11 GMT, Andrey Turbanov wrote: >> 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo > > 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 > revert changes in Apache Santuario src/java.management/share/classes/javax/management/loading/MLet.java line 1147: > 1145: .toFile(); > 1146: file.deleteOnExit(); > 1147: Files.copy(is, file.toPath()); One thing to check here is the existing behavior when the file already exists (as Files.copy will fail if the file exists, need to specify REPLACE_EXISTING to get the semantics of the existing code). The code that follows checks if the file exists, this will always be true when Files.copy succeeds. src/java.base/share/classes/sun/net/www/MimeLauncher.java line 2: > 1: /* > 2: * Copyright (c) 1994, 2020, Oracle and/or its affiliates. All rights > reserved. Is MimeEntry.launch still used? I'm wondering if the MimeLauncher can be deleted. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: JDK-8261237: isClassPathAttributePresent use try with resources with JarFile
On Fri, 5 Feb 2021 15:23:59 GMT, Matthias Baesken wrote:
> Hello,
> Currently in jdk/internal/vm/VMSupport.java , we create a JarFile without a
> related finally clause or try with resources. That should better be changed.
> See also the Sonar check result :
>
> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM8zf8sPJZZzON5qG&resolved=false&severities=BLOCKER&types=BUG
>
> public static boolean isClassPathAttributePresent(String path) {
> try {
> Manifest man = (new JarFile(path)).getManifest();
> Use try-with-resources or close this "JarFile" in a "finally" clause.
src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 87:
> 85: * path is not a JAR file or some other error occurs.
> 86: */
> 87: public static boolean isClassPathAttributePresent(String path) {
Is isClassPathAttributePresent used anywhere? I remember we added VMSupport for
use by JVMTI and the attach mechanism but I don't see any usages of this method
now.
-
PR: https://git.openjdk.java.net/jdk/pull/2429
Re: RFR: 8261254: Initialize charset mapping data lazily
On Mon, 8 Feb 2021 12:16:23 GMT, Claes Redestad wrote:
>> src/jdk.charsets/share/classes/sun/nio/cs/ext/AbstractCharsetProvider.java
>> line 75:
>>
>>> 73:
>>> 74: protected AbstractCharsetProvider(String pkgPrefixName) {
>>> 75: packagePrefix = pkgPrefixName.concat(".");
>>
>> Hm, I wonder why not just `pkgPrefixName + '.'` here and below? Is it
>> something about early init of `StringConcatFactory`?
>
> Yes, I wanted to measure the overhead of `Charset` class initialization done
> by `Charset.availableCharsets()` and the `StringConcatFactory` bootstraps was
> a reasonable chunk of the cost so I moved them out of the picture. I didn't
> mind what I ended up with, but if you prefer I can move back to
> `pkgPrefixName + '.'` here.
Let's keep it as is, to me it's a readable as concatenation via `+`
-
PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:44:41 GMT, Johannes Kuhn wrote:
>> This patch refactor JDK internal charsets to initialize charset mapping data
>> lazily when needed via holder classes. This means both a startup improvement
>> in some cases, and possible throughput improvements for all DoubleByte-based
>> Charsets.
>>
>> Testing: tier1-3
>
> src/java.base/share/classes/java/lang/ModuleLayer.java line 936:
>
>> 934: for (Module m : nameToModule.values()) {
>> 935: servicesCatalog.register(m);
>> 936: }
>
> Seems to be unrelated, but it's not a bad change.
As with the `StringConcatFactory` changes @stsypanov commented on, I did this
to reduce noise when zooming in on the cost of `Charset` class initialization
stemming from `Charset.availableCharsets()`. I ended up preferring the
desugared version, so I left it in.
-
PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8261254: Initialize charset mapping data lazily
On Mon, 8 Feb 2021 11:42:23 GMT, Сергей Цыпанов
wrote:
>> This patch refactor JDK internal charsets to initialize charset mapping data
>> lazily when needed via holder classes. This means both a startup improvement
>> in some cases, and possible throughput improvements for all DoubleByte-based
>> Charsets.
>>
>> Testing: tier1-3
>
> src/jdk.charsets/share/classes/sun/nio/cs/ext/AbstractCharsetProvider.java
> line 75:
>
>> 73:
>> 74: protected AbstractCharsetProvider(String pkgPrefixName) {
>> 75: packagePrefix = pkgPrefixName.concat(".");
>
> Hm, I wonder why not just `pkgPrefixName + '.'` here and below? Is it
> something about early init of `StringConcatFactory`?
Yes, I wanted to measure the overhead of `Charset` class initialization done by
`Charset.availableCharsets()` and the `StringConcatFactory` bootstraps was a
reasonable chunk of the cost so I moved them out of the picture. I didn't mind
what I ended up with, but if you prefer I can move back to `pkgPrefixName +
'.'` here.
-
PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote:
> This patch refactor JDK internal charsets to initialize charset mapping data
> lazily when needed via holder classes. This means both a startup improvement
> in some cases, and possible throughput improvements for all DoubleByte-based
> Charsets.
>
> Testing: tier1-3
Looks good to me.
src/java.base/share/classes/java/lang/ModuleLayer.java line 936:
> 934: for (Module m : nameToModule.values()) {
> 935: servicesCatalog.register(m);
> 936: }
Seems to be unrelated, but it's not a bad change.
-
Marked as reviewed by jkuhn (Author).
PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote:
> This patch refactor JDK internal charsets to initialize charset mapping data
> lazily when needed via holder classes. This means both a startup improvement
> in some cases, and possible throughput improvements for all DoubleByte-based
> Charsets.
>
> Testing: tier1-3
src/jdk.charsets/share/classes/sun/nio/cs/ext/AbstractCharsetProvider.java line
75:
> 73:
> 74: protected AbstractCharsetProvider(String pkgPrefixName) {
> 75: packagePrefix = pkgPrefixName.concat(".");
Hm, I wonder why not just `pkgPrefixName + '.'` here and below?
-
PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8261254: Initialize charset mapping data lazily
On Mon, 8 Feb 2021 08:36:21 GMT, Alan Bateman wrote: >> This patch refactor JDK internal charsets to initialize charset mapping data >> lazily when needed via holder classes. This means both a startup improvement >> in some cases, and possible throughput improvements for all DoubleByte-based >> Charsets. >> >> Testing: tier1-3 > > I wouldn't expect enumerating all charsets with Charset::availableCharsets to > be too common but moving the data to holder class looks okay. The missing > "final" in a few places was an oversight. The replacement of the foreach and > method ref in getServicesCatalog with imperative code is disappointment but > okay here. I spotted usage of this in a real application. While they could work around it and remove the usage to gain an even larger startup win I figured I should do those that can't do so a favor, too. - PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: JDK-8261237: isClassPathAttributePresent use try with resources with JarFile
On Fri, 5 Feb 2021 15:23:59 GMT, Matthias Baesken wrote:
> Hello,
> Currently in jdk/internal/vm/VMSupport.java , we create a JarFile without a
> related finally clause or try with resources. That should better be changed.
> See also the Sonar check result :
>
> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM8zf8sPJZZzON5qG&resolved=false&severities=BLOCKER&types=BUG
>
> public static boolean isClassPathAttributePresent(String path) {
> try {
> Manifest man = (new JarFile(path)).getManifest();
> Use try-with-resources or close this "JarFile" in a "finally" clause.
Marked as reviewed by redestad (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2429
Re: RFR: JDK-8261237: isClassPathAttributePresent use try with resources with JarFile
On Fri, 5 Feb 2021 15:23:59 GMT, Matthias Baesken wrote:
> Hello,
> Currently in jdk/internal/vm/VMSupport.java , we create a JarFile without a
> related finally clause or try with resources. That should better be changed.
> See also the Sonar check result :
>
> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM8zf8sPJZZzON5qG&resolved=false&severities=BLOCKER&types=BUG
>
> public static boolean isClassPathAttributePresent(String path) {
> try {
> Manifest man = (new JarFile(path)).getManifest();
> Use try-with-resources or close this "JarFile" in a "finally" clause.
Looks correct and straightforward.
-
Marked as reviewed by clanger (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2429
Re: RFR: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
On Mon, 8 Feb 2021 09:05:23 GMT, Aleksey Shipilev wrote:
> SonarCloud instance reports a new warning after JDK-8254702:
> "Use of memory after it is freed"
>
> char* getJvmLauncherLibPath(void) {
>...
> popenStatus = popenCommand(pkgQueryCmd, pkg->name, findLauncherLib,
> &launcherLibPath);
> if (popenStatus) {
> free(launcherLibPath); < frees here
> goto cleanup;
> }
> }
>
> cleanup:
> free(modulePath);
> freePackageDesc(pkg);
>
> return launcherLibPath; <--- here
> }
>
> We need to null it out before returning.
>
> Additional testing:
> - [x] Linux x86_64 (Ubuntu) `tools/jpackage`
Marked as reviewed by stuefe (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2453
Re: RFR: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
On Mon, 8 Feb 2021 09:05:23 GMT, Aleksey Shipilev wrote:
> SonarCloud instance reports a new warning after JDK-8254702:
> "Use of memory after it is freed"
>
> char* getJvmLauncherLibPath(void) {
>...
> popenStatus = popenCommand(pkgQueryCmd, pkg->name, findLauncherLib,
> &launcherLibPath);
> if (popenStatus) {
> free(launcherLibPath); < frees here
> goto cleanup;
> }
> }
>
> cleanup:
> free(modulePath);
> freePackageDesc(pkg);
>
> return launcherLibPath; <--- here
> }
>
> We need to null it out before returning.
>
> Additional testing:
> - [x] Linux x86_64 (Ubuntu) `tools/jpackage`
Fine and trivial. Thanks for fixing.
I tried to understand what the fix for JDK-8254702 does but the issue
description and the PR are a bit taciturn. This seems to be way more than just
a split of C- and C++ sources as the PR suggested.
..Thomas
-
PR: https://git.openjdk.java.net/jdk/pull/2453
RFR: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks
After JDK-8254702, SonarCloud instance complains about blocks like these:
"Change this loop body so that it can be executed more than once."
int initJvmlLauncherData(JvmlLauncherData* ptr) const {
// Store path to JLI library just behind JvmlLauncherData header.
char* curPtr = reinterpret_cast(ptr + 1);
do {
const size_t count = sizeof(char)
* (jliLibPath.size() + 1 /* trailing zero */);
if (ptr) {
std::memcpy(curPtr, jliLibPath.c_str(), count);
ptr->jliLibPath = curPtr;
}
curPtr += count;
} while (false); // < here
There is no sense in having `while(false)` here, where the syntactic `{}` block
would do. There are also other uses in the jpackage code that employ `while(0)`
for this, and then they also trigger the inspection about the implicit
conversion of zero int to boolean.
Additional testing:
- [x] Linux x86_64 (Ubuntu) tools/jpackage
-
Commit messages:
- Replace with {}
Changes: https://git.openjdk.java.net/jdk/pull/2454/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2454&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8261300
Stats: 22 lines in 1 file changed: 0 ins; 0 del; 22 mod
Patch: https://git.openjdk.java.net/jdk/pull/2454.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2454/head:pull/2454
PR: https://git.openjdk.java.net/jdk/pull/2454
RFR: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
SonarCloud instance reports a new warning after JDK-8254702:
"Use of memory after it is freed"
char* getJvmLauncherLibPath(void) {
...
popenStatus = popenCommand(pkgQueryCmd, pkg->name, findLauncherLib,
&launcherLibPath);
if (popenStatus) {
free(launcherLibPath); < frees here
goto cleanup;
}
}
cleanup:
free(modulePath);
freePackageDesc(pkg);
return launcherLibPath; <--- here
}
We need to null it out before returning.
Additional testing:
- [x] Linux x86_64 (Ubuntu) `tools/jpackage`
-
Commit messages:
- Null it out
Changes: https://git.openjdk.java.net/jdk/pull/2453/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2453&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8261299
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/2453.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2453/head:pull/2453
PR: https://git.openjdk.java.net/jdk/pull/2453
RFR: 8261298: LinuxPackage.c, getJvmLauncherLibPath RPM->DEB typo
SonarCloud instance reports as new warning after JDK-8254702:
This branch can not be reached because the condition duplicates a previous
condition in the same sequence of "if/else if" statements.
char* getJvmLauncherLibPath(void) {
...
if (PACKAGE_TYPE_RPM == pkg->type) {
pkgQueryCmd = "rpm -ql '%s' 2>/dev/null";
} else if (PACKAGE_TYPE_RPM == pkg->type) { <--- here
pkgQueryCmd = "dpkg -L '%s' 2>/dev/null";
Seems like an obvious typo.
Additional tests:
- [x] Linux x86_64 (Ubuntu) `tools/jpackage`
-
Commit messages:
- Typo fix
Changes: https://git.openjdk.java.net/jdk/pull/2452/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2452&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8261298
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2452.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2452/head:pull/2452
PR: https://git.openjdk.java.net/jdk/pull/2452
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote: > This patch refactor JDK internal charsets to initialize charset mapping data > lazily when needed via holder classes. This means both a startup improvement > in some cases, and possible throughput improvements for all DoubleByte-based > Charsets. > > Testing: tier1-3 I wouldn't expect enumerating all charsets with Charset::availableCharsets to be too common but moving the data to holder class looks okay. The missing "final" in a few places was an oversight. The replacement of the foreach and method ref in getServicesCatalog with imperative code is disappointment but okay here. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: JDK-8261237: isClassPathAttributePresent use try with resources with JarFile
On Fri, 5 Feb 2021 15:23:59 GMT, Matthias Baesken wrote:
> Hello,
> Currently in jdk/internal/vm/VMSupport.java , we create a JarFile without a
> related finally clause or try with resources. That should better be changed.
> See also the Sonar check result :
>
> https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM8zf8sPJZZzON5qG&resolved=false&severities=BLOCKER&types=BUG
>
> public static boolean isClassPathAttributePresent(String path) {
> try {
> Manifest man = (new JarFile(path)).getManifest();
> Use try-with-resources or close this "JarFile" in a "finally" clause.
This looks good to me.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2429
