Re: RFR: 8333396: Performance regression of new DecimalFormat and DecimalFormat.format [v2]
> ### Performance regression of DecimalFormat.format
> From the output of perf, we can see the hottest regions contain atomic
> instructions. But when run with JDK 11, there is no such problem. The reason
> is the removed biased locking.
> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
> contains many synchronized methods.
> So I added support for some new methods that accept StringBuilder which is
> lock-free.
>
> ### Performance regression of new DecimalFormat
> After comparing the flame graph between current jdk and jdk 11, the method
> java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time.
> The performance becomes as good as jdk11 after replacing it with a simple
> loop implementation.
>
>
>
> ### Test result
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> private DecimalFormat format;
>
> @Setup(Level.Trial)
> public void setup() {
> format = new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testNewAndFormat() throws InterruptedException {
> new DecimalFormat("#0.0").format(9524234.1236457);
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
>
> @Benchmark
> public void testFormatOnly() throws InterruptedException {
> format.format(9524234.1236457);
> }
> }
>
> Current JDK before optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> Current JDK after optimize
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> ### JDK 11
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
lingjun-cg has updated the pull request incrementally with one additional
commit since the last revision:
896: Performance regression of new DecimalFormat and DecimalFormat.format
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19513/files
- new: https://git.openjdk.org/jdk/pull/19513/files/a6755f8f..b48962b5
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19513.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19513/head:pull/19513
PR: https://git.openjdk.org/jdk/pull/19513
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
On Mon, 3 Jun 2024 05:06:00 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. The CSR is available for review at https://bugs.openjdk.org/browse/JDK-8333400 - PR Comment: https://git.openjdk.org/jdk/pull/19515#issuecomment-2144323834
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v2]
> Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: also note that the method may block, just like DeflaterInputStream specifies it - Changes: - all: https://git.openjdk.org/jdk/pull/19515/files - new: https://git.openjdk.org/jdk/pull/19515/files/2a116fe4..e13e65d2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19515&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19515&range=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
On Mon, 3 Jun 2024 04:25:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which uncomments an > additional test that was commented out in > `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`? > > As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue > due to which this line was commented out in the test, has been fixed several > releases back and thus this line can now be uncommented. > > I've verified that this test continues to pass with these changes. Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19514#pullrequestreview-2092824056
RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
Can I please get a review of this change which updates the API specification of `java.util.zip.InflaterInputStream.skip()` method to match its current implementation? `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a `long` value `n` representing the number of bytes to skip. When a value greater than `Integer.MAX_VALUE` is passed to this `InflaterInputStream.skip()` method, the current implementation in that method only skips at most `Integer.MAX_VALUE` bytes. `DeflaterInputStream.skip()` too has the same behaviour. However, in the case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that method's API documentation. `InflaterInputStream.skip()` currently doesn't specify this behaviour. The commit in this PR updates the `InflaterInputStream.skip()` to specify the current implementation. I'll open a CSR for this change. - Commit messages: - 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE Changes: https://git.openjdk.org/jdk/pull/19515/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19515&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8206447 Stats: 10 lines in 2 files changed: 7 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
Can I please get a review of this test-only change which uncomments an additional test that was commented out in `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`? As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue due to which this line was commented out in the test, has been fixed several releases back and thus this line can now be uncommented. I've verified that this test continues to pass with these changes. - Commit messages: - 898: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java Changes: https://git.openjdk.org/jdk/pull/19514/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19514&range=00 Issue: https://bugs.openjdk.org/browse/JDK-898 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19514.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19514/head:pull/19514 PR: https://git.openjdk.org/jdk/pull/19514
RFR: 8333396: Performance regression of new DecimalFormat and DecimalFormat.format
### Performance regression of DecimalFormat.format >From the output of perf, we can see the hottest regions contain atomic >instructions. But when run with JDK 11, there is no such problem. The reason >is the removed biased locking. The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself contains many synchronized methods. So I added support for some new methods that accept StringBuilder which is lock-free. ### Performance regression of new DecimalFormat After comparing the flame graph between current jdk and jdk 11, the method java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time. The performance becomes as good as jdk11 after replacing it with a simple loop implementation. - Commit messages: - 896: Performance regression of new DecimalFormat and DecimalFormat.format Changes: https://git.openjdk.org/jdk/pull/19513/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19513&range=00 Issue: https://bugs.openjdk.org/browse/JDK-896 Stats: 318 lines in 11 files changed: 268 ins; 0 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/19513.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19513/head:pull/19513 PR: https://git.openjdk.org/jdk/pull/19513
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Wed, 22 May 2024 13:23:25 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 with a new target base due to a > merge or a rebase. The pull request now contains 110 commits: > > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - Use shorter name for the build tool > - Review feedback from Erik J. > - Remove dependency on CDS which isn't needed anymore > - ... and 100 more: https://git.openjdk.org/jdk/compare/4f1a10f8...e1e3f02f I've been looking through the changes. One thing that I'm wondering about is whether --generate-runtime-link-image should disable the keeping of packaged modules (set JLINK_KEEP_PACKAGED_MODULES to false). It seems surprising to use this configure option and have the jlink command in the build also copy the packaged modules into the image. (Doing that would of course mean that several existing jlink tests would need some changes, either to `@requires` or to remove the checks for the `jmods` directory) - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2143963280
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v17]
On Fri, 31 May 2024 13:18:33 GMT, Doug Lea wrote:
>> This set of changes address causes of poor utilization with small numbers of
>> cores due to overly aggressive contention avoidance. A number of further
>> adjustments were needed to still avoid most contention effects in
>> deployments with large numbers of cores
>
> Doug Lea has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reconcile changes
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1345:
> 1343: int b = base, p = top, cap;
> 1344: if (p - b > 0 && a != null && (cap = a.length) > 0) {
> 1345: for (int m = cap - 1, s, nb;;) {
@DougLea I'm curious, what effect did you see if moving the `p - b > 0` out of
the loop conditional?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1623453077
