Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v18]

[Scott Gibbons]

> 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:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

Scott Gibbons has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 50 commits:

 - Merge remote-tracking branch 'origin/master' into indexof
 - Move arrays_equals back to c2_MacroAssembler
 - Merge branch 'openjdk:master' into indexof
 - Remove infinite loop (used for debugging)
 - Merge branch 'openjdk:master' into indexof
 - Cleaned up, ready for review
 - Pre-cleanup code
 - Add JMH. Add 16-byte compares to arrays_equals
 - Better method for mask creation
 - Merge branch 'openjdk:master' into indexof
 - ... and 40 more: https://git.openjdk.org/jdk/compare/b20fa7b4...f52d281d

-

Changes: https://git.openjdk.org/jdk/pull/16753/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=16753=17
  Stats: 4345 lines in 17 files changed: 4183 ins; 26 del; 136 mod
  Patch: https://git.openjdk.org/jdk/pull/16753.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753

PR: https://git.openjdk.org/jdk/pull/16753

Re: RFR: 8331535: Incorrect prompt for Console.readLine

[Pavel Rappo]

On Fri, 3 May 2024 19:02:41 GMT, Naoto Sato  wrote:

> We have a test that checks if `System.console()` returns the correct Console 
> (or null) from the expected module 
> (`test/jdk/java/io/Console/ModuleSelectionTest.java`)
>

Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. 
Initially, I suggested an alternative wording (i.e. "default"), but that's 
inappropriate. After all, the test in question resides in 
`test/jdk/jdk/internal/jline`. So it only makes sense that it tests that 
concrete implementation. 

> Yeah, that would be helpful.
>

I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589756489

Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[John Rose]

P.S. I didn’t directly address David’s request for that magic number N.
I would just assume 32 bits as the conservative element size (for oops)
and work from there.  If the cache line size is 64 bytes, then N=16.
These are robust assumptions, even if they waste a little space
when N could be 8 (because oops are 64 bits).  If the objects are
flattened, N=16 is still conservatively correct, as long as they
flatten to at least 4 bytes (which is very likely, and if not just
use a primitive array).  The same external considerations that
determine the D$ line size, to a value other than 64, can also
configure the parameter N, to a value other than 16.  That is
how I would build David’s widget without using Unsafe.

Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[John Rose]

Some of these sorts of use cases could be covered by somehow discovering
a platform-specific parameter N such that a[I] and a[I+N] are not likely
to have false sharing in a data cache, for any I.  (Or a series of N0,
N1, … that prevent a[N0], a[N1]… from being falsely shared.)  David is
right that this parameter depends on the physical array stride,
and that depends on (1) compressed oops or not, and/or (2) flattening
of specific element types.  It depends on cache architecture also.
Throw Leyden into the mix, and it is not clear that just querying
a number is the right answer, either, since it might be predicted
one way by Leyden and end up differently in application deployment.

But this is fragile and error-prone, I think.  It might be nicer
to encapsulate it as folks suggest, even though its users are
power users.  (They are probably power users in a different
domain from VM implementation.)

So I also would prefer to see, rather than a fragile idiom for using
arrays, an API that features an opaque container object and a varhandle
for accessing its (un-)contended “lanes”. The thing would feel like
an “array with varhandles”.  And the opaque object might even be
an array of some sort, if you looked at it closely.  I don’t think
it needs to be wrapped in an envelope to hide its nature.  Power
users ought to know not to look too closely at the container object.

It seems to me that this should be built first on top of the JDK,
not inside the JDK, as a way to shake out the design before the JDK
commits to such a design.

The @Contended feature does not ensure that (un-)contended fields
fall at the start of a cache line, because the VM does not align
objects that precisely (by default).  Likewise, an indexed version
like we are talking about here would not guarantee any particular
offset that the data lanes would fall in (within cache lines).
It would simply set unrelated ones far enough apart to ensure that
that they cannot fall on the same cache line; this requires that
the platform guarantee an appropriate minimum distance.

BTW, the @Contended feature allows fields to be grouped.  A good
corresponding feature for arrays (with varhandles) should allow
for grouping similarly.

So, a first draft API might look something like this:

class ContendedArrayFactory {
  ContendedArrayFactory(Class elementType, int groupSize);
  VarHandle accessor(int which);  // which < groupSize
  ContendedArrayFactory(Class elementType); // default groupSize=1
  VarHandle accessor();  // default which=0
  Object make(int length);  // storage len = roundup(groupSize,D$len)*length
}

In a Leyden setting, each ContendedArrayFactory, and the arrays it
makes, should be (re)generated fresh at startup, if there is suspicion
that data cache size could change.  Frankly, I don’t think there will
be such a suspicion.  Leyden tends to “bake in” environmental settings
such as whether oop compression is enabled.

(The real core libs people should weigh in; I’m just brainstorming here.)

On 3 May 2024, at 11:20, Ron Pressler wrote:

