Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 06:27:20 GMT, David Holmes wrote: >> I think of this as an expression that is always evaluated to the same value. >> The value itself is not interesting, it is the set of values that this >> expression can take that we are talking about. > > This seems really weird to me for Java code. The method doesn't get the > original "expression" it only gets the value of that expression after it has > been evaluated. Is there some kind of weird "magic" happening here? @dholmes-ora Indeed it's a compiler magic, albeit not really weird. While the method execution only receives the evaluated value of `expr`, the method compilation has the expression in its original form. As a result, it can determine the result based on this information. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464415357
Re: RFR: 8320712: Rewrite BadFactoryTest in pure Java [v2]
On Wed, 24 Jan 2024 06:50:36 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which rewrites the BadFactoryTest to pure Java/JUnit. >> The test is currently implemented using a mix of shell script and a Java >> main method. >> >> Reviewers may notice the following changes: >> >> - The shell script file `BadFactoryTest.sh` has been retired and jtreg tags >> moved over to `BadFactoryTest.java` >> - The main method has been replaced with a JUnit `@Test` method >> - The service definition file used to be packaged into a jar file, now the >> source directory is instead directly added to the classpath using `@library >> /javax/script/JDK_8196959` >> - The `@summary` tag was updated since the existing summary was slightly >> confusing >> - To make jtreg happy, the SecurityManager-enabled invocation now runs with >> `-Djava.security.manager=allow` instead of just `-Djava.security.manager` >> - A sanity check was added to verify that `ScriptEngineManager` can actually >> find and load `BadFactory`. Such a check would have detected the issue which >> inspired this rewrite, see #16585. >> >> Verified by running: >> >> >> % make test TEST="test/jdk/javax/script/JDK_8196959" >> [..] >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/jdk/javax/script/JDK_8196959 1 1 0 0 >> >> >> >> Note that the original issue JDK-8196959 is not available in JBS, so my >> understanding of what the original test does is based on code inspection. > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - Remove the @library tag, the META-INF service definition file is already > on the classpath > - Merge branch 'master' into badfactory-java-rewritte > - Add the Java rewrite issue to the @bug list > - Merge branch 'master' into badfactory-java-rewritte > ># Conflicts: ># test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh > - Move jtreg tags from before to after imports > - Merge branch 'master' into badfactory-java-rewritte > - Update the @summary to describe the purpose of the test, not the BadFactory > - Add comment describing the initialization of ScriptEngineManager > - Implement BadFactoryTest in pure Java Thank you for the update. This test-only change looks OK to me. Hello Sundar @sundararajana, given your past work on this test, do you have any thoughts on this change? - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16830#pullrequestreview-1840572685
Re: RFR: 8320712: Rewrite BadFactoryTest in pure Java
On Wed, 24 Jan 2024 05:03:29 GMT, Jaikiran Pai wrote: > The only part that is unclear to me is the use of `@library > /javax/script/JDK_8196959`. Does the test fail if that isn't added? Thanks! I must have thought this was required to put the service definition file on the classpath. But it isn't. I have removed the `@library` tag and also merged with a more recent master. - PR Comment: https://git.openjdk.org/jdk/pull/16830#issuecomment-1907474466
Re: RFR: 8320712: Rewrite BadFactoryTest in pure Java [v2]
> Please review this PR which rewrites the BadFactoryTest to pure Java/JUnit. > The test is currently implemented using a mix of shell script and a Java main > method. > > Reviewers may notice the following changes: > > - The shell script file `BadFactoryTest.sh` has been retired and jtreg tags > moved over to `BadFactoryTest.java` > - The main method has been replaced with a JUnit `@Test` method > - The service definition file used to be packaged into a jar file, now the > source directory is instead directly added to the classpath using `@library > /javax/script/JDK_8196959` > - The `@summary` tag was updated since the existing summary was slightly > confusing > - To make jtreg happy, the SecurityManager-enabled invocation now runs with > `-Djava.security.manager=allow` instead of just `-Djava.security.manager` > - A sanity check was added to verify that `ScriptEngineManager` can actually > find and load `BadFactory`. Such a check would have detected the issue which > inspired this rewrite, see #16585. > > Verified by running: > > > % make test TEST="test/jdk/javax/script/JDK_8196959" > [..] >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/javax/script/JDK_8196959 1 1 0 0 > > > > Note that the original issue JDK-8196959 is not available in JBS, so my > understanding of what the original test does is based on code inspection. Eirik Bjørsnøs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Remove the @library tag, the META-INF service definition file is already on the classpath - Merge branch 'master' into badfactory-java-rewritte - Add the Java rewrite issue to the @bug list - Merge branch 'master' into badfactory-java-rewritte # Conflicts: #test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh - Move jtreg tags from before to after imports - Merge branch 'master' into badfactory-java-rewritte - Update the @summary to describe the purpose of the test, not the BadFactory - Add comment describing the initialization of ScriptEngineManager - Implement BadFactoryTest in pure Java - Changes: https://git.openjdk.org/jdk/pull/16830/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16830&range=01 Stats: 86 lines in 2 files changed: 24 ins; 60 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16830.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16830/head:pull/16830 PR: https://git.openjdk.org/jdk/pull/16830
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 22:46:20 GMT, Quan Anh Mai wrote:
>> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
>>
>>> 54: */
>>> 55: @IntrinsicCandidate
>>> 56: public static boolean isCompileConstant(boolean expr) {
>>
>> Here and in other places: probably not `expr`, but just `val` or something?
>
> I think of this as an expression that is always evaluated to the same value.
> The value itself is not interesting, it is the set of values that this
> expression can take that we are talking about.
This seems really weird to me for Java code. The method doesn't get the
original "expression" it only gets the value of that expression after it has
been evaluated. Is there some kind of weird "magic" happening here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464361310
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform >> class loader instead of the boot class loader. This allows Native Image to >> load a version of JVMCI different than the version on top of which Native >> Image is running. This capability is demonstrated and tested by >> `LoadAlternativeJVMCI.java`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent The actual changes to load JVMCI via the platform loader seem fine. I'm still puzzled by the need to do this as any non-delegating classloader would have allowed this even if JVMCI were loaded by the bootloader. And I don't understand how your test is working as it is using a delegating classloader. test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54: > 52: > 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader(); > 54: URLClassLoader ucl = new URLClassLoader(cp, null); I am missing something here, a `URLClassLoader` first delegates to its parent before searching its URLs, so how does this not find the platform loader versions of the JVMCI classes? - PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1840477028 PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464348710
Re: RFR: 8320712: Rewrite BadFactoryTest in pure Java
On Mon, 27 Nov 2023 17:44:24 GMT, Eirik Bjørsnøs wrote: > Please review this PR which rewrites the BadFactoryTest to pure Java/JUnit. > The test is currently implemented using a mix of shell script and a Java main > method. > > Reviewers may notice the following changes: > > - The shell script file `BadFactoryTest.sh` has been retired and jtreg tags > moved over to `BadFactoryTest.java` > - The main method has been replaced with a JUnit `@Test` method > - The service definition file used to be packaged into a jar file, now the > source directory is instead directly added to the classpath using `@library > /javax/script/JDK_8196959` > - The `@summary` tag was updated since the existing summary was slightly > confusing > - To make jtreg happy, the SecurityManager-enabled invocation now runs with > `-Djava.security.manager=allow` instead of just `-Djava.security.manager` > - A sanity check was added to verify that `ScriptEngineManager` can actually > find and load `BadFactory`. Such a check would have detected the issue which > inspired this rewrite, see #16585. > > Verified by running: > > > % make test TEST="test/jdk/javax/script/JDK_8196959" > [..] >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/javax/script/JDK_8196959 1 1 0 0 > > > > Note that the original issue JDK-8196959 is not available in JBS, so my > understanding of what the original test does is based on code inspection. Hello Eirik, what's being proposed here looks good to me. It's good that this is being migrated from a shell based test to a java based test. So if you are interested in pursuing this further, please reopen the PR. The only part that is unclear to me is the use of `@library /javax/script/JDK_8196959`. Does the test fail if that isn't added? - PR Comment: https://git.openjdk.org/jdk/pull/16830#issuecomment-1907384336
Withdrawn: 8320712: Rewrite BadFactoryTest in pure Java
On Mon, 27 Nov 2023 17:44:24 GMT, Eirik Bjørsnøs wrote: > Please review this PR which rewrites the BadFactoryTest to pure Java/JUnit. > The test is currently implemented using a mix of shell script and a Java main > method. > > Reviewers may notice the following changes: > > - The shell script file `BadFactoryTest.sh` has been retired and jtreg tags > moved over to `BadFactoryTest.java` > - The main method has been replaced with a JUnit `@Test` method > - The service definition file used to be packaged into a jar file, now the > source directory is instead directly added to the classpath using `@library > /javax/script/JDK_8196959` > - The `@summary` tag was updated since the existing summary was slightly > confusing > - To make jtreg happy, the SecurityManager-enabled invocation now runs with > `-Djava.security.manager=allow` instead of just `-Djava.security.manager` > - A sanity check was added to verify that `ScriptEngineManager` can actually > find and load `BadFactory`. Such a check would have detected the issue which > inspired this rewrite, see #16585. > > Verified by running: > > > % make test TEST="test/jdk/javax/script/JDK_8196959" > [..] >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/javax/script/JDK_8196959 1 1 0 0 > > > > Note that the original issue JDK-8196959 is not available in JBS, so my > understanding of what the original test does is based on code inspection. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16830
Re: RFR: 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview) [v41]
On Mon, 22 Jan 2024 18:42:56 GMT, Aggelos Biboudis wrote: >> This is the proposed patch for Primitive types in patterns, instanceof, and >> switch (Preview). >> >> Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/ > > Aggelos Biboudis has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 60 commits: > > - Improve Javadoc of ExactConversionsSupport > - Merge branch 'master' into primitive-patterns > - Update ExactConversionsSupport Javadoc and JDK version > - Revert "Update copyright year, javadoc, JDK version" > >This reverts commit 676af9de90d946f64f34762a6df94dbd91bce41b. > - Update copyright year, javadoc, JDK version > - Merge branch 'master' into primitive-patterns > - Merge branch 'master' into primitive-patterns > - Cleanup > - Merge branch 'master' into primitive-patterns > - Remove trailing spaces > - ... and 50 more: https://git.openjdk.org/jdk/compare/c9cacfb2...50cb9832 src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 72: > 70: private static final Object SENTINEL = new Object(); > 71: private static final MethodHandles.Lookup LOOKUP = > MethodHandles.lookup(); > 72: private static final boolean previewEnabled = true; not sure about the use of this constant, do we really need it? - PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1464262036
RFR: 8324573: HashMap::putAll should resize to sum of both map sizes
This change mirrors what we did for ConcurrentHashMap in https://github.com/openjdk/jdk/pull/17116. When we add all entries from one map to anther, we should resize that map to the size of the sum of both maps. I used the command below to run the benchmarks. I set a high heap to reduce garbage collection noise. java -Xms25G -jar benchmarks.jar -p size=10 -p addSize=10 -gc true org.openjdk.bench.java.util.HashMapBench Before change Benchmark(addSize)(mapType) (size) Mode Cnt Score Error Units HashMapBench.putAll 10 HASH_MAP 10 avgt4 22.927 ± 3.170 ms/op HashMapBench.putAll 10 LINKED_HASH_MAP 10 avgt4 25.198 ± 2.189 ms/op After change Benchmark(addSize)(mapType) (size) Mode Cnt Score Error Units HashMapBench.putAll 10 HASH_MAP 10 avgt4 16.780 ± 0.526 ms/op HashMapBench.putAll 10 LINKED_HASH_MAP 10 avgt4 19.721 ± 0.349 ms/op We see about average time improvements of 26% in HashMap and 20% in LinkedHashMap. - Commit messages: - 8324573: HashMap::putAll should resize to sum of both map sizes Changes: https://git.openjdk.org/jdk/pull/17544/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17544&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324573 Stats: 39 lines in 2 files changed: 13 ins; 7 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/17544.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17544/head:pull/17544 PR: https://git.openjdk.org/jdk/pull/17544
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v4]
> Please consider this fix to ensure that going from `MessageFormat` to pattern
> string via `toPattern()` and then back via `new MessageFormat()` results in a
> format that is equivalent to the original.
>
> The quoting and escaping rules for `MessageFormat` pattern strings are really
> tricky. I admit not completely understanding them. At a high level, they work
> like this: The normal way one would "nest" strings containing special
> characters is with straightforward recursive escaping like with the `bash`
> command line. For example, if you want to echo `a "quoted string" example`
> then you enter `echo "a "quoted string" example"`. With this scheme it's
> always the "outer" layer's job to (un)escape special characters as needed.
> That is, the echo command never sees the backslash characters.
>
> In contrast, with `MessageFormat` and friends, nested subformat pattern
> strings are always provided "pre-escaped". So to build an "outer" string
> (e.g., for `ChoiceFormat`) the "inner" subformat pattern strings are more or
> less just concatenated, and then only the `ChoiceFormat` option separator
> characters (e.g., `<`, `#`, `|`, etc.) are escaped.
>
> The "pre-escape" escaping algorithm escapes `{` characters, because `{`
> indicates the beginning of a format argument. However, it doesn't escape `}`
> characters. This is OK because the format string parser treats any "extra"
> closing braces (where "extra" means not matching an opening brace) as plain
> characters.
>
> So far, so good... at least, until a format string containing an extra
> closing brace is nested inside a larger format string, where the extra
> closing brace, which was previously "extra", can now suddenly match an
> opening brace in the outer pattern containing it, thus truncating it by
> "stealing" the match from some subsequent closing brace.
>
> An example is the `MessageFormat` string `"{0,choice,0.0#option A:
> {1}|1.0#option B: {1}'}'}"`. Note the second option format string has a
> trailing closing brace in plain text. If you create a `MessageFormat` with
> this string, you see a trailing `}` only with the second option.
>
> However, if you then invoke `toPattern()`, the result is
> `"{0,choice,0.0#option A: {1}|1.0#option B: {1}}}"`. Oops, now because the
> "extra" closing brace is no longer quoted, it matches the opening brace at
> the beginning of the string, and the following closing brace, which was the
> previous match, is now just plain text in the outer `MessageFormat` string.
>
> As a result, invoking `f.format(new Object{} { 0, 5 })` will retur...
Archie Cobbs has updated the pull request incrementally with six additional
commits since the last revision:
- Add more test cases and more pattern string variety.
- Make it easier to debug & show what the test is doing.
- Add comment explaining what MAX_FORMAT_NESTING is for.
- Clean up code a bit by using instanceof patterns.
- Tweak @implNote to clarify only referring to MessageFormat class.
- Update copyright year in MessageFormat.java.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17416/files
- new: https://git.openjdk.org/jdk/pull/17416/files/36d70b8a..58e8cc68
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17416&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17416&range=02-03
Stats: 94 lines in 2 files changed: 50 ins; 12 del; 32 mod
Patch: https://git.openjdk.org/jdk/pull/17416.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17416/head:pull/17416
PR: https://git.openjdk.org/jdk/pull/17416
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v3]
On Tue, 23 Jan 2024 22:15:40 GMT, Justin Lu wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add @implNote to Javadoc for toPattern().
>
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 90:
>
>> 88: // A few more test cases from the PR#17416 discussion
>> 89: testRoundTrip(new MessageFormat("Test: {0,number,foo'{'#.00}"));
>> 90: testRoundTrip(new MessageFormat("Test: {0,number,foo'}'#.00}"));
>
> Can we add some more concrete test cases, for example, escaped quotes within
> a MessageFormat pattern (represented by doubled single quotes).
>
> Another funny case is something like a MessageFormat created by `"{0,number,'
> abc }'' ' 0.00}"` which becomes `"{0,number, abc '}''' #0.00}"` after
> created from the value returned by toPattern() from the first MessageFormat.
> The Strings appear really different but format equivalently, it would be nice
> to have suspicious cases like these explicitly defined.
Sure thing - added in 58e8cc68f45.
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 154:
>
>> 152: // Generate a "random" MessageFormat. We do this by creating a
>> MessageFormat with "{0}" placeholders
>> 153: // and then substituting in random DecimalFormat, DateFormat, and
>> ChoiceFormat subformats. The goal here
>> 154: // is to avoid using pattern strings to construct formats, because
>> they're what we're trying to check.
>
> Can you add a general example of a randomly generated MessageFormat in the
> comments, (I know they vary by quite a lot), but it would be hard for someone
> to piece if together without digging into the code. And unfortunately the
> current toString() of MessageFormat is not implemented, so although JUnit
> prints the MessageFormat out, it's just gibberish.
Added in 58e8cc68f45 as a test case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464143414
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464143622
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v3]
On Tue, 23 Jan 2024 23:12:19 GMT, Archie Cobbs wrote:
>> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
>> 96:
>>
>>> 94: @ParameterizedTest
>>> 95: @MethodSource("testCases")
>>> 96: public void testRoundTrip(MessageFormat format1) {
>>
>> Can we also include the original String pattern that created format1, to
>> help with debugging. I find myself wondering what the original String was.
>>
>> Since technically, the full round trip is _pattern string -> MessageFormat1
>> -> pattern string -> MessageFormat2_
>
> I would have done that but it's not (easily) possible. The `MessageFormat`'s
> are created not from format strings, but by piecing together plain text and
> sub-`Format` objects manually. This was we are sure what we're dealing with.
>
> Trying to create format strings with multiple levels of nesting from scratch
> is too complex for my brain due to all the levels of quoting required.
Right, should have noted that, definitely not worth to try and re-synthesize
the original String pattern for each Format. If we're adding some additional
concrete cases, its fine since those will clearly have the original String
patterns there.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464131694
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v3]
On Tue, 23 Jan 2024 23:00:29 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/text/MessageFormat.java line 556:
>>
>>> 554: * does not necessarily equal the previously applied pattern.
>>> 555: *
>>> 556: * @implNote The string returned by this method can be used to
>>> create
>>
>> Hmm, I'm not sure about the current note, because its not true in all cases
>> (for example, some unknown subclass of Format). Maybe @naotoj has thoughts?
>
> Yes, my comment to the CSR was to explain the default implementation as the
> `@implNote`. Making it explicit would be helpful to readers.
Yes my mistake, I'll change this to:
@implNote The implementation in {@link MessageFormat} returns a string
that can be used to create a new instance that is semantically equivalent
to this instance.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104922
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v3]
On Tue, 23 Jan 2024 22:13:09 GMT, Justin Lu wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add @implNote to Javadoc for toPattern().
>
> src/java.base/share/classes/java/text/MessageFormat.java line 1:
>
>> 1: /*
>
> For this file, please also bump the latter copyright year to 2024.
Will do.
> src/java.base/share/classes/java/text/MessageFormat.java line 585:
>
>> 583: if (fmt instanceof DecimalFormat) {
>> 584: result.append(",number");
>> 585: subformatPattern =
>> ((DecimalFormat)fmt).toPattern();
>
> Could use pattern matching `instanceof` here and in the other instances, but
> I understand if you're trying to stay consistent with the existing code.
Agreed! Old habits :) I'll fix.
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 56:
>
>> 54:
>> 55: private static final int NUM_RANDOM_TEST_CASES = 1000;
>> 56: private static final int MAX_FORMAT_NESTING = 3;
>
> Can you add a comment defining `MAX_FORMAT_TESTING`, might not be immediately
> understood without further investigation
Will do.
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 96:
>
>> 94: @ParameterizedTest
>> 95: @MethodSource("testCases")
>> 96: public void testRoundTrip(MessageFormat format1) {
>
> Can we also include the original String pattern that created format1, to help
> with debugging. I find myself wondering what the original String was.
>
> Since technically, the full round trip is _pattern string -> MessageFormat1
> -> pattern string -> MessageFormat2_
I would have done that but it's not (easily) possible. The `MessageFormat`'s
are created not from format strings, but by piecing together plain text and
sub-`Format` objects manually. This was we are sure what we're dealing with.
Trying to create format strings with multiple levels of nesting from scratch is
too complex for my brain due to all the levels of quoting required.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104293
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104138
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104181
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104467
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v3]
On Tue, 23 Jan 2024 22:13:14 GMT, Justin Lu wrote: >> Archie Cobbs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add @implNote to Javadoc for toPattern(). > > src/java.base/share/classes/java/text/MessageFormat.java line 556: > >> 554: * does not necessarily equal the previously applied pattern. >> 555: * >> 556: * @implNote The string returned by this method can be used to >> create > > Hmm, I'm not sure about the current note, because its not true in all cases > (for example, some unknown subclass of Format). Maybe @naotoj has thoughts? Yes, my comment to the CSR was to explain the default implementation as the `@implNote`. Making it explicit would be helpful to readers. - PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464096885
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 20:01:45 GMT, Paul Sandoz wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> ident
>
> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 32:
>
>> 30: * Just-in-time-compiler-related queries
>> 31: */
>> 32: public class JitCompiler {
>
> An alternative name and location is `jdk.internal.vm.ConstantSupport` with
> initial class doc:
>
> Defines methods to test if a value has been evaluated to a compile-time
> constant value by the HotSpot VM.
That sounds like a better name for the class, although I think
`jdk.internal.misc` is more suitable than `jdk.internal.vm`. Do you have any
preference? Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464087772
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:42:40 GMT, Aleksey Shipilev wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> ident
>
> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
>
>> 54: */
>> 55: @IntrinsicCandidate
>> 56: public static boolean isCompileConstant(boolean expr) {
>
> Here and in other places: probably not `expr`, but just `val` or something?
I think of this as an expression that is always evaluated to the same value.
The value itself is not interesting, it is the set of values that this
expression can take that we are talking about.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464085126
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:40:52 GMT, Aleksey Shipilev wrote: >> Quan Anh Mai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> ident > > src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119: > >> 117: * @see #isCompileConstant(boolean) >> 118: */ >> 119: @IntrinsicCandidate > > Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. > These methods should have `@Hidden` too then? Probably applies to other > entries too. I don't understand why this needs to be `@Hidden`, the javadoc says that a function annotated with `@Hidden` is omitted from the stacktraces. This function does not call anything so what is the point of hiding it? - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464081953
Integrated: JDK-8321545: Override toString() for Format subclasses
On Wed, 10 Jan 2024 21:05:38 GMT, Justin Lu wrote:
> Please review this PR which implements toString() for the `Format`
> subclasses. Corresponding CSR:
> [JDK-8323088](https://bugs.openjdk.org/browse/JDK-8323088)
>
> The general specification follows a template that provides the locale (if the
> class is localized) and any relevant patterns. The specification was
> intentionally kept minimal and deliberately worded as "for debugging".
>
> An example of all the classes has output such as
>
>
> CompactNumberFormat [locale: "English (United States)", decimal pattern:
> "foo#0.00#baz", compact patterns: "[, , , {one:0K other:0K}, {one:00K
> other:00K}, {one:000K other:000K}, {one:0M other:0M}, {one:00M other:00M},
> {one:000M other:000M}, {one:0B other:0B}, {one:00B other:00B}, {one:000B
> other:000B}, {one:0T other:0T}, {one:00T other:00T}, {one:000T other:000T}]"]
>
> DecimalFormat [locale: "English (United States)", pattern: "foo#0.00#baz"]
>
> SimpleDateFormat [locale: "Chinese (China)", pattern: "EEE, MMM d, ''yy"]
>
> ListFormat [locale: "English (United States)", start: "{0}, {1}", middle:
> "{0}, {1}", end: "{0}, and {1}", two: "{0} and {1}", three: "{0}, {1}, and
> {2}"]
>
> MessageFormat [locale: "Chinese (China)", pattern: "foo {0}"]
>
> ChoiceFormat [pattern: "0#foo"]
This pull request has now been integrated.
Changeset: 96607df7
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/96607df7f055a80d56ea4c19f3f4fcb32838b1f8
Stats: 419 lines in 12 files changed: 412 ins; 0 del; 7 mod
8321545: Override toString() for Format subclasses
Reviewed-by: naoto, rriggs
-
PR: https://git.openjdk.org/jdk/pull/17355
Re: RFR: JDK-6503196: API doc for DecimalFormat::getMaximumIntegerDigits is unclear
On Tue, 23 Jan 2024 18:40:42 GMT, Justin Lu wrote: > Please review this PR which clarifies some confusion for the digit getter and > setter methods of DecimalFormat. > > When formatting non `BigInteger` and `BigDecimal` values, DecimalFormat uses > 309/340 as hard limits for integer and fraction digit limits, regardless of > the value set by the user. There was some confusion, that those numbers might > be returned by the getters, when in reality, they are only used internally. > > Moving the 309/340 wording to the class description and linking to it reduces > the repetitive wording, but also eliminates the confusion that the getters > may return those values. This should be OK, as setting limits higher than > those values are likely rare, so the warning does not need to be in every > method description. > > Additionally, `getMaximumIntegerDigits()` is updated to point to the patterns > section to warn about the non-obvious rules for max integer digits. Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17541#pullrequestreview-1840051983
Re: RFR: JDK-6503196: API doc for DecimalFormat::getMaximumIntegerDigits is unclear
On Tue, 23 Jan 2024 18:40:42 GMT, Justin Lu wrote: > Please review this PR which clarifies some confusion for the digit getter and > setter methods of DecimalFormat. > > When formatting non `BigInteger` and `BigDecimal` values, DecimalFormat uses > 309/340 as hard limits for integer and fraction digit limits, regardless of > the value set by the user. There was some confusion, that those numbers might > be returned by the getters, when in reality, they are only used internally. > > Moving the 309/340 wording to the class description and linking to it reduces > the repetitive wording, but also eliminates the confusion that the getters > may return those values. This should be OK, as setting limits higher than > those values are likely rare, so the warning does not need to be in every > method description. > > Additionally, `getMaximumIntegerDigits()` is updated to point to the patterns > section to warn about the non-obvious rules for max integer digits. LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17541#pullrequestreview-1840039217
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v3]
On Fri, 19 Jan 2024 23:30:43 GMT, Archie Cobbs wrote:
>> Please consider this fix to ensure that going from `MessageFormat` to
>> pattern string via `toPattern()` and then back via `new MessageFormat()`
>> results in a format that is equivalent to the original.
>>
>> The quoting and escaping rules for `MessageFormat` pattern strings are
>> really tricky. I admit not completely understanding them. At a high level,
>> they work like this: The normal way one would "nest" strings containing
>> special characters is with straightforward recursive escaping like with the
>> `bash` command line. For example, if you want to echo `a "quoted string"
>> example` then you enter `echo "a "quoted string" example"`. With this scheme
>> it's always the "outer" layer's job to (un)escape special characters as
>> needed. That is, the echo command never sees the backslash characters.
>>
>> In contrast, with `MessageFormat` and friends, nested subformat pattern
>> strings are always provided "pre-escaped". So to build an "outer" string
>> (e.g., for `ChoiceFormat`) the "inner" subformat pattern strings are more or
>> less just concatenated, and then only the `ChoiceFormat` option separator
>> characters (e.g., `<`, `#`, `|`, etc.) are escaped.
>>
>> The "pre-escape" escaping algorithm escapes `{` characters, because `{`
>> indicates the beginning of a format argument. However, it doesn't escape `}`
>> characters. This is OK because the format string parser treats any "extra"
>> closing braces (where "extra" means not matching an opening brace) as plain
>> characters.
>>
>> So far, so good... at least, until a format string containing an extra
>> closing brace is nested inside a larger format string, where the extra
>> closing brace, which was previously "extra", can now suddenly match an
>> opening brace in the outer pattern containing it, thus truncating it by
>> "stealing" the match from some subsequent closing brace.
>>
>> An example is the `MessageFormat` string `"{0,choice,0.0#option A:
>> {1}|1.0#option B: {1}'}'}"`. Note the second option format string has a
>> trailing closing brace in plain text. If you create a `MessageFormat` with
>> this string, you see a trailing `}` only with the second option.
>>
>> However, if you then invoke `toPattern()`, the result is
>> `"{0,choice,0.0#option A: {1}|1.0#option B: {1}}}"`. Oops, now because the
>> "extra" closing brace is no longer quoted, it matches the opening brace at
>> the beginning of the string, and the following closing brace, which was the
>> previous match, is now just plain text in the outer `MessageFormat` string.
>>
>> As a result, invoking `f.format(new ...
>
> Archie Cobbs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add @implNote to Javadoc for toPattern().
Hi @archiecobbs, left some initial comments.
But I think the logic is sound so far. Quote all quotable characters that need
to be quoted, at the MessageFormat level this is simply opening and closing
bracket, which you accomplished with the 2 pass copyAndQuoteBraces method. The
goal is to create a resultant String that can create a MessageFormat that
formats equivalently as the original, (even if calling toPattern() on both
MessageFormats returns different String values).
Let's also try and get the CSR approved first.
src/java.base/share/classes/java/text/MessageFormat.java line 1:
> 1: /*
For this file, please also bump the latter copyright year to 2024.
src/java.base/share/classes/java/text/MessageFormat.java line 556:
> 554: * does not necessarily equal the previously applied pattern.
> 555: *
> 556: * @implNote The string returned by this method can be used to create
Hmm, I'm not sure about the current note, because its not true in all cases
(for example, some unknown subclass of Format). Maybe @naotoj has thoughts?
src/java.base/share/classes/java/text/MessageFormat.java line 585:
> 583: if (fmt instanceof DecimalFormat) {
> 584: result.append(",number");
> 585: subformatPattern =
> ((DecimalFormat)fmt).toPattern();
Could use pattern matching `instanceof` here and in the other instances, but I
understand if you're trying to stay consistent with the existing code.
test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line 56:
> 54:
> 55: private static final int NUM_RANDOM_TEST_CASES = 1000;
> 56: private static final int MAX_FORMAT_NESTING = 3;
Can you add a comment defining `MAX_FORMAT_TESTING`, might not be immediately
understood without further investigation
test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line 90:
> 88: // A few more test cases from the PR#17416 discussion
> 89: testRoundTrip(new MessageFormat("Test: {0,number,foo'{'#.00}"));
> 90: testRoundTrip(new MessageFormat("Test: {0,number,foo'}'#.00}"));
Can we ad
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v8]
On Tue, 23 Jan 2024 18:53:39 GMT, Jim Laskey wrote:
>> Currently String::translateEscapes does not support unicode escapes,
>> reported as a IllegalArgumentException("Invalid escape sequence: ...").
>> String::translateEscapes should translate unicode escape sequences to
>> provide full coverage,
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Requested changes
src/java.base/share/classes/java/lang/String.java line 4229:
> 4227: * {@code \u005Cu...u}
> 4228: * unicode escape
> 4229: * single character UTF-16 equivalent
It would be clearer to have "single UTF-16 code unit equivalent" as the
translation explanation
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1464029302
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Tue, 23 Jan 2024 18:24:59 GMT, Jorn Vernee wrote: >> Should we make these unaligned access modes throw ISE like before, when the >> given index is unaligned? > > You mean `get` and `set`? They should never throw, as unaligned access is > fine. For other access modes, we can never guarantee that an access is > aligned, so UOE is appropriate. (IIRC this is mandated by existing spec. I'll > try to find it again) > > P.S. See e.g. the javadoc of `VarHandle::getVolatile`: > >> @throws UnsupportedOperationException if the access mode is unsupported >> for this VarHandle. > > P.P.S. Also remembering that we can not have any implementation for the > access methods, in order for `isAccessModeSupported` to work correctly. And > the logic that handles unsupported methods throws UOE (see > `VarForm::getMemberName`) Good point, so previous behavior of throwing ISE is out of spec - PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1463958704
Re: Gatherer: spliterator characteristics are not propagated
> From: "Viktor Klang" > To: "Remi Forax" > Cc: "core-libs-dev" , "Paul Sandoz" > > Sent: Monday, January 22, 2024 10:06:27 PM > Subject: Re: Gatherer: spliterator characteristics are not propagated >> The flags are in sync with the implementation because the only way to create >> a >> Gatherer if through the factory methods and those factory methods (and only >> them) compute the proper combination of SEQUENTIAL | STATELESS | GREEDY. A >> user >> should not be able to set those flags. Only the flags KEEP_* are settable by >> a >> user. > But I presume this also requires to have a `int characteristics()`-method on > the > Gatherer interfacewhich means that users who are not using the factory methods > will have full possibility of not only returning the flags, but returning any > int. The current implementation suffers the same kind of issue, it's easy to write a mutable Gatherer and change the functions after creation, worst, right in the middle of a call to stream.gather(...). Perhaps the Gatherer interface should be sealed ? We did not have that option during the 1.8 timeframe, when the Collector API was created. >> The stream implementation has a whole mechanism in place to >> propagate/preverse >> flags like SIZED, DISTINCT or SORTED. For me, discussing about the merit of >> this mechanism seems a little off topic. I would prefer the Gatherer to be a >> good citizen and works seemlessly with the other intermediary operations. > I can see where you're coming from here, but to me, adding API surface needs > to > pull its weight. > In this case I wasn't convinced that it did, hence we're having this > conversation. \uD83D\uDE42 > Cheers, > √ regards, Rémi > Viktor Klang > Software Architect, Java Platform Group > Oracle > From: fo...@univ-mlv.fr > Sent: Monday, 22 January 2024 19:56 > To: Viktor Klang > Cc: core-libs-dev ; Paul Sandoz > > Subject: [External] : Re: Gatherer: spliterator characteristics are not > propagated >> From: "Viktor Klang" >> To: "Remi Forax" >> Cc: "core-libs-dev" , "Paul Sandoz" >> >> Sent: Monday, January 22, 2024 4:22:11 PM >> Subject: Re: Gatherer: spliterator characteristics are not propagated >> Hi Rémi, > Hello, >> For instance, stateless is neither recessive nor dominant, since the >> composition >> of two stateless operations is only ever stateless if they both are greedy as >> well: [ >> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java*L588__;Iw!!ACWV5N9M2RV99hQ!Lm52jd6kovd5t-cmrqqSLiRcIajBGXLxh85LO3eeiL6UxbKZuNPcUnO6z2i0FzMEoNr7U-cOBuWPCjo57FVW$ >> | >> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java#L588 >> ] > Okay, so choosing SEQUENTIAL vs PARALELLIZABLE is not that important given > that > the combination is ad-hoc, reflecting the characterristics is enough. >> So even if making it represented as ints (more like Spliterator, rather than >> Collector) makes things faster, it's still both work to track, propagate, and >> also becomes a side-channel that needs to remain in sync with the actual >> implementation of the logic. > The flags are in sync with the implementation because the only way to create a > Gatherer if through the factory methods and those factory methods (and only > them) compute the proper combination of SEQUENTIAL | STATELESS | GREEDY. A > user > should not be able to set those flags. Only the flags KEEP_* are settable by a > user. >> One could argue that logic such as: someCollection.stream().map(…).count() >> is a >> performance bug/inefficiency in an of itself as it would be faster to do >> someCollection.size(). > The stream implementation has a whole mechanism in place to propagate/preverse > flags like SIZED, DISTINCT or SORTED. For me, discussing about the merit of > this mechanism seems a little off topic. I would prefer the Gatherer to be a > good citizen and works seemlessly with the other intermediary operations. > Cheers, > √ > Viktor Klang > Software Architect, Java Platform Group > Oracle > From: fo...@univ-mlv.fr > Sent: Monday, 22 January 2024 19:56 > To: Viktor Klang > Cc: core-libs-dev ; Paul Sandoz > > Subject: [External] : Re: Gatherer: spliterator characteristics are not > propagated >> From: "Viktor Klang" >> To: "Remi Forax" >> Cc: "core-libs-dev" , "Paul Sandoz" >> >> Sent: Monday, January 22, 2024 4:22:11 PM >> Subject: Re: Gatherer: spliterator characteristics are not propagated >> Hi Rémi, > Hello, >> For instance, stateless is neither recessive nor dominant, since the >> composition >> of two stateless operations is only ever stateless if they both are greedy as >> well: [ >> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java*L588__;Iw!!ACWV5N9M2RV99hQ!Lm52jd6kovd5t-cmrqqSLiRcIajBGXLxh85LO3eeiL6UxbKZuNPcUnO6z2i0FzMEoNr7U-cOBuWPCjo57FVW$ >> | >> https:
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:21:47 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.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ident
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 32:
> 30: * Just-in-time-compiler-related queries
> 31: */
> 32: public class JitCompiler {
An alternative name and location is `jdk.internal.vm.ConstantSupport` with
initial class doc:
Defines methods to test if a value has been evaluated to a compile-time
constant value by the HotSpot VM.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463926393
Re: RFR: JDK-8321545: Override toString() for Format subclasses [v6]
> Please review this PR which implements toString() for the `Format`
> subclasses. Corresponding CSR:
> [JDK-8323088](https://bugs.openjdk.org/browse/JDK-8323088)
>
> The general specification follows a template that provides the locale (if the
> class is localized) and any relevant patterns. The specification was
> intentionally kept minimal and deliberately worded as "for debugging".
>
> An example of all the classes has output such as
>
>
> CompactNumberFormat [locale: "English (United States)", decimal pattern:
> "foo#0.00#baz", compact patterns: "[, , , {one:0K other:0K}, {one:00K
> other:00K}, {one:000K other:000K}, {one:0M other:0M}, {one:00M other:00M},
> {one:000M other:000M}, {one:0B other:0B}, {one:00B other:00B}, {one:000B
> other:000B}, {one:0T other:0T}, {one:00T other:00T}, {one:000T other:000T}]"]
>
> DecimalFormat [locale: "English (United States)", pattern: "foo#0.00#baz"]
>
> SimpleDateFormat [locale: "Chinese (China)", pattern: "EEE, MMM d, ''yy"]
>
> ListFormat [locale: "English (United States)", start: "{0}, {1}", middle:
> "{0}, {1}", end: "{0}, and {1}", two: "{0} and {1}", three: "{0}, {1}, and
> {2}"]
>
> MessageFormat [locale: "Chinese (China)", pattern: "foo {0}"]
>
> ChoiceFormat [pattern: "0#foo"]
Justin Lu 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 11 additional commits since the
last revision:
- reflect Rogers comments: style changes
- Merge branch 'master' into JDK-8321545-toString-j.text.Format
- remove quotes around locale when equal to null
- replace 'None' with 'null' for applicable classes
- Merge branch 'master' into JDK-8321545-toString-j.text.Format
- swap placement of decimal pattern and compact patterns. Expand on tests
- add unit tests
- Merge branch 'master' into JDK-8321545-toString-j.text.Format
- account for null locale for SDF through deserialization
- Merge branch 'master' into JDK-8321545-toString-j.text.Format
- ... and 1 more: https://git.openjdk.org/jdk/compare/9baaa352...d65cbcd1
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17355/files
- new: https://git.openjdk.org/jdk/pull/17355/files/70e0a175..d65cbcd1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17355&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17355&range=04-05
Stats: 14587 lines in 430 files changed: 7332 ins; 5481 del; 1774 mod
Patch: https://git.openjdk.org/jdk/pull/17355.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17355/head:pull/17355
PR: https://git.openjdk.org/jdk/pull/17355
Re: RFR: 8323832: Load JVMCI with the platform class loader
On Tue, 23 Jan 2024 17:00:20 GMT, xxDark wrote: > Passing `null` as parent class loader would suffice as boot loader just uses > `findBootstrapClassOrNull` in `JavaLangAccess` either way Thanks! I've simplified the test accordingly: 1642276ea22a5d789e01a5ecb1059d8c5c8be284 - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1906753878
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform > class loader instead of the boot class loader. This allows Native Image to > load a version of JVMCI different than the version on top of which Native > Image is running. This capability is demonstrated and tested by > `LoadAlternativeJVMCI.java`. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: use null to denote boot class loader as delegation parent - Changes: - all: https://git.openjdk.org/jdk/pull/17520/files - new: https://git.openjdk.org/jdk/pull/17520/files/e7d5801a..1642276e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17520&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17520&range=00-01 Stats: 8 lines in 1 file changed: 1 ins; 7 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17520.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17520/head:pull/17520 PR: https://git.openjdk.org/jdk/pull/17520
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v8]
> Currently String::translateEscapes does not support unicode escapes, reported
> as a IllegalArgumentException("Invalid escape sequence: ...").
> String::translateEscapes should translate unicode escape sequences to provide
> full coverage,
Jim Laskey has updated the pull request incrementally with one additional
commit since the last revision:
Requested changes
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17491/files
- new: https://git.openjdk.org/jdk/pull/17491/files/2eb5c194..eba05c2d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17491&range=07
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17491&range=06-07
Stats: 15 lines in 2 files changed: 10 ins; 0 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/17491.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17491/head:pull/17491
PR: https://git.openjdk.org/jdk/pull/17491
RFR: JDK-6503196: API doc for DecimalFormat::getMaximumIntegerDigits is unclear
Please review this PR which clarifies some confusion for the digit getter and setter methods of DecimalFormat. When formatting non `BigInteger` and `BigDecimal` values, DecimalFormat uses 309/340 as hard limits for integer and fraction digit limits, regardless of the value set by the user. There was some confusion, that those numbers might be returned by the getters, when in reality, they are only used internally. Moving the 309/340 wording to the class description and linking to it reduces the repetitive wording, but also eliminates the confusion that the getters may return those values. This should be OK, as setting limits higher than those values are likely rare, so the warning does not need to be in every method description. Additionally, `getMaximumIntegerDigits()` is updated to point to the patterns section to warn about the non-obvious rules for max integer digits. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/17541/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17541&range=00 Issue: https://bugs.openjdk.org/browse/JDK-6503196 Stats: 47 lines in 1 file changed: 17 ins; 22 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17541.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17541/head:pull/17541 PR: https://git.openjdk.org/jdk/pull/17541
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Tue, 23 Jan 2024 14:43:24 GMT, Chen Liang wrote: > Also curious, is there any documentation (like JVMS) that allows JVMs to make > no offset into byte arrays aligned for larger access? Would be helpful if we > can have a reference here. There is simply no guarantee in the JVMS that array elements are aligned more than their size. So, the public API may not assume that they are, since it needs to be implementable by an arbitrary JVM that is JVMS compliant. - PR Comment: https://git.openjdk.org/jdk/pull/16681#issuecomment-1906687425
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Tue, 23 Jan 2024 14:30:08 GMT, Chen Liang wrote: >> Good idea. Thanks > > Should we make these unaligned access modes throw ISE like before, when the > given index is unaligned? You mean `get` and `set`? They should never throw, as unaligned access is fine. For other access modes, we can never guarantee that an access is aligned, so UOE is appropriate. (IIRC this is mandated by existing spec. I'll try to find it again) - PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1463769755
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 16:01:05 GMT, Aleksey Shipilev wrote:
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
> A similar thing is already used in JDK:
> https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
>
> Extending this for more common use allows doing things like optimizing
> `Integer.toString(int)`:
>
>
> @Stable
> static final String[] CONST_STRINGS = {"-1", "0", "1"};
>
> @IntrinsicCandidate
> public static String toString(int i) {
> if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
> return CONST_STRINGS[i + 1];
> }
> ...
>
>
> Note how this code would fold away to one of the paths, depending on whether
> the compiler knows it is a constant or not. Generated-code-wise it is a
> zero-cost thing :)
> @shipilev Thanks a lot for the detailed reviews and suggestions, I hope I
> have addressed all of them.
Sure thing, I just effectively merged my draft implementation into yours :)
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906602556
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:21:47 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.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ident
A few more stylistic comments :)
Still thinking the better home for these might be just
`jdk.internal.misc.VM`... But I would not insist, if others are happy.
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
> 54: */
> 55: @IntrinsicCandidate
> 56: public static boolean isCompileConstant(boolean expr) {
Here and in other places: probably not `expr`, but just `val` or something?
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119:
> 117: * @see #isCompileConstant(boolean)
> 118: */
> 119: @IntrinsicCandidate
Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. These
methods should have `@Hidden` too then? Probably applies to other entries too.
-
PR Review: https://git.openjdk.org/jdk/pull/17527#pullrequestreview-1839475907
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463705907
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463703771
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 16:01:05 GMT, Aleksey Shipilev wrote:
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
> A similar thing is already used in JDK:
> https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
>
> Extending this for more common use allows doing things like optimizing
> `Integer.toString(int)`:
>
>
> @Stable
> static final String[] CONST_STRINGS = {"-1", "0", "1"};
>
> @IntrinsicCandidate
> public static String toString(int i) {
> if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
> return CONST_STRINGS[i + 1];
> }
> ...
>
>
> Note how this code would fold away to one of the paths, depending on whether
> the compiler knows it is a constant or not. Generated-code-wise it is a
> zero-cost thing :)
@shipilev Thanks a lot for the detailed reviews and suggestions, I hope I have
addressed all of them.
@kimbarrett TIL about that builtin, updated the PR description to mention that
instead. Thanks very much.
@AlanBateman Another potential usage I mentioned in the JBS issue is that
`GlobalSession` is noncloseable, but there is no way for the accessor to know
that without doing a checkcast. Using this we can eliminate the check if the
session is statically known to be a global session without imposing additional
checks on other kinds of memory sessions.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906549618
Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v3]
On Mon, 22 Jan 2024 22:10:44 GMT, Joshua Cao wrote:
>> test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 122:
>>
>>> 120: @Benchmark
>>> 121: public ConcurrentHashMap
>>> testConcurrentHashMapPutAll() {
>>> 122: ConcurrentHashMap map = new
>>> ConcurrentHashMap<>();
>>
>> I think this benchmark could be made more accurate by creating the new,
>> temporary map with the right initial size (i.e.
>> `ConcurrentHashMap<>(nkeys)`) to avoid calls to `tryPresize()` in this setup
>> step.
>
> I updated. The numbers are surprisingly the same. I guess the benchmark
> compute time is dominated by putAll().
OK, thanks nevertheless.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1463660578
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
> 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. Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision: ident - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/18f7d482..3ecb2c66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17527/head:pull/17527 PR: https://git.openjdk.org/jdk/pull/17527
Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v4]
On Mon, 22 Jan 2024 22:13:50 GMT, Joshua Cao wrote: >> ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` -> >> `transfer()`. When coming from the copy constructor, the Map is empty, so >> there is nothing to transfer. But `transfer()` will still copy all the empty >> nodes from the old table to the new table. >> >> This patch avoids this work by only calling `tryPresize()` if the table is >> already initialized. If `table` is null, the initialization is deferred to >> `putVal()`, which calls `initTable()`. >> >> --- >> >> ### JMH results for testCopyConstructor >> >> before patch: >> >> >> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": >> 937395.686 ±(99.9%) 99074.324 ns/op [Average] >> (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184 >> CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution) >> >> >> after patch: >> >> >> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": >> 620871.469 ±(99.9%) 59195.406 ns/op [Average] >> (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419 >> CI (99.9%): [561676.063, 680066.875] (assumes normal distribution) >> >> >> Average time is decreased by about 33%. >> >> ### JMH results for testPutAll (size = 1) >> >> before patch: >> >> >> Result >> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll": >> 4315291.542 ±(99.9%) 336034.190 ns/op [Average] >> (min, avg, max) = (3974688.194, 4315291.542, 4871772.209), stdev = >> 314326.589 >> CI (99.9%): [3979257.352, 4651325.731] (assumes normal distribution) >> >> >> after patch: >> >> >> Result >> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll": >> 3006955.723 ±(99.9%) 271757.969 ns/op [Average] >> (min, avg, max) = (2801264.198, 3006955.723, 3553084.135), stdev = >> 254202.573 >> CI (99.9%): [2735197.754, 3278713.692] (assumes normal distribution) >> >> >> Average time is decreased about 30%. > > Joshua Cao has updated the pull request incrementally with one additional > commit since the last revision: > > pass initial size to putAll benchmark map construction Looks good now. - Marked as reviewed by simonis (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1839420378
Re: RFR: 8323832: Load JVMCI with the platform class loader
On Mon, 22 Jan 2024 17:34:16 GMT, Doug Simon wrote: > This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform > class loader instead of the boot class loader. This allows Native Image to > load a version of JVMCI different than the version on top of which Native > Image is running. This capability is demonstrated and tested by > `LoadAlternativeJVMCI.java`. Hello. I'm not a reviewer but I read through the conversation in JIRA and saw this comment: > [~pwoegerer] currently has a Native Image patch where he creates a > URLClassLoader whose parent is jdk.internal.loader.ClassLoaders.BOOT_LOADER > (retrieved via reflection and use of required --add-exports and --add-opens > command line options). That is, he's using the non-delegating approach you > mention. There is zero reason to do this. Passing `null` as parent class loader would suffice as boot loader just uses `findBootstrapClassOrNull` in `JavaLangAccess` either way. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1906515587
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Thank you all!
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1906520760
Integrated: 8323515: Create test alias "all" for all test roots
On Mon, 15 Jan 2024 11:05:09 GMT, Aleksey Shipilev wrote:
> Since recent work to improve `tier4` performance, we actually test
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would
> be more convenient to just have the `all` test group/alias, so that we can do
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we
> do not wait for tests in each tier to complete before moving to next tier.
>
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
> environments one also needs to supply a few keywords like `!printer` to skip
> tests that cannot complete without failure due to misconfiguration. I left
> the keywords as is to show how would a failing run look. There is also an
> existing shortcut in build system that allows to run this with `make
> test-all`.
>
>
> % make test TEST=all
>
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
>
> (...about 6 hours later...)
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>>> jtreg:test/jdk:all 9962 995111 0 <<
>jtreg:test/langtools:all 4469 4469 0 0
>
>jtreg:test/jaxp:all 513 513 0 0
>
>jtreg:test/lib-test:all 3232 0 0
>
> ==
> TEST FAILURE
This pull request has now been integrated.
Changeset: 8b9bf758
Author:Aleksey Shipilev
URL:
https://git.openjdk.org/jdk/commit/8b9bf758801400e4491326cd4c90fc117b9d97e1
Stats: 49 lines in 5 files changed: 42 ins; 5 del; 2 mod
8323515: Create test alias "all" for all test roots
Reviewed-by: dholmes, alanb, joehw, lmesnik
-
PR: https://git.openjdk.org/jdk/pull/17422
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Marked as reviewed by lmesnik (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17422#pullrequestreview-1839361504
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 15:44:40 GMT, Aleksey Shipilev wrote: >> Quan Anh Mai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add more overloads > > src/hotspot/share/classfile/vmIntrinsics.hpp line 927: > >> 925: >> \ >> 926: do_class(jdk_internal_misc_JitCompiler, >> "jdk/internal/misc/JitCompiler") >>\ >> 927: do_intrinsic(_isConstantExpressionZ, >> jdk_internal_misc_JitCompiler,isConstantExpression_name, >> bool_bool_signature, F_S) \ > > It would be cleaner to follow the current naming for existing intrinsic: > > > do_intrinsic(_isCompileConstant, java_lang_invoke_MethodHandleImpl, > isCompileConstant_name, isCompileConstant_signature, F_S) \ >do_name( isCompileConstant_name, > "isCompileConstant") \ >do_alias(isCompileConstant_signature, > object_boolean_signature) \ > > > I.e. rename `isConstantExpression` -> `isCompileConstant`. It clearly > communicates that we are not dealing with expressions as arguments, and that > we underline this is the (JIT) _compile_ constant, not just a constant > expression from JLS 15.28 "Constant Expressions". > > Maybe even replace that `MHImpl` method with the new intrinsic. Yes you are right, I have renamed it to `isCompileConstant`. > src/hotspot/share/opto/c2compiler.cpp line 727: > >> 725: case vmIntrinsics::_storeStoreFence: >> 726: case vmIntrinsics::_fullFence: >> 727: case vmIntrinsics::_isConstantExpressionZ: > > Move this closer to `vmIntrinsics::_isCompileConstant:`, if not outright > replace it? I have replaced `MHImpl::isCompileConstant` with the new one. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463617016 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463616039
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v4]
> 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 inspired by `std::is_constant_evaluated` > in C++. > > Please kindly give your opinion as well as your reviews, thanks very much. Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision: address reviews: rename to isCompileConstant, remove duplication, revert unnecessary changes - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/31403d6f..18f7d482 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=02-03 Stats: 92 lines in 8 files changed: 10 ins; 13 del; 69 mod Patch: https://git.openjdk.org/jdk/pull/17527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17527/head:pull/17527 PR: https://git.openjdk.org/jdk/pull/17527
Re: RFR: 8323832: Load JVMCI with the platform class loader
On Mon, 22 Jan 2024 17:34:16 GMT, Doug Simon wrote:
> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform
> class loader instead of the boot class loader. This allows Native Image to
> load a version of JVMCI different than the version on top of which Native
> Image is running. This capability is demonstrated and tested by
> `LoadAlternativeJVMCI.java`.
src/java.base/share/lib/security/default.policy line 166:
> 164: };
> 165:
> 166: grant codeBase "jrt:/jdk.internal.vm.ci" {
This is required as JVMCI is no longer loaded by the boot loader but should
retain all permissions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1463601925
RFR: 8323832: Load JVMCI with the platform class loader
This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform class loader instead of the boot class loader. This allows Native Image to load a version of JVMCI different than the version on top of which Native Image is running. This capability is demonstrated and tested by `LoadAlternativeJVMCI.java`. - Commit messages: - load JVMCI with platform class loader Changes: https://git.openjdk.org/jdk/pull/17520/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17520&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323832 Stats: 227 lines in 8 files changed: 219 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/17520.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17520/head:pull/17520 PR: https://git.openjdk.org/jdk/pull/17520
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 15:52:29 GMT, Alan Bateman wrote:
> Would it be possible to list further examples where this might be used?
> Asking because I'm wondering about the usability and maintainability of
> if-then-else code.
A similar thing is already used in JDK:
https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
Extending this for more common use allows doing things like optimizing
`Integer.toString(int)`:
@Stable
static final String[] CONST_STRINGS = {"-1", "0", "1"};
@IntrinsicCandidate
public static String toString(int i) {
if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
return CONST_STRINGS[i + 1];
}
...
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906379544
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > add more overloads Would it be possible to list further examples where this might be used? Asking because I'm wondering about the usability and maintainability of if-then-else code. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906362127
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > add more overloads All right, this is very close :) I now have stylistic comments: src/hotspot/share/classfile/vmIntrinsics.hpp line 912: > 910: do_intrinsic(_getAndSetInt, jdk_internal_misc_Unsafe, > getAndSetInt_name, getAndSetInt_signature, F_R) \ > 911:do_name( getAndSetInt_name, > "getAndSetInt") \ > 912:do_alias(getAndSetInt_signature, > /*"(Ljava/lang/Object;JI)I"*/ getAndAddInt_signature) \ I don't think we need to do these formatting changes in this PR. src/hotspot/share/classfile/vmIntrinsics.hpp line 927: > 925: > \ > 926: do_class(jdk_internal_misc_JitCompiler, > "jdk/internal/misc/JitCompiler") > \ > 927: do_intrinsic(_isConstantExpressionZ, > jdk_internal_misc_JitCompiler,isConstantExpression_name, bool_bool_signature, > F_S) \ It would be cleaner to follow the current naming for existing intrinsic: do_intrinsic(_isCompileConstant, java_lang_invoke_MethodHandleImpl, isCompileConstant_name, isCompileConstant_signature, F_S) \ do_name( isCompileConstant_name, "isCompileConstant") \ do_alias(isCompileConstant_signature, object_boolean_signature) \ I.e. rename `isConstantExpression` -> `isCompileConstant`. It clearly communicates that we are not dealing with expressions as arguments, and that we underline this is the (JIT) _compile_ constant, not just a constant expression from JLS 15.28 "Constant Expressions". Maybe even replace that `MHImpl` method with the new intrinsic. src/hotspot/share/opto/c2compiler.cpp line 727: > 725: case vmIntrinsics::_storeStoreFence: > 726: case vmIntrinsics::_fullFence: > 727: case vmIntrinsics::_isConstantExpressionZ: Move this closer to `vmIntrinsics::_isCompileConstant:`, if not outright replace it? src/hotspot/share/opto/library_call.hpp line 2: > 1: /* > 2: * Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights > reserved. Unnecessary update? - PR Review: https://git.openjdk.org/jdk/pull/17527#pullrequestreview-1839148507 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463490470 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463493124 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463497227 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463497518
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v15]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Rename singleHop field. - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/9635ba52..1e69bc9f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=13-14 Stats: 9 lines in 2 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
All right, thanks! @lmesnik, I realized I forgot to ask if you had objections
to this.
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1906338893
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > add more overloads Not a review, just a drive-by comment. >From the description for this PR: "This is inspired by >std::is_constant_evaluated in C++." I think what is being proposed here is more like gcc's `__builtin_constant_p`. std::is_constant_evaluated is a different thing, used to detect evaluation in a manifestly constexpr context. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906337427
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Tue, 23 Jan 2024 14:42:20 GMT, Roger Riggs wrote: > Is there any place to document the new keyword or its usage; it does not seem > very discoverable just existing in the TEST.ROOT and some tests. I don't think there is a place to describe keywords, except in the relevant `TEST.ROOT`-s. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1906324934
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v6]
On Wed, 10 Jan 2024 22:12:40 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:
>
> Tweak Reference.enqueue memory consistency effects wording
src/java.base/share/classes/java/lang/ref/Reference.java line 508:
> 506: * Use of this method allows the registered queue's
> 507: * {@link ReferenceQueue#poll} and {@link ReferenceQueue#remove}
> methods
> 508: * to return this reference even though the referent is still
> strongly
Perhaps s/is still/may still be/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1463145471
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v9]
On Tue, 23 Jan 2024 11:56:58 GMT, Jatin Bhateja wrote: >> Hi, >> >> Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 >> only targets. >> Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 >> instruction set. >> These are very frequently used APIs in columnar database filter operation. >> >> Implementation uses a lookup table to record permute indices. Table index is >> computed using >> mask argument of compress/expand operation. >> >> Following are the performance number of JMH micro included with the patch. >> >> >> System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids) >> >> Baseline: >> Benchmark (size) Mode CntScore >> Error Units >> ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 142.767 >> ops/ms >> ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 71.436 >> ops/ms >> ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 35.992 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 182.151 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 91.096 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 44.757 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 184.099 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 2047 thrpt2 91.981 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 4096 thrpt2 45.170 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 1024 thrpt2 148.017 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 2047 thrpt2 73.516 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 4096 thrpt2 36.844 >> ops/ms >> >> Withopt: >> Benchmark (size) Mode Cnt Score >> Error Units >> ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 2051.707 >>ops/ms >> ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 914.072 >>ops/ms >> ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 489.898 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 5324.195 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 2587.229 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 1278.665 >>ops/ms >> ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 4149.384 >>ops/ms >> ColumnFilterBenchmark.filterIntColumn 2047 thrpt ... > > Jatin Bhateja 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 10 additional > commits since the last revision: > > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8322768 > - Modifying comments. > - Review comments resolution > - Modified code comment for clarity. > - Space fixup > - Using emulated variable blend E-Core optimized instruction. > - Review suggestions incorporated. > - Review comments resolutions. > - Updating copyright year of modified files. > - 8322768: Optimize non-subword vector compress and expand APIs for AVX2 > target. Ok, I'll just run the testing again, and then I will approve this :) - PR Review: https://git.openjdk.org/jdk/pull/17261#pullrequestreview-1839057311
Re: RFR: JDK-8321545: Override toString() for Format subclasses [v5]
On Fri, 12 Jan 2024 22:41:46 GMT, Justin Lu wrote:
>> Please review this PR which implements toString() for the `Format`
>> subclasses. Corresponding CSR:
>> [JDK-8323088](https://bugs.openjdk.org/browse/JDK-8323088)
>>
>> The general specification follows a template that provides the locale (if
>> the class is localized) and any relevant patterns. The specification was
>> intentionally kept minimal and deliberately worded as "for debugging".
>>
>> An example of all the classes has output such as
>>
>>
>> CompactNumberFormat [locale: "English (United States)", decimal pattern:
>> "foo#0.00#baz", compact patterns: "[, , , {one:0K other:0K}, {one:00K
>> other:00K}, {one:000K other:000K}, {one:0M other:0M}, {one:00M other:00M},
>> {one:000M other:000M}, {one:0B other:0B}, {one:00B other:00B}, {one:000B
>> other:000B}, {one:0T other:0T}, {one:00T other:00T}, {one:000T other:000T}]"]
>>
>> DecimalFormat [locale: "English (United States)", pattern: "foo#0.00#baz"]
>>
>> SimpleDateFormat [locale: "Chinese (China)", pattern: "EEE, MMM d, ''yy"]
>>
>> ListFormat [locale: "English (United States)", start: "{0}, {1}", middle:
>> "{0}, {1}", end: "{0}, and {1}", two: "{0} and {1}", three: "{0}, {1}, and
>> {2}"]
>>
>> MessageFormat [locale: "Chinese (China)", pattern: "foo {0}"]
>>
>> ChoiceFormat [pattern: "0#foo"]
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove quotes around locale when equal to null
Marked as reviewed by rriggs (Reviewer).
src/java.base/share/classes/java/text/MessageFormat.java line 1195:
> 1193: """
> 1194: MessageFormat [locale: %s, pattern: "%s"]
> 1195: """.formatted(locale == null ? null :
> '"'+locale.getDisplayName()+'"', toPattern());
Style wise, operators (like "+") should have spaces around them.
src/java.base/share/classes/java/text/SimpleDateFormat.java line 2433:
> 2431: """
> 2432: SimpleDateFormat [locale: %s, pattern: "%s"]
> 2433: """.formatted(locale == null ? null :
> '"'+locale.getDisplayName()+'"', toPattern());
Ditto, add spaces around +.
test/jdk/java/text/Format/DateFormat/ToStringTest.java line 59:
> 57: public void oddValueTest() {
> 58: String expectedStr =
> 59: "SimpleDateFormat [locale:
> \""+Locale.getDefault().getDisplayName()+"\", pattern: \"MMM d, y\"]\n";
Add spaces around operator +
-
PR Review: https://git.openjdk.org/jdk/pull/17355#pullrequestreview-1839039270
PR Review Comment: https://git.openjdk.org/jdk/pull/17355#discussion_r1463432450
PR Review Comment: https://git.openjdk.org/jdk/pull/17355#discussion_r1463433138
PR Review Comment: https://git.openjdk.org/jdk/pull/17355#discussion_r1463434281
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Thu, 16 Nov 2023 18:10:28 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4518:
>>
>>> 4516: * Only plain {@linkplain VarHandle.AccessMode#GET get} and
>>> {@linkplain VarHandle.AccessMode#SET set}
>>> 4517: * access modes are supported by the returned var handle. For all
>>> other access modes, an
>>> 4518: * {@link UnsupportedOperationException} will be thrown.
>>
>> I recommend adding an api note explaining that native memory segments,
>> direct byte buffers, or heap memory segments backed by long[] should be used
>> if support for other access modes are required.
>
> Good idea. Thanks
Should we make these unaligned access modes throw ISE like before, when the
given index is unaligned?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1463367572
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v13]
On Thu, 18 Jan 2024 21:35:06 GMT, Mandy Chung wrote:
> > > If I read the code correctly, the image created with this option will
> > > enable multi-hops unconditionally? i.e. no timestamp file created and
> > > disable the check completely. I think the .stamp file should be present
> > > in any image created without packaged modules.
OK, done in the recent version.
> What I tried to point out is the following:
>
> 1. run `myruntime/bin/jlink` to create `image1` without packaged module
> 2. run `image1/bin/jlink` to create a runtime image will fail
> 3. run `image1/bin/jlink --ignore-modified-runtime` will successfully to
> create `image2`
>
> I expect `image2/bin/jlink` should fail creating a runtime image since it's
> multi-hop. Another scenario: If I modify `image2/conf/net.properties` ,
> `image2/bin/jlink` should fail. In both cases, `image2/bin/jlink
> --ignore-modified-runtime` will ignore the error and create the runtime image.
This should now be fixed. The stamp file indicating a runtime image created
image is now always added.
$ rm -rf ./build/jdk.jlink-runtime &&
./build/linux-x86_64-server-release/images/jdk/bin/jlink --output
./build/jdk.jlink-runtime --add-modules jdk.jlink
$ rm -rf ./build/image1/ && ./build/jdk.jlink-runtime/bin/jlink --add-modules
jdk.jlink --output ./build/image1/ --verbose
Linking based on the current run-time image.
'./build/image1' is linked equivalent to using jlink with JDK packaged modules
as:
jlink --add-modules jdk.jlink --output ./build/image1
java.base jrt:/java.base (run-time image)
java.compiler jrt:/java.compiler (run-time image)
jdk.compiler jrt:/jdk.compiler (run-time image)
jdk.internal.opt jrt:/jdk.internal.opt (run-time image)
jdk.jdeps jrt:/jdk.jdeps (run-time image)
jdk.jlink jrt:/jdk.jlink (run-time image)
jdk.zipfs jrt:/jdk.zipfs (run-time image)
Providers:
jdk.compiler provides com.sun.tools.javac.platform.PlatformProvider used by
jdk.compiler
java.base provides java.nio.file.spi.FileSystemProvider used by java.base
jdk.zipfs provides java.nio.file.spi.FileSystemProvider used by java.base
java.base provides java.util.random.RandomGenerator used by java.base
jdk.compiler provides java.util.spi.ToolProvider used by java.base
jdk.jdeps provides java.util.spi.ToolProvider used by java.base
jdk.jlink provides java.util.spi.ToolProvider used by java.base
jdk.compiler provides javax.tools.JavaCompiler used by java.compiler
jdk.compiler provides javax.tools.Tool not used by any observable module
jdk.jlink provides jdk.tools.jlink.plugin.Plugin used by jdk.jlink
$ rm -rf ./build/image2/ && ./build/image1/bin/jlink --add-modules jdk.jlink
--output ./build/image2/
Error: Module path to the JDK packaged modules must be specified. Run-time
image based linking is not supported as $java.home was already created from a
run-time image.
$ rm -rf ./build/image2/ && ./build/image1/bin/jlink --ignore-modified-runtime
--add-modules jdk.jlink --output ./build/image2/
Error: Module path to the JDK packaged modules must be specified. Run-time
image based linking is not supported as $java.home was already created from a
run-time image.
> `JRTArchive::collectFiles` is the relevant code:
>
>```
>// add/persist a special, empty file for jdk.jlink so as to support
>// the single-hop-only run-time image jlink
>if (singleHop && JDK_JLINK_MODULE.equals(module)) {
>files.add(createRuntimeImageSingleHopStamp());
>}
>```
This is now:
if (JDK_JLINK_MODULE.equals(module)) {
files.add(createRuntimeImageSingleHopStamp());
}
> My intent is to bring up my observation for discussion. The plugin authors
> are the ones who should decide if the plugin should restore the data from the
> runtime image to original form. It needs a way to indicate that the
> transformed data won't persist for this plugin. So far I think FILTER
> category may be adequate but it may be better to define a new type. This
> requires some prototype to determine what is the right way.
In the current patch, there is `boolean Plugin::runTimeImageLinkPersistent()`
which is exactly what you describe here. The default implementation returns
true for `FILTER` or `TRANSFORMER` categories, but this could be explicit for
`--vendor-version` et. al. plugins. Unless I misunderstood. I.e. if the
`transform()` method is persistent for runtime image based links then it must
return `true`.
> I also agree with Alan that --verbose output is a bit confusing (my initial
> feedback). The image
has already been created. The users can't tell which plugins are implicitly
run. To list the plugin options, it has to delete the already-created image
and re-do the jlink command with --verbose.
This has been addressed in the most recent version by this output when
performing a run-time image based link:
$ rm -rf ./build/image1/ && ./build/jdk.jlink-runtime/bin/jlink --add-modules
jdk.jlink
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Wed, 15 Nov 2023 22:46:03 GMT, Jorn Vernee wrote: > See the JBS issue for an extended problem description. > > This patch changes the specification and implementation of > `MethodHandles::byteArrayViewVarHandle`, > `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and > `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the > alignment of Java array elements, in order to bring them in line with the > guarantees made by an arbitrary JVM implementation (which makes no guarantees > about array element alignment beyond them being aligned to their natural > alignment). > > - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment > for the accesses. Which means we can only reliably support plain get and set > access modes. The javadoc text explaining which other access modes are > supported, or how to compute aligned offsets into the array is dropped, as it > is not guaranteed to be correct on all JVM implementations. The > implementation of the returned VarHandle is changed to throw an > `UnsupportedOperationException` for the unsupported access modes, as mandated > by the spec of `VarHandle` [1]. > > - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is > incorrect when accessing a heap buffer (wrapping a byte[]), for the same > reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when > accessing a _heap buffer_, only plain get and set access modes are supported. > The implementation of the returned var handle is changed to throw an > `IllegalStateException` when an access is attempted on a heap buffer using an > access mode other than plain get or set. Note that we don't throw an outright > `UnsupportedOperationException` for this case, since whether the access modes > are supported depends on the byte buffer instance being used. > > - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former > method depends directly on the latter for all its alignment computations. We > change the implementation of the latter method to throw an > `UnsupportedOperationException` for all unit sizes greater than 1, when the > buffer is non-direct. This change is largely covered by the existing > specification: > > > * @throws UnsupportedOperationException > * If the native platform does not guarantee stable alignment > offset > * values for the given unit size when managing the memory regions > * of buffers of the same kind as this buffer (direct or > * non-direct). For example, if garbage collection would result > * in the mo... Also curious, is there any documentation (like JVMS) that allows JVMs to make no offset into byte arrays aligned for larger access? Would be helpful if we can have a reference here. - PR Comment: https://git.openjdk.org/jdk/pull/16681#issuecomment-1906200196
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev wrote: > Some jtreg tests require resolvable external dependencies. This resolution is > delegated to JIB, which is not used in vanilla OpenJDK testing. It would be > convenient to add a keyword that marks tests that require these external > dependencies, so that we could exclude those tests from runs. This would > allow us to: a) run all tests in hotspot:tier4, which now excludes > `applications/` specifically; b) make all tests runs (#17422) cleaner on many > environments. > > I provisionally call this flag `external-dep`, but I am open for other > suggestions. > > Note that some tests that pull `@Artifact`-s provide special paths that do > limited testing anyway. However, there are tests which cannot run without > external dependencies at all. These include at least `applications/jcstress` > and `applications/scimark` tests. > > Ironically, I cannot run the jcstress test generator because the dependencies > are lacking here. I regenerated those test using a self-built jcstress 0.16 > bundle. > > Additional testing: > - [x] `make test TEST=applications/` fails > - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, > skipping most of the tests Is there any place to document the new keyword or its usage; it does not seem very discoverable just existing in the TEST.ROOT and some tests. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1906198126
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v14]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 55 commits: - Add some details for run-time link Also clean up the --verbose output. - Always add the runtime image stamp file - Merge branch 'master' into jdk-8311302-jmodless-link - Rename '--unlock-run-image' to '--ignore-modified-runtime' - Remove --unlock-run-image from jlink.properties - Fix IntegrationTest (no runtime image link) - The constants such as the pathname of the timestamp file and the internal file listing per-module non-class non-resource files are part of jlink. Move the constants to JlinkTask to follow where OPTIONS_RESOURCE is defined. JRTArchive scans the class and resource files of a given module from the runtime image. It should also read `fs_$MODULE_files` to find the list of non-class and non-resource files. The current implementation checks if a file is modified lazily when Entry::stream is called and so it has to remember if file modification has been checked and warning has been emitted. I think doing the file modification check eagerly when collectFiles is called would simplify the code. For maintainability, better to move the reading of and writing to `fs_$MODULE_files` together in a single class rather than separated in JRTArchive and JlinkResourcesListPlugin. - Disallow packaged modules and run-time image link - Only check for existing path when not a scratch task When using a run-time image link and the initial build was produced with the --keep-packaged-modules option, we don't need to check existing paths to the location where packaged modules need to be copied. This breaks the --verbose output validation. - Add @enablePreview for JImageValidator that uses classfile API - ... and 45 more: https://git.openjdk.org/jdk/compare/c9cacfb2...9635ba52 - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14787&range=13 Stats: 3211 lines in 39 files changed: 3041 ins; 132 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Integrated: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed
On Mon, 22 Jan 2024 09:31:43 GMT, sendaoYan wrote: > 8323640: [TESTBUG]testMemoryFailCount in > jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because > OOM killed This pull request has now been integrated. Changeset: 791b427f Author:sendaoYan Committer: Severin Gehwolf URL: https://git.openjdk.org/jdk/commit/791b427f4410057cdcdf8fd8ea0dcce71f7dc513 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed Reviewed-by: sgehwolf - PR: https://git.openjdk.org/jdk/pull/17514
Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v3]
On Tue, 23 Jan 2024 13:04:43 GMT, sendaoYan wrote: >> 8323640: [TESTBUG]testMemoryFailCount in >> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail >> because OOM killed > > sendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > 8323640: [TESTBUG]testMemoryFailCount in > jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because > OOM killed > > Signed-off-by: sendaoYan Marked as reviewed by sgehwolf (Reviewer). Please enable GHA for your fork for future PRs. - PR Review: https://git.openjdk.org/jdk/pull/17514#pullrequestreview-1838761515 PR Comment: https://git.openjdk.org/jdk/pull/17514#issuecomment-1906034584
Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v3]
On Tue, 23 Jan 2024 13:04:43 GMT, sendaoYan wrote: >> 8323640: [TESTBUG]testMemoryFailCount in >> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail >> because OOM killed > > sendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > 8323640: [TESTBUG]testMemoryFailCount in > jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because > OOM killed > > Signed-off-by: sendaoYan > GHA > Please enable GHA for your fork for future PRs. OK - PR Comment: https://git.openjdk.org/jdk/pull/17514#issuecomment-1906038625
Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v3]
> 8323640: [TESTBUG]testMemoryFailCount in > jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because > OOM killed sendaoYan has updated the pull request incrementally with one additional commit since the last revision: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed Signed-off-by: sendaoYan - Changes: - all: https://git.openjdk.org/jdk/pull/17514/files - new: https://git.openjdk.org/jdk/pull/17514/files/969b608d..d1eb4fac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17514&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17514&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17514.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17514/head:pull/17514 PR: https://git.openjdk.org/jdk/pull/17514
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v8]
On Tue, 23 Jan 2024 08:17:13 GMT, Emanuel Peter wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review comments resolution > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5301: > >> 5299: vmovmskps(rtmp, mask, vec_enc); >> 5300: } >> 5301: shlq(rtmp, 5); // for 32 byte permute row of 8 x 32 bits. > > Suggestion: > > shlq(rtmp, 5); // for 32 byte permute row of 8 x 32 bits / 4 x 64 bits. > > Since you now merged the code of the two paths As per the latest patch, we are doing a double word permute, hence semantically its ok and in accordance with instruction sequence :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1463160336
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v9]
> Hi, > > Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 only > targets. > Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 > instruction set. > These are very frequently used APIs in columnar database filter operation. > > Implementation uses a lookup table to record permute indices. Table index is > computed using > mask argument of compress/expand operation. > > Following are the performance number of JMH micro included with the patch. > > > System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids) > > Baseline: > Benchmark (size) Mode CntScore Error > Units > ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 142.767 > ops/ms > ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 71.436 > ops/ms > ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 35.992 > ops/ms > ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 182.151 > ops/ms > ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 91.096 > ops/ms > ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 44.757 > ops/ms > ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 184.099 > ops/ms > ColumnFilterBenchmark.filterIntColumn 2047 thrpt2 91.981 > ops/ms > ColumnFilterBenchmark.filterIntColumn 4096 thrpt2 45.170 > ops/ms > ColumnFilterBenchmark.filterLongColumn 1024 thrpt2 148.017 > ops/ms > ColumnFilterBenchmark.filterLongColumn 2047 thrpt2 73.516 > ops/ms > ColumnFilterBenchmark.filterLongColumn 4096 thrpt2 36.844 > ops/ms > > Withopt: > Benchmark (size) Mode Cnt Score > Error Units > ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 2051.707 > ops/ms > ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 914.072 > ops/ms > ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 489.898 > ops/ms > ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 5324.195 > ops/ms > ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 2587.229 > ops/ms > ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 1278.665 > ops/ms > ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 4149.384 > ops/ms > ColumnFilterBenchmark.filterIntColumn 2047 thrpt2 1791.170 > ops/ms > ColumnFilterBenchmark.filterIntColumn 4096... Jatin Bhateja 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 10 additional commits since the last revision: - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8322768 - Modifying comments. - Review comments resolution - Modified code comment for clarity. - Space fixup - Using emulated variable blend E-Core optimized instruction. - Review suggestions incorporated. - Review comments resolutions. - Updating copyright year of modified files. - 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. - Changes: - all: https://git.openjdk.org/jdk/pull/17261/files - new: https://git.openjdk.org/jdk/pull/17261/files/cd912308..83e4065e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17261&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17261&range=07-08 Stats: 41105 lines in 1072 files changed: 24738 ins; 11390 del; 4977 mod Patch: https://git.openjdk.org/jdk/pull/17261.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17261/head:pull/17261 PR: https://git.openjdk.org/jdk/pull/17261
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
I have nothing against it, but I don't think I know enough about details of
jtreg configuration to provide approval. OTOH, I personally don't see a strong
need for it.
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1905864727
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > add more overloads I get your idea. I have added overloads for all types. They will all invoke `inlint_isCompileConstant`. Given that there are now 7 methods I think a separate class is more justified. Another issue is the duplication of `isConstantExpression(Object)`, but I think a separate issue to deduplicate it would be easier. Thanks a lot. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905836929
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
> 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 inspired by `std::is_constant_evaluated` > in C++. > > Please kindly give your opinion as well as your reviews, thanks very much. Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision: add more overloads - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/9dd95393..31403d6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=01-02 Stats: 346 lines in 6 files changed: 333 ins; 1 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/17527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17527/head:pull/17527 PR: https://git.openjdk.org/jdk/pull/17527
[jdk22] RFR: 8319128: sun/security/pkcs11 tests fail on OL 7.9 aarch64
I backport this to fix this issue in 22. We see it failing there in our CI. - Commit messages: - Backport c2e77e2f17b624e750dea8fd51bbfde99596690e Changes: https://git.openjdk.org/jdk22/pull/95/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=95&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8319128 Stats: 45 lines in 6 files changed: 36 ins; 4 del; 5 mod Patch: https://git.openjdk.org/jdk22/pull/95.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/95/head:pull/95 PR: https://git.openjdk.org/jdk22/pull/95
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Tue, 23 Jan 2024 01:32:45 GMT, David Holmes wrote: > Seems quite reasonable. Thanks! I shall wait for more reviewers, in case someone has an issue with `external-dep` as the flag name. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1905719123
Withdrawn: 8318966: Some methods make promises about Java array element alignment that are too strong
On Wed, 15 Nov 2023 22:46:03 GMT, Jorn Vernee wrote: > See the JBS issue for an extended problem description. > > This patch changes the specification and implementation of > `MethodHandles::byteArrayViewVarHandle`, > `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and > `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the > alignment of Java array elements, in order to bring them in line with the > guarantees made by an arbitrary JVM implementation (which makes no guarantees > about array element alignment beyond them being aligned to their natural > alignment). > > - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment > for the accesses. Which means we can only reliably support plain get and set > access modes. The javadoc text explaining which other access modes are > supported, or how to compute aligned offsets into the array is dropped, as it > is not guaranteed to be correct on all JVM implementations. The > implementation of the returned VarHandle is changed to throw an > `UnsupportedOperationException` for the unsupported access modes, as mandated > by the spec of `VarHandle` [1]. > > - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is > incorrect when accessing a heap buffer (wrapping a byte[]), for the same > reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when > accessing a _heap buffer_, only plain get and set access modes are supported. > The implementation of the returned var handle is changed to throw an > `IllegalStateException` when an access is attempted on a heap buffer using an > access mode other than plain get or set. Note that we don't throw an outright > `UnsupportedOperationException` for this case, since whether the access modes > are supported depends on the byte buffer instance being used. > > - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former > method depends directly on the latter for all its alignment computations. We > change the implementation of the latter method to throw an > `UnsupportedOperationException` for all unit sizes greater than 1, when the > buffer is non-direct. This change is largely covered by the existing > specification: > > > * @throws UnsupportedOperationException > * If the native platform does not guarantee stable alignment > offset > * values for the given unit size when managing the memory regions > * of buffers of the same kind as this buffer (direct or > * non-direct). For example, if garbage collection would result > * in the mo... This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16681
Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v2]
On Tue, 23 Jan 2024 01:58:40 GMT, sendaoYan wrote: >> 8323640: [TESTBUG]testMemoryFailCount in >> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail >> because OOM killed > > sendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > 8323640: [TESTBUG]testMemoryFailCount in > jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because > OOM killed > > Signed-off-by: sendaoYan Looks fine now. test/jdk/jdk/internal/platform/docker/MetricsMemoryTester.java line 75: > 73: long count = Metrics.systemMetrics().getMemoryFailCount(); > 74: > 75: // Allocate 512M of data Suggestion: Amend the comment to `Allocate 512M of data in 1M chunks per iteration` - Marked as reviewed by sgehwolf (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17514#pullrequestreview-1838306348 PR Review Comment: https://git.openjdk.org/jdk/pull/17514#discussion_r1462991886
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 09:31:51 GMT, Quan Anh Mai wrote: > Maybe I am ignorant but doesn't the definition of an intrinsics contain the > signature of the method as well? The definitions in `vmIntrinsics`, sure, they require full signature for `@IntrinsicCandidate` methods. It would yield some unfortunate duplication. But after that, we can map on the same `inline_isCompileConstant` intrinsic that just asks `arg(0)->is_Con()`, and it would not care about the type of the constant. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905667006
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev 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 inspired by
>> `std::is_constant_evaluated` in C++.
>>
>> Please kindly give your opinion as well as your reviews, thanks very much.
>
> Nice. I had a similar thing stashed in my todo queue. Note that there is
> already `isCompileConstant` that does similar thing:
> https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
> -- maybe we should just expose that more widely. I would suggest we just do
> the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
> to that intrinsic.
> @shipilev Thanks a lot for your suggestions. Yes I can just use
> `inline_isCompileConstant` instead.
>
> Regarding the place of the method, I'm not really sure as putting in
> `java.lang.Long` seems out-of-place for an internal mechanism that is
> obviously not only used in `java.lang`, which will force a new entry in
> `JavaLangAccess`.
Ah yes, if you need to use it across module boundaries, putting the
private/protected method would require `JavaLangAccess`, which is burdensome. I
am just icky about introducing a whole new internal class for this. Is there
anything in current `jdk.internal.vm.*` that fits it? Maybe `misc.Unsafe` or
`misc.VM`?
> Finally, I think accepting a `long` would be enough (maybe `double`, too?)
> since `int`, `boolean` etc can be converted losslessly to `long`.
Right, that would work for primitives, since we could probably rely on
conversion for constants to be folded. But I also see the value for asking
`isCompileConstant(Object)`, which is not easily convertible. So I would just
do the overloads for all primitives and `Object`. The C2 intrinsic would not
care about the `arg(0)` type, it would reply `isCon` on those constants just
the same.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905655407
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev wrote:
> I would suggest we just do the private
> `java.lang.{Integer,...}.isCompileConstant` methods and bind them to that
> intrinsic.
Maybe I am ignorant but doesn't the definition of an intrinsics contain the
signature of the method as well?
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905648723
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev 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 inspired by
>> `std::is_constant_evaluated` in C++.
>>
>> Please kindly give your opinion as well as your reviews, thanks very much.
>
> Nice. I had a similar thing stashed in my todo queue. Note that there is
> already `isCompileConstant` that does similar thing:
> https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
> -- maybe we should just expose that more widely. I would suggest we just do
> the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
> to that intrinsic.
@shipilev Thanks a lot for your suggestions. Yes I can just use
`inline_isCompileConstant` instead.
Regarding the place of the method, I'm not really sure as putting in
`java.lang.Long` seems out-of-place for an internal mechanism that is obviously
not only used in `java.lang`, which will force a new entry in `JavaLangAccess`.
Finally, I think accepting a `long` would be enough (maybe `double`, too?)
since `int`, `boolean` etc can be converted losslessly to `long`.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905641141
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v2]
> 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 inspired by `std::is_constant_evaluated` > in C++. > > Please kindly give your opinion as well as your reviews, thanks very much. Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision: use inline_isCompileConstant - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/4d0fc3dd..9dd95393 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=00-01 Stats: 13 lines in 3 files changed: 0 ins; 9 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17527/head:pull/17527 PR: https://git.openjdk.org/jdk/pull/17527
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v8]
On Sat, 20 Jan 2024 09:55:45 GMT, Jatin Bhateja wrote: >> Hi, >> >> Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 >> only targets. >> Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 >> instruction set. >> These are very frequently used APIs in columnar database filter operation. >> >> Implementation uses a lookup table to record permute indices. Table index is >> computed using >> mask argument of compress/expand operation. >> >> Following are the performance number of JMH micro included with the patch. >> >> >> System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids) >> >> Baseline: >> Benchmark (size) Mode CntScore >> Error Units >> ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 142.767 >> ops/ms >> ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 71.436 >> ops/ms >> ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 35.992 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 182.151 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 91.096 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 44.757 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 184.099 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 2047 thrpt2 91.981 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 4096 thrpt2 45.170 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 1024 thrpt2 148.017 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 2047 thrpt2 73.516 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 4096 thrpt2 36.844 >> ops/ms >> >> Withopt: >> Benchmark (size) Mode Cnt Score >> Error Units >> ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 2051.707 >>ops/ms >> ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 914.072 >>ops/ms >> ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 489.898 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 5324.195 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 2587.229 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 1278.665 >>ops/ms >> ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 4149.384 >>ops/ms >> ColumnFilterBenchmark.filterIntColumn 2047 thrpt ... > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments resolution Looks good, except for one detail ;) src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5301: > 5299: vmovmskps(rtmp, mask, vec_enc); > 5300: } > 5301: shlq(rtmp, 5); // for 32 byte permute row of 8 x 32 bits. Suggestion: shlq(rtmp, 5); // for 32 byte permute row of 8 x 32 bits / 4 x 64 bits. Since you now merged the code of the two paths - Changes requested by epeter (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17261#pullrequestreview-1838095271 PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1462873120
Re: RFR: 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 inspired by `std::is_constant_evaluated`
> in C++.
>
> Please kindly give your opinion as well as your reviews, thanks very much.
Nice. I had a similar thing stashed in my todo queue. Note that there is
already `isCompileConstant` that does similar thing:
https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
-- maybe we should just expose that more widely. I would suggest we just do
the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
to that intrinsic.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905504206
RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
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 inspired by `std::is_constant_evaluated` in C++. Please kindly give your opinion as well as your reviews, thanks very much. - Commit messages: - bug number - add isConstantExpression Changes: https://git.openjdk.org/jdk/pull/17527/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17527&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324433 Stats: 162 lines in 6 files changed: 158 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17527/head:pull/17527 PR: https://git.openjdk.org/jdk/pull/17527
