Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 19:37:44 GMT, Stephen Colebourne
wrote:
>> Please review this PR to introduce `java.time.Duration.isPositive()` method.
>> A CSR is also drafted.
>
> src/java.base/share/classes/java/time/Duration.java line 596:
>
>> 594: */
>> 595: public boolean isPositive() {
>> 596: return (seconds | nanos) > 0;
>
> I had to think whether this logic is correct, but I believe it is because
> `nanos` is 32 bits and positive so won't impact the negative bit of `seconds`.
Thanks, Stephen. Yes, `nanos` is even smaller, less than `1,000,000,000`.
-
PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote:
> Please review this PR to introduce `java.time.Duration.isPositive()` method.
> A CSR is also drafted.
Marked as reviewed by scolebourne (Author).
src/java.base/share/classes/java/time/Duration.java line 596:
> 594: */
> 595: public boolean isPositive() {
> 596: return (seconds | nanos) > 0;
I had to think whether this logic is correct, but I believe it is because
`nanos` is 32 bits and positive so won't impact the negative bit of `seconds`.
-
PR: https://git.openjdk.java.net/jdk/pull/4892
Integrated: 8199594: Add doc describing how (?x) ignores spaces in character classes
On Mon, 28 Jun 2021 20:50:42 GMT, Ian Graves wrote: > 8199594: Add doc describing how (?x) ignores spaces in character classes This pull request has now been integrated. Changeset: a1c0a6aa Author:Ian Graves URL: https://git.openjdk.java.net/jdk/commit/a1c0a6aafb575e3d5c76dd3a279e4fe03ca07223 Stats: 11 lines in 1 file changed: 10 ins; 0 del; 1 mod 8199594: Add doc describing how (?x) ignores spaces in character classes Reviewed-by: darcy, naoto, iris, lancea, bpb - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v3]
On Thu, 22 Jul 2021 01:46:09 GMT, Ian Graves wrote: >> 8199594: Add doc describing how (?x) ignores spaces in character classes > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Rewording repetitive phrase Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. Looks good! I've also Reviewed the associated CSR. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v3]
On Thu, 22 Jul 2021 01:46:09 GMT, Ian Graves wrote: >> 8199594: Add doc describing how (?x) ignores spaces in character classes > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Rewording repetitive phrase Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev wrote: > Dear colleagues, > > Please review the patch that replaces the lambdas with anonymous classes > which solves the startup time regression as shown below. > > I attached the Bytestacks flamegraphs for both original (regression) and > fixed versions. The flamegraphs clearly show the lambdas were causing the > performance issue. > > [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) > > Although the proposed JDK-8270321 patch fixes the startup time (it might > appear even better than it was before the regression was introduced, i.e. > before JDK-8266310), it increases MaxRSS slightly compared to the version > before JDK-8266310, which is shown in the below graphs. > >  > >  > > I additionally include the heap objects histograms to show the change does > not increase the total live objects size significantly with only 1000 bytes > the total difference, namely 1116128 bytes in 25002 live objects after the > proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the > version with the original patch reverted (i.e. before JDK-8266310). > > [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) > > The patch was tested w/hotspot/tier1/tier2 test groups. I don't understand your analysis, you are testing the startup time with -Xint which disable JITs, but there is no mention of -Xint in the bug report. It's obvious to me that there is a regression with -Xint given that the lambda creation is using invokedynamic which is only optimizable by JITs. I think you should first try to reproduce the issue with the correct flags and then follow the advice from Mandy on how to implement the fix. Using an anonymous class may introduce more allocation than using a lambda once the code is JITed. - PR: https://git.openjdk.java.net/jdk/pull/4893
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v3]
On Thu, 22 Jul 2021 01:46:09 GMT, Ian Graves wrote: >> 8199594: Add doc describing how (?x) ignores spaces in character classes > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Rewording repetitive phrase Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4892
RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
Dear colleagues, Please review the patch that replaces the lambdas with anonymous classes which solves the startup time regression as shown below. I attached the Bytestacks flamegraphs for both original (regression) and fixed versions. The flamegraphs clearly show the lambdas were causing the performance issue. [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) Although the proposed JDK-8270321 patch fixes the startup time (it might appear even better than it was before the regression was introduced, i.e. before JDK-8266310), it increases MaxRSS slightly compared to the version before JDK-8266310, which is shown in the below graphs.   I additionally include the heap objects histograms to show the change does not increase the total live objects size significantly with only 1000 bytes the total difference, namely 1116128 bytes in 25002 live objects after the proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the version with the original patch reverted (i.e. before JDK-8266310). [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) - Commit messages: - 8270321: Startup regressions in 18-b5 caused by JDK-8266310 Changes: https://git.openjdk.java.net/jdk/pull/4893/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4893=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270321 Stats: 35 lines in 1 file changed: 17 ins; 3 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/4893.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4893/head:pull/4893 PR: https://git.openjdk.java.net/jdk/pull/4893
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. Thanks, Roger. Modified the CSR as suggested. - PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. In the CSR, I would omit the "which is odd" from the "Problem". and in the Solution, replace "integrity" with "completeness". - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4892
RFR: 8171382: java.time.Duration missing isPositive method
Please review this PR to introduce `java.time.Duration.isPositive()` method. A CSR is also drafted. - Commit messages: - 8171382: java.time.Duration missing isPositive method Changes: https://git.openjdk.java.net/jdk/pull/4892/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4892=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8171382 Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4892.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4892/head:pull/4892 PR: https://git.openjdk.java.net/jdk/pull/4892
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Fri, 18 Jun 2021 18:28:26 GMT, Sean Mullan wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > The change to `PKCS7::verify(byte[])` looks fine. > @seanjmullan Are you planning to sponsor this? Yes, I can do that, but I will wait until Monday in case there are any post-integration issues that I need to follow up on. - PR: https://git.openjdk.java.net/jdk/pull/4482
Re: RFR: 8265474: Dubious 'null' assignment in CompactByteArray.expand
On Wed, 23 Dec 2020 16:06:30 GMT, Andrey Turbanov wrote: > I propose to remove 'null' assignment of field CompactByteArray.values in > `expand` method. In the next statement this field is overridden with another > value - `tempArray`. > This code was there from initial load of OpenJDK sources. I believe it was > just leftovers from development phase of this class. There is no practical > reason to assign `null` to non-volatile field and then overwrite it with > another value. > Also I've removed unused method `getArray`. I hope it's ok to cleanup such > unused stuff in the same PR. Yes, it does. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1880
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v11]
On Fri, 23 Jul 2021 15:23:07 GMT, Jaikiran Pai wrote: > Thank you for the review Alan. > > @LanceAndersen, I've run the tier1 tests locally with the latest PR and they > have passed without any regressions. Given that we changed the implementation > to wrap ByteArrayOutputStream instead of extending it, would you want to > rerun some of your other tests that you had previously run, before I > integrate this? Yes, I will run additional tests and report back after they complete - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Fri, 18 Jun 2021 18:28:26 GMT, Sean Mullan wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > The change to `PKCS7::verify(byte[])` looks fine. @seanjmullan Are you planning to sponsor this? - PR: https://git.openjdk.java.net/jdk/pull/4482
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v11]
On Fri, 23 Jul 2021 14:58:01 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8190753? >> >> The commit here checks for the size of the zip entry before trying to create >> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has >> been included in this commit to reproduce the issue and verify the fix. >> >> P.S: It's still a bit arguable whether it's a good idea to create the >> `ByteArrayOutputStream` with an initial size potentially as large as the >> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default >> value. However, I think that can be addressed separately while dealing with >> https://bugs.openjdk.java.net/browse/JDK-8011146 > > Jaikiran Pai 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 15 additional > commits since the last revision: > > - Merge latest from master branch > - Implement review suggestions: > - remove unnecessary "synchronized" > - no need to open the temp file twice > - remove no longer necessary javadoc comment on test > - review suggestion - wrap ByteArrayOutputStream instead of extending it > - reorganize the tests now that the temp file creation threshold isn't > exposed as a user configurable value > - minor update to comment on FileRolloverOutputStream class > - remove no longer used constant > - use jtreg's construct of manual test > - Implement Alan's review suggestion - don't expose the threshold as a > configuration property, instead set it internally to a specific value > - propagate back the original checked IOException to the callers > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/e1196067...991de6b9 Thank you for the review Alan. @LanceAndersen, I've run the tier1 tests locally with the latest PR and they have passed without any regressions. Given that we changed the implementation to wrap ByteArrayOutputStream instead of extending it, would you want to rerun some of your other tests that you had previously run, before I integrate this? - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v6]
On Tue, 8 Jun 2021 16:53:38 GMT, Henry Jen wrote:
>> …d on macOS
>>
>> This patch simply round up the specified stack size to multiple of the
>> system page size.
>>
>> Test is trivial, simply run java with -Xss option against following code. On
>> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get
>> `7183` and `649` respectively. After fix, both would output `649`, while
>> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
>>
>> ```code:java
>> public class StackLeak {
>> public int depth = 0;
>> public void stackLeak() {
>> depth++;
>> stackLeak();
>> }
>>
>> public static void main(String[] args) {
>> var test = new StackLeak();
>> try {
>> test.stackLeak();
>> } catch (Throwable e) {
>> System.out.println(test.depth);
>> }
>> }
>> }
>
> Henry Jen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update help text
Marked as reviewed by alanb (Reviewer).
src/java.base/unix/native/libjli/java_md.c line 681:
> 679:
> 680: if (stack_size > 0) {
> 681: if (EINVAL == pthread_attr_setstacksize(, stack_size)) {
Style-wise it might be more consistent put EINVAL on the RHS of the ==.
-
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8265474: Dubious 'null' assignment in CompactByteArray.expand
On Wed, 23 Dec 2020 16:06:30 GMT, Andrey Turbanov wrote: > I propose to remove 'null' assignment of field CompactByteArray.values in > `expand` method. In the next statement this field is overridden with another > value - `tempArray`. > This code was there from initial load of OpenJDK sources. I believe it was > just leftovers from development phase of this class. There is no practical > reason to assign `null` to non-volatile field and then overwrite it with > another value. > Also I've removed unused method `getArray`. I hope it's ok to cleanup such > unused stuff in the same PR. This cleanup looks okay, @naotoj? - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1880
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v9]
On Fri, 23 Jul 2021 11:57:34 GMT, Jaikiran Pai wrote: > The updated version of this PR now fixes this part to open it just once. I > had reviewed this `transferTo` multiple times before, but clearly I > overlooked this part of the implementation. > > Thank you for these inputs. The updated PR continues to pass the new tests > and the existing ones in `test/jdk/jdk/nio/zipfs/`. The updated implementation looks okay, I don't think I have any more questions. - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v11]
> Can I please get a review for this proposed fix for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8190753? > > The commit here checks for the size of the zip entry before trying to create > a `ByteArrayOutputStream` for that entry's content. A new jtreg test has been > included in this commit to reproduce the issue and verify the fix. > > P.S: It's still a bit arguable whether it's a good idea to create the > `ByteArrayOutputStream` with an initial size potentially as large as the > `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default > value. However, I think that can be addressed separately while dealing with > https://bugs.openjdk.java.net/browse/JDK-8011146 Jaikiran Pai 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 15 additional commits since the last revision: - Merge latest from master branch - Implement review suggestions: - remove unnecessary "synchronized" - no need to open the temp file twice - remove no longer necessary javadoc comment on test - review suggestion - wrap ByteArrayOutputStream instead of extending it - reorganize the tests now that the temp file creation threshold isn't exposed as a user configurable value - minor update to comment on FileRolloverOutputStream class - remove no longer used constant - use jtreg's construct of manual test - Implement Alan's review suggestion - don't expose the threshold as a configuration property, instead set it internally to a specific value - propagate back the original checked IOException to the callers - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d783ad9d...991de6b9 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4607/files - new: https://git.openjdk.java.net/jdk/pull/4607/files/9e78ba06..991de6b9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4607=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4607=09-10 Stats: 43946 lines in 1112 files changed: 20903 ins; 17871 del; 5172 mod Patch: https://git.openjdk.java.net/jdk/pull/4607.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4607/head:pull/4607 PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v6]
On Fri, 23 Jul 2021 12:28:47 GMT, Severin Gehwolf wrote: > Could this be some setting configured on your box that is picked up as an > additional limit ? Possibly. Not sure where to look, though. - PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v6]
On Fri, 23 Jul 2021 11:02:13 GMT, Matthias Baesken wrote: > > No, I don't know what to do about it. All I can see it comes back with a > > pids limit of `38019` when set to `-1`. It does seem like a bug or an > > intentional setting so as to avoid fork bombs. > > Very strange indeed, I have docker 20.10.2 on my Ubuntu test server and there > the tests work and no "38019" is coming back for -1/unlimited . What distro > are you using? I did a quick search but did not find much about the > mysterious `38019` . Could this be some setting configured on your box that > is picked up as an additional limit ? I'm on Fedora 34 and have the moby distro build of docker: https://koji.fedoraproject.org/koji/buildinfo?buildID=1781164 - PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 23 Jul 2021 10:36:51 GMT, Alan Bateman wrote: > I wasn't suggesting there is a patch attached to that issue. Rather I was > just pointing out that JDK-8251329 was being worked on already before this PR > was created. Ok, that makes sense. Thank you for the details. - PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v9]
On Fri, 23 Jul 2021 10:28:02 GMT, Alan Bateman wrote: > So I think the approach looks good but I think the synchronization needs to > be re-checked it is not obvious that is correct or needed. Are there any > cases where FileRolloverOutputStream is returned to user code? I don't think > so, instead users of the zip file system will get an EntryOutputStream that > wraps the FileRolloverOutputStream. The EntryOutputStream methods are > synchronized so I assume that FileRolloverOutputStream does not need to it > and that would avoid the inconsistency between the write methods and the > flush/close methods. I hadn't paid any thoughts on the "synchronized" part. You are right - the new `FileRolloverOutputStream` doesn't get sent back to the callers directly and instead either the `EntryOutputStream` or the `DeflatingEntryOutputStream` get returned. Both of them have the necessary syncrhonizations in place for `write` and `close` operations. The `flush` of the `FileRolloverOutputStream` calls the `flush` on the `BufferedOutputStream` which already has the necessary synchronization. I've updated the PR to remove the use of `synchronized` from this new class and added a brief note about this for future maintainers, just like the existing `EntryOutputStreamDef` has. > One other thing to point out is that transferToFile shouldn't need to open > the file twice, instead it should be able to open the tmp file for writing > once. The updated version of this PR now fixes this part to open it just once. I had reviewed this `transferTo` multiple times before, but clearly I overlooked this part of the implementation. Thank you for these inputs. The updated PR continues to pass the new tests and the existing ones in `test/jdk/jdk/nio/zipfs/`. - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: [jdk17] RFR: 8271155: Wrong path separator in env variable
On Thu, 22 Jul 2021 19:35:59 GMT, Alexey Semenyuk wrote: > Replace `";"` with `FileUtils::pathSeparator` in the expression adding 'app' > dir to env variable in jpackage app launcher. Alexey filed [JDK-8271170](https://bugs.openjdk.java.net/browse/JDK-8271170) to cover this. - PR: https://git.openjdk.java.net/jdk17/pull/271
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v10]
> Can I please get a review for this proposed fix for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8190753? > > The commit here checks for the size of the zip entry before trying to create > a `ByteArrayOutputStream` for that entry's content. A new jtreg test has been > included in this commit to reproduce the issue and verify the fix. > > P.S: It's still a bit arguable whether it's a good idea to create the > `ByteArrayOutputStream` with an initial size potentially as large as the > `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default > value. However, I think that can be addressed separately while dealing with > https://bugs.openjdk.java.net/browse/JDK-8011146 Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Implement review suggestions: - remove unnecessary "synchronized" - no need to open the temp file twice - Changes: - all: https://git.openjdk.java.net/jdk/pull/4607/files - new: https://git.openjdk.java.net/jdk/pull/4607/files/90101d45..9e78ba06 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4607=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4607=08-09 Stats: 12 lines in 1 file changed: 5 ins; 4 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4607.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4607/head:pull/4607 PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v5]
On Tue, 2 Mar 2021 18:07:24 GMT, Stuart Marks wrote:
>> test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 100:
>>
>>> 98: int r = ArraysSupport.newLength(old, min, pref);
>>> 99: fail("expected OutOfMemoryError, got normal return value of
>>> " + r);
>>> 100: } catch (OutOfMemoryError success) { }
>>
>> Consider using `expectThrows` or `assertThrows` from `org.testng.Assert`.
>
> Good point. However, I actually tried to use assertThrows, but there doesn't
> seem to be a way to get the unexpected return value into the error message. I
> think having this value in the test output is important for diagnosing
> failures.
That tradeoff you made, makes sense.
Indeed, neither `assertThrows` nor `expectThrows` in `org.testng.Assert` logs
an unexpectedly returned value. This is because the code being tested is
expressed as the `Assert.ThrowingRunnable` interface which has a single `void`
method. So the code being tested cannot return a value to the testing framework.
Now, if `org.testng.Assert` were to provide the respective methods accepting,
say, `Assert.ThrowingCallable` that had a value-returning method, TestNG would
run into the same issues that JUnit ran into when they attempted to do that:
https://github.com/junit-team/junit5/issues/1576
-
PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v6]
On Fri, 23 Jul 2021 08:39:37 GMT, Severin Gehwolf wrote: > > No, I don't know what to do about it. All I can see it comes back with a pids > limit of `38019` when set to `-1`. It does seem like a bug or an intentional > setting so as to avoid fork bombs. Very strange indeed, I have docker 20.10.2 on my Ubuntu test server and there the tests work and no "38019" is coming back for -1/unlimited . What distro are you using? I did a quick search but did not find much about the mysterious `38019` . Could this be some setting configured on your box that is picked up as an additional limit ? - PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 23 Jul 2021 08:47:46 GMT, Jaikiran Pai wrote: > This part I didn't understand. Did you mean to refer some other JBS issue? > Because from what I see in https://bugs.openjdk.java.net/browse/JDK-8251329 > there's no patch attached to it (unless of course it's restricted to specific > JBS user roles?). I wasn't suggesting there is a patch attached to that issue. Rather I was just pointing out that JDK-8251329 was being worked on already before this PR was created. Lance's initial patch for JDK-8251329 changed ZipPath::getResolvedPath when creating the inode entry but that approach leads to anomalies. Overall I think the discussion has been useful but I don't think it's feasible to have a file system view over entries that look like links to the current or parent directory. - PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v9]
On Thu, 22 Jul 2021 16:01:30 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8190753? >> >> The commit here checks for the size of the zip entry before trying to create >> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has >> been included in this commit to reproduce the issue and verify the fix. >> >> P.S: It's still a bit arguable whether it's a good idea to create the >> `ByteArrayOutputStream` with an initial size potentially as large as the >> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default >> value. However, I think that can be addressed separately while dealing with >> https://bugs.openjdk.java.net/browse/JDK-8011146 > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - remove no longer necessary javadoc comment on test > - review suggestion - wrap ByteArrayOutputStream instead of extending it Thanks for changing it to wrap the BOAS rather than existing it, that avoids the annoying wrapping/unwrapping of exceptions. So I think the approach looks good but I think the synchronization needs to be re-checked it is not obvious that is correct or needed. Are there any cases where FileRolloverOutputStream is returned to user code? I don't think so, instead users of the zip file system will get an EntryOutputStream that wraps the FileRolloverOutputStream. The EntryOutputStream methods are synchronized so I assume that FileRolloverOutputStream does not need to it and that would avoid the inconsistency between the write methods and the flush/close methods. One other thing to point out is that transferToFile shouldn't need to open the file twice, instead it should be able to open the tmp file for writing once. - PR: https://git.openjdk.java.net/jdk/pull/4607
Withdrawn: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Sun, 27 Jun 2021 13:13:42 GMT, Jaikiran Pai wrote: > Can I please get a review of this proposed fix for > https://bugs.openjdk.java.net/browse/JDK-8251329? > > As noted in that issue, if a zip filesystem created on top of a jar > containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads > to a infinite never ending iteration (which ultimately fails with Java heap > space OOM). > > Alan notes in that issue that: > >> This is more likely an issue with the zipfs DirectoryStream implementation. >> A DirectoryStream is specified to not include elements that for the special >> links to the current or parent directory. It should be rare. > > This indeed turned out to be an issue in the > `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls > the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation. The > implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` > states, was including the "." and ".." paths in its returned iterator: > >> The elements returned by the iterator are in no specific order. Some file > systems maintain special links to the directory itself and the directory's > parent directory. Entries representing these links are not returned by the > iterator. > > > The proposed fix in this patch checks the paths for "." and "..", similar to > what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and > skips those paths from being added into the returned iterator. The > `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been > done) is currently only used by `jdk.nio.zipfs.ZipDirectoryStream#iterator()` > and has package-private visibility, so this change shouldn't impact any other > usage/expectations. > > A new jtreg test has been added to reproduce this issue and verify the fix. > Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine > without any issues after this change. I will be triggering a `tier1` test > locally in a while. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this proposed fix for
>> https://bugs.openjdk.java.net/browse/JDK-8251329?
>>
>> As noted in that issue, if a zip filesystem created on top of a jar
>> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads
>> to a infinite never ending iteration (which ultimately fails with Java heap
>> space OOM).
>>
>> Alan notes in that issue that:
>>
>>> This is more likely an issue with the zipfs DirectoryStream implementation.
>>> A DirectoryStream is specified to not include elements that for the special
>>> links to the current or parent directory. It should be rare.
>>
>> This indeed turned out to be an issue in the
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls
>> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation. The
>> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream`
>> states, was including the "." and ".." paths in its returned iterator:
>>
>>> The elements returned by the iterator are in no specific order. Some file
>> systems maintain special links to the directory itself and the directory's
>> parent directory. Entries representing these links are not returned by the
>> iterator.
>>
>>
>> The proposed fix in this patch checks the paths for "." and "..", similar to
>> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and
>> skips those paths from being added into the returned iterator. The
>> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been
>> done) is currently only used by
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private
>> visibility, so this change shouldn't impact any other usage/expectations.
>>
>> A new jtreg test has been added to reproduce this issue and verify the fix.
>> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine
>> without any issues after this change. I will be triggering a `tier1` test
>> locally in a while.
>
> Jaikiran Pai 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 four additional
> commits since the last revision:
>
> - Merge latest from upstream master branch to bring in fixes that might help
> fix the unrelated tier1 failures in Github Actions job runs
> - implement review suggestions:
> - reduce the toString() calls by creating a helper
> - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
> - implement review suggestion - move isSelfOrParent to ZipPath class
> - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named
> "." inside
Closing this PR based on the above discussion.
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this proposed fix for
>> https://bugs.openjdk.java.net/browse/JDK-8251329?
>>
>> As noted in that issue, if a zip filesystem created on top of a jar
>> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads
>> to a infinite never ending iteration (which ultimately fails with Java heap
>> space OOM).
>>
>> Alan notes in that issue that:
>>
>>> This is more likely an issue with the zipfs DirectoryStream implementation.
>>> A DirectoryStream is specified to not include elements that for the special
>>> links to the current or parent directory. It should be rare.
>>
>> This indeed turned out to be an issue in the
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls
>> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation. The
>> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream`
>> states, was including the "." and ".." paths in its returned iterator:
>>
>>> The elements returned by the iterator are in no specific order. Some file
>> systems maintain special links to the directory itself and the directory's
>> parent directory. Entries representing these links are not returned by the
>> iterator.
>>
>>
>> The proposed fix in this patch checks the paths for "." and "..", similar to
>> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and
>> skips those paths from being added into the returned iterator. The
>> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been
>> done) is currently only used by
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private
>> visibility, so this change shouldn't impact any other usage/expectations.
>>
>> A new jtreg test has been added to reproduce this issue and verify the fix.
>> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine
>> without any issues after this change. I will be triggering a `tier1` test
>> locally in a while.
>
> Jaikiran Pai 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 four additional
> commits since the last revision:
>
> - Merge latest from upstream master branch to bring in fixes that might help
> fix the unrelated tier1 failures in Github Actions job runs
> - implement review suggestions:
> - reduce the toString() calls by creating a helper
> - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
> - implement review suggestion - move isSelfOrParent to ZipPath class
> - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named
> "." inside
Hello Alan,
> So I think we should close this PR and restart JDK-8251329. @jaikiran Are you
> okay this this?
Yes, this sounds fine to me. I don't have any objection in closing this PR and
I'll do it shortly.
> JDK-8251329 is still assigned to JDK-8251329 and has a patch with the changes
> that we think are the right way for newFileSystem to reject ZIP files
This part I didn't understand. Did you mean to refer some other JBS issue?
Because from what I see in https://bugs.openjdk.java.net/browse/JDK-8251329
there's no patch attached to it (unless of course it's restricted to specific
JBS user roles?).
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v6]
On Fri, 23 Jul 2021 06:49:15 GMT, Matthias Baesken wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Minor adjustments, handling of Unlimited > >> @MBaesken Thanks. We need a solution for [#4518 >> (comment)](https://github.com/openjdk/jdk/pull/4518#issuecomment-882637594) >> though. `--pids-limit=-1` doesn't seem to make it unlimited on all container >> runtimes. For example it fails for me here with: >> >> ``` >> $ docker --version >> Docker version 20.10.6, build 370c289 >> ``` > > Hi Severin, that's a pity and looks like a bug, because the docker > documentation says > https://docs.docker.com/engine/reference/commandline/run/ > > > > > > --pids-limit | | Tune container pids limit (set -1 for unlimited) > -- | -- | -- > > > > > > > Do you have an idea what to set with docker 20 on your setup? I did not find > much about this in the docker 20 release notes > https://docs.docker.com/engine/release-notes/ . > > @MBaesken Thanks. We need a solution for [#4518 > > (comment)](https://github.com/openjdk/jdk/pull/4518#issuecomment-882637594) > > though. `--pids-limit=-1` doesn't seem to make it unlimited on all > > container runtimes. For example it fails for me here with: > > ``` > > $ docker --version > > Docker version 20.10.6, build 370c289 > > ``` > > Hi Severin, that's a pity and looks like a bug, because the docker > documentation says > https://docs.docker.com/engine/reference/commandline/run/ > --pids-limit Tune container pids limit (set -1 for unlimited) > > Do you have an idea what to set with docker 20 on your setup? I did not find > much about this in the docker 20 release notes > https://docs.docker.com/engine/release-notes/ . No, I don't know what to do about it. All I can see it comes back with a pids limit of `38019` when set to `-1`. It does seem like a bug or an intentional setting so as to avoid fork bombs. - PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:08:43 GMT, Lance Andersen wrote:
>> Jaikiran Pai 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 four additional
>> commits since the last revision:
>>
>> - Merge latest from upstream master branch to bring in fixes that might
>> help fix the unrelated tier1 failures in Github Actions job runs
>> - implement review suggestions:
>> - reduce the toString() calls by creating a helper
>> - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
>> - implement review suggestion - move isSelfOrParent to ZipPath class
>> - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named
>> "." inside
>
> Hi Jaikiran,
>
> Consider:
>
>
> try (var os = Files.newOutputStream(ZIPFILE);
> ZipOutputStream zos = new ZipOutputStream(os)) {
> zos.putNextEntry(new ZipEntry("../Hello.txt"));
> zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
> }
>
>
> With your proposed fix, you will only return "/" when you walk the the Zip
> file and we should also return "/Hello.txt"
>
> If. you resolve the path when the Inode entry is created:
>
>` name(new ZipPath(null, normalize(name),
> true).getResolvedPath());`
>
> That should address the issue and also allow you to access the entry.
I discussed this issue with @LanceAndersen and I think we are in agreement that
a zip file system needs to use "." and ".." as links to the current and parent
directories and cannot reliably support entries that have "." and ".." as name
elements. The other options on the table (including Lance's origin prototype
fix and the proposal in this PR) lead to anomalies. So I think we should close
this PR and restart JDK-8251329. @jaikiran Are you okay this this? JDK-8251329
is still assigned to JDK-8251329 and has a patch with the changes that we think
are the right way for newFileSystem to reject ZIP files that have entries in
the CEN that can't be used for files in a file system.
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: [jdk17] RFR: 8271155: Wrong path separator in env variable
On Thu, 22 Jul 2021 19:35:59 GMT, Alexey Semenyuk wrote: > Replace `";"` with `FileUtils::pathSeparator` in the expression adding 'app' > dir to env variable in jpackage app launcher. Is there a test planned for this change or is it covered by an existing test? Just asking as this has been committed to openjdk/jdk17 and wondering what the test coverage is. - PR: https://git.openjdk.java.net/jdk17/pull/271
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v6]
On Thu, 22 Jul 2021 12:18:20 GMT, Matthias Baesken wrote:
>> Hello, please review this PR; it extend the OSContainer API in order to also
>> support the pids controller of cgroups.
>>
>> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct",
>> "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids
>> controller might not be there (or not fully supported) so it was added as
>> optional , see the coding
>>
>>
>> if (!cg_infos[PIDS_IDX]._data_complete) {
>> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
>> // keep the other controller info, pids is optional
>> }
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Minor adjustments, handling of Unlimited
> @MBaesken Thanks. We need a solution for [#4518
> (comment)](https://github.com/openjdk/jdk/pull/4518#issuecomment-882637594)
> though. `--pids-limit=-1` doesn't seem to make it unlimited on all container
> runtimes. For example it fails for me here with:
>
> ```
> $ docker --version
> Docker version 20.10.6, build 370c289
> ```
Hi Severin, that's a pity and looks like a bug, because the docker
documentation says
https://docs.docker.com/engine/reference/commandline/run/
--pids-limit | | Tune container pids limit (set -1 for unlimited)
-- | -- | --
Do you have an idea what to set with docker 20 on your setup? I did not find
much about this in the docker 20 release notes
https://docs.docker.com/engine/release-notes/ .
-
PR: https://git.openjdk.java.net/jdk/pull/4518
