Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Dean Long]

On Thu, 5 Nov 2020 23:58:21 GMT, Dean Long  wrote:

>> Roland Westrelin has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains ten commits:
>> 
>>  - Jorn's comments
>>  - Update headers and add intrinsic to Graal test ignore list
>>  - move compiler test and add bug to test
>>  - non x86_64 arch support
>>  - c2 test case
>>  - intrinsic
>>  - Use overloads of method names.
>>
>>Simplify internally to avoid overload resolution
>>issues, leverging List for the exception
>>mapper.
>>  - Vladimir's comments
>>  - checkLongIndex
>
> src/hotspot/share/opto/library_call.cpp line 1015:
> 
>> 1013:   Deoptimization::Action_make_not_entrant);
>> 1014:   }
>> 1015: 
> 
> A comment here explaining what the code below is doing would be helpful.

This code wasn't here before, so I'm guessing it's needed for T_LONG.  For 
T_INT is it just wasted work?

-

PR: https://git.openjdk.java.net/jdk/pull/1003

Re: RFR: 8180352: Add Stream.toList() method

[Stuart Marks]

On Fri, 6 Nov 2020 03:01:42 GMT, Stuart Marks  wrote:

>> Looking at the linked issue, I see [this comment from Rémi 
>> Forax](https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14171626=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14171626):
>> 
>>> Please talk with Brian about this change because it nulls a property of the 
>>> Stream API we (the lambda-util group) have take time to keep.
>> The whole Stream API doesn't depends on the Collection API, [...] so the 
>> Stream API can be easily integrated with other collection API if in the 
>> future we want by example add a persistent collection API with no link with 
>> java.util.List.
>> 
>> That's an argument I can understand – as is, the Stream API works just as 
>> well with collections from e.g. Scala's or Kotlin's (or Ceylon's) collection 
>> libraries instead of the java.util ones, just using different collectors. 
>> Adding a method which directly returns a java.util.List somewhat couples it 
>> to the Java Collection API.
>> 
>> Now this was mentioned two and a half year ago. Did something change which 
>> made this consideration irrelevant? I would expect at least some mention of 
>> it in the discussion here.
>
> @ePaul wrote:
> 
>> The Stream API works just as well with [third party] collection libraries 
>> instead of the java.util ones, just using different collectors. Adding a 
>> method which directly returns a java.util.List somewhat couples it to the 
>> Java Collection API.
>> 
>> Now this was mentioned two and a half year ago. Did something change which 
>> made this consideration irrelevant? I would expect at least some mention of 
>> it in the discussion here.
> 
> The separation between streams and the java.util Collections Framework is a 
> good design principle, but it isn't an ironclad rule. It's still easy to have 
> streams create instances of other collections libraries using the Collector 
> interface. What's different here is that the Collections Framework has 
> "leaked into" streams a little bit more, so that they're now more 
> interdependent. This doesn't seem to have any disadvantages; it seems 
> unlikely that the Collections Framework will ever be unplugged from the JDK. 
> However, the benefits are that a List is the closest thing we have to an 
> unmodifiable array that also plays well with generics and that can be 
> value-based; these benefits are considerable.

> Simon Roberts wrote:

> This discussion of unmodifiable lists brings me back to the thought that
> there would be good client-side reasons for inserting an UnmodifiableList
> interface as a parent of LIst, not least because all our unmodifiable
> variants from the Collections.unmodifiableList proxy onward fail the Liskov
> substitution test for actually "being contract-fulfilling Lists".

At some point there probably will need to be a long article explaining all the 
issues here, but at the moment the best writeup I have is this one:

https://stackoverflow.com/a/57926310/1441122

TL;DR there are a few different ways to approach retrofitting something like 
this, but they all have enough compromises that the net benefits are unclear.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

[Stuart Marks]

On Wed, 4 Nov 2020 23:18:29 GMT, Paŭlo Ebermann 
 wrote:

>> Changes requested by orio...@github.com (no known OpenJDK username).
>
> Looking at the linked issue, I see [this comment from Rémi 
> Forax](https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14171626=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14171626):
> 
>> Please talk with Brian about this change because it nulls a property of the 
>> Stream API we (the lambda-util group) have take time to keep.
> The whole Stream API doesn't depends on the Collection API, [...] so the 
> Stream API can be easily integrated with other collection API if in the 
> future we want by example add a persistent collection API with no link with 
> java.util.List.
> 
> That's an argument I can understand – as is, the Stream API works just as 
> well with collections from e.g. Scala's or Kotlin's (or Ceylon's) collection 
> libraries instead of the java.util ones, just using different collectors. 
> Adding a method which directly returns a java.util.List somewhat couples it 
> to the Java Collection API.
> 
> Now this was mentioned two and a half year ago. Did something change which 
> made this consideration irrelevant? I would expect at least some mention of 
> it in the discussion here.

@ePaul wrote:

> The Stream API works just as well with [third party] collection libraries 
> instead of the java.util ones, just using different collectors. Adding a 
> method which directly returns a java.util.List somewhat couples it to the 
> Java Collection API.
> 
> Now this was mentioned two and a half year ago. Did something change which 
> made this consideration irrelevant? I would expect at least some mention of 
> it in the discussion here.

The separation between streams and the java.util Collections Framework is a 
good design principle, but it isn't an ironclad rule. It's still easy to have 
streams create instances of other collections libraries using the Collector 
interface. What's different here is that the Collections Framework has "leaked 
into" streams a little bit more, so that they're now more interdependent. This 
doesn't seem to have any disadvantages; it seems unlikely that the Collections 
Framework will ever be unplugged from the JDK. However, the benefits are that a 
List is the closest thing we have to an unmodifiable array that also plays well 
with generics and that can be value-based; these benefits are considerable.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8247781: Day periods support [v7]

[Naoto Sato]

On Thu, 5 Nov 2020 23:25:38 GMT, Stephen Colebourne  
wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed typo/grammatical error.
>
> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line 
> 858:
> 
>> 856: return new Object[][]{
>> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
>> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
> 
> As mentioned in my other comment, this seems odd in STRICT mode.

Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in 
SMART/LENIENT modes?

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8180352: Add Stream.toList() method

[Paul Sandoz]

On Fri, 6 Nov 2020 02:50:29 GMT, Stuart Marks  wrote:

>> test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToListOpTest.java
>>  line 73:
>> 
>>> 71: }
>>> 72: 
>>> 73: @Test(dataProvider = "withNull:StreamTestData", 
>>> dataProviderClass = StreamTestDataProvider.class)
>> 
>> Given the non-default `toList()` implementation defers to the `toArray()` 
>> terminal op there is no need for this and the following tests, which are 
>> really designed to shake out the optimizations when producing and operating 
>> on arrays.
>
> OK, I'll remove all the tests starting from here to the end of the file. I'm 
> assuming that's the set of tests you're referring to.

Yes from line 73 to G (the end).

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

[Stuart Marks]

On Thu, 5 Nov 2020 19:47:43 GMT, Paul Sandoz  wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToListOpTest.java
>  line 73:
> 
>> 71: }
>> 72: 
>> 73: @Test(dataProvider = "withNull:StreamTestData", 
>> dataProviderClass = StreamTestDataProvider.class)
> 
> Given the non-default `toList()` implementation defers to the `toArray()` 
> terminal op there is no need for this and the following tests, which are 
> really designed to shake out the optimizations when producing and operating 
> on arrays.

OK, I'll remove all the tests starting from here to the end of the file. I'm 
assuming that's the set of tests you're referring to.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: Filtered XMLStreamReader Exceptions (java.xml)

[Joe Wang]

Hi Mike,

I saw that you are on the OCA list but not census 
, I therefore assumed you'd need a 
sponsor, or would you?  Let me know if I'm mistaken. If you're an 
openjdk author/committer and have a JBS account, please go ahead take 
over the bug and create a CSR. Creating a CSR is straight-forward: from 
the JBS bug, click more -> create CSR and follow the template.


You mentioned you're going to submit a PR. Are you familiar with the 
Skara process or have you already done it?


Thanks,
Joe

On 11/5/20 1:35 PM, Michael Edgar wrote:
The bug for this issue was accepted: JDK-8255918. I have made the 
change that I suggested in my original email and tested, but have not 
yet opened a pull request. Please let me know what (if anything) needs 
to occur for the CSR process due to the method signature change (added 
`throws`).


Thank you,
Mike


On Wed, Oct 28, 2020 at 12:52 PM Joe Wang > wrote:


Hi Mike,

As you said, creating a bug report would be a good start. If it
involves
a signature change, it'd need to go through a proper review (CSR)
process.

When you are ready to submit a bug report, please make sure to add a
test case to illustrate the use case scenario.

Thanks,
Joe

On 10/28/20 5:14 AM, Michael Edgar wrote:
> Hi everyone,
> I'm working on a project that makes use of the StAX API and an
issue I have
> encountered is that when wrapping an `XMLStreamReader` with a
> `StreamFilter`, errors encountered in the setup are not thrown
to the
> caller. The source of the error could be any stream error that
is triggered
> as the `XMLStreamFilterImpl` advances to the next acceptable event.
> Ultimately, when attempting to utilize the filtered reader, some
secondary
> exception will occur, but the original `Exception` is lost.
>
> I have not seen any other issues related specifically to this
problem, so I
> would like to propose removal of the try/catch in the constructor of
> `com.sun.org.apache.xerces.internal.impl.XMLStreamFilterImpl`
and the
> method signature changed to declare that `XMLStreamException` is
thrown.
> The constructor is used by
>

`com.sun.xml.internal.stream.XMLInputFactoryImpl.createFilteredReader(XMLStreamReader,
> StreamFilter)` which itself already declares the same exception
and is an
> implementation of the public `XMLInputFactory` interface.
>
> Further, the `nextTag` method of the same class has a bug where
it checks
> for `START_ELEMENT` events twice.
>
> I have an OCA in place and I am happy to submit a PR, but I
believe that a
> bug record needs to be opened in order to proceed.
>
> Thank you,
> Mike

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[Hui Shi]

On Thu, 5 Nov 2020 14:59:56 GMT, Alan Bateman  wrote:

> If we are changing NativeMethodAccessorImpl.invoke then we should probably do 
> NativeConstructorAccessorImpl.newInstance at the same time. 

Yes, NativeConstructorAccessorImpl should also apply this change.

> Also wondering if we should, while in the area, add "return acc.invoke(obj, 
> args)" after setting the delegate so that it invokes the newly generated 
> accessor.
> 