>> On 3 May 2024, at 18:33, David Lloyd  wrote:
>>
>>
>> On Fri, May 3, 2024 at 10:12 AM Mark Reinhold  
>> wrote:
>> https://openjdk.org/jeps/471
>>
>>   Summary: Deprecate the memory-access methods in sun.misc.Unsafe for
>>   removal in a future release.
>>
>>
>> We still use Unsafe fairly often in various Red Hat products (primarily 
>> because our baseline support JDK for these products is typically 11 or 17 at 
>> present), in a variety of ways for a variety of reasons. Most of these uses 
>> of Unsafe should be transitionable to `MemorySegment` using multi-release 
>> JARs, and a bit of exploratory work has already been done on this. However 
>> there is one unfortunate exception (that I know of).
>>
>> In order to avoid false sharing in certain specific high-concurrency 
>> situations, I have lately used arrays to space out certain value locations 
>> by using the smallest data cache line size (which is detected via an 
>> existing library) and dividing it by the array scale to determine the length 
>> of array to allocate in order to accommodate these values. I then use 
>> multiples of the cache line size (in bytes), offset from the array base, to 
>> locate the elements to access.
>>
>> It is possible to continue this more or less as-is for primitive types (at 
>> least, it is if one assumes certain facts around primitive data type size 
>> and alignment to be true), but for objects, without knowing their size, we 
>> can't know how much padding to reserve around the value location to ensure 
>> that the contended values are not falsely shared.
>>
>> I seem to recall (years ago now so I might be a bit fuzzy on it) that the 
>> lack of public API around `@Contended` was mildly controversial in the past. 
>> The proposed remedy was to use arrays for this purpose, if I recall 
>> correctly. However there does not seem to be any good way to do this anymore 
>> (at least for objects) without simply guessing, 

Re: RFR: 8330276: Console methods with explicit Locale [v2]

[Roger Riggs]

On Mon, 29 Apr 2024 23:22:21 GMT, Naoto Sato  wrote:

>> Proposing new overloaded methods in `java.io.Console` class that explicitly 
>> take a `Locale` argument. Existing methods rely on the default locale, so 
>> the users won't be able to format their prompts/outputs in a certain locale 
>> explicitly.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed review comments

lgtm

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18923#pullrequestreview-2038907502

Re: Need for a sponsor for JDK-8313674

[Kevin Rushforth]

Also , as a helpful hint, Skara has reminded you not to force push [1]. 
I see that you have done this a couple times, which suggests you might 
be in the habit of doing this as part of your workflow. Please don't. 
Instead, if you need to make changes, push the changes as additional 
commits. Skara will squash all of the commits into a single commit with 
the proper title, list of reviewers, etc.


Thanks.

-- Kevin

[1] https://github.com/openjdk/jdk/pull/19021#issuecomment-2085328329


On 5/3/2024 12:01 PM, Brian Burkhalter wrote:

Hello,

This is the PR [1]. This is the sequence that should be followed:

1. Align the name title of the PR with the issue. (you)
2. Continue the conversation until at least one Reviewer approves it. 
(one or more of us)

3. Comment with /integrate command. (you)
4. Comment with /sponsor command. (one of us).

Alan might have more comments on this, and given that it is after 
close of business in his time zone, I don’t think we’ll see further 
progress before Monday.


Thanks,

Brian

[1] https://github.com/openjdk/jdk/pull/19021


On Apr 24, 2024, at 9:23 PM, Iñigo Mediavilla  wrote:

For my first contribution to OpenJDK, I've started looking into 
JDK-8313674, an issue that falls into core-libs. According to the 
OpenJDK Developers’ Guide I'd need a sponsor to help me through the 
contribution process, would anyone be available to help me ?

Integrated: 8331655: ClassFile API ClassCastException with verbose output of certain class files

[Adam Sotona]

On Fri, 3 May 2024 15:28:05 GMT, Adam Sotona  wrote:

> Specifically corrupted constant pool of a class file can cause 
> ClassCastException, when the entries are accessed by Class-File API in exact 
> order.
> 
> This fix avoids the ClassCastException and throws ConstantPoolException 
> instead.
> Test is attached.
> 
> Please review.
> 
> Thanks,
> Adam

This pull request has now been integrated.

Changeset: c1a16452
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/c1a164528a538d5de78f99c4c92291b1906703f5
Stats: 24 lines in 2 files changed: 9 ins; 11 del; 4 mod

8331655: ClassFile API ClassCastException with verbose output of certain class 
files

Reviewed-by: psandoz

-

PR: https://git.openjdk.org/jdk/pull/19088

Re: RFR: 8331535: Incorrect prompt for Console.readLine

[Naoto Sato]

On Fri, 3 May 2024 18:52:12 GMT, Pavel Rappo  wrote:

> Ideally, we should have separate tests that make sure that jdk.internal.le is 
> the default impl.

We have a test that checks if `System.console()` returns the correct Console 
(or null) from the expected module 
(`test/jdk/java/io/Console/ModuleSelectionTest.java`)

> We should also test this behaviour (i.e. % interpretation) on jdk.internal.io 
> Console impl.

Yeah, that would be helpful.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589629655

Re: Need for a sponsor for JDK-8313674

[Brian Burkhalter]

Hello,

This is the PR [1]. This is the sequence that should be followed:

1. Align the name title of the PR with the issue. (you)
2. Continue the conversation until at least one Reviewer approves it. (one or 
more of us)
3. Comment with /integrate command. (you)
4. Comment with /sponsor command. (one of us).

Alan might have more comments on this, and given that it is after close of 
business in his time zone, I don’t think we’ll see further progress before 
Monday.

Thanks,

Brian

[1] https://github.com/openjdk/jdk/pull/19021

On Apr 24, 2024, at 9:23 PM, Iñigo Mediavilla  wrote:

