Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 19:46:18 GMT, Pavel Rappo wrote: >> Please review this PR which introduces the `java.io.IO` top-level class and >> three methods to `java.io.Console` for [Implicitly Declared Classes and >> Instance Main Methods (Third Preview)]. >> >> This PR has been obtained as `git merge --squash` of a now obsolete [draft >> PR]. >> >> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: >> https://bugs.openjdk.org/browse/JDK-8323335 >> [draft PR]: https://github.com/openjdk/jdk/pull/18921 > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Strengthen tests after 8330998 > > https://github.com/openjdk/jdk/pull/18996 now allows us to test > Console IO better. src/java.base/share/classes/java/io/Console.java line 151: > 149: /** > 150: * Writes a string representation of the specified object to this > console's > 151: * output stream, terminates the line and then flushes the console. Should this specify if the line termination will be platform dependent character(s) or independent of the platform? - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593420018
Re: RFR: 8305457: Implement java.io.IO [v3]
On Wed, 8 May 2024 05:45:55 GMT, Jaikiran Pai wrote: >> Pavel Rappo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Strengthen tests after 8330998 >> >> https://github.com/openjdk/jdk/pull/18996 now allows us to test >> Console IO better. > > src/java.base/share/classes/java/io/Console.java line 194: > >> 192: * A prompt string. >> 193: * >> 194: * @throws IOError > > I'm guessing this specifies `IOError` instead of `IOException` so that the > caller doesn't have to handle a checked exception? If so, would it be better > to throw an `java.io.UncheckedIOException`, to avoid throwing `Error`s? Actually, it appears that the existing APIs on `java.io.Console` are specified to throw a `java.io.IOError`. So this isn't something new that is being introduced. So please ignore that question. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593416464
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 19:46:18 GMT, Pavel Rappo wrote: >> Please review this PR which introduces the `java.io.IO` top-level class and >> three methods to `java.io.Console` for [Implicitly Declared Classes and >> Instance Main Methods (Third Preview)]. >> >> This PR has been obtained as `git merge --squash` of a now obsolete [draft >> PR]. >> >> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: >> https://bugs.openjdk.org/browse/JDK-8323335 >> [draft PR]: https://github.com/openjdk/jdk/pull/18921 > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Strengthen tests after 8330998 > > https://github.com/openjdk/jdk/pull/18996 now allows us to test > Console IO better. src/java.base/share/classes/java/io/Console.java line 194: > 192: * A prompt string. > 193: * > 194: * @throws IOError I'm guessing this specifies `IOError` instead of `IOException` so that the caller doesn't have to handle a checked exception? If so, would it be better to throw an `java.io.UncheckedIOException`, to avoid throwing `Error`s? - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593414616
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 19:46:18 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Strengthen tests after 8330998
>
> https://github.com/openjdk/jdk/pull/18996 now allows us to test
> Console IO better.
src/java.base/share/classes/java/io/Console.java line 188:
> 186:
> 187: /**
> 188: * Writes a prompt as if by calling {@code print}, then reads a
> single line
Should `{@code print}` instead be `{@link Console#print() print()}`?
src/java.base/share/classes/java/io/Console.java line 192:
> 190: *
> 191: * @param prompt
> 192: * A prompt string.
Hello Pavel, should this specify whether `prompt` can be null?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593411945
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593411393
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v5]
> Can I please get a review for this patch which proposes to implement the > enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970? > > The commit in this PR introduces the `-o` and `--output-dir` option to the > `jar` command. The option takes a path to a destination directory as a value > and extracts the contents of the jar into that directory. This is an optional > option and the changes in the commit continue to maintain backward > compatibility where the jar is extracted into current directory, if no `-o` > or `--output-dir` option has been specified. > > As far as I know, there hasn't been any discussion on what the name of this > new option should be. I was initially thinking of using `-d` but that is > currently used by the `jar` command for a different purpose. So I decided to > use `-o` and `--output-dir`. This is of course open for change depending on > any suggestions in this PR. > > The commit in this PR also updates the `jar.properties` file which contains > the English version of the jar command's `--help` output. However, no changes > have been done to the internationalization files corresponding to this one > (for example: `jar_de.properties`), because I don't know what process needs > to be followed to have those files updated (if at all they need to be > updated). > > The commit also includes a jtreg testcase which verifies the usage of this > new option. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - cleanup testExtractNoDestDirWithPFlag() test - merge latest from master branch - merge latest from master branch - convert the new test to junit - merge latest from master branch - Lance's review - include tests for --extract long form option - cleanup after each test - Adjust test for new error messages - Lance's review - add a code comment for xdestDir - Lance's review - updates to the help messages in jar.properties - ... and 5 more: https://git.openjdk.org/jdk/compare/3b8227ba...46fb5a8e - Changes: https://git.openjdk.org/jdk/pull/2752/files Webrev: https://webrevs.openjdk.org/?repo=jdk=2752=04 Stats: 569 lines in 4 files changed: 558 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/2752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752 PR: https://git.openjdk.org/jdk/pull/2752
Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]
On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti wrote: >> Move all random generators mandated in package `java.util.random` and >> currently implemented in module `jdk.random` to module `java.base`, and >> remove module `jdk.random`. > > Raffaello Giulietti has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge branch 'master' into 8330005 > - Restrict RandomGenerator service providers to those loadable by the > platform class loader. > - Typo. > - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base. > - Terminology changes. > - Renamed package jdk.random to jdk.internal.random. > - 8330005: RandomGeneratorFactory.getDefault() throws exception when the > runtime image only has java.base module Hello Raffaello, the proposed changes look OK to me. Do you think we should add a jtreg test for this? In general, the test coverage for these APIs appears to be missing. I think that can be addressed separately in some of the upcoming changes. - PR Review: https://git.openjdk.org/jdk/pull/18932#pullrequestreview-2044656083
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v9]
On Wed, 8 May 2024 02:56:10 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 49 commits: > > - centos7 compat > - 64a5feb6: > - fixes > - e514824f: > - ebb459e9: > - Merge branch 'jerboaarefactor-merge' into jerboaarefactor-merge-cgroup > - Merge branch 'jerboaarefactor' into jerboaarefactor-merge > - Merge remote-tracking branch 'origin/master' into jerboaarefactor > - Merge remote-tracking branch 'origin/master' into jerboaarefactor-merge > - Merge branch 'master-cgroup' into jerboaarefactor-merge-cgroup > - ... and 39 more: https://git.openjdk.org/jdk/compare/9347bb7d...3da3a9e5 Your patch https://github.com/jerboaa/jdk/commit/92aaa6fd7e3ff8b64de064fecfcd725a157cb5bb#diff-1c49a6b40a810aef82b7da9bfea8f03e07a43062977ba65f75df63c4b7c5b149R89 contains a tab. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2099650738
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v10]
> The testcase requires root permissions. > > Designed by Severin Gehwolf, implemented by Jan Kratochvil. Jan Kratochvil has updated the pull request incrementally with one additional commit since the last revision: whitespace fix - Changes: - all: https://git.openjdk.org/jdk/pull/17198/files - new: https://git.openjdk.org/jdk/pull/17198/files/3da3a9e5..efb83999 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17198=09 - incr: https://webrevs.openjdk.org/?repo=jdk=17198=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17198/head:pull/17198 PR: https://git.openjdk.org/jdk/pull/17198
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v9]
> The testcase requires root permissions. > > Designed by Severin Gehwolf, implemented by Jan Kratochvil. Jan Kratochvil has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 49 commits: - centos7 compat - 64a5feb6: - fixes - e514824f: - ebb459e9: - Merge branch 'jerboaarefactor-merge' into jerboaarefactor-merge-cgroup - Merge branch 'jerboaarefactor' into jerboaarefactor-merge - Merge remote-tracking branch 'origin/master' into jerboaarefactor - Merge remote-tracking branch 'origin/master' into jerboaarefactor-merge - Merge branch 'master-cgroup' into jerboaarefactor-merge-cgroup - ... and 39 more: https://git.openjdk.org/jdk/compare/9347bb7d...3da3a9e5 - Changes: https://git.openjdk.org/jdk/pull/17198/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17198=08 Stats: 2254 lines in 20 files changed: 1078 ins; 810 del; 366 mod Patch: https://git.openjdk.org/jdk/pull/17198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17198/head:pull/17198 PR: https://git.openjdk.org/jdk/pull/17198
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 22:12:55 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/io/IO.java line 37:
>>
>>> 35: * is {@code null}; otherwise, the effect is as if a similarly-named
>>> method
>>> 36: * had been called on that console.
>>> 37: *
>>
>> Add a note here on encoding (character set), something like
>>
>>
>> Output from methods in this class uses the character set of the system
>> console as specified by {@link Console#charset}.
>
> Seems redundant since we express `IO` methods in therms of those of `Console`
> in the specification, no?
It's strictly redundant in the sense that, if one reads all the specifications
and applies reasoning using the right facts, one could already reach that
conclusion. However, I think it's a useful clarification, because if you're
looking at `java.io.IO` and you ask what charset it uses, you might not know
that you need to look at the `Console.charset` method. You might go looking on
Console to find the answer, and you might or might not find that method.
(Arguably Console's specs should be improved too since the charset is a global
property of the Console instance and this should be mentioned in the class
specs.)
Anyway Joe asked about Locales in the comments on the CSR
[JDK-8331610](https://bugs.openjdk.org/browse/JDK-8331610) and while Locale
isn't relevant, charset is, so it seems reasonable to mention it explicitly
here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593257656
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]
On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons wrote:
>> Re-write the IndexOf code without the use of the pcmpestri instruction, only
>> using AVX2 instructions. This change accelerates String.IndexOf on average
>> 1.3x for AVX2. The benchmark numbers:
>>
>>
>> Benchmark Score
>> Latest
>> StringIndexOf.advancedWithMediumSub 343.573317.934
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081 1053.96
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541
>> 1.980027943x
>> StringIndexOf.constantPattern9.361 11.906
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess 4.216 4.218
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133 3.216
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761
>> 1.000265957x
>> StringIndexOf.success9.186
>> 9.713 1.057369911x
>> StringIndexOf.successBig 14.34146.343
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String 7132.541
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char 16013.389 16162.437
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String 7386.12314771.622
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671 9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rearrange; add lambdas for clarity
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1082:
> 1080: // noMatch - label bound outside to jump to if there is no match
> 1081: // haystack - the address of the first byte of the haystack
> 1082: // hsLen - the sizeof the haystack
Good to specify if the size (size of needle) and hsLen (size of haystack) is
in bytes or elements.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1149:
> 1147:
> 1148: if (size == (isU ? 2 : 1)) {
> 1149: __ vpmovmskb(eq_mask, cmp_0, Assembler::AVX_256bit);
vpmovmskb is being done twice if doEarlyBailout is set to 1 (the setting we
have currently).
If it helps to simplify, we could assume that doEarlyBailout is always set to 1
and remove this configurability.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1174:
> 1172: #define lastMask rTmp
> 1173: __ vpmovmskb(lastMask, cmp_k, Assembler::AVX_256bit);
> 1174: __ shrq(lastMask);
did you mean to shift the lastMask by shiftVal here?
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1185:
> 1183: if (size > (isU ? 4 : 2)) {
> 1184: if (doEarlyBailout) {
> 1185: __ testl(eq_mask, eq_mask);
The masks are 32 bit as we are comparing max 32 byes (256 bits) at a time. So
we could consistently do either andl, testl, shrl or andq, testq, shrq.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1593225178
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1593225488
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1593227487
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1593229554
RFR: 8322732: ForkJoinPool may underutilize cores in async mode
This set of changes address causes of poor utilization with small numbers of cores due to overly aggressive contention avoidance. A number of further adjustments were needed to still avoid most contention effects in deployments with large numbers of cores - Commit messages: - Merge branch 'openjdk:master' into JDK-8322732 - Repack some fields; adjust control flow - Merge branch 'openjdk:master' into JDK-8322732 - Next version - Merge branch 'openjdk:master' into JDK-8322732 - Reduce unneeded signals - Merge branch 'openjdk:master' into JDK-8322732 - reduce memory contention - Merge branch 'openjdk:master' into JDK-8322732 - re-integrate docs - ... and 23 more: https://git.openjdk.org/jdk/compare/f12ed061...7e64cdfc Changes: https://git.openjdk.org/jdk/pull/19131/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19131=00 Issue: https://bugs.openjdk.org/browse/JDK-8322732 Stats: 593 lines in 1 file changed: 162 ins; 159 del; 272 mod Patch: https://git.openjdk.org/jdk/pull/19131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131 PR: https://git.openjdk.org/jdk/pull/19131
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Tue, 7 May 2024 11:53:19 GMT, Pavel Rappo wrote: > Please review this mechanical change to man pages. This PR should be > integrated after https://github.com/openjdk/jdk/pull/18787. Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19119#pullrequestreview-2044314419
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 21:19:59 GMT, Stuart Marks wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Strengthen tests after 8330998
>>
>> https://github.com/openjdk/jdk/pull/18996 now allows us to test
>> Console IO better.
>
> src/java.base/share/classes/java/io/IO.java line 37:
>
>> 35: * is {@code null}; otherwise, the effect is as if a similarly-named
>> method
>> 36: * had been called on that console.
>> 37: *
>
> Add a note here on encoding (character set), something like
>
>
> Output from methods in this class uses the character set of the system
> console as specified by {@link Console#charset}.
Seems redundant since we express `IO` methods in therms of those of `Console`
in the specification, no?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593161920
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Tue, 7 May 2024 11:53:19 GMT, Pavel Rappo wrote: > Please review this mechanical change to man pages. This PR should be > integrated after https://github.com/openjdk/jdk/pull/18787. Thanks for reviewing it Joe, I'm now delegating integration of this PR to @JesperIRL, you, or anyone who will be integrating https://github.com/openjdk/jdk/pull/18787. - PR Comment: https://git.openjdk.org/jdk/pull/19119#issuecomment-2099395990
Enumerated streams
I have created some drafts of the EnumeratedStream API. But firstly, I have
noticed that we have not formalized problems we are solving with them.
Moreover, during the implementation process, I have discovered that there
are possibly a much wider range of applications for those streams. Instead
of just enumerating streams, those streams could be adapted to supply any
kind of metadata along with the object itself, which would provide much
more opportunities and EnumeratedStreams could be just one of possible
implementations of this API. So let's point out problems solved by the new
API.
For enumerated streams specifically:
1. Data-oriented design. Data oriented design is a programming paradigm
that is applied when a system operates on large amounts of data. In this
paradigm objects are basically being split into "dimensions", which are
basically 1d collections of values List of collections of values together
represent a list of objects. You can think of this as a table where columns
are 1d collections (dimensions), and rows are objects. For this approach
stream enumeration is crucial to be able to access value from another
dimension "on the same row". It could seem that data-oriented programming
is fairly rare to be applied, but, actually, this is the main approach to
work with large amounts of data. For example, pandas in python, to the best
of my knowledge, implement a data-oriented approach.
2. Index lookup operations. While I still stand my ground that
List.findIndex is a preferred approach, enumerated streams could also be
used for things like "find all matching indexes in list".
3. Working with distances and hotspots. I am not really familiar
with this, but David Alayachew in a related thread has explained it like so
(quoting below):
2. Building jumps/skips -- Fetching the indexes at hotspots, and then using
them to create a skip-list-esque structure between the hotspots. Very
useful for realigning search strategies.
3. Calculating distance using index -- This is a sister concept to bullet
1. Let's say I built my skip list above, and have determined that my
desired target is not in the hotspots. I can make a better decision to go
parallel or sequential by seeing the distance between hotspots. That
distance would be taken by subtracting hotspot index by hotspot index.\
End of quote.
I am sure there is more to it, but for now, let's get over this section.
As for metadata-supplying streams in general:
1. Replacing visitor pattern, Not necessarily streams, but fluent API in
general are widely used to provide a better alternative for Visitor
pattern. Such metadata could include current path, node position etc.
2. Any complex custom data processing pipeline. Metadata could represent
some data that is evaluated based on a processed object. Such processing
pipelines could extend the hypothetical "MetadataPipeline", implement a
method that provides metadata based on an object that is processed and
enjoy all benefits of fluent data processing zipped with metadata for it.
3. Virtually any kind of metadata-aware operations. Timestamps, geospatial
data, tagging/classifying, possible stateful operations on data, source or
origin info, quality?confidence scores etc. I think there is application
for this in virtually every field.
Now that we described problems being solved. Let's move on to
implementation details. There were discussions on a gatherer-based vs
new-pipeline-type-based approach. I have implemented 6 operations (map,
flatMap, filter, peek, takeWhile, dropWhile) both ways and here are some of
my thoughts.
1. Complexity of implementation. Gatherers, without a doubt, were much
easier to implement. For now class-based streams don't even implement
parallel evaluation and it will be really hard to implement parallel
evaluation in a way that enumerates elements in order of their occurence,
2. Performance. Both approaches demonstrate fairly similar performance.
Gatherers outperform for 10-20% with findAny() terminal operation, while
for toList() terminal operation class-based approach is usually 10-20%
faster. Im sure my code could use A LOT of optimization, but this could
give initial understanding of performance of both approaches. As for
memory usage, the gatherer-based approach usually consumes up to 2 times
more memory. This is just initial lightweight benchmark results, more
comprehensive results with visualization will come in a few days after more
complex benchmarking completes.
3. Preserving indexes. Expectedly, gatherers do not preserve indexes for
further operations. Imo, this is what kills any benefits of this approach.
This just removes a major part of potential use cases. Things like "find
all matching indexes are still possible, but in an undesired (as I think)
way. Consider following:
With stream-based approach:
list,stream().filter((i, x) -> ...).map((i, _) -> i)
With gatherer-based:
list.stream().gather(Gathrers.mapMultiEnumerated((sink, i, x) -> { if
(predicate.test(i, x)
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 19:46:18 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Strengthen tests after 8330998
>
> https://github.com/openjdk/jdk/pull/18996 now allows us to test
> Console IO better.
src/java.base/share/classes/java/io/IO.java line 37:
> 35: * is {@code null}; otherwise, the effect is as if a similarly-named
> method
> 36: * had been called on that console.
> 37: *
Add a note here on encoding (character set), something like
Output from methods in this class uses the character set of the system console
as specified by {@link Console#charset}.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593109243
RFR: 8331876: JFR: Move file read and write events to java.base
Hi, Could I have a review of a change that moves the jdk.FileRead and jdk.FileWrite events to java.base to remove the use of the ASM instrumentation. Testing: jdk/jdk/jfr Thanks Erik - Commit messages: - Update comment - Initial Changes: https://git.openjdk.org/jdk/pull/19129/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19129=00 Issue: https://bugs.openjdk.org/browse/JDK-8331876 Stats: 1241 lines in 19 files changed: 567 ins; 651 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/19129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19129/head:pull/19129 PR: https://git.openjdk.org/jdk/pull/19129
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]
On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons wrote:
>> Re-write the IndexOf code without the use of the pcmpestri instruction, only
>> using AVX2 instructions. This change accelerates String.IndexOf on average
>> 1.3x for AVX2. The benchmark numbers:
>>
>>
>> Benchmark Score
>> Latest
>> StringIndexOf.advancedWithMediumSub 343.573317.934
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081 1053.96
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541
>> 1.980027943x
>> StringIndexOf.constantPattern9.361 11.906
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess 4.216 4.218
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133 3.216
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761
>> 1.000265957x
>> StringIndexOf.success9.186
>> 9.713 1.057369911x
>> StringIndexOf.successBig 14.34146.343
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String 7132.541
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char 16013.389 16162.437
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String 7386.12314771.622
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671 9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rearrange; add lambdas for clarity
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 576:
> 574: broadcast_additional_needles(false, 0 /* unknown */,
> NUMBER_OF_NEEDLE_BYTES_TO_COMPARE, needle, needleLen, rTmp3,
> 575:isUU, isUL, _masm);
> 576:
Good to pass output xmm registers to this method.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 587:
> 585: // firstNeedleCompare has address of second element of needle
> 586: // compLen has length of comparison to do
> 587:
This is not clear. firstNeedleCompare gets needle +
NUMBER_OF_NEEDLE_BYTES_TO_COMPARE - 1 which is not necessarily the second
element of needle. If it helps let us fix the NUMBER_OF_NEEDLE_BYTES_TO_COMPARE
to 3 and have comments and code versus that only.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 590:
> 588: compare_haystack_to_needle(false, 0,
> NUMBER_OF_NEEDLE_BYTES_TO_COMPARE, L_returnRBP, haystack, isU,
> 589: DO_EARLY_BAILOUT, mask, needleLen,
> rTmp3, _masm);
> 590:
It is better to pass the broadcasted xmm registers to
compare_haystack_to_nedle. Basically pass input, output, and temps to all the
methods.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 639:
> 637: __ movl(rax, r8);
> 638: __ subq(rcx, rbx);
> 639: __ addq(rcx, rax);
This could be:
__ subq(rcx, rbx);
__ addq(rcx, r8);
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 647:
> 645: __ cmpq(r11, r10);
> 646: __ movq(rbp, -1);
> 647: __ cmovq(Assembler::belowEqual, rbp, r11);
This could be directly computed in rax:
__ movq(rax, -1);
__ cmovq(Assembler::belowEqual, rax, r11);
Also is it possible to not do cmov on some paths? It is an expensive operation.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1010:
> 1008: static void broadcast_additional_needles(bool sizeKnown, int size, int
> bytesToCompare, Register needle,
> 1009: Register needleLen, Register
> rTmp, bool isUU, bool isUL,
> 1010: MacroAssembler *_masm) {
Good to add output XMM registers to the parameter list.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1040:
> 1038: __ vpbroadcastb(byte_1, Address(needle, 1),
> Assembler::AVX_256bit);
> 1039: }
> 1040: }
It will be good to have a function which broadcasts a needle element from a
given offset into a vector register.
That function could take (needle address, offset, outout vector register,
temps).
Such a function could then be called twice from here and from main function for
offset 0.
-
PR Review Comment:
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 02:54:39 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/java/io/IO.java line 41:
>>
>>> 39: */
>>> 40: @PreviewFeature(feature = PreviewFeature.Feature.IMPLICIT_CLASSES)
>>> 41: public class IO {
>>
>> Should this be final?
>
>> Should this be final?
>
> With only the private constructor, it doesn't matter too much in practice,
> but an explicit `final` would be good documentation if that is the intent.
If the sole constructor of a class is private, not only does it preclude the
class from being instantiated, but it also precludes the class from being
extended. Mind you, even with the sole private constructor, both instantiation
and extension are still possible from inside the class or its nested classes.
As long as we don't leak such instances to API clients, they won't seem to be
able to observe any difference between "the private constructor" and "the
private constructor of a final class".
I think that just having that constructor private but the class "non-final"
makes bigger bang for the buck. We can defer the decision to make the class
final.
If this helps, here are a few well-known similar classes:
- java.util.Collections
- java.util.concurrent.Executors
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593033564
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 16:09:08 GMT, Pavel Rappo wrote: >> I do not think the step to "standardise" a preview feature exists ? When a >> preview feature becomes a released feature, the code is very lightly edited, >> at least it this is my experience. >> >> You can change both readln and readLine and if `java.io.IO` is removed, at >> least the code of readLine() will be > >> I do not think the step to "standardise" a preview feature exists ? When a >> preview feature becomes a released feature, the code is very lightly edited, >> at least it this is my experience. > > We may call it differently, but I think both you and I are referring to this > part of [JEP 12](https://openjdk.org/jeps/12) (emphasis mine): > >> Eventually, the JEP owner must decide the preview feature's fate. If the >> decision is to remove the preview feature, then the owner must file an issue >> in JBS to remove the feature in the next JDK feature release; no new JEP is >> needed. **On the other hand, if the decision is to finalize, then the owner >> must file a new JEP, noting refinements informed by developer feedback. The >> title of this JEP should be the feature's name, omitting the earlier suffix >> of (Preview) / (Second Preview), and without adding any new suffix such as >> (Standard) or (Final). This JEP will ultimately reach Targeted status for >> the next JDK feature release.** > >> You can change both readln and readLine and if `java.io.IO` is removed, at >> least the code of readLine() will be > > Sorry, Rémi, but no. As long as this feature is in preview, I'd optimise for > easier removal (back out) of the feature rather than clean combined code. Okay - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593013670
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Tue, 7 May 2024 11:53:19 GMT, Pavel Rappo wrote: > Please review this mechanical change to man pages. This PR should be > integrated after https://github.com/openjdk/jdk/pull/18787. Marked as reviewed by darcy (Reviewer). > This PR is standalone as opposed to dependent because the #18787 dependency > currently has a merge conflict, which complicates the required workflow. > > This comment serves as a reminder to merge master into this PR once #18787 > has been integrated. If we don't do that, Skara will likely create a backport > issue: > > > (⚠️ The fixVersion in this issue is [24] but the fixVersion in .jcheck/conf > > is 23, a new backport will be created when this pr is integrated.) Thanks @pavelrappo. For as a small risk-reduction exercise, the fixVersion on the bug could also be changed to tbd, but if it is integrated after the main start-of-release PR it should be fine. - PR Review: https://git.openjdk.org/jdk/pull/19119#pullrequestreview-2044060259 PR Comment: https://git.openjdk.org/jdk/pull/19119#issuecomment-2099210018
Re: RFR: 8305457: Implement java.io.IO [v3]
On Tue, 7 May 2024 17:37:57 GMT, Pavel Rappo wrote:
> Yes, we do. There's a common misconception that `{@inheritDoc}` inherits the
> complete doc comment. In reality, `{@inheritDoc}` inherits only the main
> description, which does not include any `@throws` tags.
>
> A `@throws` tag is either inherited explicitly, such as in L107, or
> implicitly. Implicit inheritance occurs when an exception is listed in the
> `throws` clause.
>
> Since it's uncommon for unchecked exceptions (errors included) to be listed
> in the `throws` clause, unless inherited explicitly, their documentation will
> be missing from the overriding method documentation. Assuming, of course,
> that your intention is to have them there.
While it may be surprising that `{@inheritDoc}` doesn't inherit the complete
doc, it is a feature rather than a bug since an overridden method is allowed to
throw fewer exceptions than the method it overrides.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592985138
Re: RFR: 8305457: Implement java.io.IO [v3]
> Please review this PR which introduces the `java.io.IO` top-level class and > three methods to `java.io.Console` for [Implicitly Declared Classes and > Instance Main Methods (Third Preview)]. > > This PR has been obtained as `git merge --squash` of a now obsolete [draft > PR]. > > [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: > https://bugs.openjdk.org/browse/JDK-8323335 > [draft PR]: https://github.com/openjdk/jdk/pull/18921 Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Strengthen tests after 8330998 https://github.com/openjdk/jdk/pull/18996 now allows us to test Console IO better. - Changes: - all: https://git.openjdk.org/jdk/pull/19112/files - new: https://git.openjdk.org/jdk/pull/19112/files/60050834..73a20a7c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19112=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19112=01-02 Stats: 6 lines in 1 file changed: 2 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112 PR: https://git.openjdk.org/jdk/pull/19112
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Mon, 6 May 2024 19:06:10 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing indentation > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Just noting, I don't think the GHA failures are related to my patch. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2099126506
Integrated: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
On Tue, 7 May 2024 14:58:00 GMT, Viktor Klang wrote: > Removes SORTED if not also ORDERED for escape-hatch `Stream::spliterator()` This pull request has now been integrated. Changeset: f12ed061 Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/f12ed061ae3fa9d5620a7c6c7ea441f9f33bb745 Stats: 28 lines in 2 files changed: 26 ins; 0 del; 2 mod 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator Reviewed-by: psandoz, alanb - PR: https://git.openjdk.org/jdk/pull/19123
Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v4]
On Thu, 2 May 2024 10:30:06 GMT, Adam Sotona wrote:
>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with
>> additional class checks inspired by
>> `hotspot/share/classfile/classFileParser.cpp`.
>>
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 28 commits:
>
> - Merge branch 'master' into JDK-8320396-verifier-extension
> - added references to jvms
> - Merge remote-tracking branch 'openjdk/master' into
> JDK-8320396-verifier-extension
> - work in progress
> - work in progress
> - work in progress
> - work in progress
> - work in progress
> - removed string templates from test
> - work in progress
> - ... and 18 more: https://git.openjdk.org/jdk/compare/ae82405f...3ebc780a
src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
line 97:
> 95: check.accept(fre.owner()::asSymbol);
> 96: check.accept(fre::typeSymbol);
> 97: yield () -> verifyFieldName(fre.name().stringValue());
Nitpick, we should avoid capturing the check instance and just do something
like:
case FieldRefEntry fre -> () -> {
fre.owner().asSymbol();
fre.typeSymbol();
verifyFieldName(fre.name().stringValue());
};
src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
line 151:
> 149: var fields = new HashSet();
> 150: for (var f : classModel.fields()) try {
> 151: if (!fields.add(f.fieldName().stringValue() +
> f.fieldType().stringValue())) {
We should declare a local record, concat is not safe if we have fields like:
Loop foo;
oop fooL;
both will producce `fooLLoop;`
src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
line 163:
> 161: var methods = new HashSet();
> 162: for (var m : classModel.methods()) try {
> 163: if (!methods.add(m.methodName().stringValue() +
> m.methodType().stringValue())) {
This one is safe as `(` is safe, but still preferable to use a local record as
key
src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
line 167:
> 165: }
> 166: if (m.methodName().equalsString(CLASS_INIT_NAME)
> 167: && !m.flags().has(AccessFlag.STATIC)) {
Do we verify it has void return and (since class file version JAVA 7) takes no
args? The static requirement is since JAVA 7 too.
src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
line 364:
> 362: className(),
> 363: m.methodName().stringValue(),
> 364:
> m.methodTypeSymbol().parameterList().stream().map(ClassDesc::displayName).collect(Collectors.joining(",")));
Suggestion:
m.methodTypeSymbol().displayDescriptor();
Can remove the parentheses above.
src/java.base/share/classes/jdk/internal/classfile/impl/verifier/VerificationWrapper.java
line 134:
> 132: }
> 133:
> 134: String parameters() {
We should just use the mtd's displayDescriptor, a class file can have bridge
methods with covariant return types and the bridge may be broken while the
concrete method is ok.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592914387
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592917226
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592919450
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592922781
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592925732
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1592926751
Re: RFR: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
On Tue, 7 May 2024 14:58:00 GMT, Viktor Klang wrote: > Removes SORTED if not also ORDERED for escape-hatch `Stream::spliterator()` Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19123#pullrequestreview-2043916265
Re: RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts
On Tue, 7 May 2024 15:42:23 GMT, Maurizio Cimadamore wrote: > This PR fixes an issue that has crept into the FFM API implementation. > > From very early stages, the FFM API used to disable the alignment check on > nested layout elements, in favor of an alignment check against the memory > segment base address. The rationale was that the JIT had issue with > eliminating redundant alignment checks, and accessing nested elements could > never result in alignment issues, since the alignment of the container is > provably bigger than that of the contained element. This means that, when > creating a var handle for a nested layout element, we set the nested layout > alignment to 1 (unaligned), derive its var handle, and then decorate the var > handle with an alignment check for the container. > > At some point in 22, we tweaked the API to throw > `UnsupportedOperationException` when using an access mode incompatible with > the alignment constraint of the accessed layout. That is, a volatile read on > an `int` is only possible if the access occurs at an address that is at least > 4-byte aligned. Otherwise an `UOE` is thrown. > > Unfortunately this change broke the aforementioned optimization: creating a > var handle for an unaligned layout works, but the resulting layout will *not* > support any of the atomic access modes. > > Since this optimization is not really required anymore (proper C2 support to > hoist/eliminate alignment checks has been added since then), it is better to > disable this implementation quirk, and leave optimizations to C2. > > (If we really really wanted to optimize things a bit, we could remove the > container alignment check in the case the accessed value is the first layout > element nested in the container, but this PR doesn't go that far). > > I've run relevant benchmarks before/after and found no differences. In part > this is because `arrayElementVarHandle` is unaffected. But, even after > tweaking the `LoopOverNonConstant` benchmark to add explicit tests for the > code path affected, no significant difference was found, sign that C2 is > indeed able to spot (and remove) the redundant alignment check. Note: if we > know that `aligned_to_N(base)` holds, then it's easy to prove that > `aligned_to_M(base + offset + index * scale)` holds, when `N >= M` and with > `offset` and `scale` known (the latter a power of two). Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19124#pullrequestreview-2043909050
Re: RFR: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
On Tue, 7 May 2024 14:58:00 GMT, Viktor Klang wrote: > Removes SORTED if not also ORDERED for escape-hatch `Stream::spliterator()` Marked as reviewed by psandoz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19123#pullrequestreview-2043897612
Re: RFR: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
On Tue, 7 May 2024 14:58:00 GMT, Viktor Klang wrote: > Removes SORTED if not also ORDERED for escape-hatch `Stream::spliterator()` Tagging @PaulSandoz for review :) - PR Comment: https://git.openjdk.org/jdk/pull/19123#issuecomment-2099048586
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v5]
On Tue, 7 May 2024 07:46:22 GMT, Justin Lu wrote:
>> Please review this PR which corrects an edge case bug for
>> java.text.DecimalFormat that causes incorrect parsing results for strings
>> with very large exponent values.
>>
>> When parsing values with large exponents, if the value of the exponent
>> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value
>> of the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
>> mantissa. Both results are confusing and incorrect.
>>
>> For example,
>>
>>
>> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
>> fmt.parse(".1E2147483648"); // returns 0.0
>> fmt.parse(".1E9223372036854775808"); // returns 0.1
>> // For comparison
>> Double.parseDouble(".1E2147483648"); // returns Infinity
>> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>>
>>
>> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> correct behavior when underflow from adjustment + use MIN
Implementation looks good to me now.
Thanks again for taking care of this.
-
Marked as reviewed by ahauschu...@github.com (no known OpenJDK username).
PR Review: https://git.openjdk.org/jdk/pull/19075#pullrequestreview-2043881285
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v19]
On Sat, 4 May 2024 19:35:21 GMT, Scott Gibbons wrote:
>> Re-write the IndexOf code without the use of the pcmpestri instruction, only
>> using AVX2 instructions. This change accelerates String.IndexOf on average
>> 1.3x for AVX2. The benchmark numbers:
>>
>>
>> Benchmark Score
>> Latest
>> StringIndexOf.advancedWithMediumSub 343.573317.934
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081 1053.96
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541
>> 1.980027943x
>> StringIndexOf.constantPattern9.361 11.906
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess 4.216 4.218
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133 3.216
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761
>> 1.000265957x
>> StringIndexOf.success9.186
>> 9.713 1.057369911x
>> StringIndexOf.successBig 14.34146.343
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String 7132.541
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char 16013.389 16162.437
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String 7386.12314771.622
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671 9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rearrange; add lambdas for clarity
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 383:
> 381: {
> 382: Label L_short;
> 383:
A comment here:
// Broadcast the beginning of needle into a vector register.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 390:
> 388: __ vpbroadcastb(byte_0, Address(needle, 0),
> Assembler::AVX_256bit);
> 389: }
> 390:
A comment here:
// Broadcast the end of needle into a vector register. This step is not needed
for single element needle.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 418:
> 416: __ cmpq(haystack_len, 0x10);
> 417: __ ja_b(L_moreThan16);
> 418:
An assert here to check for header size >= 16 would be good.
Also a comment here would he good, something like:
// Copy 16 or 32 bytes prior to haystack end onto stack
// This will possibly including some object header bytes when haystack length
is less than 16 or 32 bytes // Set the new haystack address to beginning of
copied haystack on stack adjusting for extra bytes copied
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 498:
> 496:
> 497: // big_case_loop_helper will fall through to this point if one or
> more potential matches are found
> 498: // The mask will have a bitmask indicating the position of the
> potential matches within the haystack
If no potential match, which label does the big_case_loop_helper jump to?
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 517:
> 515: __C2 arrays_equals(false, haystackStart, firstNeedleCompare,
> compLen, retval, rScratch, xmm_tmp3, xmm_tmp4,
> 516: false /* char */, knoreg);
> 517: __ testl(retval, retval);
Since this is byte compare even for isU, the retval here could be a 64-bit
quantity so the testl should be a testq.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 553:
> 551: // Haystack always copied to stack, so 32-byte reads OK
> 552: // Haystack length < 32
> 553: // 10 < needle length < 32
The comment below may need update as we come here for needle_len >
OPT_NEEDLE_SIZE_MAX which is currently set as 5:
// 10 < needle length < 32
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 611:
> 609: __C2 arrays_equals(false, rTmp, firstNeedleCompare, compLen,
> rTmp3, rTmp2, xmm_tmp3, xmm_tmp4, false /* char */,
> 610: knoreg);
> 611: __ testl(rTmp3, rTmp3);
Since this is byte compare even for isU, the rtmp3 here could be a 64-bit
quantity so the testl should be a testq.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 629:
> 627:
> 628: __ bind(L_returnError);
> 629: __ movq(rbp, -1);
This could directly be rax instead of
Re: RFR: 8311175: Move BufWriter::asByteBuffer to BufWriterImpl
On Tue, 7 May 2024 05:41:27 GMT, Adam Sotona wrote: >> As discussed on the mailing list >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html, >> BufWriter::asByteBuffer has a behavior not suitable for API and is only used >> by internal StackMapGenerator/StackCounter, so it will be converted to an >> internal API. >> >> Somehow the ByteBuffer needs to be sliced, or StackMapGenerator encounters >> IOOBE. Not sure what the exact cause was. > > @liach Do you have any plans with this PR? @asotona classfile tests pass, can you review this patch and its associated CSR? - PR Comment: https://git.openjdk.org/jdk/pull/14736#issuecomment-2099038162
Re: RFR: 8305457: Implement java.io.IO [v2]
> Please review this PR which introduces the `java.io.IO` top-level class and > three methods to `java.io.Console` for [Implicitly Declared Classes and > Instance Main Methods (Third Preview)]. > > This PR has been obtained as `git merge --squash` of a now obsolete [draft > PR]. > > [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: > https://bugs.openjdk.org/browse/JDK-8323335 > [draft PR]: https://github.com/openjdk/jdk/pull/18921 Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Respond to feedback from @naotoj - typo - trailing newlines - better wording - Changes: - all: https://git.openjdk.org/jdk/pull/19112/files - new: https://git.openjdk.org/jdk/pull/19112/files/ef888fd8..60050834 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19112=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19112=00-01 Stats: 5 lines in 4 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112 PR: https://git.openjdk.org/jdk/pull/19112
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 17:30:46 GMT, Naoto Sato wrote: > Sorry, I read it wrong. Your comment is clear so no need for rewording Still, I think that your misreading is a symptom of a problem with my wording. Let me try to rephrase it; let's see if you like it more. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592852458
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 16:24:52 GMT, Naoto Sato wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> src/java.base/share/classes/java/io/ProxyingConsole.java line 107:
>
>> 105: * {@inheritDoc}
>> 106: *
>> 107: * @throws IOError {@inheritDoc}
>
> Probably I am missing something, but I see `Console` declares this throws
> clause. Do we need this inheritDoc here?
Yes, we do. There's a common misconception that `{@inheritDoc}` inherits the
complete doc comment. In reality, `{@inheritDoc}` inherits only the main
description, which does not include any `@throws` tags.
A `@throws` tag is either inherited explicitly, such as in L107, or implicitly.
Implicit inheritance occurs when an exception is listed in the `throws` clause.
Since it's uncommon for unchecked exceptions (errors included) to be listed in
the `throws` clause, unless inherited explicitly, their documentation will be
missing from the overriding method documentation. Assuming, of course, that
your intention is to have them there.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592848314
Re: RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v13]
On Tue, 7 May 2024 13:44:06 GMT, Emanuel Peter wrote: > Thanks for the extra tests! > Thanks for reviewing. > Can you measure how much time each test now takes on your machine? > Only TestRoundVectorFloatAll.java took longer, but still in one minute, others run rather quicker than it. > I think we are getting there. Still a little worried about some random bugs > in the whole number generation... But I'd prefer having these tests to not > having them for sure ;) Agree! - PR Comment: https://git.openjdk.org/jdk/pull/17753#issuecomment-2098965761
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 17:15:43 GMT, Pavel Rappo wrote:
>> test/jdk/java/io/IO/IO.java line 99:
>>
>>> 97: System.getProperty("test.jdk") + "/bin/java",
>>> 98: "--enable-preview",
>>> 99: "-Djdk.console=gibberish",
>>
>> The test comment suggests this test is testing the default console
>> implementation, but the invocation specifies `-Djdk.console=gibberish` which
>> defaults to java.base. Is this what you intended?
>
> That comment says that this test tests jdk.internal.io.JdkConsoleImpl, which
> belongs to java.base. But, if you read it the way you described, I should
> definitely rephrase it.
Sorry, I read it wrong. Your comment is clear so no need for rewording
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592840539
Re: RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v13]
On Tue, 7 May 2024 13:30:12 GMT, Emanuel Peter wrote:
>> Hamlin Li has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix issues; modify vm options to make sure test the expected behaviors.
>
> test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 75:
>
>> 73: return (int) a;
>> 74: }
>> 75: }
>
> At first, I was worried about the indentation, then realized the original
> code had the strange indentation.
> Would there be a way to put this method in a shared file, so that you do not
> need to paste it everywhere?
moved to a shared lib file.
> test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatAll.java line
> 34:
>
>> 32: * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation
>> -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=8 -XX:+UseSuperWord
>> -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test*
>> compiler.vectorization.TestRoundVectorFloatAll
>> 33: * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation
>> -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=16 -XX:+UseSuperWord
>> -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test*
>> compiler.vectorization.TestRoundVectorFloatAll
>> 34: * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation
>> -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=32 -XX:+UseSuperWord
>> -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test*
>> compiler.vectorization.TestRoundVectorFloatAll
>
> Please check which flags you actually need here
removed `-XX:+PrintIdeal`
others seems useful to me.
> test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatAll.java line
> 43:
>
>> 41: public class TestRoundVectorFloatAll {
>> 42: private static final int ITERS = 11000;
>> 43: private static final int ARRLEN = 997;
>
> Could you randomize this value ever so slightly? That way, the boundaries of
> the array are at different places. I think also that the size should be a
> little larger, just to ensure that we get maximum vector lengths.
Make sense, done.
> test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatRandom.java
> line 202:
>
>> 200: }
>> 201:
>> 202: // test cases for NaN, Inf, subnormal, and so on
>
> just for completeness: +0.0 and -0.0
added
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592838750
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592838951
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592839461
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592838230
Re: RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v14]
> HI, > Can you have a look at this patch adding some tests for Math.round > instrinsics? > Thanks! > > ### FYI: > During the development of RoundVF/RoundF, we faced the issues which were only > spotted by running test exhaustively against 32/64 bits range of int/long. > It's helpful to add these exhaustive tests in jdk for future possible usage, > rather than build it everytime when needed. > Of course, we need to put it in `manual` mode, so it's not run when > `-automatic` jtreg option is specified which I guess is the mode CI used, > please correct me if I'm assume incorrectly. Hamlin Li has updated the pull request incrementally with one additional commit since the last revision: misc fixes - Changes: - all: https://git.openjdk.org/jdk/pull/17753/files - new: https://git.openjdk.org/jdk/pull/17753/files/b5207436..7c2ef4fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17753=13 - incr: https://webrevs.openjdk.org/?repo=jdk=17753=12-13 Stats: 251 lines in 5 files changed: 107 ins; 131 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/17753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17753/head:pull/17753 PR: https://git.openjdk.org/jdk/pull/17753
Re: RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v13]
On Tue, 7 May 2024 13:36:55 GMT, Emanuel Peter wrote: >> test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 31: >> >>> 29: * @library /test/lib / >>> 30: * @modules java.base/jdk.internal.math >>> 31: * @run main/othervm -XX:-TieredCompilation >>> -XX:CompileThresholdScaling=0.3 -XX:+PrintIdeal >>> -XX:CompileCommand=compileonly,compiler.floatingpoint.TestRoundFloatAll::test* >>> -XX:-UseSuperWord compiler.floatingpoint.TestRoundFloatAll >> >> please break up the line for easier reading > > Why these flags: > `-XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 -XX:+PrintIdeal > -XX:-UseSuperWord` ? > > I also suggest that you use `-Xbatch`, just to make sure we have compiled all > relevant methods after the warmup. If things get too slow, then maybe you > want to consider using explicit compile exclusion / forbidding inlining for > the `test*` method, rather than the compileonly, which prevents everything > else from compiling. Thanks for suggestion, added `-Xbatch`. removed `-XX:+PrintIdeal`. keep `-XX:-UseSuperWord`, as we are testing scalar version intrinsic in this test. `-XX:-TieredCompilation -XX:CompileThresholdScaling=0.3` are just from previous tests. - PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592837993
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 16:32:46 GMT, Naoto Sato wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> test/jdk/java/io/IO/IO.java line 99:
>
>> 97: System.getProperty("test.jdk") + "/bin/java",
>> 98: "--enable-preview",
>> 99: "-Djdk.console=gibberish",
>
> The test comment suggests this test is testing the default console
> implementation, but the invocation specifies `-Djdk.console=gibberish` which
> defaults to java.base. Is this what you intended?
That comment says that this test tests jdk.internal.io.JdkConsoleImpl, which
belongs to java.base. But, if you read it the way you described, I should
definitely rephrase it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592824491
RFR: 8331864: Update Public Suffix List to 1cbd6e7
Update PSL to the latest upstream version. - Commit messages: - the change Changes: https://git.openjdk.org/jdk/pull/19127/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19127=00 Issue: https://bugs.openjdk.org/browse/JDK-8331864 Stats: 568 lines in 5 files changed: 408 ins; 104 del; 56 mod Patch: https://git.openjdk.org/jdk/pull/19127.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19127/head:pull/19127 PR: https://git.openjdk.org/jdk/pull/19127
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v4]
On Tue, 7 May 2024 16:40:08 GMT, ExE Boss wrote: >> `currentDepth` must be 0 in this case, so `rank` or `netRank` doesn't >> matter. Overriding in `PrimitiveClassDescImpl` sounds reasonable, but then >> perhaps default method should be removed, too, since it would look strange >> to have the default method be specialized for instance/array types. Sounds >> like a CSR might be needed(?), so let's do that in a follow up. > > I don’t think a **CSR** is needed for changing the `default`ness of > JDK‑sealed interface methods, as source and binary compatibility for external > users is unaffected. A CSR is required when defaultness is removed, as in https://bugs.openjdk.org/browse/JDK-8309755 - PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1592810075
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 01:20:39 GMT, Naoto Sato wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> src/java.base/share/classes/java/io/Console.java line 189:
>
>> 187: /**
>> 188: * Writes a prompt as if by calling {@code print}, then reads a
>> single line
>> 189: * of text from this system console.
>
> Nit: I would not add `system` here as `this console` is consistent with other
> locations.
That is a typo; thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592805571
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 16 Apr 2024 13:54:53 GMT, Alan Bateman wrote: >>> > @mlchung @AlanBateman Any thoughts on this latest version? Is this going >>> > into the direction you had envisioned? Any more blockers? The CSR should >>> > be up-to-date and is open for review as well. If no more blockers I'll go >>> > over this patch once more and clean it up to prepare for integration. >>> > Thanks! >>> >>> Thanks for all the efforts on this. >> >> Thanks for looking at this, Alan! >> >>> I've looked through the latest version. I'm a little bit comfortable with >>> LinkDeltaProducer. One reason it's build tool that is tied tied to code in >>> the jdk.jlink module, and secondly is that it copies >>> runtime-image-link.delta into the run-time image. Our long standing >>> position is that the run-time image is created by jlink rather than a >>> combination of jlink and extra stuff copied in by the build. >>> >>> The part that I like with the current proposal is >>> lib/runtime-image-link.delta as it's less complicated that previous >>> iterations that added a resource into the jimage container. >>> >>> What would you (and @mlchung) think of having jlink generate >>> lib/runtime-image-link.delta as a step between the pipeline and image >>> generation? >> >> If I understand you correctly, this would be no longer a build-time only >> approach to produce the "linkable runtime"? It would be some-kind of >> jlink-option driven approach (as it would run some code that should only run >> when producing a linkable runtime is requested)? Other than that, it should >> be fine as the previous iteration basically did that but at a different >> phase. Also note that the previous iteration used a build-only jlink plugin >> so as to satisfy the build-time only approach, yet it ran as part of the >> plugin-pipeline which wasn't desired either. But it was something similar to >> what you seem to be describing now. Did I get something wrong? > >> If I understand you correctly, this would be no longer a build-time only >> approach to produce the "linkable runtime"? It would be some-kind of >> jlink-option driven approach (as it would run some code that should only run >> when producing a linkable runtime is requested)? Other than that, it should >> be fine as the previous iteration basically did that but at a different >> phase. Also note that the previous iteration used a build-only jlink plugin >> so as to satisfy the build-time only approach, yet it ran as part of the >> plugin-pipeline which wasn't desired either. But it was something similar to >> what you seem to be describing now. Did I get something wrong? > > I think it continues to build time, it's just using some hidden jlink option. > So yes, it similar to a previous iteration except that it doesn't run as a > plugin the pipeline and the delta goes to the lib directory. > > Let's see what @mlchung says. You've put a lot of work into this so let's see > if we can converge to avoid too many more rounds. @AlanBateman @mlchung The latest update now uses the `jlink` build time option `--generate-linkable-runtime` to add needed resources to the `jimage` when a runtime linkable JDK image is being asked for using the configure option. This now runs outside the plugin-pipeline. I think this is what you meant. Sorry it took longer to get back to this... - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2098895722
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v4]
On Mon, 6 May 2024 15:18:17 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 222:
>>
>>> 220: }
>>> 221: if (desc.length() == 1 && desc.charAt(0) == 'V') {
>>> 222: throw new IllegalArgumentException(String.format("not a
>>> valid reference type descriptor: %sV", "[".repeat(rank)));
>>
>> Suggestion:
>>
>> throw new IllegalArgumentException(String.format("not a valid
>> reference type descriptor: %sV", "[".repeat(netRank)));
>>
>> Or should we override this in `PrimitiveClassDescImpl`, which can bypass the
>> rank sum computation?
>
> `currentDepth` must be 0 in this case, so `rank` or `netRank` doesn't matter.
> Overriding in `PrimitiveClassDescImpl` sounds reasonable, but then perhaps
> default method should be removed, too, since it would look strange to have
> the default method be specialized for instance/array types. Sounds like a CSR
> might be needed(?), so let's do that in a follow up.
I don’t think a **CSR** is needed for changing the `default`ness of JDK‑sealed
interface methods, as source and binary compatibility for external users is
unaffected.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1592783700
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
> 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 105 commits: - Generate the runtime image link diff at jlink time But only do that once the plugin-pipeline has run. The generation is enabled with the hidden option '--generate-linkable-runtime' and the resource pools available at jlink time are being used to generate the diff. - Merge branch 'master' into jdk-8311302-jmodless-link - Use shorter name for the build tool - Review feedback from Erik J. - Remove dependency on CDS which isn't needed anymore - Fix coment - Fix comment - Fix typo - Revert some now unneded build changes - Follow build tools naming convention - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca - Changes: https://git.openjdk.org/jdk/pull/14787/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14787=25 Stats: 3424 lines in 36 files changed: 3272 ins; 110 del; 42 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: JDK-8331646: Add specific regression leap year tests (Task - P4) [v2]
On Tue, 7 May 2024 09:44:09 GMT, serhiysachkov wrote: >> Calendar.add() tests that describe its behavior. > > serhiysachkov has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8331646 updating to ParameterizedTests according to review request Sorry if I was unclear, but I meant to consolidate the test methods, as they are duplicates other than the calendar values. Those values should be parametrized and consolidate the test body into a single method. - PR Comment: https://git.openjdk.org/jdk/pull/19082#issuecomment-2098881008
Re: RFR: 8305457: Implement java.io.IO
On Mon, 6 May 2024 21:45:12 GMT, Pavel Rappo wrote:
> Please review this PR which introduces the `java.io.IO` top-level class and
> three methods to `java.io.Console` for [Implicitly Declared Classes and
> Instance Main Methods (Third Preview)].
>
> This PR has been obtained as `git merge --squash` of a now obsolete [draft
> PR].
>
> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
> https://bugs.openjdk.org/browse/JDK-8323335
> [draft PR]: https://github.com/openjdk/jdk/pull/18921
Looks good overall. Left some minor comments.
src/java.base/share/classes/java/io/Console.java line 189:
> 187: /**
> 188: * Writes a prompt as if by calling {@code print}, then reads a
> single line
> 189: * of text from this system console.
Nit: I would not add `system` here as `this console` is consistent with other
locations.
src/java.base/share/classes/java/io/ProxyingConsole.java line 107:
> 105: * {@inheritDoc}
> 106: *
> 107: * @throws IOError {@inheritDoc}
Probably I am missing something, but I see `Console` declares this throws
clause. Do we need this inheritDoc here?
test/jdk/java/io/IO/IO.java line 99:
> 97: System.getProperty("test.jdk") + "/bin/java",
> 98: "--enable-preview",
> 99: "-Djdk.console=gibberish",
The test comment suggests this test is testing the default console
implementation, but the invocation specifies `-Djdk.console=gibberish` which
defaults to java.base. Is this what you intended?
test/jdk/java/io/IO/Input.java line 33:
> 31: System.out.print(readln("?"));
> 32: }
> 33: }
Needs a newline
-
PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2041904750
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1591704961
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592764263
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592773917
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592776068
Re: RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts
On Tue, 7 May 2024 15:42:23 GMT, Maurizio Cimadamore wrote: > This PR fixes an issue that has crept into the FFM API implementation. > > From very early stages, the FFM API used to disable the alignment check on > nested layout elements, in favor of an alignment check against the memory > segment base address. The rationale was that the JIT had issue with > eliminating redundant alignment checks, and accessing nested elements could > never result in alignment issues, since the alignment of the container is > provably bigger than that of the contained element. This means that, when > creating a var handle for a nested layout element, we set the nested layout > alignment to 1 (unaligned), derive its var handle, and then decorate the var > handle with an alignment check for the container. > > At some point in 22, we tweaked the API to throw > `UnsupportedOperationException` when using an access mode incompatible with > the alignment constraint of the accessed layout. That is, a volatile read on > an `int` is only possible if the access occurs at an address that is at least > 4-byte aligned. Otherwise an `UOE` is thrown. > > Unfortunately this change broke the aforementioned optimization: creating a > var handle for an unaligned layout works, but the resulting layout will *not* > support any of the atomic access modes. > > Since this optimization is not really required anymore (proper C2 support to > hoist/eliminate alignment checks has been added since then), it is better to > disable this implementation quirk, and leave optimizations to C2. > > (If we really really wanted to optimize things a bit, we could remove the > container alignment check in the case the accessed value is the first layout > element nested in the container, but this PR doesn't go that far). > > I've run relevant benchmarks before/after and found no differences. In part > this is because `arrayElementVarHandle` is unaffected. But, even after > tweaking the `LoopOverNonConstant` benchmark to add explicit tests for the > code path affected, no significant difference was found, sign that C2 is > indeed able to spot (and remove) the redundant alignment check. Note: if we > know that `aligned_to_N(base)` holds, then it's easy to prove that > `aligned_to_M(base + offset + index * scale)` holds, when `N >= M` and with > `offset` and `scale` known (the latter a power of two). LGTM. As mentioned in the PR notes, additional optimizations could be made and this could be the objective of a future PR. - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19124#pullrequestreview-2043680093
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 12:17:36 GMT, Rémi Forax wrote: > I do not think the step to "standardise" a preview feature exists ? When a > preview feature becomes a released feature, the code is very lightly edited, > at least it this is my experience. We may call it differently, but I think both you and I are referring to this part of [JEP 12](https://openjdk.org/jeps/12) (emphasis mine): > Eventually, the JEP owner must decide the preview feature's fate. If the > decision is to remove the preview feature, then the owner must file an issue > in JBS to remove the feature in the next JDK feature release; no new JEP is > needed. **On the other hand, if the decision is to finalize, then the owner > must file a new JEP, noting refinements informed by developer feedback. The > title of this JEP should be the feature's name, omitting the earlier suffix > of (Preview) / (Second Preview), and without adding any new suffix such as > (Standard) or (Final). This JEP will ultimately reach Targeted status for the > next JDK feature release.** > You can change both readln and readLine and if `java.io.IO` is removed, at > least the code of readLine() will be Sorry, Rémi, but no. As long as this feature is in preview, I'd optimise for easier removal (back out) of the feature rather than clean combined code. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592740269
RFR: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts
This PR fixes an issue that has crept into the FFM API implementation. >From very early stages, the FFM API used to disable the alignment check on >nested layout elements, in favor of an alignment check against the memory >segment base address. The rationale was that the JIT had issue with >eliminating redundant alignment checks, and accessing nested elements could >never result in alignment issues, since the alignment of the container is >provably bigger than that of the contained element. This means that, when >creating a var handle for a nested layout element, we set the nested layout >alignment to 1 (unaligned), derive its var handle, and then decorate the var >handle with an alignment check for the container. At some point in 22, we tweaked the API to throw `UnsupportedOperationException` when using an access mode incompatible with the alignment constraint of the accessed layout. That is, a volatile read on an `int` is only possible if the access occurs at an address that is at least 4-byte aligned. Otherwise an `UOE` is thrown. Unfortunately this change broke the aforementioned optimization: creating a var handle for an unaligned layout works, but the resulting layout will *not* support any of the atomic access modes. Since this optimization is not really required anymore (proper C2 support to hoist/eliminate alignment checks has been added since then), it is better to disable this implementation quirk, and leave optimizations to C2. (If we really really wanted to optimize things a bit, we could remove the container alignment check in the case the accessed value is the first layout element nested in the container, but this PR doesn't go that far). I've run relevant benchmarks before/after and found no differences. In part this is because `arrayElementVarHandle` is unaffected. But, even after tweaking the `LoopOverNonConstant` benchmark to add explicit tests for the code path affected, no significant difference was found, sign that C2 is indeed able to spot (and remove) the redundant alignment check. Note: if we know that `aligned_to_N(base)` holds, then it's easy to prove that `aligned_to_M(base + offset + index * scale)` holds, when `N >= M` and with `offset` and `scale` known (the latter a power of two). - Commit messages: - Drop JDK property - Initial pusg Changes: https://git.openjdk.org/jdk/pull/19124/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19124=00 Issue: https://bugs.openjdk.org/browse/JDK-8331734 Stats: 56 lines in 3 files changed: 40 ins; 7 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/19124.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19124/head:pull/19124 PR: https://git.openjdk.org/jdk/pull/19124
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v4]
On Tue, 7 May 2024 14:59:21 GMT, Claes Redestad wrote: >> This PR suggests refactoring the implementation classes of >> java.lang.constant into a new package jdk.internal.constant to enable >> sharing some trusted static factory methods with users elsewhere in >> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring >> also adds some "trusted" methods for use when input is pre-validated, and >> makes use of them where applicable in the current implementation. There are >> more changes in the pipeline which will be folded into #17108 or PR'ed once >> that is integrated. >> >> There are two optimizations mixed up here. One in >> `MethodTypeDesc.ofDescriptor`: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.ofDescriptor >> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ± >> 1,126 ns/op 1,01x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ± >> 3,211 ns/op 1,07x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x >> (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op >> 1,89x (p = 0,000*) >> * = significant >> >> >> The other in `ClassDesc::nested`, where to get rid of a simple static method >> in `ConstantUtils` I ended up speeding up and simplifying the public factory >> method: >> >> Name CntBase ErrorTest >> Error Unit Change >> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± >> 0,573 ns/op 9,53x (p = 0,000*) >> * = significant >> >> >> The optimizations could be split out and PRd independently, but I think they >> are simple enough to include in this refactoring. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > pre-validated Since you are removing the redundant validation for `MethodTypeDesc::ofDescriptor`, we can move the ClassDesc array scan to the `of(ClassDesc, ClassDesc...)` factory and the factories that insert parameter types/replace a parameter. This way, we can reduce a lot of validation as well, especially that we in fact do insert parameters a lot (for `ofCallsiteBootstrap` and `ofConstantBoostrap`) - PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2098743577
Re: RFR: 8311175: Move BufWriter::asByteBuffer to BufWriterImpl [v2]
> As discussed on the mailing list > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html, > BufWriter::asByteBuffer has a behavior not suitable for API and is only used > by internal StackMapGenerator/StackCounter, so it will be converted to an > internal API. > > Somehow the ByteBuffer needs to be sliced, or StackMapGenerator encounters > IOOBE. Not sure what the exact cause was. Chen Liang 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 two additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into fix/asbb-internal - Convert asByteBuffer to an internal API - Changes: - all: https://git.openjdk.org/jdk/pull/14736/files - new: https://git.openjdk.org/jdk/pull/14736/files/5c25188c..d82789c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14736=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14736=00-01 Stats: 1808836 lines in 17143 files changed: 485717 ins; 823338 del; 499781 mod Patch: https://git.openjdk.org/jdk/pull/14736.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14736/head:pull/14736 PR: https://git.openjdk.org/jdk/pull/14736
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v3]
On Tue, 7 May 2024 10:46:41 GMT, Claes Redestad wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add a mixed scenario
>
> Nice improvement to the micro. It might be preferable to use random
> generation with a hardcoded or parameterized seed to reduce run-to-run
> variation, eg. `new Random(1)`.
>
> Agree that call sites are unlikely to mix `CD` and `Class`, but we're likely
> to see the bimorphicness from passing Primitive and ReferenceCDs. This is
> typically a very small and constant call overhead.
>
> The primitives path seem to be the weak link, and maybe we could take a cue
> from `sun.util.invoke.Wrapper` and set up a similar perfect hash table
> instead of a switch. This seem somewhat profitable:
>
> Name(type) CntBaseError Test
> Error Unit Change
> TypeKindBench.fromClassDescs PRIMITIVE 6 196,203 ± 2,469 147,898 ± 8,786
> ns/op 1,33x (p = 0,000*)
> TypeKindBench.fromClassesPRIMITIVE 6 325,336 ± 12,203 206,084 ± 2,180
> ns/op 1,58x (p = 0,000*)
>
>
> The `fromClasses PRIMITIVE` case is still a bit behind since getting the
> descriptorString takes a few extra (non-allocating) steps.
>
> Patch:
>
> diff --git a/src/java.base/share/classes/java/lang/classfile/TypeKind.java
> b/src/java.base/share/classes/java/lang/classfile/TypeKind.java
> index 5ba566b3d06..dd0a06c63ea 100644
> --- a/src/java.base/share/classes/java/lang/classfile/TypeKind.java
> +++ b/src/java.base/share/classes/java/lang/classfile/TypeKind.java
> @@ -58,6 +58,7 @@ public enum TypeKind {
>
> private final String name;
> private final String descriptor;
> +private final char basicTypeChar;
> private final int newarrayCode;
>
> /** {@return the human-readable name corresponding to this type} */
> @@ -100,6 +101,7 @@ public TypeKind asLoadable() {
> TypeKind(String name, String descriptor, int newarrayCode) {
> this.name = name;
> this.descriptor = descriptor;
> +this.basicTypeChar = descriptor.charAt(0);
> this.newarrayCode = newarrayCode;
> }
>
> @@ -154,7 +156,29 @@ public static TypeKind fromDescriptor(CharSequence s) {
> */
> public static TypeKind from(TypeDescriptor.OfField descriptor) {
> return descriptor.isPrimitive() // implicit null check
> -? fromDescriptor(descriptor.descriptorString())
> +? ofPrimitive(descriptor.descriptorString())
> : TypeKind.ReferenceType;
> }
> +
> +// Perfect hashing for basic types, borrowed from sun.invoke.util.Wrapper
> +private static final TypeKind[] FROM_CHAR = new TypeKind[16];
> +
> +static {
> +for (TypeKind w : values()) {
> +if...
@cl4es I have tried 3 scenarios with the fixed seed: baseline before your
comment, hash table over only primitives (your proposal), hash table over all
chars (my uploaded patch)
All of these are without the specialization of `isPrimitive()`.
I observe that hash for all chars boosts the mixed scenario; cannot explain why
exactly, but I go for hashing all chars because it should benefit the
general-purpose `fromDescriptor`, used by ClassFile API's implementations too.
What do you think?
No hash table:
Benchmark(type) Mode CntScore Error Units
TypeKindBench.fromClassDescs PRIMITIVE avgt6 121.863 ± 3.968 ns/op
TypeKindBench.fromClassDescs REFERENCE avgt6 56.158 ± 1.469 ns/op
TypeKindBench.fromClassDescs MIXED avgt6 141.508 ± 5.412 ns/op
TypeKindBench.fromClasses PRIMITIVE avgt6 232.201 ± 3.988 ns/op
TypeKindBench.fromClasses REFERENCE avgt6 15.663 ± 0.359 ns/op
TypeKindBench.fromClasses MIXED avgt6 155.033 ± 1.927 ns/op
Primitive-only hash:
Benchmark(type) Mode CntScore Error Units
TypeKindBench.fromClassDescs PRIMITIVE avgt6 112.731 ± 1.985 ns/op
TypeKindBench.fromClassDescs REFERENCE avgt6 57.339 ± 1.119 ns/op
TypeKindBench.fromClassDescs MIXED avgt6 126.926 ± 3.432 ns/op
TypeKindBench.fromClasses PRIMITIVE avgt6 154.779 ± 4.008 ns/op
TypeKindBench.fromClasses REFERENCE avgt6 15.619 ± 0.343 ns/op
TypeKindBench.fromClasses MIXED avgt6 132.254 ± 2.133 ns/op
Current hash:
Benchmark(type) Mode CntScore Error Units
TypeKindBench.fromClassDescs PRIMITIVE avgt6 116.446 ± 6.085 ns/op
TypeKindBench.fromClassDescs REFERENCE avgt6 56.399 ± 1.145 ns/op
TypeKindBench.fromClassDescs MIXED avgt6 121.087 ± 2.410 ns/op
TypeKindBench.fromClasses PRIMITIVE avgt6 154.842 ± 4.600 ns/op
TypeKindBench.fromClasses REFERENCE avgt6 15.897 ± 0.454 ns/op
TypeKindBench.fromClasses MIXED avgt6 118.444 ± 3.616 ns/op
-
PR Comment:
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
> A peek into TypeKind during the research for #19105 reveals that TypeKind has > a few issues: > 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to > use "newarray code" > 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to > throw IAE and added tests. > 3. `from(Class)` can be slow due to descriptor computation: added benchmark, > will share result in next comment (as it may change with code changes). > > The first 2 changes involves API changes, and a CSR has been created. > Requesting @asotona for a review. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Hash table, use fixed random seed - Changes: - all: https://git.openjdk.org/jdk/pull/19109/files - new: https://git.openjdk.org/jdk/pull/19109/files/adf1218c..9af30c65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19109=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19109=02-03 Stats: 68 lines in 3 files changed: 53 ins; 8 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19109.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19109/head:pull/19109 PR: https://git.openjdk.org/jdk/pull/19109
RFR: 8048691: Spliterator.SORTED characteristics gets cleared for BaseStream.spliterator
Removes SORTED if not also ORDERED for escape-hatch `Stream::spliterator()` - Commit messages: - Make sure that escape-hatch spliterator()s don't report SORTED if they aren't also ORDERED Changes: https://git.openjdk.org/jdk/pull/19123/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19123=00 Issue: https://bugs.openjdk.org/browse/JDK-8048691 Stats: 28 lines in 2 files changed: 26 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19123.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19123/head:pull/19123 PR: https://git.openjdk.org/jdk/pull/19123
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v4]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: pre-validated - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/f2f90193..543d0709 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v13]
On Tue, 7 May 2024 13:23:48 GMT, Emanuel Peter wrote: >> Hamlin Li has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix issues; modify vm options to make sure test the expected behaviors. > > test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 31: > >> 29: * @library /test/lib / >> 30: * @modules java.base/jdk.internal.math >> 31: * @run main/othervm -XX:-TieredCompilation >> -XX:CompileThresholdScaling=0.3 -XX:+PrintIdeal >> -XX:CompileCommand=compileonly,compiler.floatingpoint.TestRoundFloatAll::test* >> -XX:-UseSuperWord compiler.floatingpoint.TestRoundFloatAll > > please break up the line for easier reading Why these flags: `-XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 -XX:+PrintIdeal -XX:-UseSuperWord` ? I also suggest that you use `-Xbatch`, just to make sure we have compiled all relevant methods after the warmup. If things get too slow, then maybe you want to consider using explicit compile exclusion / forbidding inlining for the `test*` method, rather than the compileonly, which prevents everything else from compiling. - PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592498081
Re: RFR: 8325438: Add exhaustive tests for Math.round intrinsics [v13]
On Mon, 29 Apr 2024 11:38:27 GMT, Hamlin Li wrote:
>> HI,
>> Can you have a look at this patch adding some tests for Math.round
>> instrinsics?
>> Thanks!
>>
>> ### FYI:
>> During the development of RoundVF/RoundF, we faced the issues which were
>> only spotted by running test exhaustively against 32/64 bits range of
>> int/long.
>> It's helpful to add these exhaustive tests in jdk for future possible usage,
>> rather than build it everytime when needed.
>> Of course, we need to put it in `manual` mode, so it's not run when
>> `-automatic` jtreg option is specified which I guess is the mode CI used,
>> please correct me if I'm assume incorrectly.
>
> Hamlin Li has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix issues; modify vm options to make sure test the expected behaviors.
Thanks for the extra tests!
Can you measure how much time each test now takes on your machine?
I think we are getting there. Still a little worried about some random bugs in
the whole number generation... But I'd prefer having these tests to not having
them for sure ;)
test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 31:
> 29: * @library /test/lib /
> 30: * @modules java.base/jdk.internal.math
> 31: * @run main/othervm -XX:-TieredCompilation
> -XX:CompileThresholdScaling=0.3 -XX:+PrintIdeal
> -XX:CompileCommand=compileonly,compiler.floatingpoint.TestRoundFloatAll::test*
> -XX:-UseSuperWord compiler.floatingpoint.TestRoundFloatAll
please break up the line for easier reading
test/hotspot/jtreg/compiler/floatingpoint/TestRoundFloatAll.java line 75:
> 73: return (int) a;
> 74: }
> 75: }
At first, I was worried about the indentation, then realized the original code
had the strange indentation.
Would there be a way to put this method in a shared file, so that you do not
need to paste it everywhere?
test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatAll.java line 34:
> 32: * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation
> -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=8 -XX:+UseSuperWord
> -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test*
> compiler.vectorization.TestRoundVectorFloatAll
> 33: * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation
> -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=16 -XX:+UseSuperWord
> -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test*
> compiler.vectorization.TestRoundVectorFloatAll
> 34: * @run main/othervm -XX:+PrintIdeal -XX:-TieredCompilation
> -XX:CompileThresholdScaling=0.3 -XX:MaxVectorSize=32 -XX:+UseSuperWord
> -XX:CompileCommand=compileonly,compiler.vectorization.TestRoundVectorFloatAll::test*
> compiler.vectorization.TestRoundVectorFloatAll
Please check which flags you actually need here
test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatAll.java line 43:
> 41: public class TestRoundVectorFloatAll {
> 42: private static final int ITERS = 11000;
> 43: private static final int ARRLEN = 997;
Could you randomize this value ever so slightly? That way, the boundaries of
the array are at different places. I think also that the size should be a
little larger, just to ensure that we get maximum vector lengths.
test/hotspot/jtreg/compiler/vectorization/TestRoundVectorFloatRandom.java line
202:
> 200: }
> 201:
> 202: // test cases for NaN, Inf, subnormal, and so on
just for completeness: +0.0 and -0.0
-
PR Review: https://git.openjdk.org/jdk/pull/17753#pullrequestreview-2043182218
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592477207
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592487797
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592499343
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592508616
PR Review Comment: https://git.openjdk.org/jdk/pull/17753#discussion_r1592481581
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 12:18:52 GMT, Rémi Forax wrote: >> I assume it's about performance. If so, I would defer any >> performance-related tweaks until they are necessary. Interactive reading >> from console does not sound like something requiring that level of >> performance tweaking. > > yes, let see what @cl4es will say when he will test the time to print "hello > world" @forax I don't think anything touched on here is used during bootstrap; perhaps there are apps we could cover that uses these APIs, but I think for line-based console IO a few calls in the interpreter is not going to make a noticeable difference. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592438417
Re: RFR: 8311175: Move BufWriter::asByteBuffer to BufWriterImpl
On Fri, 30 Jun 2023 14:43:36 GMT, Chen Liang wrote: > As discussed on the mailing list > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-June/000381.html, > BufWriter::asByteBuffer has a behavior not suitable for API and is only used > by internal StackMapGenerator/StackCounter, so it will be converted to an > internal API. > > Somehow the ByteBuffer needs to be sliced, or StackMapGenerator encounters > IOOBE. Not sure what the exact cause was. Indeed, I should revive this patch. - PR Comment: https://git.openjdk.org/jdk/pull/14736#issuecomment-2098319220
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 11:00:52 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 74:
>>
>>> 72:
>>> 73: @Override
>>> 74: public String readln(String prompt) {
>>
>> this code can be simplified using an early return (and the body of the
>> try/catch can be reduced so it is more clear which statement can cause the
>> IOException)
>>
>> synchronized (writeLock) {
>> synchronized(readLock) {
>> if (!prompt.isEmpty()) {
>> pw.print(prompt);
>> pw.flush(); // automatic flushing does not cover print
>> }
>>char[] array;
>> try {
>> array = readline(false);
>> } catch (IOException x) {
>> throw new IOError(x);
>> }
>> if (array != null) {
>> return new String(array);
>> }
>> }
>> }
>> return null;
>
> This method started as a copy of `readLine`. In its current form, it's very
> evident how `readln` differs from `readLine`, and I would leave it this way
> for now. If the feature is standardised, we could refactor both methods
> together, as you suggested.
>
> If the `java.io.IO` part of the feature or the feature itself is withdrawn,
> then we could consider refactoring the existing `readLine`. Does it make
> sense?
I do not think the step to "standardise" a preview feature exists ? When a
preview feature becomes a released feature, the code is very lightly edited, at
least it this is my experience.
You can change both readln and readLine and if `java.io.IO` is removed, at
least the code of readLine() will be
>> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
>> line 88:
>>
>>> 86: @Override
>>> 87: public JdkConsole println(Object obj) {
>>> 88: writer().println(obj);
>>
>> the result of 'writer()' can be stored in a local variable (printing code
>> are not JITed as often as the rest of the codes)
>
> I assume it's about performance. If so, I would defer any performance-related
> tweaks until they are necessary. Interactive reading from console does not
> sound like something requiring that level of performance tweaking.
yes, let see what @cl4es will say when he will test the time to print "hello
world"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592385449
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592387034
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Tue, 7 May 2024 11:53:19 GMT, Pavel Rappo wrote: > Please review this mechanical change to man pages. This PR should be > integrated after https://github.com/openjdk/jdk/pull/18787. This PR is standalone as opposed to dependent because the https://github.com/openjdk/jdk/pull/18787 dependency currently has a merge conflict, which complicates the required workflow. This comment serves as a reminder to merge master into this PR once https://github.com/openjdk/jdk/pull/18787 has been integrated. If we don't do that, Skara will likely create a backport issue: > (⚠️ The fixVersion in this issue is [24] but the fixVersion in .jcheck/conf > is 23, a new backport will be created when this pr is integrated.) - PR Comment: https://git.openjdk.org/jdk/pull/19119#issuecomment-2098246931
RFR: 8330205: Initial troff manpage generation for JDK 24
Please review this mechanical change to man pages. This PR should be integrated after https://github.com/openjdk/jdk/pull/18787. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/19119/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19119=00 Issue: https://bugs.openjdk.org/browse/JDK-8330205 Stats: 30 lines in 28 files changed: 0 ins; 0 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19119.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19119/head:pull/19119 PR: https://git.openjdk.org/jdk/pull/19119
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 05:52:12 GMT, Rémi Forax wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
> line 88:
>
>> 86: @Override
>> 87: public JdkConsole println(Object obj) {
>> 88: writer().println(obj);
>
> the result of 'writer()' can be stored in a local variable (printing code are
> not JITed as often as the rest of the codes)
I assume it's about performance. If so, I would defer any performance-related
tweaks until they are necessary. Interactive reading from console does not
sound like something requiring that level of performance tweaking.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592303463
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 05:49:58 GMT, Rémi Forax wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 74:
>
>> 72:
>> 73: @Override
>> 74: public String readln(String prompt) {
>
> this code can be simplified using an early return (and the body of the
> try/catch can be reduced so it is more clear which statement can cause the
> IOException)
>
> synchronized (writeLock) {
> synchronized(readLock) {
> if (!prompt.isEmpty()) {
> pw.print(prompt);
> pw.flush(); // automatic flushing does not cover print
> }
>char[] array;
> try {
> array = readline(false);
> } catch (IOException x) {
> throw new IOError(x);
> }
> if (array != null) {
> return new String(array);
> }
> }
> }
> return null;
This method started as a copy of `readLine`. In its current form, it's very
evident how `readln` differs from `readLine`, and I would leave it this way for
now. If the feature is standardised, we could refactor both methods together,
as you suggested.
If the `java.io.IO` part of the feature or the feature itself is withdrawn,
then we could consider refactoring the existing `readLine`. Does it make sense?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592281687
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v3]
On Tue, 7 May 2024 01:49:27 GMT, Chen Liang wrote:
>> A peek into TypeKind during the research for #19105 reveals that TypeKind
>> has a few issues:
>> 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to
>> use "newarray code"
>> 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to
>> throw IAE and added tests.
>> 3. `from(Class)` can be slow due to descriptor computation: added benchmark,
>> will share result in next comment (as it may change with code changes).
>>
>> The first 2 changes involves API changes, and a CSR has been created.
>> Requesting @asotona for a review.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add a mixed scenario
Nice improvement to the micro. It might be preferable to use random generation
with a hardcoded or parameterized seed to reduce run-to-run variation, eg. `new
Random(1)`.
Agree that call sites are unlikely to mix `CD` and `Class`, but we're likely to
see the bimorphicness from passing Primitive and ReferenceCDs. This is
typically a very small and constant call overhead.
The primitives path seem to be the weak link, and maybe we could take a cue
from `sun.util.invoke.Wrapper` and set up a similar perfect hash table instead
of a switch. This seem somewhat profitable:
Name(type) CntBaseError TestError
Unit Change
TypeKindBench.fromClassDescs PRIMITIVE 6 196,203 ± 2,469 147,898 ± 8,786
ns/op 1,33x (p = 0,000*)
TypeKindBench.fromClassesPRIMITIVE 6 325,336 ± 12,203 206,084 ± 2,180
ns/op 1,58x (p = 0,000*)
The `fromClasses PRIMITIVE` case is still a bit behind since getting the
descriptorString takes a few extra (non-allocating) steps.
Patch:
diff --git a/src/java.base/share/classes/java/lang/classfile/TypeKind.java
b/src/java.base/share/classes/java/lang/classfile/TypeKind.java
index 5ba566b3d06..dd0a06c63ea 100644
--- a/src/java.base/share/classes/java/lang/classfile/TypeKind.java
+++ b/src/java.base/share/classes/java/lang/classfile/TypeKind.java
@@ -58,6 +58,7 @@ public enum TypeKind {
private final String name;
private final String descriptor;
+private final char basicTypeChar;
private final int newarrayCode;
/** {@return the human-readable name corresponding to this type} */
@@ -100,6 +101,7 @@ public TypeKind asLoadable() {
TypeKind(String name, String descriptor, int newarrayCode) {
this.name = name;
this.descriptor = descriptor;
+this.basicTypeChar = descriptor.charAt(0);
this.newarrayCode = newarrayCode;
}
@@ -154,7 +156,29 @@ public static TypeKind fromDescriptor(CharSequence s) {
*/
public static TypeKind from(TypeDescriptor.OfField descriptor) {
return descriptor.isPrimitive() // implicit null check
-? fromDescriptor(descriptor.descriptorString())
+? ofPrimitive(descriptor.descriptorString())
: TypeKind.ReferenceType;
}
+
+// Perfect hashing for basic types, borrowed from sun.invoke.util.Wrapper
+private static final TypeKind[] FROM_CHAR = new TypeKind[16];
+
+static {
+for (TypeKind w : values()) {
+if (w == ReferenceType) {
+continue;
+}
+char c = w.descriptor.charAt(0);
+FROM_CHAR[(c + (c >> 1)) & 0xf] = w;
+}
+}
+
+private static TypeKind ofPrimitive(String descriptor) {
+char basicTypeChar = descriptor.charAt(0);
+TypeKind tk = FROM_CHAR[(basicTypeChar + (basicTypeChar >> 1)) & 0xf];
+if (tk == null || tk.basicTypeChar != basicTypeChar) {
+throw new IllegalArgumentException("bad descriptor: " +
descriptor);
+}
+return tk;
+}
}
```
-
PR Comment: https://git.openjdk.org/jdk/pull/19109#issuecomment-2098060815
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Tue, 7 May 2024 09:36:10 GMT, Jan Kratochvil wrote: > Should JDK still support `memory.use_hierarchy == 0`? IMO, no. Just get rid of it and assume hierarchical everywhere. We'd be walking the hierarchy for other (lower limits), which should cover this case on those legacy systems. - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2097892763
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4) [v2]
> Calendar.add() tests that describe its behavior. serhiysachkov has updated the pull request incrementally with one additional commit since the last revision: JDK-8331646 updating to ParameterizedTests according to review request - Changes: - all: https://git.openjdk.org/jdk/pull/19082/files - new: https://git.openjdk.org/jdk/pull/19082/files/3d09d62c..bce324f3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19082=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19082=00-01 Stats: 79 lines in 1 file changed: 23 ins; 0 del; 56 mod Patch: https://git.openjdk.org/jdk/pull/19082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19082/head:pull/19082 PR: https://git.openjdk.org/jdk/pull/19082
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v8]
On Sun, 10 Mar 2024 14:40:09 GMT, Jan Kratochvil wrote: >> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 35 commits: > > - Fix whitespace > - Merge branch 'master' into master-cgroup > >Conflicts: > test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp > - Fix gtest > - Update the Java part > - Fix cgroup1 backward compatibility message > - Merge remote-tracking branch 'centos79/master-cgroup' into master-cgroup > - Disable cgroup.subtree_control testcase on cgroup1 > - Fix gtest > - Merge branch 'master' into master-cgroup > - Merge remote-tracking branch 'f38crac/master-cgroup' into master-cgroup > - ... and 25 more: https://git.openjdk.org/jdk/compare/243cb098...39c90162 Should JDK still support `memory.use_hierarchy == 0`? That has been already removed from Linux kernel: https://github.com/torvalds/linux/commit/bef8620cd8e0a117c1a0719604052e424eb418f9 This patch is apparently present even in kernel-3.10.0-1160.118.1.el7.x86_64 (CentOS-7.9 updates) It is not present in kernel-3.10.0-1160.el7.x86_64 (CentOS-7.9 release) Still CentOS-7 is almost EOLed, is there any other distro for cgroup1? - PR Comment: https://git.openjdk.org/jdk/pull/17198#issuecomment-2097873090
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4)
On Fri, 3 May 2024 10:31:14 GMT, serhiysachkov wrote: > Calendar.add() tests that describe its behavior. This PR provides additional tests that clarify behavior of Calendar.add() method for leap year, specifically behavior that led to this ticket https://bugs.openjdk.org/browse/JDK-8327088. - PR Comment: https://git.openjdk.org/jdk/pull/19082#issuecomment-2097814095
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v3]
> Since ~ end of March, after > [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), > tools/launcher/JliLaunchTest.java fails on AIX. Failure is : > > stdout: []; > stderr: [Error: could not find libjava.so > Error: Could not find Java SE Runtime Environment. > ] > exitValue = 2 > > java.lang.RuntimeException: Expected to get exit value of [0], exit value is: > [2] > at > jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521) > at JliLaunchTest.main(JliLaunchTest.java:58) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > at > com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) > at java.base/java.lang.Thread.run(Thread.java:1575) > > Maybe we need to do further adjustments to > BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / > BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ? > Or somehow adjust the coding that attempts to find parts of "JRE" > (libjava.so) ? The libjli.so is gone on AIX after > [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this > also the image discovery based on GetApplicationHomeFromDll which uses > libjli.so . > > Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. > There is no other methos available on AIX, because it lacks the $ORIGIN > feature of the rpath. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: cosmetic changes - Changes: - all: https://git.openjdk.org/jdk/pull/19000/files - new: https://git.openjdk.org/jdk/pull/19000/files/caf806b3..5890bca3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19000=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19000=01-02 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19000.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19000/head:pull/19000 PR: https://git.openjdk.org/jdk/pull/19000
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v4]
On Mon, 6 May 2024 19:47:48 GMT, Axel Hauschulte wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Check both parse methods
>
> test/jdk/java/text/Format/DecimalFormat/LargeExponentsTest.java line 150:
>
>> 148: // Long.MIN_VALUE
>> 149: Arguments.of("1.23E-9223372036854775808", 0.0)
>> 150: );
>
> I would suggest adding one more test case to the edge cases:
>
> Arguments.of("0.0123E-2147483648", 0.0)
>
> This will test the adjustment of the `digits.decimalAt` field for an exponent
> that is within the range of integer, but due to the mantissa not being in its
> standardized form an overflow will occure non the less.
Right, that's a good test case to have, (which exposed a needed update in the
implementation).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1591971646
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v5]
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
correct behavior when underflow from adjustment + use MIN
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19075/files
- new: https://git.openjdk.org/jdk/pull/19075/files/17a3b3aa..2c167493
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19075=04
- incr: https://webrevs.openjdk.org/?repo=jdk=19075=03-04
Stats: 19 lines in 2 files changed: 9 ins; 4 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v4]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Adding another test run with -Djdk.console=java.base, as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/05592871..a138981e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=03
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=02-03
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v2]
On Fri, 3 May 2024 15:25:05 GMT, Joachim Kern wrote:
>> Since ~ end of March, after
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131),
>> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
>>
>> stdout: [];
>> stderr: [Error: could not find libjava.so
>> Error: Could not find Java SE Runtime Environment.
>> ]
>> exitValue = 2
>>
>> java.lang.RuntimeException: Expected to get exit value of [0], exit value
>> is: [2]
>> at
>> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
>> at JliLaunchTest.main(JliLaunchTest.java:58)
>> at
>> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
>> at
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
>> at java.base/java.lang.Thread.run(Thread.java:1575)
>>
>> Maybe we need to do further adjustments to
>> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest /
>> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
>> Or somehow adjust the coding that attempts to find parts of "JRE"
>> (libjava.so) ? The libjli.so is gone on AIX after
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this
>> also the image discovery based on GetApplicationHomeFromDll which uses
>> libjli.so .
>>
>> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar.
>> There is no other methos available on AIX, because it lacks the $ORIGIN
>> feature of the rpath.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> only for AIX
Minor formatting suggestions
src/java.base/unix/native/libjli/java_md_common.c line 135:
> 133: */
> 134: jboolean
> 135: GetApplicationHomeFromLibpath(char *buf, jint bufsize)
Suggestion:
GetApplicationHomeFromLibpath(char *buf, jint bufsize) {
src/java.base/unix/native/libjli/java_md_common.c line 136:
> 134: jboolean
> 135: GetApplicationHomeFromLibpath(char *buf, jint bufsize)
> 136: {
Suggestion:
src/java.base/unix/native/libjli/java_md_common.c line 139:
> 137: char *env = getenv(LD_LIBRARY_PATH);
> 138: char *tmp;
> 139: char* save_ptr = NULL;
Suggestion:
char *save_ptr = NULL;
-
PR Review: https://git.openjdk.org/jdk/pull/19000#pullrequestreview-2042227569
PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1591912792
PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1591912990
PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1591911912
Re: RFR: 8329691: Support `nonlikelyScript` parent locale inheritance
On Mon, 6 May 2024 17:53:56 GMT, Naoto Sato wrote: > This PR is to implement the `nonlikelyScript` feature that went into CLDR > version 45 for migration purposes. In its release note, it states > (https://cldr.unicode.org/index/downloads/cldr-45): > > Migration > Changes to parentLocales require upgrading implementations that use that > element. In particular, they need to support the new nonlikelyScript value, > and use the appropriate explicit inheritance for each type of inheritance. > The v44 list of locales that inherit directly from root is retained for this > release, but will disappear in the future. So implementations should move as > quickly as possible to support the new value > > For example in `Russian` locales fallback, its likely script is `Cyrl` > (Cyrillic). Thus Russian locales with non-likely script, such as 'ru-Latn' > (Russian in Latin script) should fallback directly to `root`, bypassing `ru` > (Russian). CLDR has explicit parent locales for this nonlikely scripts, such > as `zh-Hant` -> `root` already, but the release note suggests this will go > away, and JDK needs to logically handle these non-likely script inheritance > cases. > > To implement this behavior, CLDRConverter build tool now generates the > `LocaleDataMetaInfo` for java.base module with the new `likelyScriptMap`, > which maps the script to its likely languages. Since the map is big, it is > lazily initialized when needed. The map is used at runtime to determine the > parent locale fallback based on implicit/explicit nonlikely Script > inheritance. Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19108#pullrequestreview-2042157221