Agree,  I see no harm to invoke generated method when it is aviable. 
Should leave this to another patch?

> Are there resource or other cases that you have observed where generateMethod 
> fails and then succeeds in a subsequent call?
> 
I have not seen exception/error happen in generate method yet.  But in case it 
fails in some ways, try - catch - reset is added to make sure behavior is same 
before/after with this change.

> @cl4es Do you know of any startup tests that might be sensitive to the eager 
> creating of a VarHandle?
> 
> I agree with @shipilev to test before the CAS.

@AlanBateman 
Thanks for you comments!

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Dean Long]

On Thu, 5 Nov 2020 15:47:31 GMT, Roland Westrelin  wrote:

>> This change add 3 new methods in Objects:
>> 
>> public static long checkIndex(long index, long length)
>> public static long checkFromToIndex(long fromIndex, long toIndex, long 
>> length)
>> public static long checkFromIndexSize(long fromIndex, long size, long length)
>> 
>> This mirrors the int utility methods that were added by JDK-8135248
>> with the same motivations.
>> 
>> As is the case with the int checkIndex(), the long checkIndex() method
>> is JIT compiled as an intrinsic. It allows the JIT to compile
>> checkIndex to an unsigned comparison and properly recognize it as
>> a range check that then becomes a candidate for the existing range check
>> optimizations. This has proven to be important for panama's
>> MemorySegment API and a prototype of this change (with some extra c2
>> improvements) showed that panama micro benchmark results improve
>> significantly.
>> 
>> This change includes:
>> 
>> - the API change
>> - the C2 intrinsic
>> - tests for the API and the C2 intrinsic
>> 
>> This is a joint work with Paul who reviewed and reworked the API change
>> and filled the CSR.
>
> Roland Westrelin has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains ten commits:
> 
>  - Jorn's comments
>  - Update headers and add intrinsic to Graal test ignore list
>  - move compiler test and add bug to test
>  - non x86_64 arch support
>  - c2 test case
>  - intrinsic
>  - Use overloads of method names.
>
>Simplify internally to avoid overload resolution
>issues, leverging List for the exception
>mapper.
>  - Vladimir's comments
>  - checkLongIndex

C2 changes look good, except for new code block in 
inline_preconditions_checkIndex could use a comment.

src/hotspot/share/opto/library_call.cpp line 1015:

> 1013:   Deoptimization::Action_make_not_entrant);
> 1014:   }
> 1015: 

A comment here explaining what the code below is doing would be helpful.

-