For my first contribution to OpenJDK, I've started looking into JDK-8313674, an 
issue that falls into core-libs. According to the OpenJDK Developers’ Guide I'd 
need a sponsor to help me through the contribution process, would anyone be 
available to help me ?

Re: RFR: 8331535: Incorrect prompt for Console.readLine

[Pavel Rappo]

On Fri, 3 May 2024 17:07:59 GMT, Naoto Sato  wrote:

> Or maybe we could explicitly specify `-Djdk.console=jdk.internal.le` to the 
> test.

We could do that, or we could replace `jdk.internal.le` with "default" in the 
summary.

Ideally, we should have separate tests that make sure that jdk.internal.le is 
the default impl. We should also test this behaviour (i.e. % interpretation) on 
jdk.internal.io Console impl. What do you think?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589620826

Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]

[Naoto Sato]

On Fri, 3 May 2024 18:29:23 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 two additional 
> commits since the last revision:
> 
>  - correct other test comment
>  - reflect review

Marked as reviewed by naoto (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19075#pullrequestreview-2038805949

Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]

[Justin Lu]

On Fri, 3 May 2024 18:24:31 GMT, Justin Lu  wrote:

>> src/java.base/share/classes/java/text/DecimalFormat.java line 2596:
>> 
>>> 2594: exponent = -exponent;
>>> 2595: }
>>> 2596: sawExponent = true;
>> 
>> I see you removed this assignment. I am wondering if we need this variable 
>> at all.
>
> Should be updated in the latest commit, I had forgotten to remove the 
> assignment as well as the check. As we always `break` once we parse the 
> exponent, we have no need for the boolean flag. Thanks for the review!

Perhaps there was a planned reason for it before, but as it stands it serves no 
purpose, so until there is a reason to have it, I think we can safely remove it 
to de-clutter the code.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589588844

Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]

[Justin Lu]

On Fri, 3 May 2024 18:02:05 GMT, Naoto Sato  wrote:

>> Justin Lu has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - correct other test comment
>>  - reflect review
>
> src/java.base/share/classes/java/text/DecimalFormat.java line 2596:
> 
>> 2594: exponent = -exponent;
>> 2595: }
>> 2596: sawExponent = true;
> 
> I see you removed this assignment. I am wondering if we need this variable at 
> all.

Should be updated in the latest commit, I had forgotten to remove the 
assignment as well as the check. As we always `break` once we parse the 
exponent, we have no need for the boolean flag. Thanks for the review!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589587379

Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]

[Justin Lu]

> 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 two additional 
commits since the last revision:

 - correct other test comment
 - reflect review

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19075/files
  - new: https://git.openjdk.org/jdk/pull/19075/files/5bf5d6ef..bc391f96

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19075=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19075=00-01

  Stats: 9 lines in 2 files changed: 0 ins; 1 del; 8 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

Need for a sponsor for JDK-8313674

[Iñigo Mediavilla]

Hello,

For my first contribution to OpenJDK, I've started looking into
JDK-8313674, an issue that falls into core-libs. According to the OpenJDK
Developers’ Guide I'd need a sponsor to help me through the contribution
process, would anyone be available to help me ?

Thanks in advance

Íñigo Mediavilla Saiz

Looking for a sponsor to help with my first contribution

[Iñigo Mediavilla]

Hello,

For my first contribution to OpenJDK, I've started looking into
JDK-8313674, an issue that falls into core-libs. According to the OpenJDK
Developers’ Guide I'd need a sponsor to help me through the contribution
process, would anyone be available to help me ?

Thanks in advance

Íñigo Mediavilla Saiz

Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[Ron Pressler]

> On 3 May 2024, at 18:33, David Lloyd  wrote:
> 
> 
> On Fri, May 3, 2024 at 10:12 AM Mark Reinhold  
> wrote:
> https://openjdk.org/jeps/471
> 
>   Summary: Deprecate the memory-access methods in sun.misc.Unsafe for
>   removal in a future release.
> 
> 
> We still use Unsafe fairly often in various Red Hat products (primarily 
> because our baseline support JDK for these products is typically 11 or 17 at 
> present), in a variety of ways for a variety of reasons. Most of these uses 
> of Unsafe should be transitionable to `MemorySegment` using multi-release 
> JARs, and a bit of exploratory work has already been done on this. However 
> there is one unfortunate exception (that I know of).
> 
> In order to avoid false sharing in certain specific high-concurrency 
> situations, I have lately used arrays to space out certain value locations by 
> using the smallest data cache line size (which is detected via an existing 
> library) and dividing it by the array scale to determine the length of array 
> to allocate in order to accommodate these values. I then use multiples of the 
> cache line size (in bytes), offset from the array base, to locate the 
> elements to access.
> 
> It is possible to continue this more or less as-is for primitive types (at 
> least, it is if one assumes certain facts around primitive data type size and 
> alignment to be true), but for objects, without knowing their size, we can't 
> know how much padding to reserve around the value location to ensure that the 
> contended values are not falsely shared.
> 
> I seem to recall (years ago now so I might be a bit fuzzy on it) that the 
> lack of public API around `@Contended` was mildly controversial in the past. 
> The proposed remedy was to use arrays for this purpose, if I recall 
> correctly. However there does not seem to be any good way to do this anymore 
> (at least for objects) without simply guessing, and this seems like a small 
> but significant hole in this plan as it stands for now.
> 
> It seems to me that the JDK could fill this gap by introducing some API which 
> can construct and provide access to an array or something like it, with 
> striding and/or alignment guarantees that each element will reside on a 
> separate data cache line (or barring that, perhaps using a minimum 
> per-element size and/or alignment that is given as an argument to the 
> factory), and with the gamut of atomic accessors via a `VarHandle` or 
> similar. This could be especially valuable if/when objects start coming in a 
> variety of shapes and sizes in memory, once value types hit.
> 
> Could such a thing be added into the plan?
> 
> -- 
> - DML • he/him

