Re: RFR: 8279361: Error in documentation of third Stream.reduce method
On Thu, 29 Sep 2022 16:29:54 GMT, Paul Sandoz wrote: >> This JavaDoc change attempts to shine some light on the `combiner`-function >> as it relates to the third variant of `Stream.reduce` since it according to >> the bug submission in JBS can be confusing that the `combiner` is not >> mentioned in the code example in the documentation. > > The current JavaDoc is not wrong :-) (same in other places). I think what you > propose may not add much clarity (given the complexities of reasoning about > parallelism). > > We could add something after "but is not constrained to execute > sequentially." e.g. > > " > Parallel execution may result in intermediate results computed as if by the > code above. Such results are combined using the combining function to produce > the final result." > " @PaulSandoz That sounds like a fair solution to me, who else can chime in? :) - PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8279361: Error in documentation of third Stream.reduce method
On Fri, 30 Sep 2022 00:44:01 GMT, Stuart Marks wrote: >> This JavaDoc change attempts to shine some light on the `combiner`-function >> as it relates to the third variant of `Stream.reduce` since it according to >> the bug submission in JBS can be confusing that the `combiner` is not >> mentioned in the code example in the documentation. > > One of the problems with pseudo-code is that people can interpret it too > literally. Even though we say things like "the result is as if..." it still > invites confusion. For example, the proposed chunk code invokes the combiner > even in the case where there is only one chunk. However, the clause that > follows says: > >> In the case where there are less than two chunks, invoking the >> combiner is not needed as there is no merging to be done. > > seems to be quasi-normative in that it implies (or does it?) that the > combiner _won't_ be called if there is only a single chunk. While that's true > of the current implementation, the current spec doesn't guarantee it, and I > don't think we want to add such a requirement even though it might be > desirable. (At least, we shouldn't add a requirement without good reason.) > > So there's still a question of whether or not the combiner is called in > sequential cases, and also about its fundamental role in the reduce > operation. I've answered a couple Stack Overflow questions on it: > > https://stackoverflow.com/questions/24308146/why-is-a-combiner-needed-for-reduce-method-that-converts-type-in-java-8/24316429#24316429 > > https://stackoverflow.com/questions/29210176/can-a-collectors-combiner-function-ever-be-used-on-sequential-streams/29295055#29295055 > > (And maybe @amaembo has too. And yes Tagir we're still thinking about ordered > reduce or fold operations.) > > So maybe a prose description of the splitting and combining process would be > preferable. Or possibly a more-pseudo pseudo-code example that doesn't have > as much detail, so people don't read into it so much. @stuart-marks Since there is no contract requiring the use of the `combiner`, all the documentation can say is that "it will be used if the implementation needs it"—which still doesn't answer (deterministically) *when* it will be applied, as even in the parallel case, presuming that the source is small enough not to warrant more than one chunk. I've taken a couple of stabs at trying to word this in a clear manner, but to be honest, perhaps the most confusion-reducing action is to delete the pseudocode altogether? 🤔 - PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8279361: Error in documentation of third Stream.reduce method [v2]
> This JavaDoc change attempts to shine some light on the `combiner`-function > as it relates to the third variant of `Stream.reduce` since it according to > the bug submission in JBS can be confusing that the `combiner` is not > mentioned in the code example in the documentation. Viktor Klang 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. The pull request contains one new commit since the last revision: 8279361 - Error in documentation of third Stream.reduce method Removes the partial pseudo-code which omitted the details regarding the `combiner` as it was deemed confusing. - Changes: - all: https://git.openjdk.org/jdk/pull/10469/files - new: https://git.openjdk.org/jdk/pull/10469/files/9bdd85b7..5f93e779 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10469&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10469&range=00-01 Stats: 47 lines in 4 files changed: 0 ins; 42 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/10469.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10469/head:pull/10469 PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8279361: Error in documentation of third Stream.reduce method
On Fri, 30 Sep 2022 21:34:44 GMT, Stuart Marks wrote: >> This JavaDoc change attempts to shine some light on the `combiner`-function >> as it relates to the third variant of `Stream.reduce` since it according to >> the bug submission in JBS can be confusing that the `combiner` is not >> mentioned in the code example in the documentation. > > Deleting the pseudocode might be the right way to go. To restate the > meta-issue, a problem with pseudocode is that it's difficult to separate the > accidental from the essential. With something that has as much essential > complexity as parallel reduction, it seems like pseudocode introduces a lot > of accidental complexity that can be confusing or misleading. So yeah, if > pseudocode isn't helping, then get rid of it. @stuart-marks I'm inclined to agree with you and I have updated this PR to remove the pseudo-code from all places I found it in./cc @PaulSandoz - PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8279361: Error in documentation of third Stream.reduce method [v3]
> This JavaDoc change attempts to shine some light on the `combiner`-function > as it relates to the third variant of `Stream.reduce` since it according to > the bug submission in JBS can be confusing that the `combiner` is not > mentioned in the code example in the documentation. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Updating the copyright year to 2022 for changes related to issue 8279361 - Changes: - all: https://git.openjdk.org/jdk/pull/10469/files - new: https://git.openjdk.org/jdk/pull/10469/files/5f93e779..83706e0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10469&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10469&range=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10469.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10469/head:pull/10469 PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8297495: j.u.concurrent updates for JDK 20 [v2]
On Thu, 1 Dec 2022 14:46:48 GMT, Alan Bateman wrote:
>> The proposed updates for JDK 20 are:
>>
>> - ForkJoinPool.externalSubmit
>> - ForkJoinWorkerThread.getQueuedTaskCount
>>
>> These methods will be used to improve the Thread.yield implementation for
>> virtual threads. The range of alternatives explored include not exposing an
>> API and protected methods such as "offerSubmission". The class description
>> speaks of "external clients" and "submissions from non-ForkJoinTask
>> clients", hence the proposed naming and javadoc text.
>
> Alan Bateman 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 six additional
> commits since the last revision:
>
> - Expand test
> - Adjust wording to make it clear that the number of tasks is non-negative
> - Merge
> - Improve javadoc
> - Merge
> - Initial commit
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2920:
> 2918: * @since 20
> 2919: */
> 2920: public ForkJoinTask externalSubmit(ForkJoinTask task) {
@AlanBateman Does it make sense to check the nullness of the `task` before
going into the storeStoreFence etc?
-
PR: https://git.openjdk.org/jdk/pull/11319
Re: RFR: 8297495: j.u.concurrent updates for JDK 20 [v2]
On Mon, 5 Dec 2022 10:33:38 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2920:
>>
>>> 2918: * @since 20
>>> 2919: */
>>> 2920: public ForkJoinTask externalSubmit(ForkJoinTask task) {
>>
>> @AlanBateman Does it make sense to check the nullness of the `task` before
>> going into the storeStoreFence etc?
>
> It's only to be the same as poolSubmit.
@AlanBateman Then I presume that there's a good reason for not checking. :)
-
PR: https://git.openjdk.org/jdk/pull/11319
RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
Currently Set.copyOf allocates both a HashSet and a new empty array when the input collection is empty. This patch avoids allocating anything for the case where the parameter collection's isEmpty returns true. - Commit messages: - 8299444: java.util.Set.copyOf allocates needlessly for empty input collections Changes: https://git.openjdk.org/jdk/pull/11847/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299444 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11847.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11847/head:pull/11847 PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Wed, 4 Jan 2023 15:20:10 GMT, Pavel Rappo wrote: >> Currently Set.copyOf allocates both a HashSet and a new empty array when the >> input collection is empty. >> >> This patch avoids allocating anything for the case where the parameter >> collection's isEmpty returns true. > > Curious: how bad was that "needless allocation" that it was required to be > optimized away? @pavelrappo Fair question! I guess it depends on what dominates the benchmark: given that the copyOf methods goes to some length to try to avoid allocating new instances of the immutable sets the proposed patch seemed like rather low-hanging fruit. I found this as I was running some (to be fair, rather specific) benches which drew my attention to it. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Thu, 5 Jan 2023 01:02:11 GMT, Sergey Bylokhov wrote: >> There's no regression test. However, with the current code (prior to this >> change) a call to `Set.of(zeroLengthArray)` returns the same instance as >> `Set.of()`, so it's difficult to write a simple functional test for this >> change. The test would have to assert that "no HashSet is allocated along >> this code path" which is much harder to test and frankly probably isn't >> worth it. So, please add one of the `noreg-*` labels to the bug to indicate >> the rationale for omitting a regression test. >> >> https://openjdk.org/guide/#jbs-label-dictionary >> >> I'd probably add the `Map.copyOf` change to this PR to avoid some >> bug/PR/review overhead. Thanks for mentioning this @shipilev. > >> so it's difficult to write a simple functional test for this change. > > It is possible to track that for some "custom" and empty collection the only > method will be called is `isEmpty` and nothing else. Not sure how it is > useful or not. @mrserb @stuart-marks Indeed, the reason for no regression test is because I didn't see any good way of testing "this operation will not allocate any memory" (which is the externally observable difference, besides having isEmpty being called, which of course could allocate memory (which I sincerely hope no implementations do)). I can add the same logic to Map.copyOf, and possibly even List.copyOf (ImmutableCollections.listCopy). But will do so in additional commits in this PR. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
> Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. Viktor Klang has updated the pull request incrementally with two additional commits since the last revision: - 8299444: java.util.Set.copyOf allocates needlessly for empty input collections Modifies ImmutableCollections.listCopy: Introduces a check for isEmpty to avoid allocation in the case of an empty input collection. - 8299444: java.util.Set.copyOf allocates needlessly for empty input collections Modifies Map.copyOf: Introduces a check for isEmpty to avoid allocation in the case of an empty input Map. - Changes: - all: https://git.openjdk.org/jdk/pull/11847/files - new: https://git.openjdk.org/jdk/pull/11847/files/52a87a2b..8e67eb86 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=00-01 Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11847.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11847/head:pull/11847 PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Thu, 5 Jan 2023 01:02:11 GMT, Sergey Bylokhov wrote: >> There's no regression test. However, with the current code (prior to this >> change) a call to `Set.of(zeroLengthArray)` returns the same instance as >> `Set.of()`, so it's difficult to write a simple functional test for this >> change. The test would have to assert that "no HashSet is allocated along >> this code path" which is much harder to test and frankly probably isn't >> worth it. So, please add one of the `noreg-*` labels to the bug to indicate >> the rationale for omitting a regression test. >> >> https://openjdk.org/guide/#jbs-label-dictionary >> >> I'd probably add the `Map.copyOf` change to this PR to avoid some >> bug/PR/review overhead. Thanks for mentioning this @shipilev. > >> so it's difficult to write a simple functional test for this change. > > It is possible to track that for some "custom" and empty collection the only > method will be called is `isEmpty` and nothing else. Not sure how it is > useful or not. @mrserb @stuart-marks I've pushed two additional commits which adds the isEmpty-check for Map and List as well, although the improvement there should be smaller, especially for Lists. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 17:09:02 GMT, Sergey Bylokhov wrote: >> Viktor Klang has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - 8299444: java.util.Set.copyOf allocates needlessly for empty input >> collections >> >>Modifies ImmutableCollections.listCopy: >>Introduces a check for isEmpty to avoid allocation in the case of an >> empty input collection. >> - 8299444: java.util.Set.copyOf allocates needlessly for empty input >> collections >> >>Modifies Map.copyOf: >>Introduces a check for isEmpty to avoid allocation in the case of an >> empty input Map. > > I wonder how it will affect (if any) the performance of the common path, for > example if non-empty but small ConcurrentHashMap is passed to the > List.copyOf(). @mrserb The "hit" is directly proportional to the cost of `isEmpty()`—but specifically for collections like ConcurrentHashMap, the very notion of "current size" is challenging at best. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 18:34:57 GMT, Per Minborg wrote: >> Viktor Klang has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - 8299444: java.util.Set.copyOf allocates needlessly for empty input >> collections >> >>Modifies ImmutableCollections.listCopy: >>Introduces a check for isEmpty to avoid allocation in the case of an >> empty input collection. >> - 8299444: java.util.Set.copyOf allocates needlessly for empty input >> collections >> >>Modifies Map.copyOf: >>Introduces a check for isEmpty to avoid allocation in the case of an >> empty input Map. > > On the note of `CHM::isEmpty`: It would be better to rewrite this method as a > short-circuitable reduction of the many CounterCells' values. As soon as at > least one of them are >0 then the map is not empty. In contrast, today we sum > all of the values, in many cases, unnecessary. @minborg Yes, I noted that when I looked into the current impl of CHM.isEmpty() as well. Seems like either a bespoke isEmpty implementation, or having isEmpty() rely on a more generalized "isLargerThan(0)" would be appropriate. With that said, I'm hesitant to make any modifications to CHM as a part of this PR :) - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8300647: Miscellaneous hashCode improvements in java.base [v2]
On Thu, 19 Jan 2023 13:46:26 GMT, Claes Redestad wrote: >> Went through the jdk and found a few more places where >> `ArraysSupport::vectorizedHashCode` can be used, and a few where adhoc >> methods could be replaced with a plain call to `java.util.Arrays` >> equivalents. This patch addresses that. >> >> After this, #12068, and #12077 I think we're reaching the limit of sensible >> direct use of `AS::vectorizedHashCode`. I've found a few places outside of >> java.base where `vectorizedHashCode` might be applicable, but none that seem >> important enough to warrant an export of this method outside of java.base. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert ZipFileSystem changes since it's additionally built as a library > dependency and can't rely on jdk.internal src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 415: > 413: } > 414: > 415: // Replace with general purpose method someday @cl4es Nice work addressing this 16+ year old comment! đź‘Ť - PR: https://git.openjdk.org/jdk/pull/12093
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v3]
> Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Removing outdated comment in ImmutableCollections regarding implicit nullchecks - Changes: - all: https://git.openjdk.org/jdk/pull/11847/files - new: https://git.openjdk.org/jdk/pull/11847/files/8e67eb86..b6bfa3f8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11847.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11847/head:pull/11847 PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 18:26:56 GMT, Stuart Marks wrote:
>> Viktor Klang has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
>> collections
>>
>>Modifies ImmutableCollections.listCopy:
>>Introduces a check for isEmpty to avoid allocation in the case of an
>> empty input collection.
>> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
>> collections
>>
>>Modifies Map.copyOf:
>>Introduces a check for isEmpty to avoid allocation in the case of an
>> empty input Map.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 169:
>
>> 167: @SuppressWarnings("unchecked")
>> 168: static List listCopy(Collection coll) {
>> 169: if (coll instanceof List12 || (coll instanceof ListN && !
>> ((ListN)coll).allowNulls)) {
>
> Maybe replace the cast with an instanceof pattern here?
@stuart-marks Sorry, missed this notification. I initially had the same idea,
but decided against it because it forces me to suppress "rawtypes" since `coll
instanceof ListN` is not considered to be a rawtype, but `coll instanceof ListN
c` is. And currently it won't allow for `coll instanceof ListN c`...
-
PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Sun, 22 Jan 2023 15:20:18 GMT, Attila Szegedi wrote:
>> Viktor Klang has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
>> collections
>>
>>Modifies ImmutableCollections.listCopy:
>>Introduces a check for isEmpty to avoid allocation in the case of an
>> empty input collection.
>> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
>> collections
>>
>>Modifies Map.copyOf:
>>Introduces a check for isEmpty to avoid allocation in the case of an
>> empty input Map.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 174:
>
>> 172: return List.of();
>> 173: } else {
>> 174: return (List)List.of(coll.toArray()); // implicit
>> nullcheck of coll
>
> The comment is no longer relevant here, as it now happens on line 171.
@szegedi Nice catch, Attila. Corrected!
-
PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v4]
> Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Switching to pattern matching to avoid raw type + cast in listCopy - Changes: - all: https://git.openjdk.org/jdk/pull/11847/files - new: https://git.openjdk.org/jdk/pull/11847/files/b6bfa3f8..860b0c99 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11847.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11847/head:pull/11847 PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Tue, 24 Jan 2023 11:07:07 GMT, RĂ©mi Forax wrote: >> @stuart-marks Sorry, missed this notification. I initially had the same >> idea, but decided against it because it forces me to suppress "rawtypes" >> since `coll instanceof ListN` is not considered to be a rawtype, but `coll >> instanceof ListN c` is. And currently it won't allow for `coll instanceof >> ListN c`... > > `coll instanceof ListN list` should work. @forax @stuart-marks Yeah, that works. It's unfortunate that it's not possible to match on the actual (generic) type, as then both sides of the || could use type unification to avoid having to do the cast on the return value as well. Submitted a commit to switch to the wildcard version. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v5]
> Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Adding comment clarifying where implicit nullchecks are made in Set and Map copyOf - Changes: - all: https://git.openjdk.org/jdk/pull/11847/files - new: https://git.openjdk.org/jdk/pull/11847/files/860b0c99..9d80411a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11847&range=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11847.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11847/head:pull/11847 PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Tue, 24 Jan 2023 18:00:30 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/java/util/ImmutableCollections.java line 174:
>>
>>> 172: return List.of();
>>> 173: } else {
>>> 174: return (List)List.of(coll.toArray()); // implicit
>>> nullcheck of coll
>>
>> The comment is no longer relevant here, as it now happens on line 171.
>
> Thanks @szegedi for catching this and @viktorklang-ora for fixing it. I like
> having comments like this in cases where we need to throw NPE for null and
> for which there's no explicit `Objects.requireNonNull`. We've had cases in
> the past where an apparently innocuous refactoring postponed an implicit
> nullcheck, which opened the possibility of a side effect occuring before NPE
> was thrown (violates failure idempotency). So I think maintaining such
> comments is important.
>
> With that in mind, for the Set and Map cases, could you (Viktor) add similar
> comments there? Arguably they should have been there already, but, oh well,
> they weren't. Thanks.
@stuart-marks Makes sense. I've added those comments to Set and Map copyOf()
-
PR: https://git.openjdk.org/jdk/pull/11847
Integrated: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Wed, 4 Jan 2023 14:41:20 GMT, Viktor Klang wrote: > Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. This pull request has now been integrated. Changeset: a2a77033 Author:Viktor Klang Committer: Stuart Marks URL: https://git.openjdk.org/jdk/commit/a2a7703370caf07afd88b5cfe44e1a78eed699e9 Stats: 10 lines in 3 files changed: 6 ins; 0 del; 4 mod 8299444: java.util.Set.copyOf allocates needlessly for empty input collections Reviewed-by: rriggs, shade, smarks - PR: https://git.openjdk.org/jdk/pull/11847
RFR: JDK-8300098 : java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java fails with internal timeout when executed with TieredCompilation1/3
The proposed fix by @DougLea ensures that the state transition into waiting is retried in the cases where a previous waiter isn't making progress and a new waiter goes into waiting. - Commit messages: - JDK-8300098 : java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java fails with internal timeout when executed with TieredCompilation1/3 Changes: https://git.openjdk.org/jdk/pull/12319/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12319&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300098 Stats: 12 lines in 1 file changed: 2 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/12319.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12319/head:pull/12319 PR: https://git.openjdk.org/jdk/pull/12319
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset
On Tue, 31 Jan 2023 16:02:07 GMT, Per Minborg wrote:
>> `ZoneOffset` instances are cached by the `ZoneOffset` class itself for
>> values in the range [-18h, 18h] for each second that is on an even quarter
>> of an hour (i.e. at most 2*18*4+1 = 145 values).
>>
>> Instead of using a `ConcurrentHashMap` for caching instanced, we could
>> instead use an `AtomicReferenceArray` with direct slot value access for said
>> even seconds. This will improve performance and reduce the number of object
>> even though the backing array will go from an initial 32 in the CHM to an
>> initial/final 145 in the ARA. The CHM will contain much more objects and
>> array slots for typical numbers of entries in the cache and will compute
>> hash/bucket/collision on the hot code path for each cache access.
>
> src/java.base/share/classes/java/time/ZoneOffset.java line 432:
>
>> 430: if (totalSeconds % (15 * SECONDS_PER_MINUTE) == 0) {
>> 431: int slot = cacheSlot(totalSeconds);
>> 432: ZoneOffset cached = SECONDS_CACHE.get(slot);
>
> I miss `AtomicReferenceArray::computeIfNull` that atomically will compute an
> element if the value at a certain index is `null`.
@minborg You could compareAndExchange in a CompletableFuture—if you succeed you
can complete it with the computation (bonus points since the computation can be
done async) and if you fail you get either a value or a CompletableFuture you
can decide if you want to block on it, and for how long?
-
PR: https://git.openjdk.org/jdk/pull/12346
Re: RFR: 8300917: Regression 2x and bimodal startup on Mac aarch64 in b27
On Wed, 25 Jan 2023 12:26:26 GMT, Per Minborg wrote:
> This PR proposed to reduce contention in synchronized methods mainly by doing
> I/O operations outside synch blocks.
src/java.base/share/classes/java/lang/Module.java line 281:
> 279:
> 280: private static boolean isNativeAccessEnabled(Module target) {
> 281: if (target.enableNativeAccess)
@minborg It'd seem a bit easier to implement using VarHandle and not having to
use `synchronized`? Is the reason that you don't want to init VarHandle in this
class? 🤔
-
PR: https://git.openjdk.org/jdk/pull/12193
Integrated: JDK-8300098 : java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java fails with internal timeout when executed with TieredCompilation1/3
On Tue, 31 Jan 2023 10:45:07 GMT, Viktor Klang wrote: > The proposed fix by @DougLea ensures that the state transition into waiting > is retried in the cases where a previous waiter isn't making progress and a > new waiter goes into waiting. This pull request has now been integrated. Changeset: ecf8842c Author: Viktor Klang Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/ecf8842cd2309210f3d5eee7f9f28a198a860686 Stats: 12 lines in 1 file changed: 2 ins; 2 del; 8 mod 8300098: java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java fails with internal timeout when executed with TieredCompilation1/3 Co-authored-by: Doug Lea Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/12319
RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask
I noticed when looking at the code that there was no real need to use a CHM to perform the tracking of activation in an ordered fashion on ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations and indirection. - Commit messages: - JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask Changes: https://git.openjdk.org/jdk/pull/12320/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8302666 Stats: 25 lines in 1 file changed: 17 ins; 5 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask
On Thu, 16 Feb 2023 15:01:34 GMT, Per Minborg wrote:
>> I noticed when looking at the code that there was no real need to use a CHM
>> to perform the tracking of activation in an ordered fashion on
>> ForEachOrderedTask, but instead a VarHandle can be used, reducing
>> allocations and indirection.
>
> src/java.base/share/classes/java/util/stream/ForEachOps.java line 372:
>
>> 370: private Node node;
>> 371:
>> 372: @SuppressWarnings("unused") private volatile
>> ForEachOrderedTask next; // Only accessed through the NEXT VarHandle
>
> I think the variable can be declared without `volatile` as the operations on
> the `VarHandle` will determine the memory semantics used regardless? So, the
> `NEXT.set()` at L431 might also be replaced with `NEXT.setVolatile()`
@minborg Good point. Reference here:
"Access modes will override any memory ordering effects specified at the
declaration site of a variable. For example, a VarHandle accessing a field
using the get access mode will access the field as specified by its access mode
even if that field is declared volatile. When mixed access is performed extreme
care should be taken since the Java Memory Model may permit surprising results.
" -
https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html
@minborg To clarify, I intended to do a plain write since safe publishing will
be done by `fork()`
-
PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask
On Thu, 16 Feb 2023 17:10:17 GMT, Paul Sandoz wrote: >> I noticed when looking at the code that there was no real need to use a CHM >> to perform the tracking of activation in an ordered fashion on >> ForEachOrderedTask, but instead a VarHandle can be used, reducing >> allocations and indirection. > > src/java.base/share/classes/java/util/stream/ForEachOps.java line 431: > >> 429: // leftChild and rightChild were just created and not >> fork():ed >> 430: // yet so no need for a volatile write >> 431: NEXT.set(leftChild, rightChild); > > Make `next` a plain field and shuffle up the assignment (`leftChild.next = > rightChild`) to occur immediately after construction of the right child task? > (FWIW `addToPendingCount` operates on a volatile field of `CountedCompleter`). @PaulSandoz I'm usually a bit weary of piggybacking if it is not done on the same object, as future reorderings of the code might break that assumption. I wouldn't want to break anything silently so I made a rather conservative change. On which side should I err? :) - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
> I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Write the initial value of the next reference without using the VarHandle - Changes: - all: https://git.openjdk.org/jdk/pull/12320/files - new: https://git.openjdk.org/jdk/pull/12320/files/d07190ab..3b38f942 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=00-01 Stats: 8 lines in 1 file changed: 4 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
On Thu, 16 Feb 2023 22:01:01 GMT, Paul Sandoz wrote: >> @PaulSandoz I'm usually a bit weary of piggybacking if it is not done on the >> same object, as future reorderings of the code might break that assumption. >> I wouldn't want to break anything silently so I made a rather conservative >> change. On which side should I err? :) > > Since as you have said the left and right nodes have yet to be "published" > (outside of their little nest) I think it should be fine to move it above and > next to the constructions. > (Also since `addToPendingCount` has volatile write semantics, via it's > getAndSet, the plain write should not float below that call if that really > matters, and its hard to see the code radically changing in this regard > unless there is a major rewrite than i guess it all has to be carefully > rethought.) @PaulSandoz Sold! Making the changes :) @PaulSandoz Done! - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
On Sat, 18 Feb 2023 02:40:01 GMT, ExE Boss wrote:
>> Viktor Klang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Write the initial value of the next reference without using the VarHandle
>
> src/java.base/share/classes/java/util/stream/ForEachOps.java line 515:
>
>> 513: //
>> 514: @SuppressWarnings("unchecked")
>> 515: var leftDescendant = (ForEachOrderedTask> T>)NEXT.getAndSet(this, null);
>
> I don’t think that this needs a `@SuppressWarnings("unchecked")`, as casts ofÂ
> signature polymorphic return values don’t emit unchecked warnings (unlessÂ
> that changed recently):
> Suggestion:
>
> var leftDescendant = (ForEachOrderedTask)
> NEXT.getAndSet(this, (ForEachOrderedTask) null);
Thanks for having a look at the PR, you're right, the suppression isn't needed.
-
PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v3]
> I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Updating copyright header of ForEachOps.java and removing unnecessary suppression of an unchecked cast. - Changes: - all: https://git.openjdk.org/jdk/pull/12320/files - new: https://git.openjdk.org/jdk/pull/12320/files/3b38f942..866d5a39 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
On Fri, 17 Feb 2023 16:48:27 GMT, Paul Sandoz wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Write the initial value of the next reference without using the VarHandle > > That's a nice find, looks good. (Update the year in the copyright header.) @PaulSandoz Thanks for the review, Paul—I've eliminated an unneeded SuppressWarnings, updated the copyright year, and pushed the update here. 👍 - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor
On Mon, 20 Feb 2023 18:34:17 GMT, Alan Bateman wrote:
> Executors.newSingleThreadExecutor returns a delegating ExecutorService that
> has finalizer to shutdown the underlying TPE when the wrapper is finalizable.
> It goes back to JDK 6 and JDK-6399443. This is the last non-empty finalizer
> in java.base. Removing it will likely lead to bug reports/complaints as the
> current behavior goes back to 2006. So the proposal is to just replace it
> with a Cleaner, trivially done in this case. As part of the changes, I've
> replaced the existing test with a more modern test that exercises more
> scenarios.
test/jdk/java/util/concurrent/Executors/AutoShutdown.java line 133:
> 131: while (!terminated) {
> 132: System.gc();
> 133: terminated = executor.awaitTermination(100,
> TimeUnit.MILLISECONDS);
@AlanBateman Perhaps worth having some upper limit as to how long it will wait
until failing the test?
-
PR: https://git.openjdk.org/jdk/pull/12675
RFR: JDK-8297605 DelayQueue javadoc is confusing
Clarifies the distinction between expiration of the head of DelayQueue and how it relates to `poll`, `take`, and `peek`. See discussion on https://bugs.openjdk.org/browse/JDK-8297605 @DougLea If possible, please weigh in on whether this is in line with your thoughts on the matter. - Commit messages: - JDK-8297605 DelayQueue javadoc is confusing Changes: https://git.openjdk.org/jdk/pull/12727/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12727&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297605 Stats: 14 lines in 1 file changed: 1 ins; 1 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/12727.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12727/head:pull/12727 PR: https://git.openjdk.org/jdk/pull/12727
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
On Fri, 17 Feb 2023 16:48:27 GMT, Paul Sandoz wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Write the initial value of the next reference without using the VarHandle > > That's a nice find, looks good. (Update the year in the copyright header.) @PaulSandoz Could you sponsor this one? 🤔 - PR: https://git.openjdk.org/jdk/pull/12320
Withdrawn: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 09:05:49 GMT, Viktor Klang wrote: > Clarifies the distinction between expiration of the head of DelayQueue and > how it relates to `poll`, `take`, and `peek`. See discussion on > https://bugs.openjdk.org/browse/JDK-8297605 > > @DougLea If possible, please weigh in on whether this is in line with your > thoughts on the matter. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/12727
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 09:05:49 GMT, Viktor Klang wrote: > Clarifies the distinction between expiration of the head of DelayQueue and > how it relates to `poll`, `take`, and `peek`. See discussion on > https://bugs.openjdk.org/browse/JDK-8297605 > > @DougLea If possible, please weigh in on whether this is in line with your > thoughts on the matter. Reopening this as a new PR due to a weird validation error. - PR: https://git.openjdk.org/jdk/pull/12727
RFR: JDK-8297605 DelayQueue javadoc is confusing
Clarifies the distinction between expiration of the head of DelayQueue and how it relates to `poll`, `take`, and `peek`. See discussion on https://bugs.openjdk.org/browse/JDK-8297605 @DougLea If possible, please weigh in on whether this is in line with your thoughts on the matter. - Commit messages: - JDK-8297605 DelayQueue javadoc is confusing Changes: https://git.openjdk.org/jdk/pull/12729/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12729&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297605 Stats: 14 lines in 1 file changed: 1 ins; 1 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/12729.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12729/head:pull/12729 PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 15:36:48 GMT, Viktor Klang wrote: > Clarifies the distinction between expiration of the head of DelayQueue and > how it relates to `poll`, `take`, and `peek`. See discussion on > https://bugs.openjdk.org/browse/JDK-8297605 > > @DougLea If possible, please weigh in on whether this is in line with your > thoughts on the matter. Updated version of #12727 - PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 18:53:35 GMT, Martin Buchholz wrote:
>> Clarifies the distinction between expiration of the head of DelayQueue and
>> how it relates to `poll`, `take`, and `peek`. See discussion on
>> https://bugs.openjdk.org/browse/JDK-8297605
>>
>> @DougLea If possible, please weigh in on whether this is in line with your
>> thoughts on the matter.
>
> src/java.base/share/classes/java/util/concurrent/DelayQueue.java line 54:
>
>> 52: * when its delay has expired. The head of the queue is that
>> 53: * {@code Delayed} element whose delay expired furthest in the
>> 54: * past. If no delay has expired there is no head and {@code poll}
>
> I like introducing the concept of "expired head".
> Aren't we defining "expired head" here, not "head"?
@Martin-Buchholz I read the line before the removed line (The head of
the queue is that {@code Delayed} element whose delay expired furthest in the
past.) as defining that the head will be the thing with the "soonest" expiry,
which is important if one is used to LIFO or FIFO. Any idea on how we could
improve that sentence? 🤔
-
PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing [v2]
> Clarifies the distinction between expiration of the head of DelayQueue and > how it relates to `poll`, `take`, and `peek`. See discussion on > https://bugs.openjdk.org/browse/JDK-8297605 > > @DougLea If possible, please weigh in on whether this is in line with your > thoughts on the matter. Viktor Klang 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 one additional commit since the last revision: JDK-8297605 DelayQueue javadoc is confusing - Changes: - all: https://git.openjdk.org/jdk/pull/12729/files - new: https://git.openjdk.org/jdk/pull/12729/files/6ef82915..df54e8a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12729&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12729&range=00-01 Stats: 519072 lines in 8393 files changed: 252325 ins; 163470 del; 103277 mod Patch: https://git.openjdk.org/jdk/pull/12729.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12729/head:pull/12729 PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)
On Fri, 24 Feb 2023 15:32:34 GMT, Raffaello Giulietti
wrote:
>> I simply extrapolated the behavior from `indexOf(int ch,int fromIndex),
>> which has a similar note:
>>
>> * There is no restriction on the value of {@code fromIndex}. If it
>> * is negative, it has the same effect as if it were zero: this entire
>> * string may be searched. If it is greater than the length of this
>> * string, it has the same effect as if it were equal to the length of
>> * this string: {@code -1} is returned.
>
> This is the behavior of the underlying implementation as well.
Personally I would expect `string.substring(fromIndex, toIndex).indexOf(ch)` to
behave isomorphically to `string.indexOf(ch, fromIndex, toIndex)`
-
PR: https://git.openjdk.org/jdk/pull/12600
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [v2]
On Fri, 24 Feb 2023 16:37:58 GMT, Raffaello Giulietti wrote: >> Then you would also expect `string.substring(fromIndex).indexOf(ch)` to >> behave isomorphically to `string.indexOf(ch, fromIndex)` in current >> releases, right? >> It does not. > > (I guess you are referring to the fact that `substring()` can throw.) @rgiulietti That's surprising, but if it is as you say, then the indexOf with or without the endIndex should behave identically and not throw. - PR: https://git.openjdk.org/jdk/pull/12600
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v4]
> I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. Viktor Klang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Updating copyright header of ForEachOps.java and removing unnecessary suppression of an unchecked cast. - Write the initial value of the next reference without using the VarHandle - JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask - Changes: https://git.openjdk.org/jdk/pull/12320/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=03 Stats: 26 lines in 1 file changed: 17 ins; 6 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 21:20:27 GMT, Martin Buchholz wrote:
> Here's my attempt:
>
> ```
> * An unbounded {@linkplain BlockingQueue blocking queue} of {@link Delayed}
> * elements, in which an element generally becomes eligible for removal when
> its
> * delay has expired.
> *
> * An element is considered expired when its {@code
> * getDelay(TimeUnit.NANOSECONDS)} method would return a value less than or
> * equal to zero.
> *
> * An element is considered the head of the queue if it is the
> * element with the earliest expiration time, whether in the past or the
> future,
> * if there is such an element.
> *
> * An element is considered the expired head of the queue if it is
> * the expired element with the earliest expiration time in the
> * past, if there is such an element.
> * The expired head, when present, is also the head.
> *
> * While this class implements the {@code BlockingQueue} interface, it
> * intentionally violates the general contract of {@code BlockingQueue}, in
> that
> * the following methods disregard the presence of unexpired elements and only
> * ever remove the expired head:
> *
> *
> * {@link #poll()}
> * {@link #poll(long,TimeUnit)}
> * {@link #take()}
> * {@link #remove()}
> *
> *
> * All other methods operate on both expired and unexpired elements. For
> * example, the {@code size} method returns the count of all elements. Method
> * {@link peek()} may return the non-null head even when {@code
> * take()} would block waiting for that element to expire.
> *
> * This queue does not permit null elements.
> ```
I think that approach sounds good. @DougLea, any opinion on this class javadoc
summary?
-
PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 21:20:27 GMT, Martin Buchholz wrote:
>> Clarifies the distinction between expiration of the head of DelayQueue and
>> how it relates to `poll`, `take`, and `peek`. See discussion on
>> https://bugs.openjdk.org/browse/JDK-8297605
>>
>> @DougLea If possible, please weigh in on whether this is in line with your
>> thoughts on the matter.
>
> Here's my attempt:
>
>
> * An unbounded {@linkplain BlockingQueue blocking queue} of {@link Delayed}
> * elements, in which an element generally becomes eligible for removal when
> its
> * delay has expired.
> *
> * An element is considered expired when its {@code
> * getDelay(TimeUnit.NANOSECONDS)} method would return a value less than or
> * equal to zero.
> *
> * An element is considered the head of the queue if it is the
> * element with the earliest expiration time, whether in the past or the
> future,
> * if there is such an element.
> *
> * An element is considered the expired head of the queue if it is
> * the expired element with the earliest expiration time in the
> * past, if there is such an element.
> * The expired head, when present, is also the head.
> *
> * While this class implements the {@code BlockingQueue} interface, it
> * intentionally violates the general contract of {@code BlockingQueue}, in
> that
> * the following methods disregard the presence of unexpired elements and only
> * ever remove the expired head:
> *
> *
> * {@link #poll()}
> * {@link #poll(long,TimeUnit)}
> * {@link #take()}
> * {@link #remove()}
> *
> *
> * All other methods operate on both expired and unexpired elements. For
> * example, the {@code size} method returns the count of all elements. Method
> * {@link peek()} may return the non-null head even when {@code
> * take()} would block waiting for that element to expire.
> *
> * This queue does not permit null elements.
@Martin-Buchholz Martin, how would you like to proceed with your proposed
wording, would you prefer a suggested edit to this PR, do a separate PR, or
otherwise? /cc @AlanBateman (any recommendation, Alan? 🤔 )
-
PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v4]
On Tue, 28 Feb 2023 09:04:21 GMT, ExE Boss wrote: >> Viktor Klang has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains three commits: >> >> - Updating copyright header of ForEachOps.java and removing unnecessary >> suppression of an unchecked cast. >> - Write the initial value of the next reference without using the VarHandle >> - JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask > > src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: > >> 511: // of right subtree (if any, which can be this task's right >> sibling) >> 512: // >> 513: var leftDescendant = (ForEachOrderedTask> T>)NEXT.getAndSet(this, null); > > Casting the `null` is required for the resolved method descriptor to be > `(ForEachOrderedTask, ForEachOrderedTask)ForEachOrderedTask` instead of > `(ForEachOrderedTask, Object)ForEachOrderedTask`, which prevents unnecessary > type conversion `LambdaForm`s from being introduced and allows > [`VarHandle::withInvokeExactBehavior`] to be used: > Suggestion: > > var leftDescendant = (ForEachOrderedTask) > NEXT.getAndSet(this, (ForEachOrderedTask) null); > > > [`VarHandle::withInvokeExactBehavior`]: > https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#withInvokeExactBehavior() @ExE-Boss Ah, sorry, it was meant to be there. :) - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v5]
> I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. Viktor Klang 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. The pull request contains one new commit since the last revision: Updating copyright header of ForEachOps.java and removing unnecessary suppression of an unchecked cast. - Changes: - all: https://git.openjdk.org/jdk/pull/12320/files - new: https://git.openjdk.org/jdk/pull/12320/files/b4b241e2..a5c6b0d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Mon, 27 Feb 2023 20:09:12 GMT, Martin Buchholz wrote:
>> Here's my attempt:
>>
>>
>> * An unbounded {@linkplain BlockingQueue blocking queue} of {@link Delayed}
>> * elements, in which an element generally becomes eligible for removal when
>> its
>> * delay has expired.
>> *
>> * An element is considered expired when its {@code
>> * getDelay(TimeUnit.NANOSECONDS)} method would return a value less than or
>> * equal to zero.
>> *
>> * An element is considered the head of the queue if it is the
>> * element with the earliest expiration time, whether in the past or the
>> future,
>> * if there is such an element.
>> *
>> * An element is considered the expired head of the queue if it
>> is
>> * the expired element with the earliest expiration time in the
>> * past, if there is such an element.
>> * The expired head, when present, is also the head.
>> *
>> * While this class implements the {@code BlockingQueue} interface, it
>> * intentionally violates the general contract of {@code BlockingQueue}, in
>> that
>> * the following methods disregard the presence of unexpired elements and
>> only
>> * ever remove the expired head:
>> *
>> *
>> * {@link #poll()}
>> * {@link #poll(long,TimeUnit)}
>> * {@link #take()}
>> * {@link #remove()}
>> *
>> *
>> * All other methods operate on both expired and unexpired elements. For
>> * example, the {@code size} method returns the count of all elements.
>> Method
>> * {@link peek()} may return the non-null head even when {@code
>> * take()} would block waiting for that element to expire.
>> *
>> * This queue does not permit null elements.
>
>> @Martin-Buchholz Martin, how would you like to proceed with your proposed
>> wording, would you prefer a suggested edit to this PR, do a separate PR, or
>> otherwise? /cc @AlanBateman (any recommendation, Alan? thinking )
>
> Talked me into it - I will dust off my github/skara skillz and make a new PR.
>
>
> I wonder if there's now a way to override javadoc for remove() without
> creating a new method body.
@Martin-Buchholz I guess `remove` could be overridden and just delegate to
`super`?
-
PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v5]
On Tue, 28 Feb 2023 16:11:25 GMT, Paul Sandoz wrote: >> Viktor Klang 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. The pull request contains one >> new commit since the last revision: >> >> Updating copyright header of ForEachOps.java and removing unnecessary >> suppression of an unchecked cast. > > src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: > >> 511: // of right subtree (if any, which can be this task's right >> sibling) >> 512: // >> 513: var leftDescendant = (ForEachOrderedTask> T>)NEXT.getAndSet(this, (ForEachOrderedTask)null); > > The reference cast on the argument is not required. `VarHandle`'s by default > have `MethodHandle` invoke semantics (but without the throwing Throwable): > > https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#invoke > > VarHandle's have been engineered so such reference casts on the arguments can > be optimized away. This makes them much easier to use than MethodHandles. @PaulSandoz Ah, that's great wisdom right there. I'll remove the cast on the argument. đź‘Ť - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing [v2]
On Fri, 24 Feb 2023 16:08:57 GMT, Viktor Klang wrote: >> Clarifies the distinction between expiration of the head of DelayQueue and >> how it relates to `poll`, `take`, and `peek`. See discussion on >> https://bugs.openjdk.org/browse/JDK-8297605 >> >> @DougLea If possible, please weigh in on whether this is in line with your >> thoughts on the matter. > > Viktor Klang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > JDK-8297605 DelayQueue javadoc is confusing Perhaps @pavelrappo has any suggestion for adding clarifying Javadoc to a subclass without having to override the method? 🤔 - PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v6]
> I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Removing unnecessary cast of argument to VarHandle getAndSet - Changes: - all: https://git.openjdk.org/jdk/pull/12320/files - new: https://git.openjdk.org/jdk/pull/12320/files/a5c6b0d0..2c8f0ab4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 2 Mar 2023 02:03:51 GMT, Martin Buchholz wrote: >>> @Martin-Buchholz Martin, how would you like to proceed with your proposed >>> wording, would you prefer a suggested edit to this PR, do a separate PR, or >>> otherwise? /cc @AlanBateman (any recommendation, Alan? thinking ) >> >> Talked me into it - I will dust off my github/skara skillz and make a new >> PR. >> >> I wonder if there's now a way to override javadoc for remove() without >> creating a new method body. > >> I wonder if there's now a way to override javadoc for remove() without >> creating a new method body. > > I thought recent javadoc features might have been useful here, but I scanned > the results from this jql: > > > subcomponent in ( "javadoc(tool)", "doclet" ) AND (resolution in ( Fixed, > Approved ) ) AND issuetype in (JEP, Enhancement) ORDER BY resolved DESC > > > and came up empty @Martin-Buchholz @pavelrappo OTOH I see that DelayQueue *has already* overridden `remove(Object o)` so you should be able to modify that? - PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: 8297605: DelayQueue javadoc is confusing
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Thu, 2 Mar 2023 20:00:56 GMT, Martin Buchholz wrote: > Inviting @DougLea and @viktorklang-ora to review. > > As usual, I couldn't resist more fiddling. > - Added missing tests for take, remove(), remove(Object). > - Improved DelayQueueTest's testing infrastructure > - added more test assertions > - linkified new javadoc definitions @Martin-Buchholz I think the updated documentation is clearer. We need some extra reviewers like @AlanBateman or @pron to sign off :) - PR: https://git.openjdk.org/jdk/pull/12838
RFR: JDK-8302360 Atomic*.compareAndExchange Javadoc unclear
I think the following proposal is very non-invasive and merely draws attention to the fact that "witness value" is a specific term related to the notion of these atomic methods. It's a small change which I think provides additional clarity, see JBS for the discussion on the topic. - Commit messages: - JDK-8302360 : Adding emphasis to the witness value term in the JavaDoc Changes: https://git.openjdk.org/jdk/pull/12880/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12880&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8302360 Stats: 34 lines in 8 files changed: 0 ins; 0 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/12880.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12880/head:pull/12880 PR: https://git.openjdk.org/jdk/pull/12880
Re: RFR: JDK-8302360 Atomic*.compareAndExchange Javadoc unclear [v2]
> I think the following proposal is very non-invasive and merely draws > attention to the fact that "witness value" is a specific term related to the > notion of these atomic methods. > > It's a small change which I think provides additional clarity, see JBS for > the discussion on the topic. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Reverting wording change to AtomicReference compareAndExchange - Changes: - all: https://git.openjdk.org/jdk/pull/12880/files - new: https://git.openjdk.org/jdk/pull/12880/files/923d23d0..d6d2e81b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12880&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12880&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12880.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12880/head:pull/12880 PR: https://git.openjdk.org/jdk/pull/12880
Re: RFR: JDK-8302360 Atomic*.compareAndExchange Javadoc unclear [v2]
On Tue, 7 Mar 2023 02:43:42 GMT, David Holmes wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reverting wording change to AtomicReference compareAndExchange > > src/java.base/share/classes/java/util/concurrent/atomic/AtomicReference.java > line 357: > >> 355: * @param newValue the new value >> 356: * @return the witness value, which is the current value >> 357: * at the time of the operation. > > This change is not appropriate as discussed in JBS. And unclear why you > changed only this version? Leftover? @dholmes-ora Good catch, will fix. - PR: https://git.openjdk.org/jdk/pull/12880
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Tue, 7 Mar 2023 03:53:59 GMT, Martin Buchholz wrote: >>> @Martin-Buchholz @pavelrappo OTOH I see that DelayQueue _has already_ >>> overridden `remove(Object o)` so you should be able to modify that? >> >> Right. But remove(Object) unlike remove() doesn't consider the expiration >> time. Confusing! > >> Right. But remove(Object) unlike remove() doesn't consider the expiration >> time. Confusing! > > Actually, confusion extends to **three** methods with the same name: > - `Queue.remove()` > - `Collection.remove(Object)` > - `Iterator.remove()` @Martin-Buchholz Oh *that* remove() :) - PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v2]
On Fri, 17 Feb 2023 16:48:27 GMT, Paul Sandoz wrote: >> Viktor Klang has refreshed the contents of this pull request, and previous >> commits have been removed. Incremental views are not available. > > That's a nice find, looks good. (Update the year in the copyright header.) @PaulSandoz Ready to integrate? 🤔 - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302360 Atomic*.compareAndExchange Javadoc unclear [v2]
On Tue, 7 Mar 2023 10:23:52 GMT, Viktor Klang wrote: >> I think the following proposal is very non-invasive and merely draws >> attention to the fact that "witness value" is a specific term related to the >> notion of these atomic methods. >> >> It's a small change which I think provides additional clarity, see JBS for >> the discussion on the topic. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Reverting wording change to AtomicReference compareAndExchange @AlanBateman Who would be willing to sponsor this? 🤔 - PR: https://git.openjdk.org/jdk/pull/12880
Integrated: JDK-8302360 Atomic*.compareAndExchange Javadoc unclear
On Mon, 6 Mar 2023 13:23:59 GMT, Viktor Klang wrote: > I think the following proposal is very non-invasive and merely draws > attention to the fact that "witness value" is a specific term related to the > notion of these atomic methods. > > It's a small change which I think provides additional clarity, see JBS for > the discussion on the topic. This pull request has now been integrated. Changeset: d06308c5 Author:Viktor Klang Committer: Martin Buchholz URL: https://git.openjdk.org/jdk/commit/d06308c54a6f3782565eae343778436013205e21 Stats: 33 lines in 8 files changed: 0 ins; 0 del; 33 mod 8302360: Atomic*.compareAndExchange Javadoc unclear Reviewed-by: martin, dholmes - PR: https://git.openjdk.org/jdk/pull/12880
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v6]
On Thu, 9 Mar 2023 16:05:32 GMT, Paul Sandoz wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing unnecessary cast of argument to VarHandle getAndSet > > Marked as reviewed by psandoz (Reviewer). @PaulSandoz Perhaps the bot needs another sponsor instruction? 🤔 - PR: https://git.openjdk.org/jdk/pull/12320
Integrated: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask
On Tue, 31 Jan 2023 10:57:58 GMT, Viktor Klang wrote: > I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. This pull request has now been integrated. Changeset: f2a36b4b Author:Viktor Klang Committer: Paul Sandoz URL: https://git.openjdk.org/jdk/commit/f2a36b4b529b1d74ca38633244dda092a15d50ac Stats: 26 lines in 1 file changed: 17 ins; 6 del; 3 mod 8302666: Replace CHM with VarHandle in ForeachOrderedTask Reviewed-by: psandoz - PR: https://git.openjdk.org/jdk/pull/12320
RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
Addresses the situation where exceptional completion of `orTimeout`:ed CompletableFutures wouldn't cancel the timeout task which could lead to memory leaks if done frequently enough with long enough timeout durations. Fix discussed with @DougLea - Commit messages: - Adding a regression test for 8303742 - Addressing JDK-8303742 by removing the check for a null exception in Canceller Changes: https://git.openjdk.org/jdk/pull/13059/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303742 Stats: 50 lines in 2 files changed: 49 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13059.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13059/head:pull/13059 PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v2]
> Addresses the situation where exceptional completion of `orTimeout`:ed > CompletableFutures wouldn't cancel the timeout task which could lead to > memory leaks if done frequently enough with long enough timeout durations. > > Fix discussed with @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/13059/files - new: https://git.openjdk.org/jdk/pull/13059/files/d46a6bed..15e8fcc2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13059.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13059/head:pull/13059 PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v2]
On Thu, 16 Mar 2023 20:51:29 GMT, Pavel Rappo wrote:
>> Viktor Klang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update
>> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java
>>
>> Co-authored-by: Andrey Turbanov
>
> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java
> line 44:
>
>> 42: void testOrTimeoutWithCompleteExceptionallyDoesNotLeak() throws
>> Exception {
>> 43: var startTime = System.currentTimeMillis();
>> 44: var testRunTime = Duration.ofSeconds(10).toMillis();
>
> This "create completable futures in a loop for t seconds" seems a bit
> brittle. Would 10 or 20 seconds be enough for a typical test machine to fail
> with OOME? Could this be improved by requiring a minimum number of CF
> instances to be created? Maybe finishing when t seconds have elapsed _and_ n
> instances have been created.
>
> Separately, as with any timeouts, consider a monotonic clock.
@pavelrappo Both good points—I'm updating it to use nanoTime + requiring 5M
iterations of the loop to be safer.
-
PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v3]
> Addresses the situation where exceptional completion of `orTimeout`:ed > CompletableFutures wouldn't cancel the timeout task which could lead to > memory leaks if done frequently enough with long enough timeout durations. > > Fix discussed with @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Making the test for orTimeout+completeExceptionally only based on iterations and not duration. - Changes: - all: https://git.openjdk.org/jdk/pull/13059/files - new: https://git.openjdk.org/jdk/pull/13059/files/15e8fcc2..05619280 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=01-02 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13059.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13059/head:pull/13059 PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v3]
On Thu, 16 Mar 2023 20:51:29 GMT, Pavel Rappo wrote:
>> Viktor Klang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Making the test for orTimeout+completeExceptionally only based on
>> iterations and not duration.
>
> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java
> line 44:
>
>> 42: void testOrTimeoutWithCompleteExceptionallyDoesNotLeak() throws
>> Exception {
>> 43: var startTime = System.currentTimeMillis();
>> 44: var testRunTime = Duration.ofSeconds(10).toMillis();
>
> This "create completable futures in a loop for t seconds" seems a bit
> brittle. Would 10 or 20 seconds be enough for a typical test machine to fail
> with OOME? Could this be improved by requiring a minimum number of CF
> instances to be created? Maybe finishing when t seconds have elapsed _and_ n
> instances have been created.
>
> Separately, as with any timeouts, consider a monotonic clock.
@pavelrappo After some extra thought I have skipped the duration-need and
instead determined the threshold for OOME and added a loop counter that counts
to 4x that to make sure if it fails it fails.
-
PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v4]
> Addresses the situation where exceptional completion of `orTimeout`:ed > CompletableFutures wouldn't cancel the timeout task which could lead to > memory leaks if done frequently enough with long enough timeout durations. > > Fix discussed with @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Adding a test for completeOnTimeout cancelling tasks when completeExceptionally - Changes: - all: https://git.openjdk.org/jdk/pull/13059/files - new: https://git.openjdk.org/jdk/pull/13059/files/05619280..e2c1c366 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=02-03 Stats: 13 lines in 1 file changed: 12 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13059.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13059/head:pull/13059 PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v4]
On Fri, 17 Mar 2023 08:55:39 GMT, Viktor Klang wrote: >> Addresses the situation where exceptional completion of `orTimeout`:ed >> CompletableFutures wouldn't cancel the timeout task which could lead to >> memory leaks if done frequently enough with long enough timeout durations. >> >> Fix discussed with @DougLea > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding a test for completeOnTimeout cancelling tasks when > completeExceptionally test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java line 2: > 1: /* > 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. @jaikiran @pavelrappo Copyright year fixed as well đź‘Ť - PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v3]
On Fri, 17 Mar 2023 06:34:03 GMT, Jaikiran Pai wrote:
>> Viktor Klang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Making the test for orTimeout+completeExceptionally only based on
>> iterations and not duration.
>
> Hello Viktor, the changes look good to me. Looking at the code, I see that
> this issue will also affect the `completeOnTimeout(...)` method on the
> `CompletableFuture`. Would you want to enhance the test to include a test for
> this method too? The following test method which tests this
> `completeOnTimeout()` reproduces the leak (and thus the OOM) without your fix:
>
>
> @Test
> void testCompleteOnTimeoutWithCompleteExceptionallyDoesNotLeak() {
> var count = 0L;
> while(count < 2_000_000) {
> new CompletableFuture<>().completeOnTimeout(null, 12,
> TimeUnit.HOURS).completeExceptionally(new RuntimeException("This is fine"));
> ++count;
>
> }
> }
@jaikiran Excellent catch, you're right—I've added such a test case as well to
this PR. đź‘Ť
-
PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v5]
> Addresses the situation where exceptional completion of `orTimeout`:ed > CompletableFutures wouldn't cancel the timeout task which could lead to > memory leaks if done frequently enough with long enough timeout durations. > > Fix discussed with @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: while-statement formatting update - Changes: - all: https://git.openjdk.org/jdk/pull/13059/files - new: https://git.openjdk.org/jdk/pull/13059/files/e2c1c366..71b67aea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13059.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13059/head:pull/13059 PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v6]
> Addresses the situation where exceptional completion of `orTimeout`:ed > CompletableFutures wouldn't cancel the timeout task which could lead to > memory leaks if done frequently enough with long enough timeout durations. > > Fix discussed with @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Reformatting the while-body to be more legible in diffs, and inlining the counter incrementation. - Changes: - all: https://git.openjdk.org/jdk/pull/13059/files - new: https://git.openjdk.org/jdk/pull/13059/files/71b67aea..80680bee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13059&range=04-05 Stats: 8 lines in 1 file changed: 2 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13059.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13059/head:pull/13059 PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v5]
On Fri, 17 Mar 2023 09:21:48 GMT, Alan Bateman wrote:
>> Viktor Klang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> while-statement formatting update
>
> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java
> line 58:
>
>> 56: while (count < 2_000_000) {
>> 57: new CompletableFuture<>().completeOnTimeout(null, 12,
>> TimeUnit.HOURS).completeExceptionally(new RuntimeException("This is fine"));
>> 58: ++count;
>
> Doing 2m iterations vs. original test to run for 10s is fine, assuming there
> is a OOME with a 128Mb heap. If I were editing this test then I'd probably
> split the overly long lines to make future side-by-side diffs easier to look
> at.
@AlanBateman Indeed—I was able to get the problem reproduced at way lower
(several factors) number of allocations for this test, so I purposely made the
final number much higher to really make sure it'd trigger if it was broken.
I'll reformat the while-body to be a bit easier on the eye for diffs. đź‘Ť
-
PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v6]
On Fri, 17 Mar 2023 09:52:51 GMT, Viktor Klang wrote: >> Addresses the situation where exceptional completion of `orTimeout`:ed >> CompletableFutures wouldn't cancel the timeout task which could lead to >> memory leaks if done frequently enough with long enough timeout durations. >> >> Fix discussed with @DougLea > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Reformatting the while-body to be more legible in diffs, and inlining the > counter incrementation. I just synced up with @DougLea—I'll let him decide how to handle it. :) - PR: https://git.openjdk.org/jdk/pull/13059
Integrated: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Thu, 16 Mar 2023 13:37:10 GMT, Viktor Klang wrote: > Addresses the situation where exceptional completion of `orTimeout`:ed > CompletableFutures wouldn't cancel the timeout task which could lead to > memory leaks if done frequently enough with long enough timeout durations. > > Fix discussed with @DougLea This pull request has now been integrated. Changeset: ded6a813 Author:Viktor Klang Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/ded6a8131970ac2f7ae59716769e6f6bae3b809a Stats: 64 lines in 2 files changed: 63 ins; 0 del; 1 mod 8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: 8266571: Sequenced Collections
On Tue, 8 Feb 2022 17:23:38 GMT, Stuart Marks wrote:
> PR for Sequenced Collections implementation.
src/java.base/share/classes/java/util/LinkedHashMap.java line 384:
> 382: return this.put(k, v);
> 383: } finally {
> 384: putMode = PUT_NORM;
@stuart-marks Would it be an alternative to have an `internalPut(mode, k, v)`
so there is no need to have an internal variable which needs to be read/written
multiple time per operation? 🤔
src/java.base/share/classes/java/util/SequencedCollection.java line 93:
> 91: * underlying collection. Changes to the underlying collection might
> or might not be visible
> 92: * in this reversed view, depending upon the implementation.
> 93: *
@stuart-marks Perhaps an opportunity to spec it so that x ==
x.reversed().reversed() (i.e. unwrap on double-reverse)?
src/java.base/share/classes/java/util/SequencedCollection.java line 155:
> 153: */
> 154: default E getLast() {
> 155: return this.reversed().iterator().next();
@stuart-marks Are these default implementation expected to be used (actually)
in the JDK? From a performance PoV, it might make sense to not have default
implementations unless strictly needed, and instead keep the code in the
JavaDoc as a guideline for "worst-case" performance profile. 🤔
-
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r987925904
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r986962055
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r987216743
Re: RFR: 8266571: Sequenced Collections
On Tue, 4 Oct 2022 14:43:44 GMT, Viktor Klang wrote: >> PR for Sequenced Collections implementation. > > src/java.base/share/classes/java/util/SequencedCollection.java line 93: > >> 91: * underlying collection. Changes to the underlying collection might >> or might not be visible >> 92: * in this reversed view, depending upon the implementation. >> 93: * > > @stuart-marks Perhaps an opportunity to spec it so that x == > x.reversed().reversed() (i.e. unwrap on double-reverse)? @stuart-marks What are examples of cases where it wouldn't be reasonable: https://github.com/openjdk/jdk/pull/7387/files#diff-2b261a88d9ed30893d24186a36ef500d284d82e8f345591fc6c44ed5ae15da4fR161 ? - PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r988048079
RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
Improves the stability of the memory leak test for CompletableFuture timeout cancellation by both reducing the count by 50% (which should still be above threshold to trigger given the ample margin set initially) as well as extending the default timeout of the test run. - Commit messages: - Hardening the memory leak test for CompletableFuture timeout cancellation Changes: https://git.openjdk.org/jdk/pull/13116/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13116&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303742 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13116.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13116/head:pull/13116 PR: https://git.openjdk.org/jdk/pull/13116
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 11:02:00 GMT, Jaikiran Pai wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java > line 28: > >> 26: * @bug 8303742 >> 27: * @summary CompletableFuture.orTimeout can leak memory if completed >> exceptionally >> 28: * @run junit/othervm/timeout=1000 -Xmx128m >> CompletableFutureOrTimeoutExceptionallyTest > > Hello Viktor, a timeout of 1000 seconds is 16 minutes. This timeout value is > then multiplied by the timeout factor (which is a jtreg option) to decide the > final timeout. The CI on which this was reported use a timeout factor of 4. > So this would translate to a timeout of 64 minutes, which I think is > extremely high (although it's just the upper bound). > > Perhaps you could increase this timeout to maybe 5 minutes (300 seconds) and > run some tests on our CI to see if that is sufficient? @jaikiran Having a long timeout doesn't seem like a problem given that it just needs enough time to run through the iterations (i.e. that's the max duration of the test before giving up). @AlanBateman That's a great observation—do you see anything about this which would impact whether it makes sense to run on top of a debug-build (I don't see any). If it doesn't need to run on a debug build then I'll add the requirement you suggested. Let me know. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1143522447
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 11:56:57 GMT, Alan Bateman wrote: >> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java >> line 28: >> >>> 26: * @bug 8303742 >>> 27: * @summary CompletableFuture.orTimeout can leak memory if completed >>> exceptionally >>> 28: * @run junit/othervm/timeout=1000 -Xmx128m >>> CompletableFutureOrTimeoutExceptionallyTest >> >> Hello Viktor, a timeout of 1000 seconds is 16 minutes. This timeout value is >> then multiplied by the timeout factor (which is a jtreg option) to decide >> the final timeout. The CI on which this was reported use a timeout factor of >> 4. So this would translate to a timeout of 64 minutes, which I think is >> extremely high (although it's just the upper bound). >> >> Perhaps you could increase this timeout to maybe 5 minutes (300 seconds) and >> run some tests on our CI to see if that is sufficient? > > If there isn't any value running this test with a debug build then you could > have it skip (`@requires vm.debug == true`) to save resources. @AlanBateman Wouldn't that be `@requires vm.debug == false` ? - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1143541565
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Wed, 22 Mar 2023 06:20:55 GMT, Jaikiran Pai wrote: >> @AlanBateman Wouldn't that be `@requires vm.debug == false` ? > > Hello Viktor, > >> @jaikiran Having a long timeout doesn't seem like a problem given that it >> just needs enough time to run through the iterations (i.e. that's the max >> duration of the test before giving up). > > Having a extremely long timeout of T1 secods for a test which one can > (reasonably) expect to finish in (T1 - X) seconds can mean that when/if it > genuinely times out, then it holds on to the limited shared resources (like > the host machine) for those X seconds longer instead of potentially letting > other tests run during that period. So I think it's always better to have a > reasonable timeout instead of an extremely large one - a value past which you > know that if the test is still running then it's surely be a sign that it > should no longer continue to run. > Typically the timeout for such tests is decided by running the test against > the hosts/environment where it failed and gathering data to see how long it > usually takes to finish successfully on those and then adding some extra > seconds to it. @jaikiran The downside is that it is hard to judge how long it will take for machine X to execute Y instructions. :/ - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1144646434
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Tue, 21 Mar 2023 11:02:52 GMT, Jaikiran Pai wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > The PR seems to be using an incorrect JBS issue id/summary. This PR should be > linked with the new https://bugs.openjdk.org/browse/JDK-8304557 issue. @jaikiran Yes, good catch, have updated the title. - PR Comment: https://git.openjdk.org/jdk/pull/13116#issuecomment-1479384471
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Wed, 22 Mar 2023 13:02:46 GMT, Pavel Rappo wrote: >> @jaikiran The downside is that it is hard to judge how long it will take for >> machine X to execute Y instructions. :/ > > 1. Is there a way to mark this test as skipped if the timeout elapses? I > suggest we don't mark it as skipped based on the build or any other > configuration, but actaully run it and see if it fails or succeeds. The > rationale is this: if the test is still running when the time is out, it > means that the test is inconclusive. > 2. Can we reduce memory configuration to conserve system resources? Say, set > `-Xmx` to 96 or even 64 megabytes. Or is so little / unusual that we'll risk > getting other issues on modern JVMs? > 3. I don't see why we need to reduce the number of iterations. What we should > do instead is compute it, otherwise that number, whatever it is, looks magic. > There needs to be some rationale behing that number, even if that rationale > is not accurate. Say, we can reasonably assume that overhead for one > `CompletableFuture` is at least one byte. So, to fill up `N` megabyte(s) of > heap we would need `N * 1_048_576` iterations. 1. I don't know. 2. It's hard to come up with a one-size-fits-all-heap-size given that differences in pointersizes, object alignment, etc can skew the need. 3. I initially set the threshold (iteration number) at about 4x what I needed locally to make it fail, just to have margin in case memory-usage-per-object would vary with the different setups being tested. The other alternative I see would be to reach into the implementation of CompletableFuture's `Delayer`'s `ScheduledThreadPoolExecutor delayer` and make sure that it's `getQueue()` eventually goes empty. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1145069912
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Wed, 22 Mar 2023 16:29:21 GMT, Pavel Rappo wrote: >> 1. I don't know. >> 2. It's hard to come up with a one-size-fits-all-heap-size given that >> differences in pointersizes, object alignment, etc can skew the need. >> 3. I initially set the threshold (iteration number) at about 4x what I >> needed locally to make it fail, just to have margin in case >> memory-usage-per-object would vary with the different setups being tested. >> >> >> The other alternative I see would be to reach into the implementation of >> CompletableFuture's `Delayer`'s `ScheduledThreadPoolExecutor delayer` and >> make sure that it's `getQueue()` eventually goes empty. > >> The other alternative I see would be to reach into the implementation of >> CompletableFuture's `Delayer`'s `ScheduledThreadPoolExecutor delayer` and >> make sure that it's `getQueue()` eventually goes empty. > > From what I've seen, JDK prefers blackbox tests to whitebox tests. Even > though the latter might conserve resources better and are easier to > implement, they are problematic in the long run: they become an obstacle if > the system under tests is modified. @pavelrappo I agree with that. I have the same experience. With that said, I have implemented a whitebox test which reaches in an monitors the task queue of the Delayer. @AlanBateman & @jaikiran any preference here? - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1145134477
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v2]
> Improves the stability of the memory leak test for CompletableFuture timeout > cancellation by both reducing the count by 50% (which should still be above > threshold to trigger given the ample margin set initially) as well as > extending the default timeout of the test run. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Changing the test to monitor the internals instead of trying to provoke an OOME - Changes: - all: https://git.openjdk.org/jdk/pull/13116/files - new: https://git.openjdk.org/jdk/pull/13116/files/a135c253..4a5dd8c8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13116&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13116&range=00-01 Stats: 32 lines in 1 file changed: 17 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/13116.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13116/head:pull/13116 PR: https://git.openjdk.org/jdk/pull/13116
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v2]
On Thu, 23 Mar 2023 10:35:15 GMT, Jaikiran Pai wrote: >> @pavelrappo I agree with that. I have the same experience. With that said, I >> have implemented a whitebox test which reaches in an monitors the task queue >> of the Delayer. @AlanBateman & @jaikiran any preference here? > > Hello Viktor, when you say whitebox test, do you mean adding: > > > @modules java.base/java.util.concurrent:open > > to the jtreg test definition and then using reflection within the test case > to assert on the internals? > > The original bug which introduced this test was about the `CompletableFuture` > implementation unnecessarily holding on to an internal `Future` even after > its use was done. So in order to test the fix effectively, if it means adding > a `:open` to the module's package, solely in this test, then I think it's OK. > I see there are existing test cases which have similar usages. @jaikiran Yes, see the new commit I pushed which illustrates the whitebox solution (which will also run dramatically faster than the old one) which relies on the implementation detail on how the timeouts are scheduled. @AlanBateman @pavelrappo Does the new approach seem acceptable? 🤔 - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1146054535
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v2]
On Thu, 23 Mar 2023 12:32:58 GMT, Pavel Rappo wrote: >>> @AlanBateman @pavelrappo Does the new approach seem acceptable? >> >> I don't have a strong opinion on this. The advantage is that the test is >> probably sub-second as it just needs one CF to complete exceptionally. The >> disadvantage is that it the test would need to be updated if the >> implementation changes, but it's not hard. >> >> So good with the approach if you want. As Jai mentioned, you can use >> @modules to open java.base/java.util.concurrent for deep reflectively. I >> would probably trim the overly long lines as they currently wrap when >> looking at the diffs. > > While the approach seems okay, it might be for the JSR 166 folk to make the > final decision on it. > > (Looking at Thread.sleep(). Sigh: unfortunately, BlockingQueue does not > provide an option to examine its head that blocks or times out.) @pavelrappo `peek(timeout, unit)` would be nice :) @AlanBateman Sounds good, I'll change to use @modules đź‘Ť - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1146160066
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v3]
On Thu, 23 Mar 2023 12:04:34 GMT, Alan Bateman wrote: >> If there isn't any value running this test with a debug build then you could >> have it skip (`@requires vm.debug == false`) to save resources. >> (fixed typo in original message) > >> @AlanBateman @pavelrappo Does the new approach seem acceptable? > > I don't have a strong opinion on this. The advantage is that the test is > probably sub-second as it just needs one CF to complete exceptionally. The > disadvantage is that it the test would need to be updated if the > implementation changes, but it's not hard. > > So good with the approach if you want. As Jai mentioned, you can use @modules > to open java.base/java.util.concurrent for deep reflectively. I would > probably trim the overly long lines as they currently wrap when looking at > the diffs. @AlanBateman @jaikiran @pavelrappo I've updated the code to use an "@modules" (thanks for the hint!) and reformatted the code slightly. I think we should be good to go here. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1146258760
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v3]
> Improves the stability of the memory leak test for CompletableFuture timeout > cancellation by both reducing the count by 50% (which should still be above > threshold to trigger given the ample margin set initially) as well as > extending the default timeout of the test run. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Adding an @modules and minor code reformatting - Changes: - all: https://git.openjdk.org/jdk/pull/13116/files - new: https://git.openjdk.org/jdk/pull/13116/files/4a5dd8c8..572687a0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13116&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13116&range=01-02 Stats: 8 lines in 1 file changed: 3 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13116.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13116/head:pull/13116 PR: https://git.openjdk.org/jdk/pull/13116
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Tue, 21 Mar 2023 11:02:52 GMT, Jaikiran Pai wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > The PR seems to be using an incorrect JBS issue id/summary. This PR should be > linked with the new https://bugs.openjdk.org/browse/JDK-8304557 issue. @jaikiran đź‘Ť I'm running a test against debug builds to verify. Will integrate once that passes. - PR Comment: https://git.openjdk.org/jdk/pull/13116#issuecomment-1481323888
Integrated: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Tue, 21 Mar 2023 10:42:55 GMT, Viktor Klang wrote: > Improves the stability of the memory leak test for CompletableFuture timeout > cancellation by both reducing the count by 50% (which should still be above > threshold to trigger given the ample margin set initially) as well as > extending the default timeout of the test run. This pull request has now been integrated. Changeset: 6f67abd3 Author:Viktor Klang Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/6f67abd352ce9605dd93188995d42a47ee07b25e Stats: 34 lines in 1 file changed: 20 ins; 0 del; 14 mod 8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/13116
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v3]
On Thu, 23 Mar 2023 14:13:14 GMT, Viktor Klang wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding an @modules and minor code reformatting Also, pinging @DougLea as a gentle FYI :) - PR Comment: https://git.openjdk.org/jdk/pull/13116#issuecomment-1481859000
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Tue, 21 Mar 2023 11:02:52 GMT, Jaikiran Pai wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > The PR seems to be using an incorrect JBS issue id/summary. This PR should be > linked with the new https://bugs.openjdk.org/browse/JDK-8304557 issue. @jaikiran The modified change has passed on a debug build now. - PR Comment: https://git.openjdk.org/jdk/pull/13116#issuecomment-1481857138
Re: RFR: 8297605: improve DelayQueue removal method javadoc [v2]
On Tue, 21 Mar 2023 17:51:26 GMT, Martin Buchholz wrote: >> Inviting @DougLea and @viktorklang-ora to review. >> >> As usual, I couldn't resist more fiddling. >> - Added missing tests for take, remove(), remove(Object). >> - Improved DelayQueueTest's testing infrastructure >> - added more test assertions >> - linkified new javadoc definitions > > Martin Buchholz has updated the pull request incrementally with one > additional commit since the last revision: > > fix punctuation Nice work @Martin-Buchholz! đź‘Ť - PR Comment: https://git.openjdk.org/jdk/pull/12838#issuecomment-1494456047
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing [v2]
On Fri, 24 Feb 2023 16:08:57 GMT, Viktor Klang wrote: >> Clarifies the distinction between expiration of the head of DelayQueue and >> how it relates to `poll`, `take`, and `peek`. See discussion on >> https://bugs.openjdk.org/browse/JDK-8297605 >> >> @DougLea If possible, please weigh in on whether this is in line with your >> thoughts on the matter. > > Viktor Klang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > JDK-8297605 DelayQueue javadoc is confusing Superseded by https://github.com/openjdk/jdk/pull/12838 - PR Comment: https://git.openjdk.org/jdk/pull/12729#issuecomment-1494886096
Withdrawn: JDK-8297605 DelayQueue javadoc is confusing
On Thu, 23 Feb 2023 15:36:48 GMT, Viktor Klang wrote: > Clarifies the distinction between expiration of the head of DelayQueue and > how it relates to `poll`, `take`, and `peek`. See discussion on > https://bugs.openjdk.org/browse/JDK-8297605 > > @DougLea If possible, please weigh in on whether this is in line with your > thoughts on the matter. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/12729