Changes requested by dlong (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1003

Re: RFR: 8247781: Day periods support [v7]

[Stephen Colebourne]

On Thu, 5 Nov 2020 17:12:11 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed typo/grammatical error.

test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line 858:

> 856: return new Object[][]{
> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
> 858: {STRICT, 1, LocalTime.of(18, 0), 1},

As mentioned in my other comment, this seems odd in STRICT mode.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5055:

> 5053: @Override
> 5054: public boolean format(DateTimePrintContext context, 
> StringBuilder buf) {
> 5055: Long value = context.getValue(MINUTE_OF_DAY);

This does not match the spec: " During formatting, the day period is obtained 
from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"

It is possible and legal to create a Temporal that returns `HOUR_OF_DAY` and 
`MINUTE_OF_HOUR` but not `MINUTE_OF_DAY`. As such, this method must be changed 
to follow the spec.

-

In addition, it is possible for `HOUR_OF_DAY` and `MINUTE_OF_HOUR` to be 
outside their normal bounds. The right behaviour would be to combine the two 
fields within this method, and then use mod to get the value into the range 0 
to 1440 before calling `dayPeriod.include`. (While the fall back behaviour 
below does cover this, it would be better to do what I propose here.)

An example of this is a `TransportTime` class where the day runs from 03:00 to 
27:00 each day (because trains run after midnight for no extra cost to the 
passenger, and it is more convenient for the operator to treat the date that 
way). A `TransportTime` of 26:30 should still resolve to "night1" rather than 
fall back to "am".

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8247781: Day periods support [v7]

[Stephen Colebourne]

On Mon, 2 Nov 2020 23:21:22 GMT, Naoto Sato  wrote:

>> Pulling on this a little more.
>> 
>> As the PR stands, it seems that if the user passes in text with just a 
>> day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in 
>> `AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? 
>> If so, I don't think this is sustainable.
>> 
>> Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of 
>> "am" or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then 
>> dayPeriod can silently take precedence
>
> I implemented what you suggested here in the latest PR, but that would be a 
> behavioral change which requires a CSR, as "AM" would be resolved to 06:00 
> which was not before. Do you think it would be acceptable? If so, I will 
> reopen the CSR and describe the change. (In fact some TCK failed with this 
> impl)

I find the whole "half way between the start and end" behaviour of day periods 
odd anyway, but if it is to be supported then it should be consistent as you've 
implemented. 

Another option I should have thought of earlier would be to simply not support 
the "half way between the start and end" behaviour of LDML in either dayPeriod 
or AM/PM. But since LDML defines it, you've implemented it, and it isn't overly 
harmful I think its OK to leave it in.

Would it be possible for STRICT mode to not have the "half way between the 
start and end" behaviour?

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: JDK-8254920: Application launched with jpackage produced .exe crashes JVM [v3]

[Alexander Zuev]

On Thu, 5 Nov 2020 18:18:16 GMT, Andy Herrick  wrote:

>> JVM
>
> Andy Herrick 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8254920
>  - JDK-8254920: Application launched with jpackage produced .exe crashes JVM
>  - JDK-8254920: Application launched with jpackage produced .exe crashes
>JVM

Looks good.

-

Marked as reviewed by kizune (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/940

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[David Holmes]

On 5/11/2020 7:09 pm, Aleksey Shipilev wrote:

On Thu, 5 Nov 2020 02:52:05 GMT, Hui Shi  wrote:


…AccessorImpl object

We met real problem when using protobuf with option optimized for code size, 
detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883

Optimize solution is adding a new boolean field to detect concurrent method 
accessor generation in same NativeMethodAccessorImpl object, only one thread is 
allowed to generate accessor, other threads still invoke in jni way until 
parent's delegator is updated from NativeMethodAccessorImpl  to generated 
accessor.

In common case, extra overhead is an atomic operation, compared with method 
accessor generate, this cost is trivial.


I do wonder if it makes sense to handle triple-state `int` here: "not yet generated", 
"generated", "in error"? So that we don't try to generate the accessor over and over 
again when it is in error?


You have no idea why generation failed so don't know whether a retry 
will succeed or not. Current code will retry so it would be a change in 
behaviour to declare it permanently "in error".


Cheers,
David


src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java 
line 80:


78: succ = true;
79: } finally {
80: if (succ == false) {


Why `succ` variable, if you can just `catch (Throwable e)` and restore the 
`accessorGenerated`?

src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java 
line 66:


64: && !method.getDeclaringClass().isHidden()
65: && 
!ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())
66: && ACCESSOR_GENERATED.compareAndSet(this, false, true)) {


As the micro-optimization, checking that `accessor_generated` is `false` before 
attempting a (potentially contended) CAS might fit better. (See 
https://en.wikipedia.org/wiki/Test_and_test-and-set).

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Re: RFR: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags [v2]

[Naoto Sato]

On Thu, 5 Nov 2020 22:10:07 GMT, Brian Burkhalter  wrote:

>> Please review this change which would replace the use of a system property 
>> and a child process with updated jtreg tags.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using 
> jtreg tags

Looks good. Thanks for fixing.

-

Marked as reviewed by naoto (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1083

Re: RFR: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags [v2]

[Brian Burkhalter]

On Thu, 5 Nov 2020 21:53:40 GMT, Naoto Sato  wrote:

>> Brian Burkhalter has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using 
>> jtreg tags
>
> test/jdk/java/io/BufferedInputStream/LargeCopyWithMark.java line 29:
> 
>> 27:  * @summary BufferedInputStream calculates negative array size with large
>> 28:  *  streams and mark
>> 29:  * @library /test/lib
> 
> Looks like this (@library /test/lib) can be removed. Otherwise looks good.

Yes, you are correct: fixed. Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/1083

Re: RFR: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags [v2]

[Brian Burkhalter]

> Please review this change which would replace the use of a system property 
> and a child process with updated jtreg tags.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using 
jtreg tags

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1083/files
  - new: https://git.openjdk.java.net/jdk/pull/1083/files/bf6249eb..a3346ff9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1083=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1083=00-01

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

PR: https://git.openjdk.java.net/jdk/pull/1083

Re: RFR: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags

[Naoto Sato]

On Thu, 5 Nov 2020 19:50:14 GMT, Brian Burkhalter  wrote:

> Please review this change which would replace the use of a system property 
> and a child process with updated jtreg tags.

test/jdk/java/io/BufferedInputStream/LargeCopyWithMark.java line 29:

> 27:  * @summary BufferedInputStream calculates negative array size with large
> 28:  *  streams and mark
> 29:  * @library /test/lib

Looks like this (@library /test/lib) can be removed. Otherwise looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/1083

Re: Filtered XMLStreamReader Exceptions (java.xml)

[Michael Edgar]

The bug for this issue was accepted: JDK-8255918. I have made the change
that I suggested in my original email and tested, but have not yet opened a
pull request. Please let me know what (if anything) needs to occur for the
CSR process due to the method signature change (added `throws`).

Thank you,
Mike


On Wed, Oct 28, 2020 at 12:52 PM Joe Wang  wrote:

> Hi Mike,
>
> As you said, creating a bug report would be a good start. If it involves
> a signature change, it'd need to go through a proper review (CSR) process.
>
> When you are ready to submit a bug report, please make sure to add a
> test case to illustrate the use case scenario.
>
> Thanks,
> Joe
>
> On 10/28/20 5:14 AM, Michael Edgar wrote:
> > Hi everyone,
> > I'm working on a project that makes use of the StAX API and an issue I
> have
> > encountered is that when wrapping an `XMLStreamReader` with a
> > `StreamFilter`, errors encountered in the setup are not thrown to the
> > caller. The source of the error could be any stream error that is
> triggered
> > as the `XMLStreamFilterImpl` advances to the next acceptable event.
> > Ultimately, when attempting to utilize the filtered reader, some
> secondary
> > exception will occur, but the original `Exception` is lost.
> >
> > I have not seen any other issues related specifically to this problem,
> so I
> > would like to propose removal of the try/catch in the constructor of
> > `com.sun.org.apache.xerces.internal.impl.XMLStreamFilterImpl` and the
> > method signature changed to declare that `XMLStreamException` is thrown.
> > The constructor is used by
> >
> `com.sun.xml.internal.stream.XMLInputFactoryImpl.createFilteredReader(XMLStreamReader,
> > StreamFilter)` which itself already declares the same exception and is an
> > implementation of the public `XMLInputFactory` interface.
> >
> > Further, the `nextTag` method of the same class has a bug where it checks
> > for `START_ELEMENT` events twice.
> >
> > I have an OCA in place and I am happy to submit a PR, but I believe that
> a
> > bug record needs to be opened in order to proceed.
> >
> > Thank you,
> > Mike
>
>

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]

[Maurizio Cimadamore]

> This patch contains the changes associated with the first incubation round of 
> the foreign linker access API incubation
> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory 
> access support (see JEP 393 [2] and associated pull request [3]).
> 
> The main goal of this API is to provide a way to call native functions from 
> Java code without the need of intermediate JNI glue code. In order to do 
> this, native calls are modeled through the MethodHandle API. I suggest 
> reading the writeup [4] I put together few weeks ago, which illustrates what 
> the foreign linker support is, and how it should be used by clients.
> 
> Disclaimer: the pull request mechanism isn't great at managing *dependent* 
> reviews. For this reasons, I'm attaching a webrev which contains only the 
> differences between this PR and the memory access PR. I will be periodically 
> uploading new webrevs, as new iterations come out, to try and make the life 
> of reviewers as simple as possible.
> 
> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main architects 
> of all the hotspot changes you see here, and without their help, the foreign 
> linker support wouldn't be what it is today. As usual, a big thank to Paul 
> Sandoz, who provided many insights (often by trying the bits first hand).
> 
> Thanks
> Maurizio
> 
> Webrev:
> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev
> 
> Javadoc:
> 
> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html
> 
> Specdiff (relative to [3]):
> 
> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html
> 
> CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8254232
> 
> 
> 
> ### API Changes
> 
> The API changes are actually rather slim:
> 
> * `LibraryLookup`
>   * This class allows clients to lookup symbols in native libraries; the 
> interface is fairly simple; you can load a library by name, or absolute path, 
> and then lookup symbols on that library.
> * `FunctionDescriptor`
>   * This is an abstraction that is very similar, in spirit, to `MethodType`; 
> it is, at its core, an aggregate of memory layouts for the function 
> arguments/return type. A function descriptor is used to describe the 
> signature of a native function.
> * `CLinker`
>   * This is the real star of the show. A `CLinker` has two main methods: 
> `downcallHandle` and `upcallStub`; the first takes a native symbol (as 
> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` and 
> returns a `MethodHandle` instance which can be used to call the target native 
> symbol. The second takes an existing method handle, and a 
> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a 
> code stub allocated by the VM which acts as a trampoline from native code to 
> the user-provided method handle. This is very useful for implementing upcalls.
>* This class also contains the various layout constants that should be 
> used by clients when describing native signatures (e.g. `C_LONG` and 
> friends); these layouts contain additional ABI classfication information (in 
> the form of layout attributes) which is used by the runtime to *infer* how 
> Java arguments should be shuffled for the native call to take place.
>   * Finally, this class provides some helper functions e.g. so that clients 
> can convert Java strings into C strings and back.
> * `NativeScope`
>   * This is an helper class which allows clients to group together logically 
> related allocations; that is, rather than allocating separate memory segments 
> using separate *try-with-resource* constructs, a `NativeScope` allows clients 
> to use a _single_ block, and allocate all the required segments there. This 
> is not only an usability boost, but also a performance boost, since not all 
> allocation requests will be turned into `malloc` calls.
> * `MemorySegment`
>   * Only one method added here - namely `handoff(NativeScope)` which allows a 
> segment to be transferred onto an existing native scope.
> 
> ### Safety
> 
> The foreign linker API is intrinsically unsafe; many things can go wrong when 
> requesting a native method handle. For instance, the description of the 
> native signature might be wrong (e.g. have too many arguments) - and the 
> runtime has, in the general case, no way to detect such mismatches. For these 
> reasons, obtaining a `CLinker` instance is a *restricted* operation, which 
> can be enabled by specifying the usual JDK property 
> `-Dforeign.restricted=permit` (as it's the case for other restricted method 
> in the foreign memory API).
> 
> ### Implementation changes
> 
> The Java changes associated with `LibraryLookup` are relative 
> straightforward; the only interesting thing to note here is that library 
> loading does _not_ depend on class loaders, so `LibraryLookup` is not subject 
> to the same restrictions which apply to JNI library loading (e.g. same 
> library 

RFR: 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using jtreg tags

[Brian Burkhalter]

Please review this change which would replace the use of a system property and 
a child process with updated jtreg tags.

-

Commit messages:
 - 8255969: Improve java/io/BufferedInputStream/LargeCopyWithMark.java using 
jtreg tags

Changes: https://git.openjdk.java.net/jdk/pull/1083/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1083=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255969
  Stats: 44 lines in 1 file changed: 1 ins; 23 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1083.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1083/head:pull/1083

PR: https://git.openjdk.java.net/jdk/pull/1083

Re: RFR: 8180352: Add Stream.toList() method

[Paul Sandoz]

On Tue, 3 Nov 2020 01:33:32 GMT, Stuart Marks  wrote:

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToListOpTest.java
 line 73:

> 71: }
> 72: 
> 73: @Test(dataProvider = "withNull:StreamTestData", 
> dataProviderClass = StreamTestDataProvider.class)

Given the non-default `toList()` implementation defers to the `toArray()` 
terminal op there is no need for this and the following tests, which are really 
designed to shake out the optimizations when producing and operating on arrays.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: JDK-8255395 Implement Enhanced Pseudo-Random Number Generators (CSR)

[Bernd Eckenfels]

Hello,

Not sure if it is needed to implement a new RandumGenerator interface instead 
of extending SecureRandom, but the extensions and the discovery mechanism looks 
good.

One thing I am wondering about is if reseed() and reseed(Param) should be part 
of the new RandomGenerator interface as well.

BTW a lot of the random number Discovery and configuration could have been 
avoided when the actual implementation just would have been stronger and less 
blocking. The focus should be on the two old factory methods to make them 
return top quality, it’s hard enough to make everyone use them for the correct 
use case.

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: core-libs-dev  im Auftrag von Jim 
Laskey 
Gesendet: Thursday, November 5, 2020 2:02:24 PM
An: core-libs-dev 
Betreff: RFR: JDK-8255395 Implement Enhanced Pseudo-Random Number Generators 
(CSR)

Please review the CSR for JEP-356 Enhanced Pseudo-Random Number Generators.

Thank you.

-- Jim

CSR: https://bugs.openjdk.java.net/browse/JDK-8255395 

JBS: https://bugs.openjdk.java.net/browse/JDK-8248862 

JEP: http://openjdk.java.net/jeps/356 

Re: RFR: JDK-8254920: Application launched with jpackage produced .exe crashes JVM [v3]

[Andy Herrick]

> JVM

Andy Herrick 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 three additional commits since 
the last revision:

 - Merge branch 'master' into jdk-8254920
 - JDK-8254920: Application launched with jpackage produced .exe crashes JVM
 - JDK-8254920: Application launched with jpackage produced .exe crashes
   JVM

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/940/files
  - new: https://git.openjdk.java.net/jdk/pull/940/files/3d83b99b..247bc2c0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=940=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=940=01-02

  Stats: 14531 lines in 808 files changed: 7882 ins; 4035 del; 2614 mod
  Patch: https://git.openjdk.java.net/jdk/pull/940.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/940/head:pull/940

PR: https://git.openjdk.java.net/jdk/pull/940

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]

[Alex Buckley]

On 11/5/2020 4:45 AM, Jan Lahoda wrote:

FWIW, a javadoc generated with the current version of the patch:
http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.01/api/index.html


Allow me to draw people's attention to the PREVIEW link in the banner of 
the generated javadoc. It shows all the preview APIs in the release on 
one page. This is very helpful for understanding the surface area of a 
preview feature.


For example, with Sealed Classes being the only preview feature likely 
to target JDK 16, the PREVIEW page shows that the feature's API is 
solely about reflection. It's clear that Sealed Classes do not 
introduce, say, a java.lang.Sealed class analogous to the 
java.lang.Record class introduced by Records in JDK 14/15 (and which 
would have appeared on the PREVIEW page had it existed then).


Alex

Re: RFR: 8180352: Add Stream.toList() method

[Simon Roberts]

At the risk of a can of worms, or at least of raising something that has
long since been discussed and rejected...

This discussion of unmodifiable lists brings me back to the thought that
there would be good client-side reasons for inserting an UnmodifiableList
interface as a parent of LIst, not least because all our unmodifiable
variants from the Collections.unmodifiableList proxy onward fail the Liskov
substitution test for actually "being contract-fulfilling Lists".

Is this something that has been considered and rejected? (If so, is it easy
to point me at the discussion, as I expect I'd find it fascinating). Is it
worth considering, might it have some merit, or merely horrible
unforeseen-by-me consequences to the implementation?

Cheers,
Simon



On Thu, Nov 5, 2020 at 10:30 AM Stuart Marks 
wrote:

> On Wed, 4 Nov 2020 23:03:02 GMT, Paŭlo Ebermann  645859+ep...@openjdk.org> wrote:
>
> >> This change introduces a new terminal operation on Stream. This looks
> like a convenience method for Stream.collect(Collectors.toList()) or
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this
> method directly on Stream enables it to do what can't easily by done by a
> Collector. In particular, it allows the stream to deposit results directly
> into a destination array (even in parallel) and have this array be wrapped
> in an unmodifiable List without copying.
> >>
> >> In the past we've kept most things from the Collections Framework as
> implementations of Collector, not directly on Stream, whereas only
> fundamental things (like toArray) appear directly on Stream. This is true
> of most Collections, but it does seem that List is special. It can be a
> thin wrapper around an array; it can handle generics better than arrays;
> and unlike an array, it can be made unmodifiable (shallowly immutable); and
> it can be value-based. See John Rose's comments in the bug report:
> >>
> >>
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> >>
> >> This operation is null-tolerant, which matches the rest of Streams.
> This isn't specified, though; a general statement about null handling in
> Streams is probably warranted at some point.
> >>
> >> Finally, this method is indeed quite convenient (if the caller can deal
> with what this operation returns), as collecting into a List is the most
> common stream terminal operation.
> >
> > src/java.base/share/classes/java/util/ImmutableCollections.java line 199:
> >
> >> 197:  * safely reused as the List's internal storage, avoiding a
> defensive copy. Declared
> >> 198:  * with Object... instead of E... as the parameter type so
> that varargs calls don't
> >> 199:  * accidentally create an array of type other than Object[].
> >
> > Why would that be a problem? If the resulting list is immutable, then
> the actual array type doesn't really matter, right?
>
> It's an implementation invariant that the internal array be Object[].
> Having it be something other than Object[] can lead to subtle bugs. See
> [JDK-6260652](https://bugs.openjdk.java.net/browse/JDK-6260652) for
> example.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/1026
>


-- 
Simon Roberts
(303) 249 3613

Re: RFR: 8180352: Add Stream.toList() method

[Stuart Marks]

On Wed, 4 Nov 2020 23:03:02 GMT, Paŭlo Ebermann 
 wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 199:
> 
>> 197:  * safely reused as the List's internal storage, avoiding a 
>> defensive copy. Declared
>> 198:  * with Object... instead of E... as the parameter type so that 
>> varargs calls don't
>> 199:  * accidentally create an array of type other than Object[].
> 
> Why would that be a problem? If the resulting list is immutable, then the 
> actual array type doesn't really matter, right?

It's an implementation invariant that the internal array be Object[]. Having it 
be something other than Object[] can lead to subtle bugs. See 
[JDK-6260652](https://bugs.openjdk.java.net/browse/JDK-6260652) for example.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

RFR: 8255964: Add jcmd Thread.print to jtreg timeout handler

[Nils Eliasson]

This patch adds jcmd Thread.print to the jtreg timeout handler.

Please review.

-

Commit messages:
 - Add jcmd thread.print to timeout handler

Changes: https://git.openjdk.java.net/jdk/pull/1080/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1080=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255964
  Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1080.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1080/head:pull/1080

PR: https://git.openjdk.java.net/jdk/pull/1080

Re: RFR: 8180352: Add Stream.toList() method

[Stuart Marks]

On Wed, 4 Nov 2020 09:46:32 GMT, Zheka Kozlov 
 wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 240:
> 
>> 238: static  List listFromTrustedArrayNullsAllowed(Object... 
>> input) {
>> 239: if (input.length == 0) {
>> 240: return (List) EMPTY_LIST;
> 
> If we return a `ListN` here, does this mean that 
> `Stream.of().toList().contains(null)` will throw an NPE? But this is 
> incorrect because `toList()` returns a null-tolerant List.

Yes, good point, we might need to have a null-tolerant empty list.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8247781: Day periods support [v7]

[Joe Wang]

On Thu, 5 Nov 2020 17:12:11 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed typo/grammatical error.

Marked as reviewed by joehw (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v22]

[Maurizio Cimadamore]

> This patch contains the changes associated with the third incubation round of 
> the foreign memory access API incubation  (see JEP 393 [1]). This iteration 
> focus on improving the usability of the API in 3 main ways:
> 
> * first, by providing a way to obtain truly *shared* segments, which can be 
> accessed and closed concurrently from multiple threads
> * second, by providing a way to register a memory segment against a 
> `Cleaner`, so as to have some (optional) guarantee that the memory will be 
> deallocated, eventually
> * third, by not requiring users to dive deep into var handles when they first 
> pick up the API; a new `MemoryAccess` class has been added, which defines 
> several useful dereference routines; these are really just thin wrappers 
> around memory access var handles, but they make the barrier of entry for 
> using this API somewhat lower.
> 
> A big conceptual shift that comes with this API refresh is that the role of 
> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used 
> to be the case that a memory address could (sometimes, not always) have a 
> back link to the memory segment which originated it; additionally, memory 
> access var handles used `MemoryAddress` as a basic unit of dereference.
> 
> This has all changed as per this API refresh;  now a `MemoryAddress` is just 
> a dumb carrier which wraps a pair of object/long addressing coordinates; 
> `MemorySegment` has become the star of the show, as far as dereferencing 
> memory is concerned. You cannot dereference memory if you don't have a 
> segment. This improves usability in a number of ways - first, it is a lot 
> easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; 
> secondly, it is crystal clear what a client has to do in order to dereference 
> memory: if a client has a segment, it can use that; otherwise, if the client 
> only has an address, it will have to create a segment *unsafely* (this can be 
> done by calling `MemoryAddress::asSegmentRestricted`).
> 
> A list of the API, implementation and test changes is provided below. If  you 
> have any questions, or need more detailed explanations, I (and the  rest of 
> the Panama team) will be happy to point at existing discussions,  and/or to 
> provide the feedback required. 
> 
> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom 
> the work on shared memory segment would not have been possible; also I'd like 
> to thank Paul Sandoz, whose insights on API design have been very helpful in 
> this journey.
> 
> Thanks 
> Maurizio 
> 
> Javadoc: 
> 
> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
> 
> Specdiff: 
> 
> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
> 
> CSR: 
> 
> https://bugs.openjdk.java.net/browse/JDK-8254163
> 
> 
> 
> ### API Changes
> 
> * `MemorySegment`
>   * drop factory for restricted segment (this has been moved to 
> `MemoryAddress`, see below)
>   * added a no-arg factory for a native restricted segment representing 
> entire native heap
>   * rename `withOwnerThread` to `handoff`
>   * add new `share` method, to create shared segments
>   * add new `registerCleaner` method, to register a segment against a cleaner
>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>   * add some `asSlice` overloads (to make up for the fact that now segments 
> are more frequently used as cursors)
>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
> `Addressable`)
> * `MemoryAddress`
>   * drop `segment` accessor
>   * drop `rebase` method and replace it with `segmentOffset` which returns 
> the offset (a `long`) of this address relative to a given segment
> * `MemoryAccess`
>   * New class supporting several static dereference helpers; the helpers are 
> organized by carrier and access mode, where a carrier is one of the usual 
> suspect (a Java primitive, minus `boolean`); the access mode can be simple 
> (e.g. access base address of given segment), or indexed, in which case the 
> accessor takes a segment and either a low-level byte offset,or a high level 
> logical index. The classification is reflected in the naming scheme (e.g. 
> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`).
> * `MemoryHandles`
>   * drop `withOffset` combinator
>   * drop `withStride` combinator
>   * the basic memory access handle factory now returns a var handle which 
> takes a `MemorySegment` and a `long` - from which it is easy to derive all 
> the other handles using plain var handle combinators.
> * `Addressable`
>   * This is a new interface which is attached to entities which can be 
> projected to a `MemoryAddress`. For now, both `MemoryAddress` and 
> `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more 
> implementations. Clients can largely ignore this interface, which comes in 
> really handy when 

Re: RFR: 8247781: Day periods support [v7]

[Naoto Sato]

> Hi,
> 
> Please review the changes for the subject issue. This is to enhance the 
> java.time package to support day periods, such as "in the morning", defined 
> in CLDR. It will add a new pattern character 'B' and its supporting builder 
> method. The motivation and its spec are in this CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8254629
> 
> Naoto

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

  Fixed typo/grammatical error.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/938/files
  - new: https://git.openjdk.java.net/jdk/pull/938/files/4aa5b197..b0649899

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=938=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=05-06

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/938.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8247781: Day periods support [v7]

[Naoto Sato]

On Thu, 5 Nov 2020 16:07:30 GMT, Roger Riggs  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed typo/grammatical error.
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java 
> line 1479:
> 
>> 1477:  * for it in the formatter locale is from 21:00 to 06:00, then 
>> {@code HOUR_OF_DAY}
>> 1478:  * is set to '1' and {@code MINUTE_OF_HOUR} set to '30'. If {@code 
>> AMPM_OF_DAY} exists
>> 1479:  * and no {@code HOUR_OF_DAY} is resolved, the parsed day period 
>> takes the precedence.
> 
> "the precedence" -> "precedence"

Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8247781: Day periods support [v3]

[Naoto Sato]

On Mon, 2 Nov 2020 19:20:10 GMT, Roger Riggs  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed exception messages.
>
> src/java.base/share/classes/java/time/format/Parsed.java line 180:
> 
>> 178: cloned.chrono = this.chrono;
>> 179: cloned.leapSecond = this.leapSecond;
>> 180: cloned.dayPeriod= this.dayPeriod;
> 
> Add space before "=".

Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Jorn Vernee]

On Thu, 5 Nov 2020 15:42:54 GMT, Roland Westrelin  wrote:

>> src/java.base/share/classes/java/lang/IndexOutOfBoundsException.java line 83:
>> 
>>> 81:  */
>>> 82: public IndexOutOfBoundsException(long index) {
>>> 83: super("Index out of range: " + index);
>> 
>> For consistency with the class name:
>> Suggestion:
>> 
>> super("Index out of bounds: " + index);
>
> Not sure about that one as it's a copy of the message from 
> IndexOutOfBoundsException(int index). Both would need to be changed for 
> consistency.

Ok, I didn't notice that it was taken from elsewhere. Never mind then.

-

PR: https://git.openjdk.java.net/jdk/pull/1003

Re: RFR: 8247781: Day periods support [v3]

[Roger Riggs]

On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed exception messages.

Marked as reviewed by rriggs (Reviewer).

src/java.base/share/classes/java/time/format/Parsed.java line 180:

> 178: cloned.chrono = this.chrono;
> 179: cloned.leapSecond = this.leapSecond;
> 180: cloned.dayPeriod= this.dayPeriod;

Add space before "=".

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8247781: Day periods support [v6]

[Roger Riggs]

On Wed, 4 Nov 2020 22:13:25 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato 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 dayperiod
>  - Change based on CSR change.
>  - Fixed TCK test failures, added the new pattern char description in 
> DateTimeFormatter
>  - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516147298
>  - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516123190
>  - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516138455
>  - Fixed exception messages.
>  - Addressed the following comments:
>- https://github.com/openjdk/jdk/pull/938#discussion_r515003422
>- https://github.com/openjdk/jdk/pull/938#discussion_r515005296
>- https://github.com/openjdk/jdk/pull/938#discussion_r515008862
>- https://github.com/openjdk/jdk/pull/938#discussion_r515030268
>- https://github.com/openjdk/jdk/pull/938#discussion_r515030880
>- https://github.com/openjdk/jdk/pull/938#discussion_r515032002
>- https://github.com/openjdk/jdk/pull/938#discussion_r515036803
>- https://github.com/openjdk/jdk/pull/938#discussion_r515037626
>- https://github.com/openjdk/jdk/pull/938#discussion_r515038069
>- https://github.com/openjdk/jdk/pull/938#discussion_r515039056
>  - 8247781: Day periods support

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
1479:

> 1477:  * for it in the formatter locale is from 21:00 to 06:00, then 
> {@code HOUR_OF_DAY}
> 1478:  * is set to '1' and {@code MINUTE_OF_HOUR} set to '30'. If {@code 
> AMPM_OF_DAY} exists
> 1479:  * and no {@code HOUR_OF_DAY} is resolved, the parsed day period 
> takes the precedence.

"the precedence" -> "precedence"

-

PR: https://git.openjdk.java.net/jdk/pull/938

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[Claes Redestad]

On Thu, 5 Nov 2020 14:59:56 GMT, Alan Bateman  wrote:

>> I do wonder if it makes sense to handle triple-state `int` here: "not yet 
>> generated", "generated", "in error"? So that we don't try to generate the 
>> accessor over and over again when it is in error?
>
> If we are changing NativeMethodAccessorImpl.invoke then we should probably do 
> NativeConstructorAccessorImpl.newInstance at the same time. Also wondering if 
> we should, while in the area, add "return acc.invoke(obj, args)" after 
> setting the delegate so that it invokes the newly generated accessor.
> 
> Are there resource or other cases that you have observed where generateMethod 
> fails and then succeeds in a subsequent call?
> 
> @cl4es Do you know of any startup tests that might be sensitive to the eager 
> creating of a VarHandle?
> 
> I agree with @shipilev to test before the CAS.

@AlanBateman I had a look and it seems unlikely the eager creation in this 
`` will matter. I can find `NativeMethodAccessorImpl` in the startup 
profiles only for netty and larger apps, which often already initialize some 
`VarHandle` or are large enough that the initialization overhead will be lost 
in the noise.

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Roland Westrelin]

> This change add 3 new methods in Objects:
> 
> public static long checkIndex(long index, long length)
> public static long checkFromToIndex(long fromIndex, long toIndex, long length)
> public static long checkFromIndexSize(long fromIndex, long size, long length)
> 
> This mirrors the int utility methods that were added by JDK-8135248
> with the same motivations.
> 
> As is the case with the int checkIndex(), the long checkIndex() method
> is JIT compiled as an intrinsic. It allows the JIT to compile
> checkIndex to an unsigned comparison and properly recognize it as
> a range check that then becomes a candidate for the existing range check
> optimizations. This has proven to be important for panama's
> MemorySegment API and a prototype of this change (with some extra c2
> improvements) showed that panama micro benchmark results improve
> significantly.
> 
> This change includes:
> 
> - the API change
> - the C2 intrinsic
> - tests for the API and the C2 intrinsic
> 
> This is a joint work with Paul who reviewed and reworked the API change
> and filled the CSR.

Roland Westrelin 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:

 - Jorn's comments
 - Update headers and add intrinsic to Graal test ignore list
 - move compiler test and add bug to test
 - non x86_64 arch support
 - c2 test case
 - intrinsic
 - Use overloads of method names.
   
   Simplify internally to avoid overload resolution
   issues, leverging List for the exception
   mapper.
 - Vladimir's comments
 - checkLongIndex

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1003/files
  - new: https://git.openjdk.java.net/jdk/pull/1003/files/74df12d3..b47184ac

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1003=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1003=00-01

  Stats: 11784 lines in 698 files changed: 6370 ins; 3584 del; 1830 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1003.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1003/head:pull/1003

PR: https://git.openjdk.java.net/jdk/pull/1003

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Roland Westrelin]

On Thu, 5 Nov 2020 11:58:43 GMT, Jorn Vernee  wrote:

> Although I'm not a 'Reviewer', I've done a pass over the code and left some 
> inline comments that I hope you find useful.

Thanks for the review! All suggestions (except the exception message one that I 
commented on) look good to me. I applied them.

> src/java.base/share/classes/java/lang/IndexOutOfBoundsException.java line 83:
> 
>> 81:  */
>> 82: public IndexOutOfBoundsException(long index) {
>> 83: super("Index out of range: " + index);
> 
> For consistency with the class name:
> Suggestion:
> 
> super("Index out of bounds: " + index);

Not sure about that one as it's a copy of the message from 
IndexOutOfBoundsException(int index). Both would need to be changed for 
consistency.

-

PR: https://git.openjdk.java.net/jdk/pull/1003

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[Alan Bateman]

On Thu, 5 Nov 2020 09:07:13 GMT, Aleksey Shipilev  wrote:

>> …AccessorImpl object
>> 
>> We met real problem when using protobuf with option optimized for code size, 
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>> 
>> Optimize solution is adding a new boolean field to detect concurrent method 
>> accessor generation in same NativeMethodAccessorImpl object, only one thread 
>> is allowed to generate accessor, other threads still invoke in jni way until 
>> parent's delegator is updated from NativeMethodAccessorImpl  to generated 
>> accessor.
>> 
>> In common case, extra overhead is an atomic operation, compared with method 
>> accessor generate, this cost is trivial.
>
> I do wonder if it makes sense to handle triple-state `int` here: "not yet 
> generated", "generated", "in error"? So that we don't try to generate the 
> accessor over and over again when it is in error?

If we are changing NativeMethodAccessorImpl.invoke then we should probably do 
NativeConstructorAccessorImpl.newInstance at the same time. Also wondering if 
we should, while in the area, add "return acc.invoke(obj, args)" after setting 
the delegate so that it invokes the newly generated accessor.

Are there resource or other cases that you have observed where generateMethod 
fails and then succeeds in a subsequent call?

@cl4es Do you know of any startup tests that might be sensitive to the eager 
creating of a VarHandle?

I agree with @shipilev to test before the CAS.

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Integrated: 8255214: Unsupported 'valign' attribute for 'th' tag used in j.u.l.LogManager

[Rahul Yadav]

On Wed, 4 Nov 2020 21:52:56 GMT, Rahul Yadav  wrote:

> Hello,
> 
> Request to review this small change, as HTML4 support has been dropped from 
> javadoc.
> Have replaced valign="top" with style="vertical-align:top".

This pull request has now been integrated.

Changeset: 867a484d
Author:Rahul Yadav 
URL:   https://git.openjdk.java.net/jdk/commit/867a484d
Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod

8255214: Unsupported 'valign' attribute for 'th' tag used in j.u.l.LogManager

Reviewed-by: mchung, dfuchs

-

PR: https://git.openjdk.java.net/jdk/pull/1064

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[Hui Shi]

On Thu, 5 Nov 2020 09:05:55 GMT, Aleksey Shipilev  wrote:

>> …AccessorImpl object
>> 
>> We met real problem when using protobuf with option optimized for code size, 
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>> 
>> Optimize solution is adding a new boolean field to detect concurrent method 
>> accessor generation in same NativeMethodAccessorImpl object, only one thread 
>> is allowed to generate accessor, other threads still invoke in jni way until 
>> parent's delegator is updated from NativeMethodAccessorImpl  to generated 
>> accessor.
>> 
>> In common case, extra overhead is an atomic operation, compared with method 
>> accessor generate, this cost is trivial.
>
> src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
>  line 66:
> 
>> 64: && !method.getDeclaringClass().isHidden()
>> 65: && 
>> !ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())
>> 66: && ACCESSOR_GENERATED.compareAndSet(this, false, true)) {
> 
> As the micro-optimization, checking that `accessor_generated` is `false` 
> before attempting a (potentially contended) CAS might fit better. (See 
> https://en.wikipedia.org/wiki/Test_and_test-and-set).

Agree this could benefit performance. Grep jdk codes (grep -R  compareAndSet 
*), not found codes with test and test-and-set pattern.

compareAndSet is intrinsic and maybe it's better let runtime/compiler generate 
Test_and_test-and-set code sequence to solve all compareAndSet operations.

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[Hui Shi]

On Thu, 5 Nov 2020 09:02:55 GMT, Aleksey Shipilev  wrote:

>> …AccessorImpl object
>> 
>> We met real problem when using protobuf with option optimized for code size, 
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>> 
>> Optimize solution is adding a new boolean field to detect concurrent method 
>> accessor generation in same NativeMethodAccessorImpl object, only one thread 
>> is allowed to generate accessor, other threads still invoke in jni way until 
>> parent's delegator is updated from NativeMethodAccessorImpl  to generated 
>> accessor.
>> 
>> In common case, extra overhead is an atomic operation, compared with method 
>> accessor generate, this cost is trivial.
>
> src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
>  line 80:
> 
>> 78: succ = true;
>> 79: } finally {
>> 80: if (succ == false) {
> 
> Why `succ` variable, if you can just `catch (Throwable e)` and restore the 
> `accessorGenerated`?

Good suggest! Will update.

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v12]

[Chris Hegarty]

On Mon, 2 Nov 2020 11:22:07 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This patch adds an asExact() combinator to VarHandle, that will return a new 
>> VarHandle that performs exact type checks, similar to 
>> MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, 
>> which can lead to performance degradation.
>> 
>> This is implemented using a boolean flag in VarForm. If the flag is set, the 
>> exact type of the invocation is checked against the exact type in the 
>> VarForm. If there is a mismatch, a WrongMethodTypeException is thrown.
>> 
>> Other than that, there is also an asGeneric() combinator added that does the 
>> inverse operation (thanks to Rémi for the suggestion). I've also added The 
>> `@Hidden` annotation to the VarHandleGuards methods, as well as a 
>> type-checking helper method called from the generic invocation lambda form, 
>> so that the stack trace we get points at the location where the VarHandle is 
>> being used.
>> 
>> Thanks,
>> Jorn
>> 
>> CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   s/an arity/and arity

src/java.base/share/classes/java/lang/invoke/VarHandle.java line 1600:

> 1598:  * If this VarHandle already has invoke-exact behavior this 
> VarHandle is returned.
> 1599:  * @apiNote
> 1600:  * Invoke-exact behavior guarantees that that upon invocation of an 
> access mode method

minor typo "that that"

src/java.base/share/classes/java/lang/invoke/VarHandle.java line 1603:

> 1601:  * the types and arity of the arguments must match the {@link 
> #accessModeType(AccessMode) access mode type},
> 1602:  * otherwise a {@link WrongMethodTypeException} is thrown.
> 1603:  *

While not strictly necessary, the pedantic part to me likes to see assertions 
like this in the spec:

"Invoking `hasInvokeExactBehavior` on the returned VarHandle is guaranteed to 
return true."  And conversely, for withInvokeBehavior.

-

PR: https://git.openjdk.java.net/jdk/pull/843

RFR: JDK-8255395 Implement Enhanced Pseudo-Random Number Generators (CSR)

[Jim Laskey]

Please review the CSR for JEP-356 Enhanced Pseudo-Random Number Generators.

Thank you.

-- Jim

CSR: https://bugs.openjdk.java.net/browse/JDK-8255395 

JBS: https://bugs.openjdk.java.net/browse/JDK-8248862 

JEP: http://openjdk.java.net/jeps/356 

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]

[Jan Lahoda]

On Wed, 4 Nov 2020 19:40:33 GMT, Jan Lahoda  wrote:

>> I have read all the files. 
>> 
>> I have added a n umber of various minor non-blocking comments (no need for 
>> re-review( to fix these.  But I have a couple of comments/questions before 
>> finally giving approval.
>> There's a comment in `PreviewListWriter` about annotation members that needs 
>> too be addressed, and I wonder is RECORD and RECORD_COMPONENT need to be 
>> added into PreviewElementKind.
>
> Thanks @jonathan-gibbons for your comments! I've tried to update the code 
> based on them, mostly in 
> https://github.com/lahodaj/jdk/commit/743f516c660b577035cdda4510a0bb97937fd9b2
>  and 
> https://github.com/lahodaj/jdk/commit/e4b02827998fc2e8f19f983aabfb3d720b03d111
> 
> A big chunk of the update is generalization of the deprecated and preview 
> list builders and writers into a "summary" list builder and writer. These 
> should also now handle records. For record components, those are a little 
> tricky, as (AFAIK) can't currently have deprecation/preview-ness for them 
> (and hence there is no good way to test any support for record components in 
> these). But the summary build and writer are looking for record components 
> and will fail in case a record component is sent into them.

FWIW, a javadoc generated with the current version of the patch:
http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.01/api/index.html

And a specdiff comparing it to the javadoc built from the corresponding master:
http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.specdiff.01/overview-summary.html

-

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: 8255214 : Unsupported 'valign' attribute for 'th' tag used in j.u.l.LogManager

[Daniel Fuchs]

On Wed, 4 Nov 2020 21:52:56 GMT, Rahul Yadav  wrote:

> Hello,
> 
> Request to review this small change, as HTML4 support has been dropped from 
> javadoc.
> Have replaced valign="top" with style="vertical-align:top".

LGTM Rahul. The generated javadoc looks OK too.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1064

Re: RFR: 8255150: Add utility methods to check long indexes and ranges

[Jorn Vernee]

On Mon, 2 Nov 2020 10:47:18 GMT, Roland Westrelin  wrote:

> This change add 3 new methods in Objects:
> 
> public static long checkIndex(long index, long length)
> public static long checkFromToIndex(long fromIndex, long toIndex, long length)
> public static long checkFromIndexSize(long fromIndex, long size, long length)
> 
> This mirrors the int utility methods that were added by JDK-8135248
> with the same motivations.
> 
> As is the case with the int checkIndex(), the long checkIndex() method
> is JIT compiled as an intrinsic. It allows the JIT to compile
> checkIndex to an unsigned comparison and properly recognize it as
> a range check that then becomes a candidate for the existing range check
> optimizations. This has proven to be important for panama's
> MemorySegment API and a prototype of this change (with some extra c2
> improvements) showed that panama micro benchmark results improve
> significantly.
> 
> This change includes:
> 
> - the API change
> - the C2 intrinsic
> - tests for the API and the C2 intrinsic
> 
> This is a joint work with Paul who reviewed and reworked the API change
> and filled the CSR.

Hi Roland, Paul. Thank you for tackling this! We have to jump through quite a 
few hoops in the implementation of the foreign memory access API in order to 
leverage the intrinsification of int-based index checks, and even then we are 
not covering the cases where the numbers are larger than ints. Looking forward 
to being able to remove those hacks!

Although I'm not a 'Reviewer', I've done a pass over the code and left some 
inline comments that I hope you find useful.

src/hotspot/share/opto/graphKit.hpp line 109:

> 107: if (bt == T_INT) {
> 108:   assert((jlong)(jint)con == con, "not an int");
> 109:   return intcon((jint) con);

Could also use `checked_cast` here (from globalDefinitions.hpp), which does a 
similar check.
Suggestion:

  return intcon(checked_cast(con));

src/hotspot/share/opto/graphKit.hpp line 111:

> 109:   return intcon((jint) con);
> 110: }
> 111: return longcon(con);

This ends up defaulting to longcon for any basic type that is not T_INT. Maybe 
it's nice to have an assert here?
Suggestion:

assert(bt == T_LONG, "basic type not an int or long");
return longcon(con);

src/hotspot/share/opto/type.cpp line 1353:

> 1351: jint int_hi = (jint)hi;
> 1352: assert(((jlong)int_lo) == lo && ((jlong)int_hi) == hi, "bounds are 
> not ints");
> 1353: return TypeInt::make(int_lo, int_hi, w);

checked_cast could also be used here.
Suggestion:

return TypeInt::make(checked_cast(lo), checked_cast(hi), w);

src/java.base/share/classes/java/lang/IndexOutOfBoundsException.java line 83:

> 81:  */
> 82: public IndexOutOfBoundsException(long index) {
> 83: super("Index out of range: " + index);

For consistency with the class name:
Suggestion:

super("Index out of bounds: " + index);

test/jdk/java/util/Objects/CheckLongIndex.java line 94:

> 92: long apply(long a, long b, long c);
> 93: }
> 94: 

Seems unused?
Suggestion:

-

PR: https://git.openjdk.java.net/jdk/pull/1003

Integrated: 8255226: (tz) Upgrade time-zone data to tzdata2020d

[Kiran Sidhartha Ravikumar]

On Mon, 2 Nov 2020 16:29:07 GMT, Kiran Sidhartha Ravikumar 
 wrote:

> Hi Guys,
> 
> Please review the integration of tzdata2020d to JDK.
> 
> Details regarding the change can be viewed at - 
> https://mm.icann.org/pipermail/tz-announce/2020-October/62.html
> Bug: https://bugs.openjdk.java.net/browse/JDK-8255226
> 
> TestZoneInfo310.java test failure is addressed along with it. The last rule 
> affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test.
> 
> Regression Tests pass along with JCK.
> 
> Please let me know if the changes are good to push.
> 
> Thanks,
> Kiran

This pull request has now been integrated.

Changeset: b65ff60a
Author:Kiran Sidhartha Ravikumar 
Committer: Sean Coffey 
URL:   https://git.openjdk.java.net/jdk/commit/b65ff60a
Stats: 61 lines in 4 files changed: 38 ins; 2 del; 21 mod

8255226: (tz) Upgrade time-zone data to tzdata2020d

Reviewed-by: naoto

-

PR: https://git.openjdk.java.net/jdk/pull/1012

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v10]

[Jan Lahoda]

> This is an update to javac and javadoc, to introduce support for Preview 
> APIs, and generally improve javac and javadoc behavior to more closely adhere 
> to JEP 12.
> 
> The notable changes are:
> 
>  * adding support for Preview APIs (javac until now supported primarily only 
> preview language features, and APIs associated with preview language 
> features). This includes:
>  * the @PreviewFeature annotation has boolean attribute "reflective", 
> which should be true for reflective Preview APIs, false otherwise. This 
> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>  * the preview warnings for preview APIs are auto-suppressed as described 
> in the JEP 12. E.g. the module that declares the preview API is free to use 
> it without warnings
>  * improving error/warning messages. Please see [1] for a list of 
> cases/examples.
>  * class files are only marked as preview if they are using a preview 
> feature. [1] also shows if a class file is marked as preview or not.
>  * the PreviewFeature annotation has been moved to jdk.internal.javac package 
> (originally was in the jdk.internal package).
>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
> the source files. javadoc will auto-generate a note for @PreviewFeature 
> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
> respectively). A summary of preview elements is also provided [4]. Existing 
> uses of @preview have been updated/removed.
>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
> of Preview elements, and for declaring elements using preview language 
> features [5].
>  
>  Please also see the CSR [6] for more information.
>  
>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>  [2] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>  [3] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>  [4] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixing typo.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/703/files
  - new: https://git.openjdk.java.net/jdk/pull/703/files/370031f0..097ae3c1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=703=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=703=08-09

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

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v12]

[Martin Doerr]

On Thu, 5 Nov 2020 01:20:08 GMT, CoreyAshford 
 wrote:

>> This patch set encompasses the following commits:
>> 
>> - Adds a new HotSpot intrinsic candidate to the java.lang.Base64 class - 
>> decodeBlock(), and provides a flexible API for the intrinsic.  The API is 
>> similar to the existing encodeBlock intrinsic.
>> - Adds the code in HotSpot to check and martial the new intrinsic's 
>> arguments to the arch-specific intrinsic implementation
>> - Adds a Power64LE-specific implementation of the decodeBlock intrinsic.
>> - Adds a JMH microbenchmark for both Base64 encoding and encoding.
>> - Enhances the JTReg hotspot intrinsic "TestBase64.java" regression test to 
>> more fully test both decoding and encoding.
>
> CoreyAshford has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   stubGenerator_ppc.cpp: fix typo (omitted 'the')

I already had the feeling that unrolling the large loop was not beneficial. 
Thanks and thumps up from my side!

-

Marked as reviewed by mdoerr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/293

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v9]

[Jan Lahoda]

> This is an update to javac and javadoc, to introduce support for Preview 
> APIs, and generally improve javac and javadoc behavior to more closely adhere 
> to JEP 12.
> 
> The notable changes are:
> 
>  * adding support for Preview APIs (javac until now supported primarily only 
> preview language features, and APIs associated with preview language 
> features). This includes:
>  * the @PreviewFeature annotation has boolean attribute "reflective", 
> which should be true for reflective Preview APIs, false otherwise. This 
> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>  * the preview warnings for preview APIs are auto-suppressed as described 
> in the JEP 12. E.g. the module that declares the preview API is free to use 
> it without warnings
>  * improving error/warning messages. Please see [1] for a list of 
> cases/examples.
>  * class files are only marked as preview if they are using a preview 
> feature. [1] also shows if a class file is marked as preview or not.
>  * the PreviewFeature annotation has been moved to jdk.internal.javac package 
> (originally was in the jdk.internal package).
>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
> the source files. javadoc will auto-generate a note for @PreviewFeature 
> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
> respectively). A summary of preview elements is also provided [4]. Existing 
> uses of @preview have been updated/removed.
>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
> of Preview elements, and for declaring elements using preview language 
> features [5].
>  
>  Please also see the CSR [6] for more information.
>  
>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>  [2] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>  [3] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>  [4] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Removing obsolette @PreviewFeature.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/703/files
  - new: https://git.openjdk.java.net/jdk/pull/703/files/3d55f909..370031f0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=703=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=703=07-08

  Stats: 10 lines in 5 files changed: 0 ins; 10 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/703.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v8]

[Jan Lahoda]

> This is an update to javac and javadoc, to introduce support for Preview 
> APIs, and generally improve javac and javadoc behavior to more closely adhere 
> to JEP 12.
> 
> The notable changes are:
> 
>  * adding support for Preview APIs (javac until now supported primarily only 
> preview language features, and APIs associated with preview language 
> features). This includes:
>  * the @PreviewFeature annotation has boolean attribute "reflective", 
> which should be true for reflective Preview APIs, false otherwise. This 
> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>  * the preview warnings for preview APIs are auto-suppressed as described 
> in the JEP 12. E.g. the module that declares the preview API is free to use 
> it without warnings
>  * improving error/warning messages. Please see [1] for a list of 
> cases/examples.
>  * class files are only marked as preview if they are using a preview 
> feature. [1] also shows if a class file is marked as preview or not.
>  * the PreviewFeature annotation has been moved to jdk.internal.javac package 
> (originally was in the jdk.internal package).
>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
> the source files. javadoc will auto-generate a note for @PreviewFeature 
> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
> respectively). A summary of preview elements is also provided [4]. Existing 
> uses of @preview have been updated/removed.
>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
> of Preview elements, and for declaring elements using preview language 
> features [5].
>  
>  Please also see the CSR [6] for more information.
>  
>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>  [2] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>  [3] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>  [4] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769

Jan Lahoda has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 51 commits:

 - Merging master into JDK-8250768
 - Removing unnecessary property keys.
 - Cleanup - removing unnecessary code.
 - Merging master into JDK-8250768-dev4
 - Reflecting review comments.
 - Removing trailing whitespace.
 - Merging master into JDK-8250768.
 - Updating tests after records are a final feature.
 - Fixing tests.
 - Finalizing removal of record preview hooks.
 - ... and 41 more: https://git.openjdk.java.net/jdk/compare/700447f7...3d55f909

-

Changes: https://git.openjdk.java.net/jdk/pull/703/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=703=07
  Stats: 3665 lines in 136 files changed: 2701 ins; 692 del; 272 mod
  Patch: https://git.openjdk.java.net/jdk/pull/703.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…

[Aleksey Shipilev]

On Thu, 5 Nov 2020 02:52:05 GMT, Hui Shi  wrote:

> …AccessorImpl object
> 
> We met real problem when using protobuf with option optimized for code size, 
> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
> 
> Optimize solution is adding a new boolean field to detect concurrent method 
> accessor generation in same NativeMethodAccessorImpl object, only one thread 
> is allowed to generate accessor, other threads still invoke in jni way until 
> parent's delegator is updated from NativeMethodAccessorImpl  to generated 
> accessor.
> 
> In common case, extra overhead is an atomic operation, compared with method 
> accessor generate, this cost is trivial.

I do wonder if it makes sense to handle triple-state `int` here: "not yet 
generated", "generated", "in error"? So that we don't try to generate the 
accessor over and over again when it is in error?

src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java 
line 80:

> 78: succ = true;
> 79: } finally {
> 80: if (succ == false) {

Why `succ` variable, if you can just `catch (Throwable e)` and restore the 
`accessorGenerated`?

src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java 
line 66:

> 64: && !method.getDeclaringClass().isHidden()
> 65: && 
> !ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())
> 66: && ACCESSOR_GENERATED.compareAndSet(this, false, true)) {

As the micro-optimization, checking that `accessor_generated` is `false` before 
attempting a (potentially contended) CAS might fit better. (See 
https://en.wikipedia.org/wiki/Test_and_test-and-set).

-

PR: https://git.openjdk.java.net/jdk/pull/1070

Integrated: 8250625: Compiler implementation of Pattern Matching for instanceof (Final)

[Jan Lahoda]

On Thu, 8 Oct 2020 11:49:17 GMT, Jan Lahoda  wrote:

> This is the current proposed patch for the upcoming JEP 394, for pattern 
> matching for instanceof.
> 
> A summary of changes:
> -making the feature permanent (non-preview)
> -making the binding variables non-final (as per current specification 
> proposal)
> -producing a compile-time error for the case where the expression's type is a 
> subtype of the type test pattern's type (as per current specification 
> proposal)
> -changing the AST structure so that the binding variable has a VariableTree 
> in the AST. BindingPatternTree is preserved and encloses the VariableTree. 
> The reason is better consistency in the API, with nodes like CatchTree, 
> EnhancedForLoop Tree, etc.
> 
> This change will not be integrated until JEP 394 is targetted.

This pull request has now been integrated.

Changeset: 18bc95ba
Author:Jan Lahoda 
URL:   https://git.openjdk.java.net/jdk/commit/18bc95ba
Stats: 651 lines in 90 files changed: 228 ins; 310 del; 113 mod

8250625: Compiler implementation of Pattern Matching for instanceof (Final)

Reviewed-by: vromero

-

PR: https://git.openjdk.java.net/jdk/pull/559