[redirecting to core-libs]

Adding some VarHandle operation that takes into account the cache lines size is 
interesting — although preserving cache-line *alignment* could be tricky as the 
GC relocates arrays, so an array element that’s at the start of a cache line at 
time t0 might not be at the start of a cache line at time t1 — but that’s 
unrelated to this JEP.

What is related to this JEP is that you’re using Unsafe to determine the size 
of an oop (in particular, to tell if oops are compressed or no)t. Is that what 
you’re asking for?

— Ron

Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent

[Naoto Sato]

On Fri, 3 May 2024 08:47:03 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.

Looks good. Left some minor comments.

src/java.base/share/classes/java/text/DecimalFormat.java line 2590:

> 2588: 
> 2589: if (subparse(text, pos, "", 
> symbols.getMinusSignText(), exponentDigits, true, stat)) {
> 2590: // We parse the exponent with isExponent true, 
> thus fitsIntoLong

Nit: `==` or `being` between `isExponent` and `true` may read better.

src/java.base/share/classes/java/text/DecimalFormat.java line 2596:

> 2594: exponent = -exponent;
> 2595: }
> 2596: sawExponent = true;

I see you removed this assignment. I am wondering if we need this variable at 
all.

test/jdk/java/text/Format/DecimalFormat/LargeExponentsTest.java line 128:

> 126: // Trailing zeroes
> 127: Arguments.of("1.23E123", 1.23E123),
> 128: // Trailing zeroes - Past Long.MAX_VALUE length

Leading zeroes?

-

PR Review: https://git.openjdk.org/jdk/pull/19075#pullrequestreview-2038744656
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589560835
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589558570
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589573332

Re: RFR: 8331655: ClassFile API ClassCastException with verbose output of certain class files [v2]

[Paul Sandoz]

On Fri, 3 May 2024 17:13:04 GMT, Adam Sotona  wrote:

>> Specifically corrupted constant pool of a class file can cause 
>> ClassCastException, when the entries are accessed by Class-File API in exact 
>> order.
>> 
>> This fix avoids the ClassCastException and throws ConstantPoolException 
>> instead.
>> Test is attached.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   applied suggested changes

Nice!

-

