Re: RFR: 8311178: JMH tests don't scale well when sharing output buffers
On Sat, 1 Jul 2023 07:53:17 GMT, Swati Sharma wrote: > The below benchmark files have scaling issues due to cache contention and > leads to poor scaling when run on multiple threads. The patch sets the scope > from benchmark level to thread level to fix the issue: > - org/openjdk/bench/java/io/DataOutputStreamTest.java > - org/openjdk/bench/java/lang/ArrayCopyObject.java > - org/openjdk/bench/java/lang/ArrayFiddle.java > - org/openjdk/bench/java/time/format/DateTimeFormatterBench.java > - org/openjdk/bench/jdk/incubator/vector/IndexInRangeBenchmark.java > - org/openjdk/bench/jdk/incubator/vector/MemorySegmentVectorAccess.java > - org/openjdk/bench/jdk/incubator/vector/StoreMaskedBenchmark.java > - org/openjdk/bench/jdk/incubator/vector/StoreMaskedIOOBEBenchmark.java > - org/openjdk/bench/vm/compiler/ArrayFill.java > - org/openjdk/bench/vm/compiler/IndexVector.java > > Also removing the static scope for variables in > org/openjdk/bench/jdk/incubator/vector/VectorFPtoIntCastOperations.java for > better scaling. > > Please review and share your feedback. > > Thanks, > Swati Not review this PR, but just raise a question. Should a JMH test, at least in JDK repo, always uses `@State(Scope.Thread)`, even though it uses only one thread? I just looked through those JMH tests, and found all of them, like the bellows, don't specify the number of threads via `@Threads`. org/openjdk/bench/java/io/DataOutputStreamTest.java org/openjdk/bench/java/lang/ArrayCopyObject.java I suppose the default number of threads is 1. Maybe the default value will be overridden via the commands when running these JMH tests in bulk (?) - PR Comment: https://git.openjdk.org/jdk/pull/14746#issuecomment-1629261727
RFR: 8313231: Redundant if statement in ZoneInfoFile
if (i < savingsInstantTransitions.length) {
// javazic writes the last GMT offset into index 0!
if (i < savingsInstantTransitions.length) {
offsets[0] = standardOffsets[standardOffsets.length - 1] * 1000;
nOffsets = 1;
}
...
}
The second if statement looks unnecessary.
-
Commit messages:
- 8313231: Redundant if statement in ZoneInfoFile
Changes: https://git.openjdk.org/jdk/pull/15052/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15052&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8313231
Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/15052.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15052/head:pull/15052
PR: https://git.openjdk.org/jdk/pull/15052
Re: RFR: 8313231: Redundant if statement in ZoneInfoFile
On Mon, 31 Jul 2023 07:11:24 GMT, Stephen Colebourne
wrote:
>> if (i < savingsInstantTransitions.length) {
>> // javazic writes the last GMT offset into index 0!
>> if (i < savingsInstantTransitions.length) {
>> offsets[0] = standardOffsets[standardOffsets.length - 1] * 1000;
>> nOffsets = 1;
>> }
>> ...
>> }
>>
>>
>> The second if statement looks unnecessary.
>
> Marked as reviewed by scolebourne (Author).
@jodastephen @DamonFool @pavelrappo
Thanks for your reviews!
-
PR Comment: https://git.openjdk.org/jdk/pull/15052#issuecomment-1657847007
Integrated: 8313231: Redundant if statement in ZoneInfoFile
On Thu, 27 Jul 2023 06:46:46 GMT, John Jiang wrote:
> if (i < savingsInstantTransitions.length) {
> // javazic writes the last GMT offset into index 0!
> if (i < savingsInstantTransitions.length) {
> offsets[0] = standardOffsets[standardOffsets.length - 1] * 1000;
> nOffsets = 1;
> }
> ...
> }
>
>
> The second if statement looks unnecessary.
This pull request has now been integrated.
Changeset: f8c2b7fe
Author:John Jiang
URL:
https://git.openjdk.org/jdk/commit/f8c2b7fee101d66107704b3ee464737c5ccdc13a
Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod
8313231: Redundant if statement in ZoneInfoFile
Reviewed-by: jiefu, scolebourne
-
PR: https://git.openjdk.org/jdk/pull/15052
Re: RFR: 8313612: Use JUnit in lib-test/jdk tests
On Wed, 2 Aug 2023 23:25:13 GMT, Qing Xiao wrote: > Modified all tests under lib-test/jdk to use JUnit test/lib-test/jdk/test/lib/format/ArrayDiffTest.java line 2: > 1: /* > 2: * Copyright (c) 2020, 2021, 2023, Oracle and/or its affiliates. All > rights reserved. Should it just modify the second year, here is `2021`, to `2023`? - PR Review Comment: https://git.openjdk.org/jdk/pull/15131#discussion_r1294304121
RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
It looks the `else-if` and `else` clauses in method `ArraysSupport::hugeLength` could be simplified by `Math::max`. - Commit messages: - 8321637: Simplify if statement in ArraysSupport::hugeLength Changes: https://git.openjdk.org/jdk/pull/17043/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17043&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321637 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17043.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17043/head:pull/17043 PR: https://git.openjdk.org/jdk/pull/17043
Re: RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Sun, 10 Dec 2023 03:57:00 GMT, Jie Fu wrote: >> It looks the `else-if` and `else` clauses in method >> `ArraysSupport::hugeLength` could be simplified by `Math::max`. > > The JBS should be an enhancement, not a bug, right? @DamonFool > The JBS should be an enhancement, not a bug, right? Yes, it should be an enhancement rather than a bug. Thanks @AlanBateman for changing this JBS type. - PR Comment: https://git.openjdk.org/jdk/pull/17043#issuecomment-1848976812
Re: RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Sat, 9 Dec 2023 23:19:52 GMT, John Jiang wrote: > It looks the `else-if` and `else` clauses in method > `ArraysSupport::hugeLength` could be simplified by `Math::max`. Could this simple PR be reviewed? - PR Comment: https://git.openjdk.org/jdk/pull/17043#issuecomment-1860729032
Re: RFR: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Tue, 19 Dec 2023 03:44:26 GMT, Stuart Marks wrote: >> It looks the `else-if` and `else` clauses in method >> `ArraysSupport::hugeLength` could be simplified by `Math::max`. > > This change would make the code shorter, but in my opinion, it obscures > what's going on. This code tries to be very careful about avoiding problems > caused by integer overflow/wraparound, and in order to do that, it needs to > make sure that the full range of int values is handled properly. With the > explicit if/else code, this is clear. Using Math.max() instead tends to make > this obscure. > > This came up previously; see > > https://github.com/openjdk/jdk/pull/1617#discussion_r536260884 @stuart-marks Thanks for your clarification! Just closed this PR and JBS. - PR Comment: https://git.openjdk.org/jdk/pull/17043#issuecomment-1862076153
Withdrawn: 8321637: Simplify if statement in ArraysSupport::hugeLength
On Sat, 9 Dec 2023 23:19:52 GMT, John Jiang wrote: > It looks the `else-if` and `else` clauses in method > `ArraysSupport::hugeLength` could be simplified by `Math::max`. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17043
