Withdrawn: 8320759: Creation of random BigIntegers can be made faster
On Sun, 26 Nov 2023 16:52:40 GMT, fabioromano1 wrote: > A faster and simpler way to generate random BigIntegers, avoiding eventually > trimming of leading zeros in magnitude array. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16817
Withdrawn: 8322292: Rearrange comparison of fields in Record.equals()
On Mon, 18 Dec 2023 13:42:35 GMT, Sergey Tsypanov wrote:
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/17143
Withdrawn: 8323186: A faster algorithm for MutablebigInteger.divWord(long, int)
On Sat, 6 Jan 2024 18:01:01 GMT, fabioromano1 wrote: > The method `MutableBigInteger.divWord(long, int)` can use the algorithm of > Hacker's Delight (2nd ed), section 9.3, the same used in > `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, > to get the computation faster. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17291
Withdrawn: 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
On Tue, 16 Jan 2024 10:19:44 GMT, Andrey Turbanov wrote: > There are 3 methods in `java.util.TimeZone` which are `public static` and > marked as `synchronized`: > 1. getTimeZone(String) > 2. getAvailableIDs(int) > 3. getAvailableIDs() > > This means it is a bottle neck for the whole VM. > I've checked the implementation and concluded that `synchronized` is > unnecessary. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17441
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote: >> This provides a slightly more accurate bounding limit for >> `computeNextExponentialSoftCapped` when calling it from >> `computeNextGaussian`. This could cause the `while >> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in >> `computeNextGaussian` on line 1402 to always be true, making the >> `nextGaussian` runtime unbounded in the worst case; but more likely, it >> would give a result that was truncated too close to zero. >> >> This change is being tested prior to submission to OpenJDK by >> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Bug fix: add-exports was for wrong package Keep open. @JimLaskey it looks like you're the author of most of the surrounding code; can you please confirm that there's a rounding error in the existing implementation and that this PR will fix it? - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2101885931
Withdrawn: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:10:54 GMT, Quan Anh Mai wrote: > Hi, > > This patch introduces `JitCompiler::isConstantExpression` which can be used > to statically determine whether an expression has been constant-folded by the > Jit compiler, leading to more constant-folding opportunities. For example, it > can be used in `MemorySessionImpl::checkValidStateRaw` to eliminate the > lifetime check on global sessions without imposing additional branches on > other non-global sessions. This is similar to `__builtin_constant_p` in GCC > and clang. > > Please kindly give your opinion as well as your reviews, thanks very much. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17527
Re: RFR: 8331957: Release Note: System Property for Java SE Specification Maintenance Version
On Thu, 9 May 2024 00:49:43 GMT, xiaotaonan wrote: > …ersion > > Release Note: System Property for Java SE Specification Maintenance Version @xiaotaonan For your information, "Release Note" issues are not bugs or features that require a pull request, but Java Bug System tasks that serve as a reminder for release notes for a new Java release like at https://www.oracle.com/java/technologies/javase/22-relnote-issues.html And this uncommenting is only intended for maintenance releases in reference implementations (Such as https://github.com/openjdk/jdk11u-ri/commit/e5ac7a1b7e8df1e56a7e78bba6a2b9ed7fc297f1 and https://github.com/openjdk/jdk17u-ri/commit/c2c9e7fb8c857e40bc43b4053c2633825d4fb68e). See #8437 as well. - PR Comment: https://git.openjdk.org/jdk/pull/19149#issuecomment-2101845256
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 9 May 2024 02:09:50 GMT, xiaotaonan wrote: > Clean up non-standard use of /// comments in `java.base` Related to #19130. Good catch, these were probably not detected because they were compiled at Java 8 language level and thus not detected by the new compiler warnings. - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2101834976
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
On Thu, 9 May 2024 02:28:16 GMT, ExE Boss wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Validate after copying to avoid TOCTOU
>
> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
> line 189:
>
>> 187: // Validate after copying to avoid TOCTOU
>> 188: for (int i = pos; i < destPos; i++) {
>> 189: validateArgument(argTypes[i]);
>
> Suggestion:
>
> validateArgument(newArgs[i]);
Weird that this is not caught by MethodTypeDescTest.assertMethodType for no-arg
method types.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594896764
RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
Clean up non-standard use of /// comments in `java.base` - Commit messages: - Clean up non-standard use of /// comments in Changes: https://git.openjdk.org/jdk/pull/19151/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19151=00 Issue: https://bugs.openjdk.org/browse/JDK-8331879 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19151.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19151/head:pull/19151 PR: https://git.openjdk.org/jdk/pull/19151
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
On Wed, 8 May 2024 20:17:18 GMT, Claes Redestad wrote:
>> This PR suggests refactoring the implementation classes of
>> java.lang.constant into a new package jdk.internal.constant to enable
>> sharing some trusted static factory methods with users elsewhere in
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring
>> also adds some "trusted" methods for use when input is pre-validated, and
>> makes use of them where applicable in the current implementation. There are
>> more changes in the pipeline which will be folded into #17108 or PR'ed once
>> that is integrated.
>>
>> There are two optimizations mixed up here. One in
>> `MethodTypeDesc.ofDescriptor`:
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ±
>> 1,126 ns/op 1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ±
>> 3,211 ns/op 1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op
>> 1,89x (p = 0,000*)
>> * = significant
>>
>>
>> The other in `ClassDesc::nested`, where to get rid of a simple static method
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory
>> method:
>>
>> Name CntBase ErrorTest
>> Error Unit Change
>> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ±
>> 0,573 ns/op 9,53x (p = 0,000*)
>> * = significant
>>
>>
>> The optimizations could be split out and PRd independently, but I think they
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Validate after copying to avoid TOCTOU
src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line
189:
> 187: // Validate after copying to avoid TOCTOU
> 188: for (int i = pos; i < destPos; i++) {
> 189: validateArgument(argTypes[i]);
Suggestion:
validateArgument(newArgs[i]);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594890009
Re: RFR: 8331957: Release Note: System Property for Java SE Specification Maintenance Version
On Thu, 9 May 2024 00:49:43 GMT, xiaotaonan wrote: > …ersion > > Release Note: System Property for Java SE Specification Maintenance Version This system property should not be set for a feature release. It should only be set if there has been a JCP Maintenance Release. Perhaps this PR should be against a different repo? - Changes requested by iris (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19149#pullrequestreview-2047019025
RFR: 8331957: Release Note: System Property for Java SE Specification Maintenance Version
…ersion Release Note: System Property for Java SE Specification Maintenance Version - Commit messages: - Release Note: System Property for Java SE Specification Maintenance Version Changes: https://git.openjdk.org/jdk/pull/19149/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19149=00 Issue: https://bugs.openjdk.org/browse/JDK-8331957 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19149.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19149/head:pull/19149 PR: https://git.openjdk.org/jdk/pull/19149
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v30]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: small review tweaks; shorten MemoryConsistency links - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/a41e6fc2..5db47889 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=29 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=28-29 Stats: 8 lines in 3 files changed: 0 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8311864: Add ArraysSupport.hashCode(int[] a, fromIndex, length, initialValue) [v6]
> This PR adds an internal method to calculate hash code from the provided > integer array, offset and length into that array, and the initial hash code > value. > > Current options for calculating a hash code for int[] are inflexible. It's > either ArraysSupport.vectorizedHashCode with an offset, length and initial > value, or Arrays.hashCode with just an array. > > For an arbitrary int[], unconditional vectorization might be unwarranted or > puzzling. Unfortunately, it's the only hash code method that operates on an > array subrange or accepts the initial hash code value. > > A more convenient method could be added and then used, for example, here: > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083 > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680 > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/sun/security/pkcs10/PKCS10.java#L356-L362 > > This PR adds such a method and provides tests for it. Additionally, this PR > adds tests for `null` passed to `java.util.Arrays.hashCode` overloads, > behaviour which was specified but untested. Pavel Rappo 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 seven additional commits since the last revision: - Merge branch 'master' into 8311864 - Merge branch 'master' into 8311864 - Merge branch 'master' into 8311864 - Merge branch 'master' into 8311864 - Merge remote-tracking branch 'jdk/master' into 8311864 - Merge branch 'master' into 8311864 - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/14831/files - new: https://git.openjdk.org/jdk/pull/14831/files/2d06e03a..aa6d053c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14831=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14831=04-05 Stats: 119667 lines in 3101 files changed: 55211 ins; 48378 del; 16078 mod Patch: https://git.openjdk.org/jdk/pull/14831.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14831/head:pull/14831 PR: https://git.openjdk.org/jdk/pull/14831
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v29]
On Wed, 8 May 2024 08:33:31 GMT, Alan Bateman wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> small grammar fixes
>
> src/java.base/share/classes/java/lang/ref/Cleaner.java line 224:
>
>> 222: * Actions in a thread prior to calling {@code Cleaner.register()}
>> 223: * > href="{@docRoot}/java.base/java/util/concurrent/package-summary.html#MemoryVisibility">happen-before
>> 224: * the cleaning action is run by the Cleaner's thread.
>
> In passing, you could reduces the html refs to
> "package-summary.html#MemoryConsistency" and
> "package-summary.html#MemoryVisibility" as it's the same package/directory.
MemoryVisibility is in java.util.concurrent
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1594785182
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 19:40:44 GMT, Stuart Marks wrote: >>> Most of these are defined in terms of `String.valueOf(Object)` which if >>> passed a null reference will return the String object containing `null`. So >>> no, I don't think NPE should be thrown. It might be worth mentioning this >>> explicitly somewhere though. >> >> Right, in our current model, `Console.readln(prompt)` can be reduced to >> `Console.print(prompt)` followed by actual reading. `Console.print(prompt)` >> can in turn be reduced to printing the result of `String.valueOf((Object) >> prompt)`, which is a string "null". >> >> The above reduction means that `Console.readln(null)` will not throw NPE. >> Now, what I'm asking if that's what we really want. >> >> With `Console.println(null)` and `Console.print(null)` not throwing NPE >> makes sense. However, with `Console.readln(null)` it might not. After all, >> we ask for a **`String`** prompt, not an `Object` prompt. All other methods >> of `Console` that accept `String`, throw NPE if that string is null. >> >> One more consideration. Whatever the behaviour we choose for >> `Console.readln(null)`, we should choose the same behaviour for >> `IO.readln(null)`. Otherwise, it would be a constant source of unpleasantly >> surprising inconsistency. >> >> Thoughts? > > A fair question, whether to treat `readln` differently because it takes a > String argument instead of an Object. > > I guess I'd like to see a scheme like the following: > > String s = cons.readln(prompt); > > is effectively the same as > > cons.print(prompt); > cons.flush(); > String s = cons.readLine(); // note no-arg readLine() method > > In other words, define `readln` to behave "as if" the other methods were > called. This would end up with a null prompt argument printing "null" and not > throwing NPE. > > This is internally consistent, at least. Beginning programmers will > eventually learn that using null will lead to NPE. It might make things > easier if NPEs were kept out of this little corner of the library. That's > just my guess though. > > Also, recall that `String.valueOf((Object) null)` and > `String.valueOf((String) null)` both result in "null". But if one were to > write `String.valueOf(null)` that throws NPE, because that selects the > `valueOf(char[])` overload. That's kind of the outlier. Note further that the > built-in `+` string concatenation operator will also convert null references > to "null". > > Personally I've always hated that string conversions of null in Java often > result in "null" but it's baked into the system fairly deeply, so I think we > should stick with it. Okay, I'll push a commit soon. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1594784225
Withdrawn: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Fri, 24 Nov 2023 10:34:02 GMT, Stefan Karlsson wrote: > Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
On Tue, 7 May 2024 16:52:12 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 105 commits: > > - 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 > - Fix coment > - Fix comment > - Fix typo > - Revert some now unneded build changes > - Follow build tools naming convention > - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca make/Images.gmk line 100: > 98: > 99: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) > 100: JLINK_JDK_EXTRA_OPTS := $(JLINK_JDK_EXTRA_OPTS) > --generate-linkable-runtime I just noticed this slight improvement: Suggestion: JLINK_JDK_EXTRA_OPTS += --generate-linkable-runtime - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1594774220
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4) [v3]
On Wed, 8 May 2024 11:36:07 GMT, serhiysachkov wrote:
>> Calendar.add() tests that describe its behavior.
>
> serhiysachkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8331646 consolidating test methods into ParameterizedTests
Thanks for the changes. Also, match this PR title to the JBS title (remove that
`Task - P4`)
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 41:
> 39:
> 40: import static java.util.Calendar.*;
> 41: import static java.util.Calendar.MONTH;
It's better to explicitly declare which fields are statically imported from
`Calendar`.
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 54:
> 52: int value, int calendarField, int
> expectedDate, int expectedMonth,
> 53: int expectedYear) {
> 54: Calendar calendar = Calendar.getInstance();
Instead of using a factory on `Calendar`, this should explicitly construct a
`GregorianCalendar` instance which guarantees the leap-year behavior.
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 69:
> 67: public void testMonthYearAddSubtractNonLeapYear() {
> 68: Calendar calendar = Calendar.getInstance();
> 69: calendar.set(2024, 1, 29, 15, 0, 0);
It's easier to understand using `FEBRUARY`, instead of the immediate value `1`.
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 72:
> 70: calendar.add(Calendar.MONTH, 1);
> 71: calendar.add(YEAR, -1);
> 72: calendar.add(Calendar.MONTH, -1);
`Calendar.MONTH` -> `MONTH`
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 118:
> 116: Arguments.of("testMonthAddLeapYear", 29, 1, 2024, 1,
> MONTH, 29, MARCH, 2024),
> 117: Arguments.of("testOneMonthSubtractLeapYear", 31, 2,
> 2024, -1, MONTH, 29, FEBRUARY, 2024),
> 118: Arguments.of("testTwoMonthSubtractLeapYear", 30, 3,
> 2024, -2, MONTH, 29, FEBRUARY, 2024)
Please use those month fields in those arguments.
-
PR Review: https://git.openjdk.org/jdk/pull/19082#pullrequestreview-2046786753
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594729131
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594732962
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594730699
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594731131
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594742243
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v10]
On Fri, 23 Feb 2024 14:37:20 GMT, Alan Bateman wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> VM -> virtual machine; also fix misspelling > > src/java.base/share/classes/java/lang/ref/Cleaner.java line 218: > >> 216: * cautions about the behavior of cleaning actions. >> 217: * >> 218: * The object being registered is kept strongly reachable (and >> therefore not eligible > > I would be tempted to drop "being registered" from this sentence. The > previous paragraph and the parameter descriptions use "the object". That > would make it "The given object is kept strongly reachable ..." which I > think is a bit easier to read. If it's clear which object is meant, then yes, that reads better. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1594756233
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 18:32:42 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove redundant constructor
>
> src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 84:
>
>> 82: */
>> 83: public static Supplier, ReferenceKey>>
>> concurrentHashMapSupplier() {
>> 84: return new Supplier<>() {
>
> Can this just `return ReferencedKeyMap.concurrentHashMapSupplier();`?
It compiles, so maybe. :-) Running some tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594703469
Re: RFR: 8331866: Add warnings for locale data dependence
On Wed, 8 May 2024 20:21:50 GMT, Naoto Sato wrote: > In order to enlighten users to not depend on a particular version of the > locale data, putting a note into the JDK vs. CLDR version chart would be > appropriate. LGTM. Wording in the beginning of the class spec makes it clear that locale sensitive factory methods are dependent on the LSP. By now adding wording that the CLDR data changes, it should hopefully help to reduce the expectation from users of consistent data between releases for these locale sensitive methods. - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/19145#pullrequestreview-2046636580
Re: RFR: 8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Mon, 6 May 2024 23:26:49 GMT, Srinivas Vamsi Parasa wrote: >> Hello Vamsi (@vamsi-parasa), >> >> Could you please run the new benchmarking to detect the best case >> for Radix sort and parallel sorting? >> >> What you need is to compile and run JavaBenchmarkHarness: >> >> javac --patch-module java.base=. -d classes *.java >> java -XX:CompileThreshold=1 -XX:-TieredCompilation --patch-module >> java.base=classes -cp classes java.util.JavaBenchmarkHarness >> >> Find the sources there: >> >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_a.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_5.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_11.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_12.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_13.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_14.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_21.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r30_23.java >> https://github.com/iaroslavski/sorting/blob/master/radixsort/JavaBenchmarkHarness.java >> >> Thank you, >> Vladimir > > Hi Vladimir (@iaroslavski), > > Please see the data below. > > Thanks, > Vamsi > > name | builder | size | mode | count | score > -- | -- | -- | -- | -- | -- > b01 | RANDOM | 600 | avg | 325677 | 6.764 > b01 | RANDOM | 3000 | avg | 52041 | 77.742 > b01 | RANDOM | 9 | avg | 1217 | 4449.668 > b01 | RANDOM | 40 | avg | 242 | 22764.05 > b01 | RANDOM | 100 | avg | 90 | 60737.71 > b01 | REPEATED | 600 | avg | 651354 | 1.723 > b01 | REPEATED | 3000 | avg | 104083 | 12.383 > b01 | REPEATED | 9 | avg | 2435 | 714.451 > b01 | REPEATED | 40 | avg | 484 | 3039.447 > b01 | REPEATED | 100 | avg | 180 | 8114.503 > b01 | SAWTOOTH | 600 | avg | 1954062 | 1.009 > b01 | SAWTOOTH | 3000 | avg | 312251 | 4.94 > b01 | SAWTOOTH | 9 | avg | 7305 | 133.192 > b01 | SAWTOOTH | 40 | avg | 1453 | 591.854 > b01 | SAWTOOTH | 100 | avg | 542 | 1494.252 > b01 | STAGGER | 600 | avg | 1954062 | 8.252 > b01 | STAGGER | 3000 | avg | 312251 | 10.449 > b01 | STAGGER | 9 | avg | 7305 | 287.811 > b01 | STAGGER | 40 | avg | 1453 | 1288.92 > b01 | STAGGER | 100 | avg | 542 | 3245.649 > b01 | SHUFFLE | 600 | avg | 325677 | 5.199 > b01 | SHUFFLE | 3000 | avg | 52041 | 29.734 > b01 | SHUFFLE | 9 | avg | 1217 | 1392.125 > b01 | SHUFFLE | 40 | avg | 242 | 5772.859 > b01 | SHUFFLE | 100 | avg | 90 | 15483.65 > r30 | RANDOM | 600 | avg | 325677 | 4.307 > r30 | RANDOM | 3000 | avg | 52041 | 71.438 > r30 | RANDOM | 9 | avg | 1217 | 3971.947 > r30 | RANDOM | 40 | avg | 242 | 19924.32 > r30 | RANDOM | 100 | avg | 90 | 53671.9 > r30 | REPEATED | 600 | avg | 651354 | 1.36 > r30 | REPEATED | 3000 | avg | 104083 | 6.415 > r30 | REPEATED | 9 | avg | 2435 | 578.708 > r30 | REPEATED | 40 | avg | 484 | 2488.414 > r30 | REPEATED | 100 | avg | 180 | 6280.025 > r30 | SAWTOOTH | 600 | avg | 1954062 | 0.488 > r30 | SAWTOOTH | 3000 | avg | 312251 | 2.409 > r30 | SAWTOOTH | 9 | avg | 7305 | 71.98 > r30 | SAWTOOTH | 40 | avg | 1453 | 343.433 > r30 | SAWTOOTH | 100 | avg | 542 | 954.287 > r30 | STAGGER | 600 | avg | 1954062 | 1.064 > r30 | STAGGER | 3000 | avg | 312251 | 4.559 > r30 | STAGGER | 9 | avg | 7305 | 135.383 > r30 | STAGGER | 40 | avg | 1453 | 626.657 > r30 | STAGGER | 100 | avg | 542 | 1653.92 > r30 | SHUFFLE | 600 | avg | 325677 | 2.924 > r30 | SHUFFLE | 3000 | avg | 52041 | 18.819 > r30 | SHUFFLE | 9 | avg | 1217 | 1019.036 > r30 | SHUFFLE | 40 | avg | 242 | 4661.484 > r30 | SHUFFLE | 100 | avg | 90 | 11333.52 > r30_a | RANDOM | 600 | avg | 325677 | 4.024 > r30_a | RANDOM | 3000 | avg | 52041 | 72.86 > r30_a | ... Hello Vamsi (@vamsi-parasa), Could you please run the new benchmarking to finalize the best version? What you need is to compile and run JavaBenchmarkHarness: javac --patch-module java.base=. -d classes *.java java -XX:CompileThreshold=1 -XX:-TieredCompilation --patch-module java.base=classes -cp classes java.util.JavaBenchmarkHarness Find the sources there: https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r31_11.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r31_11a.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r31_12.java
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
On Wed, 8 May 2024 20:24:52 GMT, Chen Liang wrote: > This patch allows us to merge parsing logic for invoke, constant, and > classfile in the future, all in jdk.internal. Thanks for reviewing! Yes, that's the idea. - PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2101371721
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
On Wed, 8 May 2024 20:17:18 GMT, Claes Redestad wrote: >> This PR suggests refactoring the implementation classes of >> java.lang.constant into a new package jdk.internal.constant to enable >> sharing some trusted static factory methods with users elsewhere in >> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring >> also adds some "trusted" methods for use when input is pre-validated, and >> makes use of them where applicable in the current implementation. There are >> more changes in the pipeline which will be folded into #17108 or PR'ed once >> that is integrated. >> >> There are two optimizations mixed up here. One in >> `MethodTypeDesc.ofDescriptor`: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.ofDescriptor >> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ± >> 1,126 ns/op 1,01x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ± >> 3,211 ns/op 1,07x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x >> (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op >> 1,89x (p = 0,000*) >> * = significant >> >> >> The other in `ClassDesc::nested`, where to get rid of a simple static method >> in `ConstantUtils` I ended up speeding up and simplifying the public factory >> method: >> >> Name CntBase ErrorTest >> Error Unit Change >> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± >> 0,573 ns/op 9,53x (p = 0,000*) >> * = significant >> >> >> The optimizations could be split out and PRd independently, but I think they >> are simple enough to include in this refactoring. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Validate after copying to avoid TOCTOU This patch allows us to merge parsing logic for invoke, constant, and classfile in the future, all in jdk.internal. - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/19105#pullrequestreview-2046571446
RFR: 8331866: Add warnings for locale data dependence
In order to enlighten users to not depend on a particular version of the locale data, putting a note into the JDK vs. CLDR version chart would be appropriate. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19145/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19145=00 Issue: https://bugs.openjdk.org/browse/JDK-8331866 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19145.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19145/head:pull/19145 PR: https://git.openjdk.org/jdk/pull/19145
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]
On Wed, 8 May 2024 19:06:35 GMT, ExE Boss wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Refactor to further avoid re-validating arguments
>
> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
> line 179:
>
>> 177:
>> 178: for (ClassDesc param : paramTypes)
>> 179: validateArgument(param);
>
> This check should be performed as part of copying `paramTypes` to `newArgs`
> to avoid TOC/TOU issues, e.g.:
>
> @Override
> public MethodTypeDesc insertParameterTypes(int pos, ClassDesc... paramTypes) {
> if (pos < 0 || pos > argTypes.length)
> throw new IndexOutOfBoundsException(pos);
>
> ClassDesc[] newArgs = new ClassDesc[argTypes.length +
> paramTypes.length];
> if (pos > 0) {
> System.arraycopy(argTypes, 0, newArgs, 0, pos);
> }
> for (int i = 0; i < paramTypes.length; i++) {
> newArgs[pos + i] = validateArgument(paramTypes[i]);
> }
> if (pos < argTypes.length) {
> System.arraycopy(argTypes, pos, newArgs, pos +
> paramTypes.length, argTypes.length - pos);
> }
> return ofValidated(returnType, newArgs);
> }
>
>
> See also:
> https://github.com/openjdk/jdk/blob/230fac80f25e9608006c8928a8a7708bf13a818c/src/java.base/share/classes/java/util/ImmutableCollections.java#L186-L195
Nice catch! I conservatively just move the validation loop after to keep the
arraycopying.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594579344
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Validate after copying to avoid TOCTOU - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/eb23cf51..b21d3e60 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=04-05 Stats: 10 lines in 1 file changed: 6 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 18:25:50 GMT, Pavel Rappo wrote: >> Most of these are defined in terms of `String.valueOf(Object)` which if >> passed a null reference will return the String object containing `null`. So >> no, I don't think NPE should be thrown. It might be worth mentioning this >> explicitly somewhere though. > >> Most of these are defined in terms of `String.valueOf(Object)` which if >> passed a null reference will return the String object containing `null`. So >> no, I don't think NPE should be thrown. It might be worth mentioning this >> explicitly somewhere though. > > Right, in our current model, `Console.readln(prompt)` can be reduced to > `Console.print(prompt)` followed by actual reading. `Console.print(prompt)` > can in turn be reduced to printing the result of `String.valueOf((Object) > prompt)`, which is a string "null". > > The above reduction means that `Console.readln(null)` will not throw NPE. > Now, what I'm asking if that's what we really want. > > With `Console.println(null)` and `Console.print(null)` not throwing NPE makes > sense. However, with `Console.readln(null)` it might not. After all, we ask > for a **`String`** prompt, not an `Object` prompt. All other methods of > `Console` that accept `String`, throw NPE if that string is null. > > One more consideration. Whatever the behaviour we choose for > `Console.readln(null)`, we should choose the same behaviour for > `IO.readln(null)`. Otherwise, it would be a constant source of unpleasantly > surprising inconsistency. > > Thoughts? A fair question, whether to treat `readln` differently because it takes a String argument instead of an Object. I guess I'd like to see a scheme like the following: String s = cons.readln(prompt); is effectively the same as cons.print(prompt); cons.flush(); String s = cons.readLine(); // note no-arg readLine() method In other words, define `readln` to behave "as if" the other methods were called. This would end up with a null prompt argument printing "null" and not throwing NPE. This is internally consistent, at least. Beginning programmers will eventually learn that using null will lead to NPE. It might make things easier if NPEs were kept out of this little corner of the library. That's just my guess though. Also, recall that `String.valueOf((Object) null)` and `String.valueOf((String) null)` both result in "null". But if one were to write `String.valueOf(null)` that throws NPE, because that selects the `valueOf(char[])` overload. That's kind of the outlier. Note further that the built-in `+` string concatenation operator will also convert null references to "null". Personally I've always hated that string conversions of null in Java often result in "null" but it's baked into the system fairly deeply, so I think we should stick with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1594542505
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]
On Wed, 8 May 2024 10:42:27 GMT, Claes Redestad wrote:
>> This PR suggests refactoring the implementation classes of
>> java.lang.constant into a new package jdk.internal.constant to enable
>> sharing some trusted static factory methods with users elsewhere in
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring
>> also adds some "trusted" methods for use when input is pre-validated, and
>> makes use of them where applicable in the current implementation. There are
>> more changes in the pipeline which will be folded into #17108 or PR'ed once
>> that is integrated.
>>
>> There are two optimizations mixed up here. One in
>> `MethodTypeDesc.ofDescriptor`:
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ±
>> 1,126 ns/op 1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ±
>> 3,211 ns/op 1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op
>> 1,89x (p = 0,000*)
>> * = significant
>>
>>
>> The other in `ClassDesc::nested`, where to get rid of a simple static method
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory
>> method:
>>
>> Name CntBase ErrorTest
>> Error Unit Change
>> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ±
>> 0,573 ns/op 9,53x (p = 0,000*)
>> * = significant
>>
>>
>> The optimizations could be split out and PRd independently, but I think they
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refactor to further avoid re-validating arguments
src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line
179:
> 177:
> 178: for (ClassDesc param : paramTypes)
> 179: validateArgument(param);
This check should be performed as part of copying `paramTypes` to `newArgs` to
avoid TOC/TOU issues, e.g.:
@Override
public MethodTypeDesc insertParameterTypes(int pos, ClassDesc... paramTypes) {
if (pos < 0 || pos > argTypes.length)
throw new IndexOutOfBoundsException(pos);
ClassDesc[] newArgs = new ClassDesc[argTypes.length +
paramTypes.length];
if (pos > 0) {
System.arraycopy(argTypes, 0, newArgs, 0, pos);
}
for (int i = 0; i < paramTypes.length; i++) {
newArgs[pos + i] = validateArgument(paramTypes[i]);
}
if (pos < argTypes.length) {
System.arraycopy(argTypes, pos, newArgs, pos +
paramTypes.length, argTypes.length - pos);
}
return ofValidated(returnType, newArgs);
}
See also:
https://github.com/openjdk/jdk/blob/230fac80f25e9608006c8928a8a7708bf13a818c/src/java.base/share/classes/java/util/ImmutableCollections.java#L186-L195
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594510390
Re: RFR: 8305457: Implement java.io.IO [v4]
> Please review this PR which introduces the `java.io.IO` top-level class and > three methods to `java.io.Console` for [Implicitly Declared Classes and > Instance Main Methods (Third Preview)]. > > This PR has been obtained as `git merge --squash` of a now obsolete [draft > PR]. > > [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: > https://bugs.openjdk.org/browse/JDK-8323335 > [draft PR]: https://github.com/openjdk/jdk/pull/18921 Pavel Rappo has updated the pull request incrementally with two additional commits since the last revision: - Specify charset of java.io.IO - Use system-dependent line separator for Console.println - Changes: - all: https://git.openjdk.org/jdk/pull/19112/files - new: https://git.openjdk.org/jdk/pull/19112/files/73a20a7c..80eacf8c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19112=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19112=02-03 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112 PR: https://git.openjdk.org/jdk/pull/19112
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote:
>> A rather large startup regression was introduced in 23-b13 from
>> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has
>> been dealt with as enhancements such as
>> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802),
>> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and
>> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide
>> both point fixes and reduce initialization overhead of certain constructs
>> more generally. The remaining issues stem from a set of lambdas added in
>> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early
>> bootstrapping of the lambda infrastructure and a bit of class generation.
>>
>> While the remaining overheads are relatively small and borderline acceptable
>> (< 2-3ms), I think it's still worth acting on them in this particular case
>> since the amount of added bootstrapping overhead is dependent on which
>> locale the system runs under, which complicates testing and comparisons due
>> to relatively large differences in paths taken on different systems.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove redundant constructor
src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 84:
> 82: */
> 83: public static Supplier, ReferenceKey>>
> concurrentHashMapSupplier() {
> 84: return new Supplier<>() {
Can this just `return ReferencedKeyMap.concurrentHashMapSupplier();`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594474286
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 16:06:29 GMT, Stuart Marks wrote: > Most of these are defined in terms of `String.valueOf(Object)` which if > passed a null reference will return the String object containing `null`. So > no, I don't think NPE should be thrown. It might be worth mentioning this > explicitly somewhere though. Right, in our current model, `Console.readln(prompt)` can be reduced to `Console.print(prompt)` followed by actual reading. `Console.print(prompt)` can in turn be reduced to printing the result of `String.valueOf((Object) prompt)`, which is a string "null". The above reduction means that `Console.readln(null)` will not throw NPE. Now, what I'm asking if that's what we really want. With `Console.println(null)` and `Console.print(null)` not throwing NPE makes sense. However, with `Console.readln(null)` it might not. After all, we ask for a **`String`** prompt, not an `Object` prompt. All other methods of `Console` that accept `String`, throw NPE if that string is null. One more consideration. Whatever the behaviour we choose for `Console.readln(null)`, we should choose the same behaviour for `IO.readln(null)`. Otherwise, it would be a constant source of unpleasantly surprising inconsistency. Thoughts? - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1594464589
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046346164
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:57:22 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove redundant constructor > > src/java.base/share/classes/sun/util/locale/BaseLocale.java line 96: > >> 94: // Interned BaseLocale cache >> 95: private static final ReferencedKeySet CACHE = >> 96: ReferencedKeySet.create(true, >> ReferencedKeySet.concurrentHashMapSupplier()); > > Should this supplier be in `BaseLocale` class? Otherwise `ReferencedKeySet` > may end up with static suppliers for each map type? Maybe, though I think most potential uses of `ReferenceKeySet/-Map` will want to be using a plain `CHM` as the backing store, so keeping these next to their respective `create` methods will encourage sharing. - PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r159407
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046311563
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor Thanks for fixing this! src/java.base/share/classes/sun/util/locale/BaseLocale.java line 96: > 94: // Interned BaseLocale cache > 95: private static final ReferencedKeySet CACHE = > 96: ReferencedKeySet.create(true, > ReferencedKeySet.concurrentHashMapSupplier()); Should this supplier be in `BaseLocale` class? Otherwise `ReferencedKeySet` may end up with static suppliers for each map type? - PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046313019 PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594429353
Re: RFR: 8331932: Startup regressions in 23-b13 [v3]
On Wed, 8 May 2024 17:23:25 GMT, Alan Bateman wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Extract singleton for interning BaseLocale > > src/java.base/share/classes/java/util/Locale.java line 1022: > >> 1020: exts = null; >> 1021: hash = 0; >> 1022: } > > I don't think you need to add this constructor now. Was certain I had removed it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594382318
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove redundant constructor - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/d8594b31..819e3d47 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=02-03 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8331932: Startup regressions in 23-b13 [v3]
On Wed, 8 May 2024 17:01:05 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Extract singleton for interning BaseLocale src/java.base/share/classes/java/util/Locale.java line 1022: > 1020: exts = null; > 1021: hash = 0; > 1022: } I don't think you need to add this constructor now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594376091
Re: RFR: 8330276: Console methods with explicit Locale [v4]
> Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed JdkConsole.printf() - Changes: - all: https://git.openjdk.org/jdk/pull/18923/files - new: https://git.openjdk.org/jdk/pull/18923/files/29ff6063..2e83fc7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18923=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18923=02-03 Stats: 20 lines in 5 files changed: 0 ins; 19 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8305457: Implement java.io.IO [v3]
I don't remember it being considered, but I think it falls outside the scope of
JEP 477 (emphasis mine):
> We achieve this effect by declaring a new top-level class in the java.io
> package named, simply, IO. It declares the above three static methods for
> **textual** I/O with the console, and nothing else.
java.io.IO is closely related to java.io.Console, which provides access to a
terminal in a more or less canonical mode to read and write _strings_. While it
might be useful for Console to separately provide access to a terminal in the
raw mode, I'm not sure how often developers, let alone on-ramp developers, need
it. In my experience, programming games, such as those you mentioned, is better
introduced with GUI, where they have java.awt.event.KeyListener.
-Pavel
> On 8 May 2024, at 14:59, Olexandr Rotan wrote:
>
> Has it been considered to add a readKey() method to IO class? In my
> experience, it is pretty commonly used by beginners when they write things
> like console games (snake, catcher game etc.). I am aware there is
> System.in.read() method, but since we decided to promote some methods to
> separate methods in IO class, maybe readKey() or simple read() should also be
> considered?
>
> On Wed, May 8, 2024 at 4:16 PM Pavel Rappo wrote:
> On Wed, 8 May 2024 10:31:59 GMT, Jaikiran Pai wrote:
>
> >> If we specify that, it would be very much unlike all other `Console`
> >> methods that are covered by this:
> >>
> >> * Unless otherwise specified, passing a {@code null} argument to any
> >> method
> >> * in this class will cause a {@link NullPointerException} to be thrown.
> >
> > I haven't given it a try, but a brief look at the code suggests that if the
> > console implementation backed by JLine gets used, then a `null` prompt may
> > not generate a `NullPointerException` (since JLine appears to allow `null`)
> > whereas if the internal JDK console implementation gets used then a
> > `NullPointerException` will get thrown. If the expectation is to disallow a
> > `null` prompt, then perhaps `Objects.requireNonNull(prompt)` before we hand
> > off to the underlying console implementations might be required.
>
> Good catch!
> `jdk.internal.org.jline.JdkConsoleProviderImpl.JdkConsoleImpl.readln(null)`
> does not throw `NullPointerException` (NPE), which it must. I'll fix the bug
> and add a test.
>
> Speaking of NPE. The newly added `Console.print` and `Console.println`
> methods do not throw NPE if passed null. While that's how it should be, they
> don't specify that, so the class-level NPE clause applies:
>
> * Unless otherwise specified, passing a {@code null} argument to any
> method
> * in this class will cause a {@link NullPointerException} to be thrown.
> ...
> public sealed class Console implements Flushable permits ProxyingConsole {
>
> I'll fix that.
>
> We should also specify NPE (or absence thereof) on methods of `IO`.
> Otherwise, this package clause applies:
>
> * Unless otherwise noted, passing a {@code null} argument to a
> constructor or
> * method in any class or interface in this package will cause a
> * {@code NullPointerException} to be thrown.
> ...
> package java.io;
>
> @stuart-marks speaking of exceptions. Do we need to add explicit `@throws`
> tag to `IO` methods?
>
> @throws {@code IOError} if {@code System.console()} returns {@code null}
>
> While it would make sense, it would also look superfluous: we already specify
> that error condition in the class comment and then in every method's main
> description. Do we need to add one more note on `IOError`?
>
> -
>
> PR Review Comment:
> https://git.openjdk.org/jdk/pull/19112#discussion_r1594013064
Re: RFR: 8331932: Startup regressions in 23-b13 [v3]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Extract singleton for interning BaseLocale - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/73097159..d8594b31 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=01-02 Stats: 21 lines in 1 file changed: 11 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Thu, 4 Apr 2024 20:56:55 GMT, Magnus Ihse Bursie wrote: >> Severin Gehwolf has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Use shorter name for the build tool >> - Review feedback from Erik J. >> - Remove dependency on CDS which isn't needed anymore > > Overall, I think the build system changes in the current revision of the > patch look good, but please let me know for a deeper review when things have > stabilized. Thanks for the review, @magicus! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2101006001
Re: RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts [v3]
> This PR fixes an issue that has crept into the FFM API implementation. > > From very early stages, the FFM API used to disable the alignment check on > nested layout elements, in favor of an alignment check against the memory > segment base address. The rationale was that the JIT had issue with > eliminating redundant alignment checks, and accessing nested elements could > never result in alignment issues, since the alignment of the container is > provably bigger than that of the contained element. This means that, when > creating a var handle for a nested layout element, we set the nested layout > alignment to 1 (unaligned), derive its var handle, and then decorate the var > handle with an alignment check for the container. > > At some point in 22, we tweaked the API to throw > `UnsupportedOperationException` when using an access mode incompatible with > the alignment constraint of the accessed layout. That is, a volatile read on > an `int` is only possible if the access occurs at an address that is at least > 4-byte aligned. Otherwise an `UOE` is thrown. > > Unfortunately this change broke the aforementioned optimization: creating a > var handle for an unaligned layout works, but the resulting layout will *not* > support any of the atomic access modes. > > Since this optimization is not really required anymore (proper C2 support to > hoist/eliminate alignment checks has been added since then), it is better to > disable this implementation quirk, and leave optimizations to C2. > > (If we really really wanted to optimize things a bit, we could remove the > container alignment check in the case the accessed value is the first layout > element nested in the container, but this PR doesn't go that far). > > I've run relevant benchmarks before/after and found no differences. In part > this is because `arrayElementVarHandle` is unaffected. But, even after > tweaking the `LoopOverNonConstant` benchmark to add explicit tests for the > code path affected, no significant difference was found, sign that C2 is > indeed able to spot (and remove) the redundant alignment check. Note: if we > know that `aligned_to_N(base)` holds, then it's easy to prove that > `aligned_to_M(base + offset + index * scale)` holds, when `N >= M` and with > `offset` and `scale` known (the latter a power of two). Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add benchmark anno - Changes: - all: https://git.openjdk.org/jdk/pull/19124/files - new: https://git.openjdk.org/jdk/pull/19124/files/829c5e28..40ba693c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19124=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19124=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19124.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19124/head:pull/19124 PR: https://git.openjdk.org/jdk/pull/19124
Re: RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts
On Wed, 8 May 2024 15:32:27 GMT, Maurizio Cimadamore wrote: > * `x_handle` is really meant to provide access to a memory segment modelling > (at least) one struct with layout `POINT_LAYOUT`. As such, the initial > segment/offset combo should (a) be adequately aligned (according to > `POINT_LAYOUT.byteAlignment()`) and have sufficient size (according to > `POINT_LAYOUT.byteSize()`) An example of this approach is attempted here: https://github.com/openjdk/jdk/compare/master...mcimadamore:jdk:bad_align_segment_var_handle+size_checks?expand=1 Again, no significant performance regression is observed - the root layout checks dominate the value layout checks, and it seems like C2 is able to spot that. - PR Comment: https://git.openjdk.org/jdk/pull/19124#issuecomment-2100970680
Re: RFR: 8331932: Startup regressions in 23-b13 [v2]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Singleton Locale creator - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/0520f953..73097159 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=00-01 Stats: 13 lines in 1 file changed: 3 ins; 7 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 09:39:13 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/io/Console.java line 151:
>>
>>> 149: /**
>>> 150: * Writes a string representation of the specified object to this
>>> console's
>>> 151: * output stream, terminates the line and then flushes the console.
>>
>> Should this specify if the line termination will be platform dependent
>> character(s) or independent of the platform?
>
> That's a good question. I think it should. That `println` method is not
> specified in terms of `printf` or `format`. Thus, we cannot reduce `println`
> to, say, `printf("%s%n", obj)`, leaning on `Formatter`'s definition of `%n`:
>
> 'n' line separator The result is the platform-specific line
> separator
>
> @stuart-marks, any thoughts on wording? Mind you, if we add any such wording,
> we'll have to update CSR too.
Well `Formatter` defers to `System.lineSeparator()` so I think we should refer
to that. Not sure that needs to be repeated in `java.io.IO`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1594281188
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 13:13:33 GMT, Pavel Rappo wrote:
>> I haven't given it a try, but a brief look at the code suggests that if the
>> console implementation backed by JLine gets used, then a `null` prompt may
>> not generate a `NullPointerException` (since JLine appears to allow `null`)
>> whereas if the internal JDK console implementation gets used then a
>> `NullPointerException` will get thrown. If the expectation is to disallow a
>> `null` prompt, then perhaps `Objects.requireNonNull(prompt)` before we hand
>> off to the underlying console implementations might be required.
>
> Good catch!
> `jdk.internal.org.jline.JdkConsoleProviderImpl.JdkConsoleImpl.readln(null)`
> does not throw `NullPointerException` (NPE), which it must. I'll fix the bug
> and add a test.
>
> Speaking of NPE. The newly added `Console.print` and `Console.println`
> methods do not throw NPE if passed null. While that's how it should be, they
> don't specify that, so the class-level NPE clause applies:
>
> * Unless otherwise specified, passing a {@code null} argument to any
> method
> * in this class will cause a {@link NullPointerException} to be thrown.
> ...
> public sealed class Console implements Flushable permits ProxyingConsole {
>
> I'll fix that.
>
> We should also specify NPE (or absence thereof) on methods of `IO`.
> Otherwise, this package clause applies:
>
> * Unless otherwise noted, passing a {@code null} argument to a
> constructor or
> * method in any class or interface in this package will cause a
> * {@code NullPointerException} to be thrown.
> ...
> package java.io;
>
> @stuart-marks speaking of exceptions. Do we need to add explicit `@throws`
> tag to `IO` methods?
>
> @throws {@code IOError} if {@code System.console()} returns {@code null}
>
> While it would make sense, it would also look superfluous: we already specify
> that error condition in the class comment and then in every method's main
> description. Do we need to add one more note on `IOError`?
Most of these are defined in terms of `String.valueOf(Object)` which if passed
a null reference will return the String object containing `null`. So no, I
don't think NPE should be thrown. It might be worth mentioning this explicitly
somewhere though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1594282580
Re: RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts [v2]
> This PR fixes an issue that has crept into the FFM API implementation. > > From very early stages, the FFM API used to disable the alignment check on > nested layout elements, in favor of an alignment check against the memory > segment base address. The rationale was that the JIT had issue with > eliminating redundant alignment checks, and accessing nested elements could > never result in alignment issues, since the alignment of the container is > provably bigger than that of the contained element. This means that, when > creating a var handle for a nested layout element, we set the nested layout > alignment to 1 (unaligned), derive its var handle, and then decorate the var > handle with an alignment check for the container. > > At some point in 22, we tweaked the API to throw > `UnsupportedOperationException` when using an access mode incompatible with > the alignment constraint of the accessed layout. That is, a volatile read on > an `int` is only possible if the access occurs at an address that is at least > 4-byte aligned. Otherwise an `UOE` is thrown. > > Unfortunately this change broke the aforementioned optimization: creating a > var handle for an unaligned layout works, but the resulting layout will *not* > support any of the atomic access modes. > > Since this optimization is not really required anymore (proper C2 support to > hoist/eliminate alignment checks has been added since then), it is better to > disable this implementation quirk, and leave optimizations to C2. > > (If we really really wanted to optimize things a bit, we could remove the > container alignment check in the case the accessed value is the first layout > element nested in the container, but this PR doesn't go that far). > > I've run relevant benchmarks before/after and found no differences. In part > this is because `arrayElementVarHandle` is unaffected. But, even after > tweaking the `LoopOverNonConstant` benchmark to add explicit tests for the > code path affected, no significant difference was found, sign that C2 is > indeed able to spot (and remove) the redundant alignment check. Note: if we > know that `aligned_to_N(base)` holds, then it's easy to prove that > `aligned_to_M(base + offset + index * scale)` holds, when `N >= M` and with > `offset` and `scale` known (the latter a power of two). Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Revert spurious change to test - Changes: - all: https://git.openjdk.org/jdk/pull/19124/files - new: https://git.openjdk.org/jdk/pull/19124/files/aa5ed595..829c5e28 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19124=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19124=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19124.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19124/head:pull/19124 PR: https://git.openjdk.org/jdk/pull/19124
Re: RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts
On Tue, 7 May 2024 15:42:23 GMT, Maurizio Cimadamore
wrote:
> This PR fixes an issue that has crept into the FFM API implementation.
>
> From very early stages, the FFM API used to disable the alignment check on
> nested layout elements, in favor of an alignment check against the memory
> segment base address. The rationale was that the JIT had issue with
> eliminating redundant alignment checks, and accessing nested elements could
> never result in alignment issues, since the alignment of the container is
> provably bigger than that of the contained element. This means that, when
> creating a var handle for a nested layout element, we set the nested layout
> alignment to 1 (unaligned), derive its var handle, and then decorate the var
> handle with an alignment check for the container.
>
> At some point in 22, we tweaked the API to throw
> `UnsupportedOperationException` when using an access mode incompatible with
> the alignment constraint of the accessed layout. That is, a volatile read on
> an `int` is only possible if the access occurs at an address that is at least
> 4-byte aligned. Otherwise an `UOE` is thrown.
>
> Unfortunately this change broke the aforementioned optimization: creating a
> var handle for an unaligned layout works, but the resulting layout will *not*
> support any of the atomic access modes.
>
> Since this optimization is not really required anymore (proper C2 support to
> hoist/eliminate alignment checks has been added since then), it is better to
> disable this implementation quirk, and leave optimizations to C2.
>
> (If we really really wanted to optimize things a bit, we could remove the
> container alignment check in the case the accessed value is the first layout
> element nested in the container, but this PR doesn't go that far).
>
> I've run relevant benchmarks before/after and found no differences. In part
> this is because `arrayElementVarHandle` is unaffected. But, even after
> tweaking the `LoopOverNonConstant` benchmark to add explicit tests for the
> code path affected, no significant difference was found, sign that C2 is
> indeed able to spot (and remove) the redundant alignment check. Note: if we
> know that `aligned_to_N(base)` holds, then it's easy to prove that
> `aligned_to_M(base + offset + index * scale)` holds, when `N >= M` and with
> `offset` and `scale` known (the latter a power of two).
Overall, I'm not too sure about the alignment check on the root layout
performed by a var handle pointing to a group layout member. We've got there
honestly - e.g. make sure the accessed segment/offset is really compatible with
root layout. But, if so, then why also not checking the segment size against
the root layout size? E.g. say I have a layout for:
var POINT_LAYOUT = structLayout(JAVA_DOUBLE.withName("x"),
JAVA_DOUBLE.withName("y"))
And I derive a var handle for `x`, like so:
VarHandle x_handle = POINT_LAYOUT.varHandle(PathElement.groupLayout("x"));
Say that I have a 8-byte segment, and I use that with the `x_handle` above:
MemorySegment halfPoint = Arena.ofAuto().allocate(8);
double x = (double)x_handle.get(halfPoint, 0);
Should this work? Currently it does, and one might claim that it does so "by
accident" - e.g. it just so happen that the provided segment/offset combo was
big enough to access `x`. But of course accessing `y` would fail.
There are of course two possible interpretations here, and both are valid:
* `x_handle` is just a regular memory access var handle for `JAVA_DOUBLE` - it
just contains some extra logic to compute the correct offset from the start
segment/offset combo, but that's it.
* `x_handle` is really meant to provide access to a memory segment modelling
(at least) one struct with layout `POINT_LAYOUT`. As such, the initial
segment/offset combo should (a) be adequately aligned (according to
`POINT_LAYOUT.byteAlignment()`) and have sufficient size (according to
`POINT_LAYOUT.byteSize()`)
The former favors performance, the latter favors safety, as it ensures that the
initial segment/offset combo do indeed describe a segment whose characteristics
are compatible with those of the selected layout.
This is also related to `MemoryLayout::arrayElementVarHandle`. Currently this
method is specified as:
MethodHandles.collectCoordinates(varHandle(elements), 1, scaleHandle())
This means that the root layout checks performed by the obtained method handle
will refer to the alignment (and size) of the current layout. For instance, if
I write:
VarHandle xs_handle =
POINT_LAYOUT.arrayElementVarHandle(PathElement.groupLayout("x"));
Assuming we picked the safer semantics described above, the resulting var
handle will check that the initial segment/offset combo will point to a segment
whose alignment (and size) are compatible with those of `POINT_LAYOUT`. I
believe this is acceptable: this var handle is meant to capture unbounded
sequence access - so the only thing we can meaningfully
Re: RFR: 8331932: Startup regressions in 23-b13
On Wed, 8 May 2024 14:53:05 GMT, Claes Redestad wrote:
> A rather large startup regression was introduced in 23-b13 from
> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has
> been dealt with as enhancements such as
> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802),
> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and
> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide
> both point fixes and reduce initialization overhead of certain constructs
> more generally. The remaining issues stem from a set of lambdas added in code
> for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early
> bootstrapping of the lambda infrastructure and a bit of class generation.
>
> While the remaining overheads are relatively small and borderline acceptable
> (< 2-3ms), I think it's still worth acting on them in this particular case
> since the amount of added bootstrapping overhead is dependent on which locale
> the system runs under, which complicates testing and comparisons due to
> relatively large differences in paths taken on different systems.
src/java.base/share/classes/java/util/Locale.java line 1001:
> 999: = ReferencedKeyMap.create(true,
> ReferencedKeyMap.concurrentHashMapSupplier());
> 1000:
> 1001: private static final LocaleKey LOCALE_LOOKUP = new LocaleKey();
Reusing an existing class with a special instance for functional interface
implementation is weird, I still wonder if we can do the local class +
singleton (essentially same as non-capturing lambda) approach.
src/java.base/share/classes/sun/util/locale/BaseLocale.java line 168:
> 166: return CACHE.intern(new BaseLocale(language, script, region,
> variant),
> 167: // Avoid lambdas since this may be on the bootstrap path
> in many locales
> 168: new UnaryOperator() {
Since this intern is called many times, can we convert this `new` to a local
class in a static method and reuse its singleton?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594201610
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594199711
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
On Tue, 7 May 2024 16:52:12 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 105 commits: > > - 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 > - Fix coment > - Fix comment > - Fix typo > - Revert some now unneded build changes > - Follow build tools naming convention > - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca The new build changes look extremely trivial. From a pure build PoV, this is a much simpler solution. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2045924409
RFR: 8331932: Startup regressions in 23-b13
A rather large startup regression was introduced in 23-b13 from [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has been dealt with as enhancements such as [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide both point fixes and reduce initialization overhead of certain constructs more generally. The remaining issues stem from a set of lambdas added in code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early bootstrapping of the lambda infrastructure and a bit of class generation. While the remaining overheads are relatively small and borderline acceptable (< 2-3ms), I think it's still worth acting on them in this particular case since the amount of added bootstrapping overhead is dependent on which locale the system runs under, which complicates testing and comparisons due to relatively large differences in paths taken on different systems. - Commit messages: - 8331932: Startup regressions in 23-b13 Changes: https://git.openjdk.org/jdk/pull/19140/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19140=00 Issue: https://bugs.openjdk.org/browse/JDK-8331932 Stats: 74 lines in 4 files changed: 57 ins; 4 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8305457: Implement java.io.IO [v3]
Has it been considered to add a readKey() method to IO class? In my
experience, it is pretty commonly used by beginners when they write things
like console games (snake, catcher game etc.). I am aware there is
System.in.read() method, but since we decided to promote some methods to
separate methods in IO class, maybe readKey() or simple read() should also
be considered?
On Wed, May 8, 2024 at 4:16 PM Pavel Rappo wrote:
> On Wed, 8 May 2024 10:31:59 GMT, Jaikiran Pai wrote:
>
> >> If we specify that, it would be very much unlike all other `Console`
> methods that are covered by this:
> >>
> >> * Unless otherwise specified, passing a {@code null} argument to
> any method
> >> * in this class will cause a {@link NullPointerException} to be
> thrown.
> >
> > I haven't given it a try, but a brief look at the code suggests that if
> the console implementation backed by JLine gets used, then a `null` prompt
> may not generate a `NullPointerException` (since JLine appears to allow
> `null`) whereas if the internal JDK console implementation gets used then a
> `NullPointerException` will get thrown. If the expectation is to disallow a
> `null` prompt, then perhaps `Objects.requireNonNull(prompt)` before we hand
> off to the underlying console implementations might be required.
>
> Good catch!
> `jdk.internal.org.jline.JdkConsoleProviderImpl.JdkConsoleImpl.readln(null)`
> does not throw `NullPointerException` (NPE), which it must. I'll fix the
> bug and add a test.
>
> Speaking of NPE. The newly added `Console.print` and `Console.println`
> methods do not throw NPE if passed null. While that's how it should be,
> they don't specify that, so the class-level NPE clause applies:
>
> * Unless otherwise specified, passing a {@code null} argument to any
> method
> * in this class will cause a {@link NullPointerException} to be thrown.
> ...
> public sealed class Console implements Flushable permits
> ProxyingConsole {
>
> I'll fix that.
>
> We should also specify NPE (or absence thereof) on methods of `IO`.
> Otherwise, this package clause applies:
>
> * Unless otherwise noted, passing a {@code null} argument to a
> constructor or
> * method in any class or interface in this package will cause a
> * {@code NullPointerException} to be thrown.
> ...
> package java.io;
>
> @stuart-marks speaking of exceptions. Do we need to add explicit `@throws`
> tag to `IO` methods?
>
> @throws {@code IOError} if {@code System.console()} returns {@code
> null}
>
> While it would make sense, it would also look superfluous: we already
> specify that error condition in the class comment and then in every
> method's main description. Do we need to add one more note on `IOError`?
>
> -
>
> PR Review Comment:
> https://git.openjdk.org/jdk/pull/19112#discussion_r1594013064
>
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 10:31:59 GMT, Jaikiran Pai wrote:
>> If we specify that, it would be very much unlike all other `Console` methods
>> that are covered by this:
>>
>> * Unless otherwise specified, passing a {@code null} argument to any
>> method
>> * in this class will cause a {@link NullPointerException} to be thrown.
>
> I haven't given it a try, but a brief look at the code suggests that if the
> console implementation backed by JLine gets used, then a `null` prompt may
> not generate a `NullPointerException` (since JLine appears to allow `null`)
> whereas if the internal JDK console implementation gets used then a
> `NullPointerException` will get thrown. If the expectation is to disallow a
> `null` prompt, then perhaps `Objects.requireNonNull(prompt)` before we hand
> off to the underlying console implementations might be required.
Good catch!
`jdk.internal.org.jline.JdkConsoleProviderImpl.JdkConsoleImpl.readln(null)`
does not throw `NullPointerException` (NPE), which it must. I'll fix the bug
and add a test.
Speaking of NPE. The newly added `Console.print` and `Console.println` methods
do not throw NPE if passed null. While that's how it should be, they don't
specify that, so the class-level NPE clause applies:
* Unless otherwise specified, passing a {@code null} argument to any method
* in this class will cause a {@link NullPointerException} to be thrown.
...
public sealed class Console implements Flushable permits ProxyingConsole {
I'll fix that.
We should also specify NPE (or absence thereof) on methods of `IO`. Otherwise,
this package clause applies:
* Unless otherwise noted, passing a {@code null} argument to a constructor
or
* method in any class or interface in this package will cause a
* {@code NullPointerException} to be thrown.
...
package java.io;
@stuart-marks speaking of exceptions. Do we need to add explicit `@throws` tag
to `IO` methods?
@throws {@code IOError} if {@code System.console()} returns {@code null}
While it would make sense, it would also look superfluous: we already specify
that error condition in the class comment and then in every method's main
description. Do we need to add one more note on `IOError`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1594013064
Re: RFR: 8331876: JFR: Move file read and write events to java.base
On Tue, 7 May 2024 19:32:57 GMT, Erik Gahlin wrote: > Hi, > > Could I have a review of a change that moves the jdk.FileRead and > jdk.FileWrite events to java.base to remove the use of the ASM > instrumentation. > > Testing: jdk/jdk/jfr > > Thanks > Erik Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19129#pullrequestreview-2045472095
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4) [v2]
On Tue, 7 May 2024 16:43:11 GMT, Naoto Sato wrote: > Sorry if I was unclear, but I meant to consolidate the test methods, as they > are duplicates other than the calendar values. Those values should be > parametrized and consolidate the test body into a single method. sorry for misunderstanding, I was trying to provide scenarios that are easy to follow and understand for devs, but you are right we don't need duplication there. - PR Comment: https://git.openjdk.org/jdk/pull/19082#issuecomment-2100383313
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4) [v3]
> Calendar.add() tests that describe its behavior. serhiysachkov has updated the pull request incrementally with one additional commit since the last revision: JDK-8331646 consolidating test methods into ParameterizedTests - Changes: - all: https://git.openjdk.org/jdk/pull/19082/files - new: https://git.openjdk.org/jdk/pull/19082/files/bce324f3..4bb425da Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19082=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19082=01-02 Stats: 234 lines in 1 file changed: 6 ins; 178 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/19082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19082/head:pull/19082 PR: https://git.openjdk.org/jdk/pull/19082
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Refactor to further avoid re-validating arguments - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/543d0709..eb23cf51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=03-04 Stats: 33 lines in 2 files changed: 21 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 08:59:18 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/io/Console.java line 192:
>>
>>> 190: *
>>> 191: * @param prompt
>>> 192: * A prompt string.
>>
>> Hello Pavel, should this specify whether `prompt` can be null?
>
> If we specify that, it would be very much unlike all other `Console` methods
> that are covered by this:
>
> * Unless otherwise specified, passing a {@code null} argument to any
> method
> * in this class will cause a {@link NullPointerException} to be thrown.
I haven't given it a try, but a brief look at the code suggests that if the
console implementation backed by JLine gets used, then a `null` prompt may not
generate a `NullPointerException` (since JLine appears to allow `null`) whereas
if the internal JDK console implementation gets used then a
`NullPointerException` will get thrown. If the expectation is to disallow a
`null` prompt, then perhaps `Objects.requireNonNull(prompt)` before we hand off
to the underlying console implementations might be required.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593809280
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 05:53:25 GMT, Jaikiran Pai wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Strengthen tests after 8330998
>>
>> https://github.com/openjdk/jdk/pull/18996 now allows us to test
>> Console IO better.
>
> src/java.base/share/classes/java/io/Console.java line 151:
>
>> 149: /**
>> 150: * Writes a string representation of the specified object to this
>> console's
>> 151: * output stream, terminates the line and then flushes the console.
>
> Should this specify if the line termination will be platform dependent
> character(s) or independent of the platform?
That's a good question. I think it should. That `println` method is not
specified in terms of `printf` or `format`. Thus, we cannot reduce `println`
to, say, `printf("%s%n", obj)`, leaning on `Formatter`'s definition of `%n`:
'n' line separator The result is the platform-specific line separator
@stuart-marks, any thoughts on wording? Mind you, if we add any such wording,
we'll have to update CSR too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593742851
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Mon, 6 May 2024 19:06:10 GMT, Sonia Zaldana Calles
wrote:
>> Hi folks,
>>
>> This PR aims to fix
>> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581).
>>
>> I think the regression got introduced in
>> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458).
>>
>> In the issue linked above,
>> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226)
>> got removed to simplify launcher code.
>>
>> Previously, we used ```getMainType``` to do the appropriate main method
>> invocation in ```JavaMain```. However, we currently attempt to do all types
>> of main method invocations at the same time
>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623).
>>
>>
>> Note how all of these invocations clear the exception reported with
>> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390).
>>
>>
>> Therefore, if a legitimate exception comes up during one of these
>> invocations, it does not get reported.
>>
>> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking
>> forward to your suggestions.
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixing indentation
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
Pre-existing: Man, I cannot grok the complex return code handling, tbh.
We have the local `ret` variable holding a return code. We also hand codes to
CHECK_EXCEPTION_LEAVE as macro argument. But we don't hand codes to
CHECK_EXCEPTION_NULL_LEAVE. LEAVE uses the locally defined `ret` instead of
getting the return code as argument. CHECK_EXCEPTION_LEAVE modifies the local
`ret`, but CHECK_EXCEPTION_NULL_LEAVE does not.
CHECK_EXCEPTION_NULL_LEAVE does not set `ret`. So, in case of an error, it
would cause the launcher to return OK, but this does not happen because the
local `ret` gets initialized to 1 before the first call to
CHECK_EXCEPTION_NULL_LEAVE (line 566 resp. 560). Not sure if this was
intentional, but it surely is very brittle. We rely on the content of `ret`,
and that changes several times throughout JavaMain.
CHECK_EXCEPTION_NULL_LEAVE argument is named CENL_exception, which I don't
understand.
To confuse matters more, the logic for internal error codes and the launcher
return code is reversed: internally, 0 means error, and externally, 0 means
success. And we only use numerical literals (`1`, `0`) instead of clearly named
constants.
This may be food for another RFE, to keep this patch minimal. But a good
solution, to me, would be like this:
- have the same logic for return codes (1 = error, 0 = success) to ease
understanding
- have clearly named constants (e.g. "LAUNCHER_OK" 0, "LAUNCHER_ERR" = 1)
- have the LEAVE macro take the launcher return code as argument
- have all xxx_LEAVE macros pass in LAUNCHER_ERR to LEAVE
- call the final LEAVE with LAUNCHER_OK
- optionally, define something like "LEAVE_ERR" and "LEAVE_OK" that call LEAVE
with either LAUNCHER_ERR or LAUNCHER_OK, for more concise coding.
For this patch, I think the return code logic is okay, but I would feel better
if others double-checked.
src/java.base/share/native/libjli/java.c line 394:
> 392: if ((*env)->ExceptionOccurred(env)) { \
> 393: return 0; \
> 394: } else if (obj == NULL) { \
Side note, I first wondered if this comparison is strictly correct, since we
now pass in `jmethodID` as well as `jobject`, which are opaque types and not
necessarily of the same size.
But seems that jmethodID==NULL is defined to mean "invalid" [1] by the spec.
Requiring NULL instead of providing an opaque invalid constant feels like an
odd choice in the original JNI spec, since it requires implementors to use a
pointer type to implement jmethodID? Which we do, in OpenJDK [2].
[1]
https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#getstaticmethodid
[2]
https://github.com/openjdk/jdk/blob/2baacfc16916220846743c6e49a99a6c41cac510/src/java.base/share/native/include/jni.h#L135-L136
src/java.base/share/native/libjli/java.c line 420:
> 418: jmethodID mainID =
> 419: (*env)->GetMethodID(env, mainClass, "main",
> "([Ljava/lang/String;)V");
> 420: CHECK_EXCEPTION_NULL_FAIL(mainID);
Is there a particular reason why you moved this section up here, from line 432
before? If not, I'd restore it to its original position to keep the diff small.
src/java.base/share/native/libjli/java.c line 452:
> 450: jobject mainObject = (*env)->NewObject(env, mainClass, constructor);
> 451: CHECK_EXCEPTION_NULL_FAIL(mainObject);
> 452: jmethodID mainID = (*env)->GetMethodID(env, mainClass, "main",
> "()V");
Unnecessary change. Please restore
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v3]
On Tue, 7 May 2024 08:08:05 GMT, Joachim Kern wrote: >> Since ~ end of March, after >> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), >> tools/launcher/JliLaunchTest.java fails on AIX. Failure is : >> >> stdout: []; >> stderr: [Error: could not find libjava.so >> Error: Could not find Java SE Runtime Environment. >> ] >> exitValue = 2 >> >> java.lang.RuntimeException: Expected to get exit value of [0], exit value >> is: [2] >> at >> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521) >> at JliLaunchTest.main(JliLaunchTest.java:58) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> at >> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) >> at java.base/java.lang.Thread.run(Thread.java:1575) >> >> Maybe we need to do further adjustments to >> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / >> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ? >> Or somehow adjust the coding that attempts to find parts of "JRE" >> (libjava.so) ? The libjli.so is gone on AIX after >> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this >> also the image discovery based on GetApplicationHomeFromDll which uses >> libjli.so . >> >> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. >> There is no other methos available on AIX, because it lacks the $ORIGIN >> feature of the rpath. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetic changes Thanks for conditionally compiling this in with `#ifdef AIX ...`, that reduces the risk of changing behavior on other platforms. I don't have any other comments to add. - PR Comment: https://git.openjdk.org/jdk/pull/19000#issuecomment-2100171882
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
On Tue, 7 May 2024 16:52:12 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 105 commits: > > - 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 > - Fix coment > - Fix comment > - Fix typo > - Revert some now unneded build changes > - Follow build tools naming convention > - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca GHA test failures [gc/shenandoah/oom/TestThreadFailure](https://github.com/jerboaa/jdk/actions/runs/8989206765#user-content-gc_shenandoah_oom_testthreadfailure) on Mac x86_64 and [java/util/zip/ZipFile/ZipSourceCache](https://github.com/jerboaa/jdk/actions/runs/8989206765#user-content-java_util_zip_zipfile_zipsourcecache) on Windows x86_64 seem unrelated to this patch. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2100159861
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 05:59:14 GMT, Jaikiran Pai wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Strengthen tests after 8330998
>>
>> https://github.com/openjdk/jdk/pull/18996 now allows us to test
>> Console IO better.
>
> src/java.base/share/classes/java/io/IO.java line 51:
>
>> 49: * console, terminates the line and then flushes that console.
>> 50: *
>> 51: * If {@code System.console()} returns {@code null}, throws {@code
>> IOError}.
>
> Unlike, `java.io.Console` whose existing methods already throw `IOError`,
> should these new methods on `java.io.IO` instead throw
> `java.io.UncheckedIOException` instead of throwing `Error`s?
As a result of several offline discussions, it was decided to throw `IOError`
to better align with `java.io.Console`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593711476
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 05:41:52 GMT, Jaikiran Pai wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Strengthen tests after 8330998
>>
>> https://github.com/openjdk/jdk/pull/18996 now allows us to test
>> Console IO better.
>
> src/java.base/share/classes/java/io/Console.java line 188:
>
>> 186:
>> 187: /**
>> 188: * Writes a prompt as if by calling {@code print}, then reads a
>> single line
>
> Should `{@code print}` instead be `{@link Console#print() print()}`?
It could be done like that, but given that it's the same class (and the same
HTML page), I'd skip it.
> src/java.base/share/classes/java/io/Console.java line 192:
>
>> 190: *
>> 191: * @param prompt
>> 192: * A prompt string.
>
> Hello Pavel, should this specify whether `prompt` can be null?
If we specify that, it would be very much unlike all other `Console` methods
that are covered by this:
* Unless otherwise specified, passing a {@code null} argument to any method
* in this class will cause a {@link NullPointerException} to be thrown.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593687355
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593684673
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v29]
On Mon, 29 Apr 2024 21:17:24 GMT, Brent Christian wrote:
>> Classes in the `java.lang.ref` package would benefit from an update to bring
>> the spec in line with how the VM already behaves. The changes would focus on
>> _happens-before_ edges at some key points during reference processing.
>>
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the
>> registered cleaning action.
>>
>> This will bring Cleaner in line (or close) with the memory visibility
>> guarantees made for finalizers in [JLS
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> small grammar fixes
src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 39:
> 37: *
> 38: * href="{@docRoot}/java.base/java/lang/ref/package-summary.html#MemoryConsistency">Memory
> consistency effects:
> 39: * The enqueueing of a reference on a queue (by the garbage collector, or
> by a
In passing, I wonder if you're actually meant to say "on a queue" here.
Elsewhere the API docs speak of adding and removing, e.g. the enqueue method
speaks of adding a reference "to" the queue.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1593659540
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v29]
On Mon, 29 Apr 2024 21:17:24 GMT, Brent Christian wrote: >> Classes in the `java.lang.ref` package would benefit from an update to bring >> the spec in line with how the VM already behaves. The changes would focus on >> _happens-before_ edges at some key points during reference processing. >> >> A couple key things we want to be able to say are: >> - `Reference.reachabilityFence(x)` _happens-before_ reference processing >> occurs for 'x'. >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the >> registered cleaning action. >> >> This will bring Cleaner in line (or close) with the memory visibility >> guarantees made for finalizers in [JLS >> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): >> _"There is a happens-before edge from the end of a constructor of an object >> to the start of a finalizer (§12.6) for that object."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > small grammar fixes src/java.base/share/classes/java/lang/ref/Reference.java line 393: > 391: * Clears this reference object. Invoking this method does not > enqueue this > 392: * object, and the garbage collector will no longer clear or enqueue > this > 393: * object. "will no longer clear" means that it won't do it again, if you see what I mean. I think this would be easier to read if changed to "will not clear ...". - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1593651535
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v13]
On Tue, 19 Mar 2024 00:40:02 GMT, Y. Srinivas Ramakrishna
wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> further tweaks to reachability
>
> src/java.base/share/classes/java/lang/ref/package-info.java line 137:
>
>> 135: *
>> 136: * A reachable object is any object that can be accessed in
>> any potential
>> 137: * continuing computation from any live thread (as stated in {@jls
>> 12.6.1}).
>
> This seems like somewhat loose and sloppy wording to me. "Any potential
> continuing computation"? "Any live thread"? Could you share a pointer to JLS
> 12.6.1 being referenced here?
The phrase "live thread" is used in a number of places in the API docs to mean
a Thread that has been started and not has not terminated. The `@jls` tag will
create a link in the generated javadoc. If it helps, then it could also link to
Thread::isAlive.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1529527453
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v29]
On Mon, 29 Apr 2024 21:17:24 GMT, Brent Christian wrote:
>> Classes in the `java.lang.ref` package would benefit from an update to bring
>> the spec in line with how the VM already behaves. The changes would focus on
>> _happens-before_ edges at some key points during reference processing.
>>
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the
>> registered cleaning action.
>>
>> This will bring Cleaner in line (or close) with the memory visibility
>> guarantees made for finalizers in [JLS
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> small grammar fixes
src/java.base/share/classes/java/lang/ref/Cleaner.java line 224:
> 222: * Actions in a thread prior to calling {@code Cleaner.register()}
> 223: * href="{@docRoot}/java.base/java/util/concurrent/package-summary.html#MemoryVisibility">happen-before
> 224: * the cleaning action is run by the Cleaner's thread.
In passing, you could reduces the html refs to
"package-summary.html#MemoryConsistency" and
"package-summary.html#MemoryVisibility" as it's the same package/directory.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r159364
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v10]
On Fri, 23 Feb 2024 06:06:21 GMT, Brent Christian wrote: >> Classes in the `java.lang.ref` package would benefit from an update to bring >> the spec in line with how the VM already behaves. The changes would focus on >> _happens-before_ edges at some key points during reference processing. >> >> A couple key things we want to be able to say are: >> - `Reference.reachabilityFence(x)` _happens-before_ reference processing >> occurs for 'x'. >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the >> registered cleaning action. >> >> This will bring Cleaner in line (or close) with the memory visibility >> guarantees made for finalizers in [JLS >> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): >> _"There is a happens-before edge from the end of a constructor of an object >> to the start of a finalizer (§12.6) for that object."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > VM -> virtual machine; also fix misspelling src/java.base/share/classes/java/lang/ref/Cleaner.java line 218: > 216: * cautions about the behavior of cleaning actions. > 217: * > 218: * The object being registered is kept strongly reachable (and > therefore not eligible I would be tempted to drop "being registered" from this sentence. The previous paragraph and the parameter descriptions use "the object". That would make it "The given object is kept strongly reachable ..." which I think is a bit easier to read. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1500757065
Re: RFR: 8331907: BigInteger and BigDecimal should use optimized division
On Wed, 8 May 2024 08:19:54 GMT, Daniel Jeliński wrote: > Replace the custom unsigned divide / remainder functions with the > compiler-optimized Long.divideUnsigned / remainderUnsigned. > > No new tests. Existing tier1-3 tests continue to pass. > > I added a new set of divide benchmarks. Results in thread. > > I also removed the BigDecimal.toString benchmarks. They no longer serve their > purpose - toString caches the returned value, so we were only benchmarking > the cache access time. Results before: Benchmark Mode CntScore Error Units BigDecimals.testHugeLargeDivide avgt 15 115.176 ± 0.965 ns/op BigDecimals.testHugeSmallDivide avgt 15 82.652 ± 0.601 ns/op BigDecimals.testLargeSmallDivideavgt 156.601 ± 0.320 ns/op BigIntegers.testHugeLargeDivide avgt 15 53.905 ± 0.734 ns/op BigIntegers.testHugeSmallDivide avgt 15 52.762 ± 0.354 ns/op BigIntegers.testLargeSmallDivideavgt 15 22.990 ± 0.058 ns/op after: Benchmark Mode CntScore Error Units BigDecimals.testHugeLargeDivide avgt 15 106.549 ± 0.695 ns/op BigDecimals.testHugeSmallDivide avgt 15 68.339 ± 0.172 ns/op BigDecimals.testLargeSmallDivideavgt 156.594 ± 0.328 ns/op BigIntegers.testHugeLargeDivide avgt 15 29.576 ± 0.411 ns/op BigIntegers.testHugeSmallDivide avgt 15 30.072 ± 0.242 ns/op BigIntegers.testLargeSmallDivideavgt 158.034 ± 0.078 ns/op - PR Comment: https://git.openjdk.org/jdk/pull/19134#issuecomment-2100030115
RFR: 8331907: BigInteger and BigDecimal should use optimized division
Replace the custom unsigned divide / remainder functions with the compiler-optimized Long.divideUnsigned / remainderUnsigned. No new tests. Existing tier1-3 tests continue to pass. I added a new set of divide benchmarks. Results in thread. I also removed the BigDecimal.toString benchmarks. They no longer serve their purpose - toString caches the returned value, so we were only benchmarking the cache access time. - Commit messages: - Fix whitespace - Remove useless toString benchmarks - Update copyright - Remove redundant benchmarks - Use unsigned ops in BigDecimal - Simplify logic - Use divide/remainderUnsigned - Add BigInteger.divide microbenchmark Changes: https://git.openjdk.org/jdk/pull/19134/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19134=00 Issue: https://bugs.openjdk.org/browse/JDK-8331907 Stats: 212 lines in 4 files changed: 39 ins; 132 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/19134.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19134/head:pull/19134 PR: https://git.openjdk.org/jdk/pull/19134
Integrated: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
On Wed, 24 Apr 2024 13:50:56 GMT, Raffaello Giulietti wrote: > Move all random generators mandated in package `java.util.random` and > currently implemented in module `jdk.random` to module `java.base`, and > remove module `jdk.random`. This pull request has now been integrated. Changeset: 7f299043 Author:Raffaello Giulietti URL: https://git.openjdk.org/jdk/commit/7f299043a99406dbd666d4f7f30445d26f3eae82 Stats: 115 lines in 14 files changed: 11 ins; 77 del; 27 mod 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/18932
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v7]
> Move all random generators mandated in package `java.util.random` and > currently implemented in module `jdk.random` to module `java.base`, and > remove module `jdk.random`. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Removed empty line. - Changes: - all: https://git.openjdk.org/jdk/pull/18932/files - new: https://git.openjdk.org/jdk/pull/18932/files/8a5598ea..d2bdf337 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18932=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18932=05-06 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18932.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932 PR: https://git.openjdk.org/jdk/pull/18932
Re: RFR: 8330542: Add two JAXP configuration files in preparation for a secure by default configuration [v6]
On Wed, 1 May 2024 22:33:29 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >> jaxp-compat.properties: used to regain compatibility from any more >> restricted configuration than previous versions such as JDK 22 > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Add implNote to java.xml module summary; Update make file; Update the > config files; Add test. Adding jaxp-strict.properties make sense as it allows developers to identify issues that will arise in the future when XML processing is secure by default. If they deploy with -Djava.xml.config.file=jaxp-strict.properties, and jaxp-strict.properties is removed as part of moving to secure by default, then it should be okay too as the defaults will be strict. I'm less sure about including jaxp-compat.properties in JDK 23. That's the config file to get temporary relief while you work through the issues with existing code or deployments that break when XML processing is secure by default. Adding in the JDK 23 sends the message that you can "prepare" your command line in advance, which I don't think should be a goal here. - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2099840683
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]
On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti wrote: >> Move all random generators mandated in package `java.util.random` and >> currently implemented in module `jdk.random` to module `java.base`, and >> remove module `jdk.random`. > > Raffaello Giulietti 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 seven additional > commits since the last revision: > > - Merge branch 'master' into 8330005 > - Restrict RandomGenerator service providers to those loadable by the > platform class loader. > - Typo. > - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base. > - Terminology changes. > - Renamed package jdk.random to jdk.internal.random. > - 8330005: RandomGeneratorFactory.getDefault() throws exception when the > runtime image only has java.base module Hello Alan, > RandomTestCoverage should be testing these APIs but if there are gaps then it > would be good to add more tests (separate PR of course) I missed that test case. It does appear to cover most of these APIs on `RandomGeneratorFactory`. There's a method in that test which even calls `RandomGeneratorFactory.getDefault()` but that doesn't seem to be exercised. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18932#pullrequestreview-2044738539
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]
On Wed, 8 May 2024 05:29:03 GMT, Jaikiran Pai wrote: > Hello Raffaello, the proposed changes look OK to me. Do you think we should > add a jtreg test for this? > > In general, the test coverage for these APIs appears to be missing. I think > that can be addressed separately in some of the upcoming changes. RandomTestCoverage should be testing these APIs but if there are gaps then it would be good to add more tests (separate PR of course). There is a second update coming once this PR is integrated, that will change the implementation to maybe not use SL. - PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2099815847
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]
On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti wrote: >> Move all random generators mandated in package `java.util.random` and >> currently implemented in module `jdk.random` to module `java.base`, and >> remove module `jdk.random`. > > Raffaello Giulietti 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 seven additional > commits since the last revision: > > - Merge branch 'master' into 8330005 > - Restrict RandomGenerator service providers to those loadable by the > platform class loader. > - Typo. > - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base. > - Terminology changes. > - Renamed package jdk.random to jdk.internal.random. > - 8330005: RandomGeneratorFactory.getDefault() throws exception when the > runtime image only has java.base module Marked as reviewed by alanb (Reviewer). src/java.base/share/classes/module-info.java line 426: > 424: java.util.Random, > 425: java.util.SplittableRandom, > 426: Can you remove this blank files, otherwise the list of service types for this provide declaration are all together? - PR Review: https://git.openjdk.org/jdk/pull/18932#pullrequestreview-2044707607 PR Review Comment: https://git.openjdk.org/jdk/pull/18932#discussion_r1593435059
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 19:46:18 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Strengthen tests after 8330998
>
> https://github.com/openjdk/jdk/pull/18996 now allows us to test
> Console IO better.
src/java.base/share/classes/java/io/IO.java line 51:
> 49: * console, terminates the line and then flushes that console.
> 50: *
> 51: * If {@code System.console()} returns {@code null}, throws {@code
> IOError}.
Unlike, `java.io.Console` whose existing methods already throw `IOError`,
should these new methods on `java.io.IO` instead throw
`java.io.UncheckedIOException` instead of throwing `Error`s?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593424569