Marked as reviewed by psandoz (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19088#pullrequestreview-2038729421

Re: RFR: 8331655: ClassFile API ClassCastException with verbose output of certain class files [v2]

[Adam Sotona]

> Specifically corrupted constant pool of a class file can cause 
> ClassCastException, when the entries are accessed by Class-File API in exact 
> order.
> 
> This fix avoids the ClassCastException and throws ConstantPoolException 
> instead.
> Test is attached.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  applied suggested changes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19088/files
  - new: https://git.openjdk.org/jdk/pull/19088/files/59191c6f..2e94a1b0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19088=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19088=00-01

  Stats: 15 lines in 1 file changed: 0 ins; 14 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19088.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19088/head:pull/19088

PR: https://git.openjdk.org/jdk/pull/19088

Re: RFR: 8331655: ClassFile API ClassCastException with verbose output of certain class files [v2]

[Adam Sotona]

On Fri, 3 May 2024 16:25:30 GMT, Paul Sandoz  wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   applied suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
> line 402:
> 
>> 400: int tag = readU1(offset);
>> 401: final int q = offset + 1;
>> 402: if (tag == TAG_UTF8) {
> 
> Can we call into the tag accepting entryByIndex? e.g.,
> 
> if (entryByIndex(index, TAG_UTF8) instanceof AbstractPoolEntry.Utf8EntryImpl 
> utf8) {
>   return ...
> }
> throw new ...
> 
> ?

Yes, it is a good opportunity to reduce the code a bit.
Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19088#discussion_r1589483133

Re: RFR: 8331535: Incorrect prompt for Console.readLine

[Naoto Sato]

On Fri, 3 May 2024 11:12:48 GMT, Pavel Rappo  wrote:

>> 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.
>
> test/jdk/jdk/internal/jline/JLineConsoleProviderTest.java line 27:
> 
>> 25:  * @test
>> 26:  * @bug 8331535
>> 27:  * @summary Verify the jdk.internal.le's console provider works properly.
> 
> There's a hidden assumption in this test below that the _default_ console is 
> `jdk.internal.le`. Is there any way to assert that?

Or maybe we could explicitly specify `-Djdk.console=jdk.internal.le` to the 
test.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r158945

Integrated: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory

[Justin Lu]

On Wed, 1 May 2024 16:10:06 GMT, Justin Lu  wrote:

> Please review this PR which converts _TestEncodingDecodingLength.java_ into a 
> whitebox test which allows for testing to be done without memory usage issues.
> 
> Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte 
> arrays worth of memory. (2 for the initial array allocation, .75 for the 
> target array in `decode()`). While the `-Xms6g -Xmx8g` options should address 
> this, there have been intermittent memory issues, as the underlying machine 
> machine may be running other tests simultaneously.
> 
> By converting this test to a white-box test not only does it get rid of 
> memory issues, but it also gets rid of the need to decode 2GB of data 3 
> times. The change is done using reflection to test the private visibility 
> methods `encodedOutLength` and `decodedOutLength`, which the public `encode` 
> and `decode` overloaded methods call respectively.

This pull request has now been integrated.

Changeset: b33096f8
Author:Justin Lu 
URL:   
https://git.openjdk.org/jdk/commit/b33096f887108c3d7e1f4e62689c2b10401234fa
Stats: 81 lines in 1 file changed: 44 ins; 4 del; 33 mod

8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory

Reviewed-by: lancea, naoto

-

PR: https://git.openjdk.org/jdk/pull/19036

Re: RFR: 8331655: ClassFile API ClassCastException with verbose output of certain class files

[Paul Sandoz]

On Fri, 3 May 2024 15:28:05 GMT, Adam Sotona  wrote:

> Specifically corrupted constant pool of a class file can cause 
> ClassCastException, when the entries are accessed by Class-File API in exact 
> order.
> 
> This fix avoids the ClassCastException and throws ConstantPoolException 
> instead.
> Test is attached.
> 
> Please review.
> 
> Thanks,
> Adam

src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
line 402:

> 400: int tag = readU1(offset);
> 401: final int q = offset + 1;
> 402: if (tag == TAG_UTF8) {

Can we call into the tag accepting entryByIndex? e.g.,

if (entryByIndex(index, TAG_UTF8) instanceof AbstractPoolEntry.Utf8EntryImpl 
utf8) {
  return ...
}
throw new ...

?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19088#discussion_r1589423345

Integrated: 8331582: Incorrect default Console provider comment

[Naoto Sato]

On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato  wrote:

> Fixing a comment in `java.io.Console`, which was a leftover from the fix to 
> https://bugs.openjdk.org/browse/JDK-8308591

This pull request has now been integrated.

Changeset: cf2c80e4
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/cf2c80e4fcd74b9d1d60e2358e7883bdd8a4ac80
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod

8331582: Incorrect default Console provider comment

Reviewed-by: joehw, jlahoda, jlu, prappo

-

PR: https://git.openjdk.org/jdk/pull/19070

Re: RFR: 8331582: Incorrect default Console provider comment

[Naoto Sato]

On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato  wrote:

> Fixing a comment in `java.io.Console`, which was a leftover from the fix to 
> https://bugs.openjdk.org/browse/JDK-8308591

Thank you for the reviews!

-

PR Comment: https://git.openjdk.org/jdk/pull/19070#issuecomment-2093318995

Re: RFR: 8331582: Incorrect default Console provider comment [v2]

[Naoto Sato]

> Fixing a comment in `java.io.Console`, which was a leftover from the fix to 
> https://bugs.openjdk.org/browse/JDK-8308591

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Update src/java.base/share/classes/java/io/Console.java
  
  Thanks. That reads better
  
  Co-authored-by: Pavel Rappo <32523691+pavelra...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19070/files
  - new: https://git.openjdk.org/jdk/pull/19070/files/2ab4fdf9..ea465ee2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19070=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19070=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/19070.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19070/head:pull/19070

PR: https://git.openjdk.org/jdk/pull/19070

Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]

[Severin Gehwolf]

On Fri, 3 May 2024 15:58:11 GMT, Severin Gehwolf  wrote:

>> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 375:
>> 
>>> 373: if (!c.isContainerized()) {
>>> 374: ostream.println(INDENT + "System not containerized.");
>>> 375: return;
>> 
>> Why return here? Would this not cut the output short in the 
>> non-containerized case?
>> 
>> And if this not intended, the not-containerized-`-XshowSettings:system` test 
>> below should test and catch this (e.g. scan for CPU set)
>
>> Why return here?
> 
> Because it's not useful to see containerized settings (other than the cg 
> version in use) after this patch. The JVM won't use them (uses the physical 
> settings instead). Why would you want to show the settings?

To clarify. `showSettings:system` output on a host system:


Operating System Metrics:
Provider: cgroupv1
System not containerized.
openjdk 23-internal 2024-09-17
OpenJDK Runtime Environment (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk)
OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, 
mixed mode, sharing)


... and in a container (with memory limit 500m):


Operating System Metrics:
Provider: cgroupv1
Effective CPU Count: 12
CPU Period: 10us
CPU Quota: -1
CPU Shares: -1
List of Processors, 12 total: 
0 1 2 3 4 5 6 7 8 9 10 11 
List of Effective Processors, 12 total: 
0 1 2 3 4 5 6 7 8 9 10 11 
List of Memory Nodes, 1 total: 
0 
List of Available Memory Nodes, 1 total: 
0 
Memory Limit: 500.00M
Memory Soft Limit: Unlimited
Memory & Swap Limit: 500.00M
Maximum Processes Limit: 2048

openjdk 23-internal 2024-09-17
OpenJDK Runtime Environment (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk)
OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, 
mixed mode, sharing)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589407238

Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container

[Severin Gehwolf]

On Mon, 22 Apr 2024 13:56:23 GMT, Jan Kratochvil  
wrote:

> Anyway in this patch one could unify naming across variables/parameters, the 
> same value is called `_is_ro`, `is_read_only`, `ro_opt`, `read_only`, `ro`.

I've tried to unify the naming a bit. Thanks for the review!

-

PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2093300919

Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v3]

