Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v7]
On Tue, 16 Jan 2024 16:38:34 GMT, Glavo wrote: >> Using `ByteArrayLittleEndian` is simpler and faster. >> >> `make test TEST="micro:java.util.zip.ZipFileOpen"`: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> - ZipFileOpen.openCloseZipFile 512 avgt 15 39052.832 ± 107.496 >> ns/op >> + ZipFileOpen.openCloseZipFile 512 avgt 15 36275.539 ± 663.193 >> ns/op >> - ZipFileOpen.openCloseZipFile1024 avgt 15 77106.494 ± 4159.300 >> ns/op >> + ZipFileOpen.openCloseZipFile1024 avgt 15 71955.013 ± 2296.050 >> ns/op > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > Add tests Hello Glavo, I'll need some more time on this to review this. In the meantime, could you update the micro benchmark latest numbers with this latest state? - PR Comment: https://git.openjdk.org/jdk/pull/14632#issuecomment-1968371386
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v6]
On Mon, 5 Feb 2024 23:53:06 GMT, Archie Cobbs wrote: >> `GZIPInputStream`, when looking for a concatenated stream, relies on what >> the underlying `InputStream` says is how many bytes are `available()`. But >> this is inappropriate because `InputStream.available()` is just an estimate >> and is allowed (for example) to always return zero. >> >> The fix is to ignore what's `available()` and just proceed and see what >> happens. If fewer bytes are available than required, the attempt to extend >> to another stream is canceled just as it was before, e.g., when the next >> stream header couldn't be read. > > Archie Cobbs 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: > > - Merge branch 'master' into JDK-7036144 > - Merge branch 'master' into JDK-7036144 > - Address third round of review comments. > - Address second round of review comments. > - Address review comments. > - Fix bug in GZIPInputStream when underlying available() returns short. Hello Archie, > Edited to add: I said "already a problem in the current code" but should > clarify: what I mean is, suppose some clever InputStream were never letting > available() return more than the number of compressed bytes remaining. That > strategy would not be reliable, because not all reads from the underlying > stream are gated with a check of what's available() (for example, see > InflaterInputStream.fill()). So I don't think we need to worry about breaking > such a case because it's already broken for other reasons. I think what you note is reasonable and accurate. I'll run some experiments against existing corpus of artifacts to see if this proposed change could cause more issues than the current state of GZIPInputStream. - PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-1968364298
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 06:12:17 GMT, Guoxiong Li wrote: > So large patch. In order to easy to review, it is good to separate such large > patch into several patches in the future. I was doing that in prior related changes (see the subtasks of JDK-8324799). But reviewers requested I batch up the remainder, hence this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18035#issuecomment-1968355666
Re: RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
On Fri, 23 Feb 2024 19:18:46 GMT, Christoph Langer wrote: > The new test JmodExcludedFiles.java checks that no debug symbol files are > contained in the jmod files. It does not take into account that with > configure option --with-external-symbols-in-bundles=public we want to have > stripped pdb files, also in jmods, to get native callstacks with line number. > > This modifies the test and checks if equivalent *stripped.pdb files exist > when .pdb files are encountered inside jmods. In that case one can assume > that --with-external-symbols-in-bundles=public was chosen as configure option. @mlchung, mind to have a look here? Thanks - PR Comment: https://git.openjdk.org/jdk/pull/17990#issuecomment-1968346603
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v4]
On Tue, 27 Feb 2024 16:48:05 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test: expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust ms Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17952#pullrequestreview-1905420462
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett wrote: > Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 So large patch. In order to easy to review, it is good to separate such large patch into several patches in the future. Two trivial places need to be adjusted. test/hotspot/jtreg/serviceability/jvmti/events/SingleStep/singlestep01/libsinglestep01.cpp line 28: > 26: #include > 27: #include > 28: #include "jvmti_common.hpp" The copyright of this file is wrong. > * Copyright (c) 200 > * git 3, 2022, Oracle and/or its affiliates. All rights reserved. > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace/GetStackTraceAndRetransformTest/libGetStackTraceAndRetransformTest.cpp line 27: > 25: #include > 26: #include "jvmti.h" > 27: #include "jvmti_common.hpp" The oracle copyright needs to be added in this file? > * Copyright (c) 2023, Datadog, Inc. All rights reserved. > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - Changes requested by gli (Committer). PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1905259144 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1505322535 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1505326445
Re: RFR: JDK-8316708: Augment WorstCaseTests with more cases [v8]
> A new paper > > "Accuracy of Mathematical Functions in Single, Double, Double Extended, and > Quadruple Precision" > by Brian Gladman, Vincenzo Innocente and Paul Zimmermann > https://members.loria.fr/PZimmermann/papers/accuracy.pdf > > details the inputs with generate the worst-case observed errors in different > math library implementations. The FDLIBM-related worst cases should be added > to the test suite. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback, adjust worst-case bounds. - Changes: - all: https://git.openjdk.org/jdk/pull/15879/files - new: https://git.openjdk.org/jdk/pull/15879/files/de217d77..98c8e69f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15879=07 - incr: https://webrevs.openjdk.org/?repo=jdk=15879=06-07 Stats: 39 lines in 3 files changed: 1 ins; 0 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/15879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15879/head:pull/15879 PR: https://git.openjdk.org/jdk/pull/15879
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett wrote: > Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 Was a blast looking through all 729 files, but have a +1 - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1905352841
Re: RFR: 8319386: Migrate Class::getEnclosingMethod/Constructor away from old generic utilities
On Fri, 3 Nov 2023 14:03:02 GMT, Chen Liang wrote: > Please review a patch that migrates `Class::getEnclosingMethod` and > `Class::getEnclosingConstructor`'s descriptor parsing from old generic > utilities to more simple utilities from java.lang.invoke implementation. This > will help migrate away from the old generic repositories in the future. > > The `getClassLoader()` call plus > https://github.com/openjdk/jdk/blob/1a21c1a783d64ca0930c358c06a43975f96ffac6/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java#L93 > should be functionally equivalent to the previous `getDeclsLoader()` > https://github.com/openjdk/jdk/blob/1a21c1a783d64ca0930c358c06a43975f96ffac6/src/java.base/share/classes/sun/reflect/generics/factory/CoreReflectionFactory.java#L60-L68 > plus > https://github.com/openjdk/jdk/blob/1a21c1a783d64ca0930c358c06a43975f96ffac6/src/java.base/share/classes/sun/reflect/generics/factory/CoreReflectionFactory.java#L113-L114 Please review this patch that migrated descriptor parsing from old, complex Java 5 signature parsing system to the java.lang.invoke's descriptor parsing utilities. - PR Comment: https://git.openjdk.org/jdk/pull/16496#issuecomment-1968053393
RFR: 8324799: Use correct extension for C++ test headers
Please review this change that renames some test .h files to .hpp. These files contain C++ code and should be named accordingly. Some of them contain uses of NULL, which we change to nullptr. The renamed files are: test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h test/lib/jdk/test/lib/jvmti/jvmti_thread.h test/lib/jdk/test/lib/jvmti/jvmti_common.h test/lib/native/testlib_threads.h The #include updates were performed mostly mechanically, and builds would fail if there were mistakes. The exception is test test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, which was added after I'd done the mechanical update, so was updated by hand. The copyright updates were similarly performed mechanically. All but a handful of the including files have already had their copyrights updated recently, likely as part of JDK-8324681. Thus, the only "interesting" changes are to the renamed files. Testing: mach5 tier1 - Commit messages: - update new test - update copyrights - fix jvmti README - rename NULLs in jvmti_common.hpp - rename jvmti_common.h - rename NULLs in jvmti_thread.hpp - rename jvmti_thread.h - rename NULLs in testlib_threads.hpp - rename testlib_threads.h -- no copyright - rename nsk_tools.h -- no copyright - ... and 5 more: https://git.openjdk.org/jdk/compare/16d917a8...4154005e Changes: https://git.openjdk.org/jdk/pull/18035/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18035=00 Issue: https://bugs.openjdk.org/browse/JDK-8324799 Stats: 1535 lines in 731 files changed: 133 ins; 133 del; 1269 mod Patch: https://git.openjdk.org/jdk/pull/18035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18035/head:pull/18035 PR: https://git.openjdk.org/jdk/pull/18035
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677 [v2]
> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. Chad Rakoczy has updated the pull request incrementally with one additional commit since the last revision: Test updates - Changes: - all: https://git.openjdk.org/jdk/pull/18033/files - new: https://git.openjdk.org/jdk/pull/18033/files/b3050e16..45d0acdd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18033=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18033=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18033/head:pull/18033 PR: https://git.openjdk.org/jdk/pull/18033
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677 [v2]
On Tue, 27 Feb 2024 20:01:20 GMT, Joe Darcy wrote: >> What is the issue with this? Is there a different way to set a timeout? The >> test tests that format does not take a long time to run so I would like to >> have a timeout > >> What is the issue with this? Is there a different way to set a timeout? The >> test tests that format does not take a long time to run so I would like to >> have a timeout > > Hard-coded timeout in a test are generally considered harmful as the test > suite is run on a wide variety of systems, a single value could be too large > for fast systems and too small for slow ones. The jtreg harness has an > overall `-timeout:N` factor which can scale up or down all the timeouts of > individual tests. I don't know offhand if there is an existing idiom to do > this with junit tests from within jtreg. Removed that timeout. Test fails before patch even when timeout factor is 10. Passes after patch within seconds. Default timeout should be good - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1505158893
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v16]
On Tue, 27 Feb 2024 02:47:13 GMT, Jatin Bhateja wrote:
>> Hi All,
>>
>> This patch optimizes sub-word gather operation for x86 targets with AVX2 and
>> AVX512 features.
>>
>> Following is the summary of changes:-
>>
>> 1) Intrinsify sub-word gather using hybrid algorithm which initially
>> partially unrolls scalar loop to accumulates values from gather indices into
>> a quadword(64bit) slice followed by vector permutation to place the slice
>> into appropriate vector lanes, it prevents code bloating and generates
>> compact JIT sequence. This coupled with savings from expansive array
>> allocation in existing java implementation translates into significant
>> performance of 1.5-10x gains with included micro.
>>
>> 
>>
>>
>> 2) Patch was also compared against modified java fallback implementation by
>> replacing temporary array allocation with zero initialized vector and a
>> scalar loops which inserts gathered values into vector. But, vector insert
>> operation in higher vector lanes is a three step process which first
>> extracts the upper vector 128 bit lane, updates it with gather subword value
>> and then inserts the lane back to its original position. This makes inserts
>> into higher order lanes costly w.r.t to proposed solution. In addition
>> generated JIT code for modified fallback implementation was very bulky. This
>> may impact in-lining decisions into caller contexts.
>>
>> Kindly review and share your feedback.
>>
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comment resolutions.
The Java code looks correct.
It would be nice to common the non-mask and mask variants to reduce the code
duplication. Perhaps even common to all element types if the loop check is
efficient?
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-Vector.java.template
line 3637:
> 3635: } else {
> 3636: lsp = isp;
> 3637: }
No need to initialize `lsp` to null, since it will be definitely assigned after
the if/else statement.
Suggestion:
// Constant folding should sweep out following conditonal logic.
VectorSpecies lsp;
if (isp.length() > IntVector.SPECIES_PREFERRED.length()) {
lsp = IntVector.SPECIES_PREFERRED;
} else {
lsp = isp;
}
-
PR Review: https://git.openjdk.org/jdk/pull/16354#pullrequestreview-1904909095
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1505073358
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Only show runtime image suffix for JDK modules >From a build point of view, I don't have much to complain about. See inline >comments for some nits. make/autoconf/jdk-options.m4 line 596: > 594: > > 595: # > 596: # jlink related. This part of the comment seems a bit redundant. make/autoconf/spec.gmk.template line 904: > 902: RL_INTERMEDIATE_IMAGE_SUBDIR := runtime-link-initial-jdk > 903: RL_INTERMEDIATE_IMAGE_DIR := > $(IMAGES_OUTPUTDIR)/$(RL_INTERMEDIATE_IMAGE_SUBDIR) > 904: This intermediate directory is only used inside the Images.gmk. I don't think we need to define it in spec. It can be a local variable in Images.gmk. Also, unless you really want this intermediate image in "images" as some kind of deliverable I would prefer to put it somewhere under `$(SUPPORT_OUTPUTDIR)/images`. - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-1904905122 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1505071076 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1505072553
Re: RFR: JDK-8316708: Augment WorstCaseTests with more cases [v7]
On Tue, 27 Feb 2024 11:20:11 GMT, Raffaello Giulietti wrote: > There are no libraries that have worse errors than OpenLibm, so I'm wondering > what these values are good for? When I was working on updating the worst-case tests for Math, I would check the input values in Math.foo() and StrictMath.foo() and if they differed pick the smaller one as the reference value. The those inputs that differ, the input is a fingerprint marker for FDLIBM and I added cases to the corresponding StrictMath test. - PR Review Comment: https://git.openjdk.org/jdk/pull/15879#discussion_r1504998720
Re: RFR: JDK-8316708: Augment WorstCaseTests with more cases [v7]
On Tue, 27 Feb 2024 11:20:01 GMT, Raffaello Giulietti wrote: > I can't find this one on the paper. Good catch; must have been a cut and paste error on my part. - PR Review Comment: https://git.openjdk.org/jdk/pull/15879#discussion_r1504993038
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Fri, 16 Feb 2024 23:43:15 GMT, Srinivas Vamsi Parasa
wrote:
>> Hi Vamsi (@vamsi-parasa),
>>
>> My fault, there was an incorrect version of ArraysSortNew.java. Methods, of
>> course, should be
>>
>> @Benchmark
>> public void sort() {
>> Arrays.sort(b);
>> }
>>
>> @Benchmark
>> public void p_sort() {
>> Arrays.parallelSort(b);
>> }
>>
>> I uploaded correct version, see
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/ArraysSortNew.java
>>
>> I also comment that pom.xml contains additional options (I guess you have
>> the same)
>> --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED
>> --add-exports=java.base/jdk.internal.vm.annotation=ALL-UNNAMED
>> full text is there
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/pom.xml
>>
>> and command to run test is
>> java --add-exports=java.base/jdk.internal.vm.annotation=ALL-UNNAMED
>> --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED -jar
>> target/benchmarks.jar
>>
>> I assume that each variant of DPQS (DualPivotQuicksort_jdk,
>> DualPivotQuicksort_r20p, DualPivotQuicksort_r20s, DualPivotQuicksort_r25p,
>> DualPivotQuicksort_r25s) is renamed to DualPivotQuicksort and put into
>> package java.util. Then benchmarking for a given variant with patched JDK is
>> executed.
>>
>> Thank you,
>> Vladimir
>
> Hello Vladimir (@iaroslavski),
>
> Please see the data below. Each DPQS class was copied to java.util and the
> JDK was recompiled.
>
> Thanks,
> Vamsi
>
> xmlns:o="urn:schemas-microsoft-com:office:office"
> xmlns:x="urn:schemas-microsoft-com:office:excel"
> xmlns="http://www.w3.org/TR/REC-html40;>
>
>
>
>
>
> href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm">
> href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml">
>
>
>
>
>
>
>
>
> Benchmark | (builder) | (size) | Stock JDK | r20p | r20s | r25p | r25s
> -- | -- | -- | -- | -- | -- | -- | --
> ArraysSort.Int.p_sort | RANDOM | 600 | 1.618 | 2.601 | 2.966 | 2.898 | 3.269
> ArraysSort.Int.p_sort | RANDOM | 2000 | 7.433 | 8.438 | 8.463 | 8.414 | 8.65
> ArraysSort.Int.p_sort | RANDOM | 9 | 258.853 | 355.261 | 326.378 | 347.65
> | 321.894
> ArraysSort.Int.p_sort | RANDOM | 40 | 842.085 | 1225.929 | 899.852 |
> 1278.681 | 932.627
> ArraysSort.Int.p_sort | RANDOM | 300 | 5723.659 | 8711.108 | 6086.974 |
> 8948.101 | 6122.612
> ArraysSort.Int.p_sort | REPEATED | 600 | 0.52 | 0.585 | 0.629 | 0.586 | 0.579
> ArraysSort.Int.p_sort | REPEATED | 2000 | 1.18 | 1.225 | 1.21 | 1.225 | 1.238
> ArraysSort.Int.p_sort | REPEATED | 9 | 102.142 | 85.79 | 86.131 | 87.954
> | 86.036
> ArraysSort.Int.p_sort | REPEATED | 40 | 244.508 | 229.142 | 227.613 |
> 228.608 | 228.367
> ArraysSort.Int.p_sort | REPEATED | 300 | 2752.745 | 2584.103 | 2544.192 |
> 2576.803 | 2609.833
> ArraysSort.Int.p_sort | STAGGER | 600 | 1.146 | 0.894 | 0.898 | 0.904 | 0.912
> ArraysSort.Int.p_sort | STAGGER | 2000 | 3.712 | 3.096 | 3.121 | 3.03 | 3.049
> ArraysSort.Int.p_sort | STAGGER | 9 | 72.763 | 77.575 | 78.366 | 79.158 |
> 77.199
> ArraysSort.Int.p_sort | STAGGER | 40 | 212.455 | 228.331 | 225.888 |
> 224.686 | 225.728
> ArraysSort.Int.p_sort | STAGGER | 300 | 2290.327 | 2216.741 | 2196.138 |
> 2236.658 | 2262.472
> ArraysSort.Int.p_sort | SHUFFLE | 600 | 2.01 | 2.92 | 2.907 | 2.91 | 2.926
> ArraysSort.Int.p_sort | SHUFFLE | 2000 | 7.06 | 7.759 | 7.776 | 7.688 | 8.062
> ArraysSort.Int.p_sort | SHUFFLE | 9 | 157.728 | 151.871 | 151.101 |
> 154.03 | 151.2
> ArraysSort.Int.p_sort | SHUFFLE | 40 | 441.166 | 715.243 | 449.698 |
> 699.75 | 447.069
> ArraysSort.Int.p_sort | SHUFFLE | 300 | 4326.88 | 7133.045 | 4205.47 |...
Hello Vamsi (@vamsi-parasa),
Could you please run benchmarking of 4 cases with **updated** test class
**ArraysSortNew2**?
https://github.com/iaroslavski/sorting/blob/master/radixsort/ArraysSortNew2.java
Put each DPQS class in java.util package and recompiling the JDK for each case
as you
did before, and run new class **ArraysSortNew2**.
Find the sources there:
https://github.com/iaroslavski/sorting/blob/master/radixsort/ArraysSortNew2.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r27b.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r27p.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r27s.java
Thank you,
Vladimir
-
PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-1967570922
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 19:46:27 GMT, Chad Rakoczy wrote: >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug >> with Formatter.format taking a long time when there is a lot of padding. >> This test runs Formatter.format with very large padding. Test fails before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes >> after. >> >> Timeout for the test was set to 10 seconds. Test passes locally with as low >> as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) >> and fails as high as 120 (before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it >> should be consistent. > > The fix in JDK-8299677 serves it's intended purpose but the test added with > it does not test that. The original test does not timeout before or after the > fix which is the issue. > > "8326718: Test java/util/Formatter/Padding.java does not timeout on large > inputs after JDK-8299677" > This is the expected case. Should the title be what the issue is or what the > fix is? To me this sounds like the test should be timing out or was timing > out after JDK-8299677 > > Maybe a better title is > "8326718: Test java/util/Formatter/Padding.java should timeout on large > inputs before fix in JDK-8299677" > @chadrako The title of the issue should succinctly describe the problem at > the time it is filed. Then I feel like the current title is correct. The issue at the time of filing is that `Padding.java` does not timeout on large inputs before the fix (which is should but doesn't) that was implemented in JDK-8299677 I'm open to other's opinions on this as well - PR Comment: https://git.openjdk.org/jdk/pull/18033#issuecomment-1967548391
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v10]
On Tue, 27 Feb 2024 20:01:30 GMT, Eirik Bjørsnøs wrote:
> After some offline discussion about the redundant text, we decided to keep to
> the new method description text, but reduce the `@deprecated` note to simply
> refer to the replacement method:
>
> ```
> /**
> * Returns the total number of compressed bytes input so far.
> *
> * This method returns the equivalent of {@code (int) getBytesRead()}
> * and therefore cannot return the correct value when it is greater
> * than {@link Integer#MAX_VALUE}.
> *
> * @deprecated Use {@link #getBytesRead()} instead
> *
> * @return the total number of compressed bytes input so far
> */
> ```
>
> Barring any objections, I'll to go ahead with the update of the rest of the
> methods and update the CSR draft tomorrow.
Yes please move forward and thank you
-
PR Comment: https://git.openjdk.org/jdk/pull/17919#issuecomment-1967517931
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 19:53:23 GMT, Joe Darcy wrote: >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug >> with Formatter.format taking a long time when there is a lot of padding. >> This test runs Formatter.format with very large padding. Test fails before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes >> after. >> >> Timeout for the test was set to 10 seconds. Test passes locally with as low >> as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) >> and fails as high as 120 (before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it >> should be consistent. > > test/jdk/java/util/Formatter/Padding.java line 2: > >> 1: /* >> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > Copyright nit: per OpenJDK conventions, the new copyright for the updated > file should be "2023, 2024," not just "2024". Suggestion: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved. with a `,` after the 2nd year as well. Otherwise a check will fail, as I learned the hard-way ;-) - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1504901855
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v10]
On Tue, 27 Feb 2024 19:59:08 GMT, Eirik Bjørsnøs wrote:
>> Please review this PR which proposes that we officially deprecate the
>> following four methods in the `java.util.zip` package:
>>
>> * `Inflater.getTotalIn()`
>> * `Inflater.getTotalOut()`
>> * `Deflater.getTotalIn()`
>> * `Deflater.getTotalOut()`
>>
>> Since these legacy methods return `int`, they cannot safely return the
>> number of bytes processed without the risk of losing information about the
>> magnitude or even sign of the returned value.
>>
>> The corresponding methods `getBytesRead()` and `getBytesWritten()` methods
>> introduced in Java 5 return `long`, and should be used instead when
>> obtaining this information.
>>
>> Unrelated to the deprecation itself, the documentation currently does not
>> specify what these methods are expected to return when the number of
>> processed bytes is higher than `Integer.MAX_VALUE`. This PR aims to clarify
>> this in the API specification.
>>
>> Initally, this PR handles only `Inflater.getTotalIn()`. The other three
>> methods will be updated once the wordsmithing for this method stabilizes.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reduce the deprecation note to "Use {@link #getBytesRead()} instead"
After some offline discussion about the redundant text, we decided to keep to
the new method description text, but reduce the `@deprecated` note to simply
refer to the replacement method:
/**
* Returns the total number of compressed bytes input so far.
*
* This method returns the equivalent of {@code (int) getBytesRead()}
* and therefore cannot return the correct value when it is greater
* than {@link Integer#MAX_VALUE}.
*
* @deprecated Use {@link #getBytesRead()} instead
*
* @return the total number of compressed bytes input so far
*/
Barring any objections, I'll to go ahead with the update of the rest of the
methods and update the CSR draft tomorrow.
-
PR Comment: https://git.openjdk.org/jdk/pull/17919#issuecomment-1967498161
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 19:21:13 GMT, Chad Rakoczy wrote: > What is the issue with this? Is there a different way to set a timeout? The > test tests that format does not take a long time to run so I would like to > have a timeout Hard-coded timeout in a test are generally considered harmful as the test suite is run on a wide variety of systems, a single value could be too large for fast systems and too small for slow ones. The jtreg harness has an overall `-timeout:N` factor which can scale up or down all the timeouts of individual tests. I don't know offhand if there is an existing idiom to do this with junit tests from within jtreg. - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1504897104
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 19:46:27 GMT, Chad Rakoczy wrote: >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug >> with Formatter.format taking a long time when there is a lot of padding. >> This test runs Formatter.format with very large padding. Test fails before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes >> after. >> >> Timeout for the test was set to 10 seconds. Test passes locally with as low >> as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) >> and fails as high as 120 (before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it >> should be consistent. > > The fix in JDK-8299677 serves it's intended purpose but the test added with > it does not test that. The original test does not timeout before or after the > fix which is the issue. > > "8326718: Test java/util/Formatter/Padding.java does not timeout on large > inputs after JDK-8299677" > This is the expected case. Should the title be what the issue is or what the > fix is? To me this sounds like the test should be timing out or was timing > out after JDK-8299677 > > Maybe a better title is > "8326718: Test java/util/Formatter/Padding.java should timeout on large > inputs before fix in JDK-8299677" @chadrako The title of the issue should succinctly describe the problem at the time it is filed. - PR Comment: https://git.openjdk.org/jdk/pull/18033#issuecomment-1967496584
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v10]
> Please review this PR which proposes that we officially deprecate the
> following four methods in the `java.util.zip` package:
>
> * `Inflater.getTotalIn()`
> * `Inflater.getTotalOut()`
> * `Deflater.getTotalIn()`
> * `Deflater.getTotalOut()`
>
> Since these legacy methods return `int`, they cannot safely return the number
> of bytes processed without the risk of losing information about the
> magnitude or even sign of the returned value.
>
> The corresponding methods `getBytesRead()` and `getBytesWritten()` methods
> introduced in Java 5 return `long`, and should be used instead when obtaining
> this information.
>
> Unrelated to the deprecation itself, the documentation currently does not
> specify what these methods are expected to return when the number of
> processed bytes is higher than `Integer.MAX_VALUE`. This PR aims to clarify
> this in the API specification.
>
> Initally, this PR handles only `Inflater.getTotalIn()`. The other three
> methods will be updated once the wordsmithing for this method stabilizes.
Eirik Bjørsnøs has updated the pull request incrementally with one additional
commit since the last revision:
Reduce the deprecation note to "Use {@link #getBytesRead()} instead"
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17919/files
- new: https://git.openjdk.org/jdk/pull/17919/files/8d80c776..4eadbf86
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17919=09
- incr: https://webrevs.openjdk.org/?repo=jdk=17919=08-09
Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17919.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17919/head:pull/17919
PR: https://git.openjdk.org/jdk/pull/17919
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 17:24:03 GMT, Chad Rakoczy wrote: > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. test/jdk/java/util/Formatter/Padding.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. Copyright nit: per OpenJDK conventions, the new copyright for the updated file should be "2023, 2024" not just "2024". - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1504884833
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 17:24:03 GMT, Chad Rakoczy wrote: > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. The fix in JDK-8299677 serves it's intended purpose but the test added with it does not test that. The original test does not timeout before or after the fix which is the issue. "8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs after JDK-8299677" This is the expected case. Should the title be what the issue is or what the fix is? To me this sounds like the test should be timing out or was timing out after JDK-8299677 Maybe a better title is "8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677" - PR Comment: https://git.openjdk.org/jdk/pull/18033#issuecomment-1967475911
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 17:24:03 GMT, Chad Rakoczy wrote: > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. "8326718: Test java/util/Formatter/Padding.java _does_ timeout on large inputs _before_ fix in JDK-8299677" or "8326718: Test java/util/Formatter/Padding.java _does not_ timeout on large inputs _after_ JDK-8299677" To me, "8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before fix in JDK-8299677" as you propose, sounds like the fix JDK-8299677 didn't serve its intended purpose. - PR Comment: https://git.openjdk.org/jdk/pull/18033#issuecomment-1967456018
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 19:20:36 GMT, Raffaello Giulietti wrote: > I guess the title should read "8326718: Test java/util/Formatter/Padding.java > does not timeout on large inputs _after_ JDK-8299677" The test should timeout/fail before the fix in JDK-8299677 and pass after - PR Comment: https://git.openjdk.org/jdk/pull/18033#issuecomment-1967444745
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 19:13:10 GMT, Alan Bateman wrote: >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug >> with Formatter.format taking a long time when there is a lot of padding. >> This test runs Formatter.format with very large padding. Test fails before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes >> after. >> >> Timeout for the test was set to 10 seconds. Test passes locally with as low >> as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) >> and fails as high as 120 (before >> [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it >> should be consistent. > > test/jdk/java/util/Formatter/Padding.java line 29: > >> 27: * @summary Tests to excercise padding on int and double values, >> 28: * with various flag combinations. >> 29: * @run junit/timeout=10 Padding > > /timeout=10 looks problematic, I don't think you want that. What is the issue with this? Is there a different way to set a timeout? The test tests that format does not take a long time to run so I would like to have a timeout - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1504833617
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 17:24:03 GMT, Chad Rakoczy wrote: > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. I guess the title should read "8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs _after_ JDK-8299677" - PR Comment: https://git.openjdk.org/jdk/pull/18033#issuecomment-1967437224
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 17:24:03 GMT, Chad Rakoczy wrote: > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. test/jdk/java/util/Formatter/Padding.java line 29: > 27: * @summary Tests to excercise padding on int and double values, > 28: * with various flag combinations. > 29: * @run junit/timeout=10 Padding /timeout=10 looks problematic, I don't think you want that. - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1504811214
Re: RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
On Tue, 27 Feb 2024 17:24:03 GMT, Chad Rakoczy wrote: > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug > with Formatter.format taking a long time when there is a lot of padding. This > test runs Formatter.format with very large padding. Test fails before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes > after. > > Timeout for the test was set to 10 seconds. Test passes locally with as low > as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) > and fails as high as 120 (before > [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should > be consistent. test/jdk/java/util/Formatter/Padding.java line 26: > 24: /* > 25: * @test > 26: * @bug 4906370 Suggestion: * @bug 4906370 8299677 8326718 or maybe just Suggestion: * @bug 4906370 8326718 - PR Review Comment: https://git.openjdk.org/jdk/pull/18033#discussion_r1504795257
RFR: 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677
[JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) fixes a bug with Formatter.format taking a long time when there is a lot of padding. This test runs Formatter.format with very large padding. Test fails before [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677) and passes after. Timeout for the test was set to 10 seconds. Test passes locally with as low as 1 (after [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) and fails as high as 120 (before [JDK-8299677](https://bugs.openjdk.java.net/browse/JDK-8299677)) so it should be consistent. - Commit messages: - 8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677 Changes: https://git.openjdk.org/jdk/pull/18033/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18033=00 Issue: https://bugs.openjdk.org/browse/JDK-8326718 Stats: 41 lines in 1 file changed: 39 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18033/head:pull/18033 PR: https://git.openjdk.org/jdk/pull/18033
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v5]
On Tue, 27 Feb 2024 11:24:08 GMT, Magnus Ihse Bursie wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressing review comments > > This looks good from a build perspective. Actually, it looks great! :) I'm > happy to get rid of this old strange construct. Thanks, @magicus > to get rid of this old strange construct. Removing legacy clutter is exactly the purpose of this exercise! - PR Comment: https://git.openjdk.org/jdk/pull/17991#issuecomment-1967125233
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v4]
On Tue, 27 Feb 2024 16:48:05 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test: expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust ms Thank you Matthias. This looks good to me. I haven't checked references for this internal `Asserts` class. Before integrating, please run relevant tests that use this class just to be sure no test relies (and now fails) on the message being printed by the failing assertion. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17952#pullrequestreview-1904154276
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Mon, 26 Feb 2024 01:17:23 GMT, Jaikiran Pai wrote: > Hello Christoph, yes that looks fine to me. I adjusted it to the given suggestion. - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1967070481
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v4]
> Currently assertEquals has in the failure case sometimes confusing output > like : > > java.lang.RuntimeException: VM output should contain exactly one RTM locking > statistics entry for method > compiler.rtm.locking.TestRTMTotalCountIncrRate$Test: expected 0 to equal 1 >at jdk.test.lib.Asserts.fail(Asserts.java:634) >at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) > > (I don't think we really expected that for some reason 0 equals 1) > This should be improved. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust ms - Changes: - all: https://git.openjdk.org/jdk/pull/17952/files - new: https://git.openjdk.org/jdk/pull/17952/files/03cf3a82..a89742ff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17952=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17952=02-03 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17952/head:pull/17952 PR: https://git.openjdk.org/jdk/pull/17952
Integrated: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
On Tue, 27 Feb 2024 03:11:37 GMT, Jiangli Zhou wrote: > Please help review this trivial change. This was branched from > https://github.com/openjdk/jdk/pull/18013, based on discussion with > @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks This pull request has now been integrated. Changeset: 81b065a9 Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/81b065a95d670ef357c36239d8c408cd72a5c48c Stats: 20 lines in 2 files changed: 0 ins; 13 del; 7 mod 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c Reviewed-by: djelinski, rriggs - PR: https://git.openjdk.org/jdk/pull/18019
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
> This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Address initial feedback for JDK-8326687 - Changes: - all: https://git.openjdk.org/jdk/pull/18011/files - new: https://git.openjdk.org/jdk/pull/18011/files/0208b860..baa67f11 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18011=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18011=00-01 Stats: 10 lines in 4 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18011/head:pull/18011 PR: https://git.openjdk.org/jdk/pull/18011
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Tue, 27 Feb 2024 16:12:03 GMT, Lance Andersen wrote: >> This PR updates the javadoc and comments within java.util.zip/jar and zipfs >> module summary so that it is consistent with the use of "ZIP". >> >> In addition, >> open/src/java.base/share/classes/java/util/zip/package-info.java has been >> updated to point to the higher level location of the PKWARE APPNOTE.TXT has >> PKWare recently changed the direct path the the latest version of the spec. >> >> It is also worth noting that error messages were not updated as part of the >> PR and will be updated separately to keep the javadoc changes separate > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Address initial feedback for JDK-8326687 Updated ZipOutputStream.java and the other file you mentioned. I intentionally did not update any files which were not generating javadoc. We can certainly address any additional files and tests not related to javadoc via a separate issue. Thank you Eirik and Daniel for the review - PR Review: https://git.openjdk.org/jdk/pull/18011#pullrequestreview-1903973731
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Tue, 27 Feb 2024 15:08:11 GMT, Eirik Bjørsnøs wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address initial feedback for JDK-8326687 > > src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 54: > >> 52: * Whether to use ZIP64 for zip files with more than 64k entries. >> 53: * Until ZIP64 support in zip implementations is ubiquitous, this >> 54: * system property allows the creation of zip files which can be > > Suggestion: > > * Whether to use ZIP64 for ZIP files with more than 64k entries. > * Until ZIP64 support in ZIP implementations is ubiquitous, this > * system property allows the creation of ZIP files which can be Fixed not sure how Missed this when updating ZipOutputStream - PR Review Comment: https://git.openjdk.org/jdk/pull/18011#discussion_r1504509576
Re: RFR: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
On Tue, 27 Feb 2024 07:29:46 GMT, Daniel Jeliński wrote: >> Please help review this trivial change. This was branched from >> https://github.com/openjdk/jdk/pull/18013, based on discussion with >> @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks > > LGTM @djelinski @RogerRiggs Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18019#issuecomment-1966903722
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Only show runtime image suffix for JDK modules - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/00caf77c..b034cce2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14787=17 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=16-17 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc
On Mon, 26 Feb 2024 19:55:47 GMT, Lance Andersen wrote: > This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate Can you confirm that "zip" in `java.util.zip.ZipUtils.loadLibrary()` was intentionally left lowercase? (Presumably the native library should be referred to by lowercase "zip"): https://github.com/openjdk/jdk/blob/c5c866aafe76f51cd5386bf5052c06691c1a0e8c/src/java.base/share/classes/java/util/zip/ZipUtils.java#L290 See separate feedback about the `ZipOutputStream.inhibitZip64` comment. src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 54: > 52: * Whether to use ZIP64 for zip files with more than 64k entries. > 53: * Until ZIP64 support in zip implementations is ubiquitous, this > 54: * system property allows the creation of zip files which can be Suggestion: * Whether to use ZIP64 for ZIP files with more than 64k entries. * Until ZIP64 support in ZIP implementations is ubiquitous, this * system property allows the creation of ZIP files which can be - PR Review: https://git.openjdk.org/jdk/pull/18011#pullrequestreview-1903793561 PR Review Comment: https://git.openjdk.org/jdk/pull/18011#discussion_r1504401597
Re: RFR: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
On Tue, 27 Feb 2024 03:11:37 GMT, Jiangli Zhou wrote: > Please help review this trivial change. This was branched from > https://github.com/openjdk/jdk/pull/18013, based on discussion with > @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18019#pullrequestreview-1903727632
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc
On Mon, 26 Feb 2024 19:55:47 GMT, Lance Andersen wrote: > This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate The class javadocs for `java.util.zip.ZipCoder` uses "zipfile" which is probably a reference to the `ZipFile` class. `jdk.nio.zipfs.ZipCoder` seems to have borrowed the class javadocs, but here `ZipFile` does not make sense. Perhaps `ZipFileSystem` would be better? Would you consider fixing this to CamelCase in this PR (although it is maybe not directly the same inconsistency the PR aims to fix)? https://github.com/openjdk/jdk/blob/c5c866aafe76f51cd5386bf5052c06691c1a0e8c/src/java.base/share/classes/java/util/zip/ZipCoder.java#L41 https://github.com/openjdk/jdk/blob/c5c866aafe76f51cd5386bf5052c06691c1a0e8c/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipCoder.java#L41 - PR Comment: https://git.openjdk.org/jdk/pull/18011#issuecomment-1966701666
Re: RFR: 8319648: java/lang/SecurityManager tests ignore vm flags [v2]
> In this PR I updated the tests to use the newer > ProcessTools.createTestJavaProcessBuilder methods to pass VM options to child > processes. Matthew Donovan has updated the pull request incrementally with one additional commit since the last revision: removed unused import statement - Changes: - all: https://git.openjdk.org/jdk/pull/17878/files - new: https://git.openjdk.org/jdk/pull/17878/files/458ac07a..5e5261bf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17878=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17878=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17878.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17878/head:pull/17878 PR: https://git.openjdk.org/jdk/pull/17878
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution I can't really say I agree, but I guess we'll see where this goes from here first - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966471047
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc
On Mon, 26 Feb 2024 19:55:47 GMT, Lance Andersen wrote: > This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate I am not a usual Reviewer for this area but the changes LGTM Lance. I haven't seen anything unexpected given the description of the changes. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18011#pullrequestreview-1903392981
Re: RFR: 8269428: java/util/concurrent/ConcurrentHashMap/ToArray.java timed out
On Tue, 27 Feb 2024 10:30:44 GMT, Viktor Klang wrote: > As an intermediate fix to the test, switching to explicit usage of an > ExecutorService seems to do the trick to make this test reliably pass. > > With that said, this test (CHM::ToArray.java) seems to trigger an issue in > ForkJoinPool, so that would need to be fixed separately. > > Tagging @DougLea as an FYI. :) This change seems OK in terms of avoiding failures due to unrelated FJP and/or GC issues (for which it may be better to develop a separate test). - PR Comment: https://git.openjdk.org/jdk/pull/18023#issuecomment-1966429236
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:44:31 GMT, Daniel Jeliński wrote: > FWIW, when I added -lstdc++ before both -static-libstdc++ and replaced LDCXX > with LD, the code compiled and linked just fine. Both GCC and G++ call the same linker, and the parameter differences are well documented. It's only a matter of deciding if we want to keep the complexity of selecting the executable to use or not. My thinking matches yours. It would be nice to get rid of LDCXX. I'll look into it as a follow-up project. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966390350
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution FWIW, when I added `-lstdc++` before both `-static-libstdc++` and replaced LDCXX with LD, the code compiled and linked just fine. Both GCC and G++ call the same linker, and the parameter differences are well documented. It's only a matter of deciding if we want to keep the complexity of selecting the executable to use or not. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966368056
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 11:09:26 GMT, Magnus Ihse Bursie wrote: > > can we get rid of LDCXX? > > Yeah, that is something I plan to look into. Linking C++ object files with > gcc will fail; and it might be that linking pure C with g++ might be > problematic. If this is the case, I hope we can at least determine this > automatically which linker frontend to use, i.e. selecting g++ as linker > frontend if there is at least one .cpp file in the set of sources. > > This PR is actually a kind of prerequisite for that kind of work. I'd certainly opt for selecting which linker driver to use automatically than using one for both; According to some discussions with several gcc maintainers (https://web.libera.chat/) they really aren't meant to be intermingled Also was fine with LANG; There wasn't a need to change it to LINK_TYPE, but oh well - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966341897
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v5]
On Tue, 27 Feb 2024 00:24:08 GMT, Naoto Sato wrote: >> This PR intends to remove the legacy `COMPAT` locale data from the JDK. The >> `COMPAT` locale data was introduced for applications' migratory purposes >> transitioning to `CLDR`. It is becoming a technical debt and now is the time >> to remove it (we've been emitting a warning at JVM startup since JDK21, if >> the app is using `COMPAT`). A corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing review comments This looks good from a build perspective. Actually, it looks great! :) I'm happy to get rid of this old strange construct. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17991#pullrequestreview-1903238470
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
> The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Merge branch 'master' into remove-toolchain-define - Rename LANG to LINK_TYPE - Reword "lib" comment to fit in better - Merge branch 'master' into remove-toolchain-define - 8326583: Remove over-generalized DefineNativeToolchain solution - Changes: https://git.openjdk.org/jdk/pull/17986/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17986=03 Stats: 231 lines in 14 files changed: 58 ins; 142 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/17986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17986/head:pull/17986 PR: https://git.openjdk.org/jdk/pull/17986
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:29:44 GMT, Julian Waters wrote: > All those new parameters to SetupNativeCompilation, were they always there > and the comments about them were just missing from the documentation about > the function? Yep. The toolchain definition was a way to "package" multiple arguments to SetupNativeCompilation, so you did not have to specify them individually in each call. But in the end they just turned into "normal" arguments to SetupNativeCompilation (but undocumented...). - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966314926
Integrated: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. This pull request has now been integrated. Changeset: ac3ce2aa Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/ac3ce2aa156332fc4e6f33018ff669657ab4b797 Stats: 231 lines in 14 files changed: 58 ins; 142 del; 31 mod 8326583: Remove over-generalized DefineNativeToolchain solution Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/17986
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:14:56 GMT, Julian Waters wrote: > can we get rid of LDCXX? Yeah, that is something I plan to look into. Linking C++ object files with gcc will fail; and it might be that linking pure C with g++ might be problematic. If this is the case, I hope we can at least determine this automatically which linker frontend to use, i.e. selecting g++ as linker frontend if there is at least one .cpp file in the set of sources. This PR is actually a kind of prerequisite for that kind of work. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966312751
Re: RFR: JDK-8316708: Augment WorstCaseTests with more cases [v7]
On Tue, 27 Feb 2024 06:13:08 GMT, Joe Darcy wrote:
>> A new paper
>>
>> "Accuracy of Mathematical Functions in Single, Double, Double Extended, and
>> Quadruple Precision"
>> by Brian Gladman, Vincenzo Innocente and Paul Zimmermann
>> https://members.loria.fr/PZimmermann/papers/accuracy.pdf
>>
>> details the inputs with generate the worst-case observed errors in different
>> math library implementations. The FDLIBM-related worst cases should be added
>> to the test suite.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Appease jcheck.
test/jdk/java/lang/StrictMath/HypotTests.java line 711:
> 709: // Empirical worst-case points in other libraries with
> 710: // larger worst-case errors than FDLIBM
> 711: {0x0.039a2p-1022, 0x0.1p-1022,
> 0x0.039a2p-1022},
I can't find this one on the paper.
Suggestion:
{-0x0.fp-1022, 0x0.1p-1022,
0x0.fp-1022},
test/jdk/java/lang/StrictMath/Log1pTests.java line 220:
> 218: {-0x1.2e496d25897ecp-2, -0x1.663d81cb08f56p-2},
> 219: {-0x1.ffbaefe27p-2, -0x1.62e42faa93817p-1},
> 220: };
I think there's another case
Suggestion:
{-0x1.5efad5491a79bp-1022, -0x1.5efad5491a79bp-1022},
};
test/jdk/java/lang/StrictMath/LogTests.java line 74:
> 72: {0x1.0ffea3878db6bp+0, 0x1.f07a0cca521fp-5},
> 73: {0x1.490af72a25a81p-1, -0x1.c4bf7ae48f078p-2},
> 74: {0x1.69e7aa6da2df5p-1, -0x1.634508c9adfp-2},
There are no libraries that have worse errors than OpenLibm, so I'm wondering
what these values are good for?
test/jdk/java/lang/StrictMath/TrigTests.java line 171:
> 169: {-0x1.0e16eb809a35dp+944, 0x1.b5e361ed01dadp-2},
> 170: {-0x1.842d8ec8f752fp+21, -0x1.6ce864edeaffep-1},
> 171: {-0x1.1c49ad613ff3bp+19, -0x1.fffe203cfabe1p-2},
Some of these cases are for libraries that have _better_ worst case errors than
OpenLibm.
Are they needed here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15879#discussion_r1504067869
PR Review Comment: https://git.openjdk.org/jdk/pull/15879#discussion_r1504067977
PR Review Comment: https://git.openjdk.org/jdk/pull/15879#discussion_r1504068053
PR Review Comment: https://git.openjdk.org/jdk/pull/15879#discussion_r1504068092
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v16]
On Tue, 27 Feb 2024 02:47:13 GMT, Jatin Bhateja wrote:
>> Hi All,
>>
>> This patch optimizes sub-word gather operation for x86 targets with AVX2 and
>> AVX512 features.
>>
>> Following is the summary of changes:-
>>
>> 1) Intrinsify sub-word gather using hybrid algorithm which initially
>> partially unrolls scalar loop to accumulates values from gather indices into
>> a quadword(64bit) slice followed by vector permutation to place the slice
>> into appropriate vector lanes, it prevents code bloating and generates
>> compact JIT sequence. This coupled with savings from expansive array
>> allocation in existing java implementation translates into significant
>> performance of 1.5-10x gains with included micro.
>>
>> 
>>
>>
>> 2) Patch was also compared against modified java fallback implementation by
>> replacing temporary array allocation with zero initialized vector and a
>> scalar loops which inserts gathered values into vector. But, vector insert
>> operation in higher vector lanes is a three step process which first
>> extracts the upper vector 128 bit lane, updates it with gather subword value
>> and then inserts the lane back to its original position. This makes inserts
>> into higher order lanes costly w.r.t to proposed solution. In addition
>> generated JIT code for modified fallback implementation was very bulky. This
>> may impact in-lining decisions into caller contexts.
>>
>> Kindly review and share your feedback.
>>
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comment resolutions.
I sent the patch for testing.
And added some suggestions for comments in `C2_MacroAssembler::vgather_subword`.
I hope I have not been annoying you too much
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1672:
> 1670: Label GATHER8_LOOP;
> 1671: XMMRegister iota = xtmp1;
> 1672: XMMRegister two_vec = xtmp2;
I'm sorry to bother you so much with this. I think adding aliases that don't
mention what register they alias to makes things worse. Now I can't even see if
two names might alias to the same register.
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1672:
> 1670: Label GATHER8_LOOP;
> 1671: XMMRegister iota = xtmp1;
> 1672: XMMRegister two_vec = xtmp2;
Suggestion:
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1681:
> 1679: vpsubd(xtmp2, xtmp1, xtmp2, vlen_enc);
> 1680: vpslld(two_vec, xtmp2, 1, vlen_enc);
> 1681: load_iota_indices(iota, vector_len * type2aelembytes(elem_ty), T_INT);
Suggestion:
vpxor(xtmp1, xtmp1, xtmp1, vlen_enc); // xtmp1 = {0, ...}
vpxor(dst, dst, dst, vlen_enc); // dst = {0, ...}
vallones(xtmp2, vlen_enc);
vpsubd(xtmp2, xtmp1, xtmp2, vlen_enc);
vpslld(xtmp2, xtmp2, 1, vlen_enc); // xtmp2 = {2, 2, ...}
load_iota_indices(xtmp1, vector_len * type2aelembytes(elem_ty), T_INT); //
xtmp1 = {0, 1, 2, ...}
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1688:
> 1686: } else {
> 1687: LP64_ONLY(vgather8b_masked_offset(elem_ty, temp_dst, base,
> idx_base, offset, mask, mask_idx, rtmp, vlen_enc));
> 1688: }
Suggestion:
// TMP_VEC_64(temp_dst) = PICK_SUB_WORDS_FROM_GATHER_INDICES
if (mask == noreg) {
vgather8b_offset(elem_ty, temp_dst, base, idx_base, offset, rtmp,
vlen_enc);
} else {
LP64_ONLY(vgather8b_masked_offset(elem_ty, temp_dst, base, idx_base,
offset, mask, mask_idx, rtmp, vlen_enc));
}
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1694:
> 1692: addptr(idx_base, 32 >> (type2aelembytes(elem_ty) - 1));
> 1693: subl(length, 8 >> (type2aelembytes(elem_ty) - 1));
> 1694: jcc(Assembler::notEqual, GATHER8_LOOP);
Suggestion:
// TEMP_PERM_VEC(temp_dst) = PERMUTE TMP_VEC_64(temp_dst) PERM_INDEX(xtmp1)
vpermd(temp_dst, xtmp1, temp_dst, vlen_enc == Assembler::AVX_512bit ?
vlen_enc : Assembler::AVX_256bit);
// PERM_INDEX(xtmp1) = PERM_INDEX(xtmp1) - TWO_VEC(xtmp2)
vpsubd(xtmp1, xtmp1, xtmp2, vlen_enc);
// DST_VEC = DST_VEC OR TEMP_PERM_VEC
vpor(dst, dst, temp_dst, vlen_enc);
addptr(idx_base, 32 >> (type2aelembytes(elem_ty) - 1));
subl(length, 8 >> (type2aelembytes(elem_ty) - 1));
jcc(Assembler::notEqual, GATHER8_LOOP);
-
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16354#pullrequestreview-1903117002
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1503996753
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1504001356
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1504007501
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1504019352
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1504017498
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v16]
On Tue, 27 Feb 2024 10:25:19 GMT, Emanuel Peter wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comment resolutions.
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1672:
>
>> 1670: Label GATHER8_LOOP;
>> 1671: XMMRegister iota = xtmp1;
>> 1672: XMMRegister two_vec = xtmp2;
>
> I'm sorry to bother you so much with this. I think adding aliases that don't
> mention what register they alias to makes things worse. Now I can't even see
> if two names might alias to the same register.
As I said: you can also comment / document the use of registers. You don't have
to use better names if that is problematic.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1681:
>
>> 1679: vpsubd(xtmp2, xtmp1, xtmp2, vlen_enc);
>> 1680: vpslld(two_vec, xtmp2, 1, vlen_enc);
>> 1681: load_iota_indices(iota, vector_len * type2aelembytes(elem_ty),
>> T_INT);
>
> Suggestion:
>
> vpxor(xtmp1, xtmp1, xtmp1, vlen_enc); // xtmp1 = {0, ...}
> vpxor(dst, dst, dst, vlen_enc); // dst = {0, ...}
> vallones(xtmp2, vlen_enc);
> vpsubd(xtmp2, xtmp1, xtmp2, vlen_enc);
> vpslld(xtmp2, xtmp2, 1, vlen_enc); // xtmp2 = {2, 2, ...}
> load_iota_indices(xtmp1, vector_len * type2aelembytes(elem_ty), T_INT); //
> xtmp1 = {0, 1, 2, ...}
vallones(xtmp2, vlen_enc);
vpsubd(xtmp2, xtmp1, xtmp2, vlen_enc);
vpslld(xtmp2, xtmp2, 1, vlen_enc);
This is all to set up `xtmp2 = {2, 2, ...}` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1504000796
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1504022599
Integrated: 8325754: Dead AbstractQueuedSynchronizer$ConditionNodes survive minor garbage collections
On Wed, 21 Feb 2024 15:01:15 GMT, Viktor Klang wrote: > More aggressively breaking chains in order to prevent nodes promoted to older > generations standing in the way for collecting younger nodes. I decided that > it was most efficient to add this logic to the else-branch of updating the > firstWaiter and lastWaiter. > > There's a race with unlinkCancelledWaiters() but according to @DougLea it > should be a benign one. > > There's a performance impact of this, but as it is a plain write, and one to > null at that, it should be acceptable. This pull request has now been integrated. Changeset: 60cbf292 Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/60cbf2925024b1c2253256688ae41741fff0a860 Stats: 10 lines in 2 files changed: 10 ins; 0 del; 0 mod 8325754: Dead AbstractQueuedSynchronizer$ConditionNodes survive minor garbage collections Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17950
RFR: 8269428: java/util/concurrent/ConcurrentHashMap/ToArray.java timed out
As an intermediate fix to the test, switching to explicit usage of an ExecutorService seems to do the trick to make this test reliably pass. With that said, this test (CHM::ToArray.java) seems to trigger an issue in ForkJoinPool, so that would need to be fixed separately. Tagging @DougLea as an FYI. :) - Commit messages: - Switching CHM.ToArray test to use a dedicated thread pool Changes: https://git.openjdk.org/jdk/pull/18023/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18023=00 Issue: https://bugs.openjdk.org/browse/JDK-8269428 Stats: 49 lines in 1 file changed: 3 ins; 0 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/18023.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18023/head:pull/18023 PR: https://git.openjdk.org/jdk/pull/18023
RFR: 8326744: Class-File API transition to Second Preview
Task providing Class-File API transition to Second Preview. - Commit messages: - 8326744: Class-File API transition to Second Preview Changes: https://git.openjdk.org/jdk/pull/18021/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18021=00 Issue: https://bugs.openjdk.org/browse/JDK-8326744 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18021.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18021/head:pull/18021 PR: https://git.openjdk.org/jdk/pull/18021
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v9]
On Mon, 26 Feb 2024 20:46:01 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which proposes that we officially deprecate the >> following four methods in the `java.util.zip` package: >> >> * `Inflater.getTotalIn()` >> * `Inflater.getTotalOut()` >> * `Deflater.getTotalIn()` >> * `Deflater.getTotalOut()` >> >> Since these legacy methods return `int`, they cannot safely return the >> number of bytes processed without the risk of losing information about the >> magnitude or even sign of the returned value. >> >> The corresponding methods `getBytesRead()` and `getBytesWritten()` methods >> introduced in Java 5 return `long`, and should be used instead when >> obtaining this information. >> >> Unrelated to the deprecation itself, the documentation currently does not >> specify what these methods are expected to return when the number of >> processed bytes is higher than `Integer.MAX_VALUE`. This PR aims to clarify >> this in the API specification. >> >> Initally, this PR handles only `Inflater.getTotalIn()`. The other three >> methods will be updated once the wordsmithing for this method stabilizes. > > Eirik Bjørsnøs 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 12 additional > commits since the last revision: > > - Use "value" instead of "number of compressed bytes input so far" > - Merge branch 'master' into deprecate-total-in-out > - Reduce new method description text to one sentence > - To align with the Java Language Specification, use "all but the 32 lowest > order bits" > - Use "highest order bits" instead of "highest bits" > - Remove empty line > - Add back a note to specify the long-standing behavior of discarding the > highest 32 bits of the return value > - Use "greater than" instead of "larger than" > - Leave first sentence as-is. Simplify the deprecation notice. > - Add since="23" to @Deprecated > - ... and 2 more: https://git.openjdk.org/jdk/compare/285554fe...8d80c776 The change in [8d80c776] looks fine. If Lance is okay with it then I think go ahead and update the other methods too. - PR Comment: https://git.openjdk.org/jdk/pull/17919#issuecomment-1966049658
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE Just a small question by the way: All those new parameters to SetupNativeCompilation, were they always there and the comments about them were just missing from the documentation about the function? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966025499
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:07:38 GMT, Daniel Jeliński wrote: > can we get rid of LDCXX? On my system LDCXX is mapped to `g++` and LD is > `gcc`; I searched for the differences, and the only thing I could find is > that `g++` implicitly adds `-lstdc++ -shared-libgcc`; I suppose we could > explicitly add these options, and use gcc everywhere. > > See: https://stackoverflow.com/a/172592 > https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html I don't think that's a good idea, the differences between the gcc and g++ drivers are supposed to be (opaque) implementation details. Doing so would make us dependent on internal mechanisms of gcc subject to change between versions and would make bugfixing hard to do if such a change really happened and the linker command line got messed up as a result (This likely would impact clang too, unless I am mistaken?) - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966002485
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE can we get rid of LDCXX? On my system LDCXX is mapped to `g++` and LD is `gcc`; I searched for the differences, and the only thing I could find is that `g++` implicitly adds `-lstdc++ -shared-libgcc`; I suppose we could explicitly add these options, and use gcc everywhere. See: https://stackoverflow.com/a/172592 https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1965991536