[Severin Gehwolf]

> Please review this enhancement to the container detection code which allows 
> it to figure out whether the JVM is actually running inside a container 
> (`podman`, `docker`, `crio`), or with some other means that enforces 
> memory/cpu limits by means of the cgroup filesystem. If neither of those 
> conditions hold, the JVM runs in not containerized mode, addressing the issue 
> described in the JBS tracker. For example, on my Linux system 
> `is_containerized() == false" is being indicated with the following trace log 
> line:
> 
> 
> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false 
> because no cpu or memory limit is present
> 
> 
> This state is being exposed by the Java `Metrics` API class using the new 
> (still JDK internal) `isContainerized()` method. Example:
> 
> 
> java -XshowSettings:system --version
> Operating System Metrics:
> Provider: cgroupv1
> System not containerized.
> openjdk 23-internal 2024-09-17
> OpenJDK Runtime Environment (fastdebug build 
> 23-internal-adhoc.sgehwolf.jdk-jdk)
> OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, 
> mixed mode, sharing)
> 
> 
> The basic property this is being built on is the observation that the cgroup 
> controllers typically get mounted read only into containers. Note that the 
> current container tests assert that `OSContainer::is_containerized() == true` 
> in various tests. Therefore, using the heuristic of "is any memory or cpu 
> limit present" isn't sufficient. I had considered that in an earlier 
> iteration, but many container tests failed.
> 
> Overall, I think, with this patch we improve the current situation of 
> claiming a containerized system being present when it's actually just a 
> regular Linux system.
> 
> Testing:
> 
> - [x] GHA (risc-v failure seems infra related)
> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including 
> gtests)
> - [x] Some manual testing using cri-o
> 
> Thoughts?

Severin Gehwolf has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 12 additional commits 
since the last revision:

 - Add doc for mountinfo scanning.
 - Unify naming of variables
 - Merge branch 'master' into jdk-8261242-is-containerized-fix
 - Merge branch 'master' into jdk-8261242-is-containerized-fix
 - jcheck fixes
 - Fix tests
 - Implement Metrics.isContainerized()
 - Some clean-up
 - Drop cgroups testing on plain Linux
 - Implement fall-back logic for non-ro controller mounts
 - ... and 2 more: https://git.openjdk.org/jdk/compare/06fa7bd3...434430ca

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18201/files
  - new: https://git.openjdk.org/jdk/pull/18201/files/0df26ebd..434430ca

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18201=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=18201=01-02

  Stats: 82529 lines in 2377 files changed: 37138 ins; 34932 del; 10459 mod
  Patch: https://git.openjdk.org/jdk/pull/18201.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201

PR: https://git.openjdk.org/jdk/pull/18201

Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]

[Severin Gehwolf]

On Tue, 16 Apr 2024 18:21:29 GMT, Thomas Stuefe  wrote:

> Why return here?

Because it's not useful to see containerized settings (other than the cg 
version in use) after this patch. The JVM won't use them (uses the physical 
settings instead). Why would you want to show the settings?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589396352

Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]

[Severin Gehwolf]

On Tue, 16 Apr 2024 18:10:08 GMT, Thomas Stuefe  wrote:

>> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 351:
>> 
>>> 349: //
>>> 350: // We collect the read only mount option in the cgroup infos so as 
>>> to have that
>>> 351: // info ready when determining is_containerized().
>> 
>> Here, and in other places: a comment indicating the line format we scan 
>> would be appreciated, possibly with argument numbers. Saves the casual code 
>> reader from looking into proc man page. Even just pasting the example line 
>> for proc manpage would be fine 
>> (https://man7.org/linux/man-pages/man5/proc.5.html) (but with order adapted 
>> to your scanf call, they count major:minor as one)
>
> Trying to parse the `%s%*[^-]-`
> 
> So, %s parses the mount options, until we encounter whitespace. Then %*[^-]- 
> parses everything that is not a dash, until we encounter the dash? Then we 
> eat the dash? This is to skip the optionals?

Correct. Note that `%s %*[^-]` doesn't work for files without optionals. Since 
`%*[^-]` requires a non-empty match and the optionals are, well, optional :-) 
I've added more verbose comments to clarify this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589390841

Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]

[Severin Gehwolf]

On Tue, 16 Apr 2024 18:16:33 GMT, Thomas Stuefe  wrote:

>> Severin Gehwolf 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 ten additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8261242-is-containerized-fix
>>  - jcheck fixes
>>  - Fix tests
>>  - Implement Metrics.isContainerized()
>>  - Some clean-up
>>  - Drop cgroups testing on plain Linux
>>  - Implement fall-back logic for non-ro controller mounts
>>  - Make find_ro static and local to compilation unit
>>  - 8261242: [Linux] OSContainer::is_containerized() returns true
>
> src/hotspot/os/linux/osContainer_linux.cpp line 78:
> 
>> 76:   const char *reason;
>> 77:   bool any_mem_cpu_limit_present = false;
>> 78:   bool ctrl_ro = cgroup_subsystem->is_containerized();
> 
> nit: naming? what does ctrl mean in this case? Maybe use 
> "cgroup_is_containerized"?

`ctrl` was short for `controller`. I've changed the naming.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1589391426

RFR: 8331655: ClassFile API ClassCastException with verbose output of certain class files

[Adam Sotona]

Specifically corrupted constant pool of a class file can cause 
ClassCastException, when the entries are accessed by Class-File API in exact 
order.

This fix avoids the ClassCastException and throws ConstantPoolException instead.
Test is attached.

Please review.

Thanks,
Adam

-

Commit messages:
 - 8331655: ClassFile API ClassCastException with verbose output of certain 
class files

Changes: https://git.openjdk.org/jdk/pull/19088/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19088=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331655
  Stats: 19 lines in 2 files changed: 12 ins; 0 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/19088.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19088/head:pull/19088

PR: https://git.openjdk.org/jdk/pull/19088

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v2]

[Joachim Kern]

> 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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19000/files
  - new: https://git.openjdk.org/jdk/pull/19000/files/d2fac20e..caf806b3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19000=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19000=00-01

  Stats: 9 lines in 3 files changed: 5 ins; 2 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: In support of Instant.minus(Instant)

[Roger Riggs]

Hi,

I would also reinforce Stephen's early observation that the pattern for 
"until" methods in java.time includes those of the XXXDate classes, with 
a single Temporal parameter.  Period and Duration are similar values 
holding relative TemporalAmounts.


    public Period until(ChronoLocalDate endDateExclusive)

In addition to Instant, the LocalTime class might also benefit from adding:

public Duration until(LocalTime endExclusive)`

The API design of java.time included an emphasis on consistent naming 
across the packages.


Regards, Roger


On 5/2/24 4:01 PM, Naoto Sato wrote:
`Temporal` interface is clear that its `minus` methods return objects 
of the same `Temporal` type, and `until` calculates the amount of time 
until another `Temporal` type. Introducing `Instant.minus` that 
returns `Duration` would be confusing to me.


Naoto

On 5/2/24 10:41 AM, Éamonn McManus wrote:
I'd say too that this makes intuitive sense based on algebra. If we 
have:

/instant1/ + /duration/ = /instant2/
then we can subtract /duration/ from both sides:
/instant1 = instant2 - duration/
or we can subtract /instant1/ from both sides:
/duration = instant2 - instant1/

There's no manipulation we can do that would cause us to try to add 
instants together, and it's a bit surprising for the API to allow the 
first subtraction but not the second.
I also think that if I see instant2.minus(instant1) it's immediately 
obvious to me what that means, while instant1.until(instant2) seems 
both less discoverable and less obvious.


On Thu, 2 May 2024 at 10:29, Louis Wasserman > wrote:


    That doesn't follow for me at all.

    The structure formed by Instants and Durations is an affine space
, with
    instants the points and durations the vectors.  (An affine space is
    a vector space without a distinguished origin, which of course
    Instants don't have.)  It is 100% standard to use the minus sign for
    the operation "point - point = vector," even when "point + point" is
    not defined, and to use all the other standard idioms for
    subtraction; the Wikipedia article uses "subtraction" and
    "difference" ubiquitously.

    Personally, I'd be willing to live with a different name for the
    operation, but consider "users keep getting it wrong" a strong
    enough argument all by itself for a version with the swapped
    argument order; it's not obvious to me that another API with the
    same argument order adds enough value over Duration.between to
    bother with.

    On Thu, May 2, 2024 at 10:04 AM Stephen Colebourne
    mailto:scolebou...@joda.org>> wrote:

    On Thu, 2 May 2024 at 15:58, Kurt Alfred Kluever mailto:k...@google.com>> wrote:
 > instant − instant = duration // what we're discussing
 > instant + duration = instant // satisfied by
    instant.plus(duration)
 > instant - duration = instant // satisfied by
    instant.minus(duration)
 > duration + duration = duration // satisfied by
    duration.plus(duration)
 > duration - duration = duration // satisfied by
    duration.minus(duration)
 > duration × real number = duration // satisfied by
    duration.multipliedBy(long)
 > duration ÷ real number = duration // satisfied by
    duration.dividedBy(long)
 >
 > All but the first operation have very clear translations from
    conceptual model to code. I'm hoping we can achieve the same
    clarity for instant - instant by using the obvious name:
    instant.minus(instant)

    But you can't have
  instant + instant = ???
    It doesn't make sense.

    This is at the heart of why minus isn't right in this case.
    Stephen



    --     Louis Wasserman (he/they)

Re: RFR: 8331582: Incorrect default Console provider comment

[Pavel Rappo]

On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato  wrote:

> Fixing a comment in `java.io.Console`, which was a leftover from the fix to 
> https://bugs.openjdk.org/browse/JDK-8308591

Looks good.

src/java.base/share/classes/java/io/Console.java line 409:

> 407:  * The JdkConsole provider used for Console instantiation 
> can be specified
> 408:  * with the system property "jdk.console", whose value 
> designates the module
> 409:  * name of the implementation, and which defaults to the 
> value

Might read slightly better:
Suggestion:

 * name of the implementation, and which defaults to the value of

-

Marked as reviewed by prappo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19070#pullrequestreview-2037983570
PR Review Comment: https://git.openjdk.org/jdk/pull/19070#discussion_r1589103639

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131

[Matthias Baesken]

On Fri, 3 May 2024 11:31:35 GMT, Alan Bateman  wrote:

> 
> I was busy with other things and missed JoKern65's comment. I would be less 
> risky to limit this to AIX for now.

Thanks for your comment;  we see the issue only on AIX so it makes sense to 
limit it to this platform.

-

PR Comment: https://git.openjdk.org/jdk/pull/19000#issuecomment-2092825223

Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131

[Alan Bateman]

On Fri, 3 May 2024 08:15:32 GMT, Matthias Baesken  wrote:

> The naming GetApplicationHomeFromLD_LIBRARY_PATH looks a bit unconventional ; 
> maybe adjust this ? Regarding if the code should be added for all platforms 
> or just AIX, let's hear what Alan and others have to say.

I was busy with other things and missed JoKern65's comment. I would be less 
risky to limit this to AIX for now.

-

PR Comment: https://git.openjdk.org/jdk/pull/19000#issuecomment-2092820489

Re: RFR: 8331535: Incorrect prompt for Console.readLine

[Pavel Rappo]

On Fri, 3 May 2024 10:11:02 GMT, Jan Lahoda  wrote:

> 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.

src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
 line 101:

> 99: try {
> 100: initJLineIfNeeded();
> 101: return jline.readLine(fmt.formatted(args).replace("%", 
> "%%"));

I understand that [JLine interprets `%` in a 
prompt](https://github.com/openjdk/jdk/blob/4ed38f5ad5f822ab948257ed39717ea919fd32ed/src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/impl/LineReaderImpl.java#L4050),
 but are the interpretation rules documented on JLine GitHub page or elsewhere?

src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
 line 116:

> 114: try {
> 115: initJLineIfNeeded();
> 116: return jline.readLine(fmt.formatted(args).replace("%", 
> "%%"), '\0')

I find `'\0'` more cryptic and confusing than `(char) 0`, but okay. (Had to 
re-read 
https://docs.oracle.com/javase/specs/jls/se22/html/jls-3.html#jls-EscapeSequence)

test/jdk/jdk/internal/jline/JLineConsoleProviderTest.java line 27:

> 25:  * @test
> 26:  * @bug 8331535
> 27:  * @summary Verify the jdk.internal.le's console provider works properly.

There's a hidden assumption in this test below that the _default_ console is 
`jdk.internal.le`. Is there any way to assert that?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589074603
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589065719
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589067927

Integrated: 8323707: Adjust Classfile API's type arg model to better represent the embodied type

[Chen Liang]

On Mon, 6 Nov 2023 07:30:41 GMT, Chen Liang  wrote:

> API changes as discussed on the mailing list: 
> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-November/000419.html
> 
> Additional questions:
> 1. Whether to rename `WildcardIndicator.DEFAULT` to `NONE`

This pull request has now been integrated.

Changeset: c60474b1
Author:Chen Liang 
Committer: Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/c60474b1229b67265acbd709f6ba081303329be4
Stats: 137 lines in 6 files changed: 54 ins; 32 del; 51 mod

8323707: Adjust Classfile API's type arg model to better represent the embodied 
type

Reviewed-by: asotona

-

PR: https://git.openjdk.org/jdk/pull/16517

Re: RFR: 8182774: Verify code in javap [v2]

[Maurizio Cimadamore]

On Thu, 2 May 2024 12:22:23 GMT, Adam Sotona  wrote:

>> This patch adds `javap -verify` option to check the class and print obvious 
>> verification errors found.
>> Implementation depends on extended Class-File API verification support, so 
>> PR #16809 is important to precede.
>> The new `javap` option is mentioned in man pages and a release note will be 
>> provided.
>> 
>> Please review.
>> 
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixed typo

Looks great - this was a long-awaited enhancement!

-

Marked as reviewed by mcimadamore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18629#pullrequestreview-2037858606

RFR: 8331535: Incorrect prompt for Console.readLine

[Jan Lahoda]

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.

-

Depends on: https://git.openjdk.org/jdk/pull/18996

Commit messages:
 - 8331535: Incorrect prompt for Console.readLine

Changes: https://git.openjdk.org/jdk/pull/19081/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19081=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331535
  Stats: 106 lines in 2 files changed: 104 ins; 0 del; 2 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

RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent

[Justin Lu]

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.

-

Commit messages:
 - init

Changes: https://git.openjdk.org/jdk/pull/19075/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19075=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331485
  Stats: 165 lines in 2 files changed: 159 ins; 1 del; 5 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: 8329653: JLILaunchTest fails on AIX after JDK-8329131

[Matthias Baesken]

On Mon, 29 Apr 2024 14:45:07 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.

The naming GetApplicationHomeFromLD_LIBRARY_PATH looks a bit unconventional ; 
maybe adjust this ?
Regarding if  the code should be added for all platforms or just AIX, let's 
hear what Alan and others have to say.

On AIX we have the bad situation that  GetApplicationHomeFromDll  stopped 
working after JDK-8329131.

-

PR Comment: https://git.openjdk.org/jdk/pull/19000#issuecomment-2092530647

Re: RFR: 8331582: Incorrect default Console provider comment

[Justin Lu]

On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato  wrote:

> Fixing a comment in `java.io.Console`, which was a leftover from the fix to 
> https://bugs.openjdk.org/browse/JDK-8308591

Marked as reviewed by jlu (Committer).

-

PR Review: https://git.openjdk.org/jdk/pull/19070#pullrequestreview-2037632558

Re: RFR: 8331582: Incorrect default Console provider comment

[Jan Lahoda]

On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato  wrote:

> Fixing a comment in `java.io.Console`, which was a leftover from the fix to 
> https://bugs.openjdk.org/browse/JDK-8308591

Looks good to me, thanks!

-

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19070#pullrequestreview-2037481621