Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 24, 2020, at 7:01 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-18 08:34, Kim Barrett wrote:
>>> On Jun 15, 2020, at 7:41 AM, Kim Barrett  wrote:
>>> 
 On Jun 14, 2020, at 12:45 AM, Philip Race  wrote:
 
 Kim,
 
 
 Until it says in "the JDK" and not "in HotSpot" you have not addressed my 
 main point.
 Please rename the JEP.
>> After some off-list discussions I have a better idea of what Phil was
>> asking for and why. In response I've changed the JEP, replacing a few
>> occurrences of "HotSpot" with "the JDK", as described below.  All
>> other occurrences of "HotSpot" have been left as-is.
>> 
>> Title:
>> JEP 347: Adopt C++14 Language Features in HotSpot
>> =>
>> JEP 347: Adopt C++14 Language Features in the JDK
>> 
>> Summary:
>> Allow the use of selected C++14 language features in the HotSpot C++ source 
>> code.
>> =>
>> Allow the use of selected C++14 language features in the JDK C++ source code.
>> 
>> Goals:
>> The purpose of this JEP is to formally allow C++ source code changes within 
>> HotSpot to take advantage of some C++14 standard language features.
>> =>
>> The purpose of this JEP is to formally allow C++ source code changes within 
>> the JDK to take advantage of some C++14 standard language features.
>> 
> This all looks good to me.
> 
> /Magnus

Thanks.

Re: RFR: JDK-8240148: Uncaught exceptions from ScheduledThreadPoolExecutor.execute aren't reported

[Jaikiran Pai]

Thank you both Martin and David for your inputs.

On 25/06/20 5:40 am, David Holmes wrote:
> Hi Jaikiran,
>
>
>> The patch is available as a webrev at
>> https://cr.openjdk.java.net/~jpai/webrev/8240148/1/
>>
>> The patch updates the ScheduledThreadPoolExecutor.execute(Runnable) to
>> wrap the user passed Runnable into a custom one which then catches any
>> Throwable and reports it through the Thread's uncaught exception
>> handler. After having reviewed the existing code in that class, this was
>> the easiest (and perhaps the only feasible?) and cleanest way that I
>> could think of, to fix this issue.
>
> I do not agree with the solution or even the formulation of the issue.
>
> This is something that has been raised and discussed a number of times
> since Java 5. See for example:
>
> http://cs.oswego.edu/pipermail/concurrency-interest/2011-April/007865.html
>
>
> The UncaughtExceptionHandler is, as the name clearly indicates, for
> execution in response to uncaught exceptions and is the last act of a
> dying thread.

You are right. I looked into the ScheduledThreadPoolExecutor
implementation details but failed to read up on the javadoc of
UncaughtExceptionHandler which already clearly states that it's meant
for use when a thread is terminating. Thank you for linking that
previous discussion too.

-Jaikiran

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Paul Sandoz]

Hi,

We traded CPS style for reusing existing functional interfaces. Originally the 
signature (my first choice) was as you indicate.

By chance it just so happens that the current signature is the same shape as 
that for the accumulating argument type of the three arg collect terminal 
operation: 

Stream
 R collect(Supplier supplier,
  BiConsumer accumulator,
  BiConsumer combiner);

IntStream
 R collect(Supplier supplier,
  ObjIntConsumer accumulator,
  BiConsumer combiner);

Same for the accumulator of a Collector too.

However, I suspect you would argue these terminal accumulation cases are 
different from the intermediate case, as we are not accumulating but passing or 
accepting (loosely returning) zero or more elements that replace the input 
element.

It’s my hope that generic specialization will allow the primitive stream types 
to fade into the background, along with the primitive functional interfaces. In 
that respect the addition of three functional interfaces for use on the 
primitive stream types is not so terrible.


Regarding the name, you should have seen the first one :-) it was terrible.

Here’s my few brush strokes on the bike shed. I wonder what people think of 
mapAccept. The specification talks about accepting elements, because that is 
the operative method name on Consumer. So we can say "T is replaced with the 
elements accepted by the Consumer", or “ The Consumer accepts the 
elements that replace T"

Paul.



> On Jun 24, 2020, at 1:01 PM, John Rose  wrote:
> 
> I like this new API point a lot; it allows flexible, local, temporary
> control inversion in the context of one stream transform.
> 
> What’s the performance model?  It seems like the SpinedBuffer
> implementation makes a worst-case assumption about the number
> of pending values, that there will be many instead of zero or one.
> 
> But I guess the pipeline stuff already works in terms of pushes, so
> the good news might be that this is really just a drill-down from the
> user API into the kinds of operations (push-flavored) that go on
> most of the time.
> 
> OK, so I like the function but I have a beef with its bike shed
> color.  First of all, this is a control-inversion (CPS) pattern,
> which is very powerful but also notoriously hard to read.
> I think that in Java APIs, at least in Stream APIs, code is
> easier to read if the logical data flow is from left to right.
> 
> (This is a language-specific observation.  Apart from varargs,
> Java method APIs read favorably when extra control arguments
> are added onto the end of the argument list.  Also, the convention
> for generic functional interfaces is that the return value type
> goes to the right, e.g., R in Function.)
> 
> So the BiConsumer is backwards, because the logical return
> should be written, if not as a true return (which would appear
> at the end of type parameter lists), at the end of the incoming
> parameters (and in the last type parameter).
> 
> I also think “multi” is needlessly “learned” sounding.  A simple
> spatial preposition would work well: mapThrough, mapAcross, etc.
> I think I prefer mapAcross because the term “across” can be read
> adverbially: “we are mapping T across to Consumer”.
> 
> So:
> 
> mapAcross(BiConsumer> mapper)
> mapAcrossToInt(BiConsumer mapper)
> mapAcross​(IntObjConsumer mapper)
> 
> This does require additional FI’s like IntObjConsumer, but
> I think that is a negligible cost.  Making the control inversion
> *readable* is the high order bit here, not minimizing the number
> of trivial FIs.
> 
> (I almost hear David Bowman, in his space suit, saying, “My API…
> It’s full of bikesheds!”  There’s a meme for that.)
> 
> — John
> 
> On Jun 24, 2020, at 3:57 AM, Patrick Concannon  
> wrote:
>> 
>> Hi,
>> 
>> Could someone please review myself and Julia's RFE and CSR for JDK-8238286 - 
>> 'Add new flatMap stream operation that is more amenable to pushing’?
>> 
>> This proposal is to add a new flatMap-like operation:
>> 
>> ` Stream mapMulti(BiConsumer, ? super T> mapper)` 
>> 
>> to the java.util.Stream class. This operation is more receptive to the 
>> pushing or yielding of values than the current implementation that 
>> internally assembles values (if any) into one or more streams. This addition 
>> includes the primitive variations of the operation i.e. mapMultiToInt, 
>> IntStream mapMulti, etc.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8238286 
>> 
>> csr: https://bugs.openjdk.java.net/browse/JDK-8248166 
>> 
>> 
>> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/ 
>> 
>> specdiff: 
>> http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html
>>   
>> 

Re: RFR: JDK-8240148: Uncaught exceptions from ScheduledThreadPoolExecutor.execute aren't reported

[David Holmes]

Hi Jaikiran,

On 24/06/2020 11:44 pm, Jaikiran Pai wrote:

Could I please get a review and a sponsor for a fix for
https://bugs.openjdk.java.net/browse/JDK-8240148?


I missed the filing of this bug. :(


The patch is available as a webrev at
https://cr.openjdk.java.net/~jpai/webrev/8240148/1/

The patch updates the ScheduledThreadPoolExecutor.execute(Runnable) to
wrap the user passed Runnable into a custom one which then catches any
Throwable and reports it through the Thread's uncaught exception
handler. After having reviewed the existing code in that class, this was
the easiest (and perhaps the only feasible?) and cleanest way that I
could think of, to fix this issue.


I do not agree with the solution or even the formulation of the issue.

This is something that has been raised and discussed a number of times 
since Java 5. See for example:


http://cs.oswego.edu/pipermail/concurrency-interest/2011-April/007865.html

The UncaughtExceptionHandler is, as the name clearly indicates, for 
execution in response to uncaught exceptions and is the last act of a 
dying thread. You are invoking it for a Runnable just because you happen 
to like to what it does by default for threads - and you don't even know 
what it will do as it may have been customised. It is an implementation 
detail with TPE and STPE whether exceptions from tasks lead to 
termination of the worker thread and thus result in execution of the 
UEH, or whether they catch them and continue. For TPE they can; but for 
STPE they never will as the task is wrapped in a Future.


The advice has always been that any Runnable submitted as a task to an 
ExecutorService should take responsibility for dealing with any 
exceptions it encounters.


David
-



A few things about this change:

1. I decided to use an anonymous class for the Runnable, in the
execute() method instead of a lambda because I was unsure if this part
of the JDK code is allowed to use a lambda. In the past I have seen
discussions where it was recommended not use lambda in certain parts of
the JDK either because of performance(?) reason or because "this part of
the code runs too early to be using a lambda" reason. I personally
prefer an anonymous class anyway, but if others feel using a lambda here
is relevant, then let me know.

2. Once the exception is reported using the UncaughtExceptionHandler, I
just return. Initially, I had considered throwing back the original
exception/throwable after reporting it via the UncaughtExceptionHandler,
but having looked at few others places within the JDK which do not throw
back the original exception, I decided to follow the same semantics.

3. Now that the passed Runnable is wrapped and submitted for execution,
I considered whether this would break any (unwritten) contract between
the ThreadPoolExecutor and the ThreadFactory#newThread(Runnable)
implementations. If any (JDK internal or user application specific)
ThreadFactory#newThread method was expecting the passed Runnable to be
of the same type or the same instance as that was submitted to the
ScheduledThreadPoolExecutor#execute(Runnable), then this change would
break such ThreadFactory#newThread implementations. I looked deeper in
the ThreadPoolExecutor code and from what I could see, this isn't a
practical concern, since even without this change, The
ThreadPoolExecutor in its private Worker class already sends a
"delegate" Runnable (an instance of the
ThreadPoolExecutor$Worker class) to the ThreadFactory's newThread method
and not the original Runnable that was submitted to the
execute(Runnable) method of the executor. So it doesn't look like
there's any written/unwritten contract that the ThreadFactory is
expected to receive the same Runnable instance that was passed to the
execute method of the executor.

4. The ScheduledThreadPoolExecutor has a different license than some of
the other files that I am used to, within the JDK code. So I haven't
edited it for any of the usual copyright year updates.

5. The patch contains a new (testng based) test case which reproduces
this issue and verifies the fix.

-Jaikiran

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Remi Forax]

Hi Patrick,

- Mail original -
> De: "Patrick Concannon" 
> À: "core-libs-dev" 
> Envoyé: Mercredi 24 Juin 2020 12:57:01
> Objet: RFR[8238286]:  'Add new flatMap stream operation that is more amenable 
> to pushing’

> Hi,

Hi,

> 
> Could someone please review myself and Julia's RFE and CSR for JDK-8238286 -
> 'Add new flatMap stream operation that is more amenable to pushing’?
> 
> This proposal is to add a new flatMap-like operation:
> 
> ` Stream mapMulti(BiConsumer, ? super T> mapper)`
> 
> to the java.util.Stream class. This operation is more receptive to the pushing
> or yielding of values than the current implementation that internally 
> assembles
> values (if any) into one or more streams. This addition includes the primitive
> variations of the operation i.e. mapMultiToInt, IntStream mapMulti, etc.

I don't really like the name "mapMulti", because flatMap can also be called 
mapMulti.
I'm sure someone will come with a better name, mapPush ?, mapYield ?

Why the BiConsumer takes a T as the second parameter and not as the first like 
in the bug description ?

I get that you want to keep Consumer instead of Consumer because 
Consumer is not a valid target type for a lambda, but the BiConsumer 
should be able to take a ? super Consumer instead of just Consumer.

In Stream.java, in the javadoc both the examples of mapMulti are using wrapper 
types instead of primitives, which is not the way we want people to use 
streams, stream of wrapper types cause the perf issue.

In *Stream.java, in the default method, the 'this' in 'this'.flatMap is useless.
 
In *Pipeline.java, i don't think you need the lambda downstream::accept given 
that a Sink is already a Consumer,
instead of
  mapper.accept(downstream::accept, u);
using the downstream directly should work
  mapper.accept(downstream, u);

also it seems to me that the implementation of cancellationRequested() is 
useless given that Sink.Chained*::cancellationRequested already delegates to 
the downstream sink.
 
I've also noticed that in ReferencePipelien.java, in  Stream 
mapMulti(BiConsumer, ? super P_OUT> mapper), the cache field is 
missing, but its should be necessary anymore.

> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8238286
> 
> csr: https://bugs.openjdk.java.net/browse/JDK-8248166
> 
> 
> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/
> 
> specdiff:
> http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html
>   
> 
> 
> 
> Kind regards,
> Patrick & Julia

regards,
Rémi

Re: [15] RFR JDK-8247785: Small clarification to the javadoc about builtin class loaders

[David Holmes]

On 25/06/2020 5:21 am, Mandy Chung wrote:

Hi Roger, David,

Thanks for the help in improving this.  As a record, this webrev shows 
the version as David suggests:

http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247785/webrev.00/


Looks good to me :)

Thanks,
David


Mandy

On 6/24/20 9:33 AM, Roger Riggs wrote:

Hi Mandy,

I'm fine with this.

Roger

Re: [15] RFR: 8248255: [macos] Add failing DMG tests to problem list

[Andy Herrick]

looks good.

/Andy

On 6/24/2020 4:09 PM, alexander.matv...@oracle.com wrote:

Please review the jpackage fix for bug [1] at [2].

Added EmptyFolderPackageTest, IconTest, AppImagePackageTest, 
SimplePackageTest, BasicTest to ProblemList.txt.


[1] https://bugs.openjdk.java.net/browse/JDK-8248255
[2] http://cr.openjdk.java.net/~almatvee/8248255/webrev.00/

Thanks,
Alexander

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[John Rose]

On Jun 24, 2020, at 1:01 PM, John Rose  wrote:
> 
> What’s the performance model?  It seems like the SpinedBuffer
> implementation makes a worst-case assumption about the number
> of pending values, that there will be many instead of zero or one.
> 
> But I guess the pipeline stuff already works in terms of pushes, so
> the good news might be that this is really just a drill-down from the
> user API into the kinds of operations (push-flavored) that go on
> most of the time.

(I dove straight into the code and missed the implementation
note where you answered this question…  Never mind.)

Re: [15] RFR: 8248255: [macos] Add failing DMG tests to problem list

[Alexey Semenyuk]

Looks good.

- Alexey

On 6/24/2020 4:09 PM, alexander.matv...@oracle.com wrote:

Please review the jpackage fix for bug [1] at [2].

Added EmptyFolderPackageTest, IconTest, AppImagePackageTest, 
SimplePackageTest, BasicTest to ProblemList.txt.


[1] https://bugs.openjdk.java.net/browse/JDK-8248255
[2] http://cr.openjdk.java.net/~almatvee/8248255/webrev.00/

Thanks,
Alexander

[15] RFR: 8248255: [macos] Add failing DMG tests to problem list

[alexander . matveev]

Please review the jpackage fix for bug [1] at [2].

Added EmptyFolderPackageTest, IconTest, AppImagePackageTest, 
SimplePackageTest, BasicTest to ProblemList.txt.


[1] https://bugs.openjdk.java.net/browse/JDK-8248255
[2] http://cr.openjdk.java.net/~almatvee/8248255/webrev.00/

Thanks,
Alexander

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[John Rose]

I like this new API point a lot; it allows flexible, local, temporary
control inversion in the context of one stream transform.

What’s the performance model?  It seems like the SpinedBuffer
implementation makes a worst-case assumption about the number
of pending values, that there will be many instead of zero or one.

But I guess the pipeline stuff already works in terms of pushes, so
the good news might be that this is really just a drill-down from the
user API into the kinds of operations (push-flavored) that go on
most of the time.

OK, so I like the function but I have a beef with its bike shed
color.  First of all, this is a control-inversion (CPS) pattern,
which is very powerful but also notoriously hard to read.
I think that in Java APIs, at least in Stream APIs, code is
easier to read if the logical data flow is from left to right.

(This is a language-specific observation.  Apart from varargs,
Java method APIs read favorably when extra control arguments
are added onto the end of the argument list.  Also, the convention
for generic functional interfaces is that the return value type
goes to the right, e.g., R in Function.)

So the BiConsumer is backwards, because the logical return
should be written, if not as a true return (which would appear
at the end of type parameter lists), at the end of the incoming
parameters (and in the last type parameter).

I also think “multi” is needlessly “learned” sounding.  A simple
spatial preposition would work well: mapThrough, mapAcross, etc.
I think I prefer mapAcross because the term “across” can be read
adverbially: “we are mapping T across to Consumer”.

So:

mapAcross(BiConsumer> mapper)
mapAcrossToInt(BiConsumer mapper)
mapAcross​(IntObjConsumer mapper)

This does require additional FI’s like IntObjConsumer, but
I think that is a negligible cost.  Making the control inversion
*readable* is the high order bit here, not minimizing the number
of trivial FIs.

(I almost hear David Bowman, in his space suit, saying, “My API…
It’s full of bikesheds!”  There’s a meme for that.)

— John

On Jun 24, 2020, at 3:57 AM, Patrick Concannon  
wrote:
> 
> Hi,
> 
> Could someone please review myself and Julia's RFE and CSR for JDK-8238286 - 
> 'Add new flatMap stream operation that is more amenable to pushing’?
> 
> This proposal is to add a new flatMap-like operation:
> 
> ` Stream mapMulti(BiConsumer, ? super T> mapper)` 
> 
> to the java.util.Stream class. This operation is more receptive to the 
> pushing or yielding of values than the current implementation that internally 
> assembles values (if any) into one or more streams. This addition includes 
> the primitive variations of the operation i.e. mapMultiToInt, IntStream 
> mapMulti, etc.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8238286 
> 
> csr: https://bugs.openjdk.java.net/browse/JDK-8248166 
> 
> 
> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/ 
> 
> specdiff: 
> http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html
>   
> 
> 
> 
> Kind regards,
> Patrick & Julia

Re: [15] RFR JDK-8247785: Small clarification to the javadoc about builtin class loaders

[Mandy Chung]

Hi Roger, David,

Thanks for the help in improving this.  As a record, this webrev shows 
the version as David suggests:

http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247785/webrev.00/

Mandy

On 6/24/20 9:33 AM, Roger Riggs wrote:

Hi Mandy,

I'm fine with this.

Roger

RE: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API (Incubator): AArch64 backend changes

[Viswanathan, Sandhya]

Hi Andrew/Yang,
 
We couldn’t propose Vector API to target in time for JDK 15 and hoping to do so 
early in JDK 16 timeframe.
The implementation reviews on other components have made good progress. 
We have so far ok to PPT from (runtime, shared compiler changes, x86 backend).
Java API implementation review is in progress.
I wanted to check with you both if we have a go ahead from aarch64 backed point 
of view.

Best Regards,
Sandhya

-Original Message-
From: hotspot-compiler-dev  On 
Behalf Of Yang Zhang
Sent: Tuesday, May 26, 2020 7:59 PM
To: Andrew Haley ; Paul Sandoz 
Cc: nd ; hotspot-compiler-...@openjdk.java.net; 
hotspot-...@openjdk.java.net; core-libs-dev@openjdk.java.net; 
aarch64-port-...@openjdk.java.net
Subject: RE: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API 
(Incubator): AArch64 backend changes

> But to my earlier question. please: can the new instructions be moved into 
> jdk head first, and then merged into the Panama branch, or not?

The new instructions can be classified as:
1. Instructions that can be matched with NEON instructions directly.
MulVB and SqrtVF have been merged into jdk master already. The patch of AbsV is 
in review [1].

2. Instructions that Jdk master has middle end support for, but they cannot be 
matched with NEON instructions directly.
Such as AddReductionVL, MulReductionVL, And/Or/XorReductionV These new 
instructions can be moved into jdk master first, but for auto-vectorization, 
the performance might not get improved.
May I have a new patch for these? 

3. Panama/Vector API specific  instructions Such as Load/StoreVector ( 16 
bits), VectorReinterpret, VectorMaskCmp, MaxV/MinV, VectorBlend etc. 
These instructions cannot be moved into jdk master first because there isn't 
middle-end support.

Regards
Yang

[1] 
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-May/008861.html

-Original Message-
From: Andrew Haley 
Sent: Tuesday, May 26, 2020 4:25 PM
To: Yang Zhang ; Paul Sandoz 
Cc: hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net; aarch64-port-...@openjdk.java.net; nd 

Subject: Re: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API 
(Incubator): AArch64 backend changes

On 25/05/2020 09:26, Yang Zhang wrote:
> In jdk master, what we need to do is that writing m4 file for existing 
> vector instructions and placed them to a new file aarch64_neon.ad.
> If no question, I will do it right away.

I'm not entirely sure that such a change is necessary now. In particular, 
reorganizing the existing vector instructions is IMO excessive, but I admit 
that it might be an improvement.

But to my earlier question. please: can the new instructions be moved into jdk 
head first, and then merged into the Panama branch, or not?
It'd help if this was possible.

--
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd.  https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Daniel Fuchs]

Hi Alexey,

The JNDI/LDAP part looks mostly good. You will need someone
from the security libs to review the security lib part of the
changes.

In the test I would recommend using the test URIBuilder to avoid
strange intermittent errors if the test is run on a
machine where looking up "localhost" doesn't yield back
InetAddress.getLoopbackAddress():

--

 * @library /test/lib
...
import java.net.URI;
import jdk.test.lib.net.URIBuilder;
...
URI uri = URIBuilder.newBuilder()
.scheme("ldaps")
.loopback()
.port(serverPort)
.build();
env.put(Context.PROVIDER_URL, uri.toString());

--

So we have now two new properties:

jdk.internal.sasl.tlschannelbinding which is a private contract between
   java.naming and jdk.security.jgss;
com.sun.jndi.ldap.tls.cbtype which is a new JDK implementation specific
   environment property for the InitialLdapContext, and is depending
   on another JDK specific environment property:
   "com.sun.jndi.ldap.connect.timeout"

None of these properties are currently documented in the JDK itself.
Although jdk.internal.sasl.tlschannelbinding probably doesn't need
to be documented (but I'll defer to the security experts for that),
the other two probably should. Where is the question.
If we had a jdk.namimg.ldap module then the documentation for
these jndi properties would probably need to go in its module-info.java
API documentation, but we don't. Obviously we will want to write
a release note for this fix that documents the new
com.sun.jndi.ldap.tls.cbtype property - but will that be
sufficient? The CSR committee might wish for more.

Anyone has advice to share on this?

best regards,

-- daniel


On 17/06/2020 12:26, Alexey Bakhtin wrote:

Hello Daniel,

Thank you for review.

As you suggested I’ve added static factory methods to create TlsChannelBinding class and 
changed connectionTimeout verification to  "if (connectTimeout <= 0)"
Also, I’ve added simple regression test to verify Channel Binding parameters.

Please find updated webrev 
at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v8/

The link to CSR for this feature 
:https://bugs.openjdk.java.net/browse/JDK-8247311

Regards
Alexey

Re: RFR [15] 8248233: Avoid superfluous Class::isRecord invocations during deserialization

[Peter Levart]

On 6/24/20 6:53 PM, Chris Hegarty wrote:


On 24 Jun 2020, at 17:15, Claes Redestad > wrote:


...
It seems ObjectInputStream#isRecord(Class) is now unused. No need for
a new webrev if you choose to remove it.


Good catch, now removed.

On 24 Jun 2020, at 17:25, Peter Levart > wrote:


Hi Chris,

The patch looks good.

Before the patch it made sense to have if (cl != null) in line 750 in 
ObjectStreamClass, but now nothing in the if block depends on cl, so 
you could use if (osc != null) instead. It is true that:


(cl != null) == (osc != null)

always holds there, but reading the code is easier that way, don't 
you think? Maybe you could even consider merging the content of this 
if block into the similar if block that starts in line 771?



Yes, good idea. Done.

Updated webrev:
https://cr.openjdk.java.net/~chegar/8248233/webrev.01

-Chris.


This looks good.


Now next thing to do perhaps is to find out why deserialization of 
classical classes is slower than deserialization of records ;-)



Regards, Peter

Re: RFR: 8247532, 8248135: Records deserialization is slow + Build microbenchmarks with --enable-preview

[Brian Goetz]

Consider a ClassValue for this, if there is not an obvious bit to be 
repurposed somewhere.


On 6/23/2020 12:30 PM, Chris Hegarty wrote:

I think it is worth considering caching the record-ness state of a j.l.Class, 
as I’m sure it will be widely used in third-party serialization libraries, as 
well as by Java Serialization.

Contributing to JEP 356: Enhanced Pseudo-Random Number Generators ?

[Raffaello Giulietti]

Hi,

seems like the last activity in JBS related to this JEP [1, 2] dates 
back to June 2019.


Are there major stumbling blocks?
Can I contribute in any way?


Greetings
Raffaello



[1] http://openjdk.java.net/jeps/356
[2] https://bugs.openjdk.java.net/browse/JDK-8193209

Re: RFR [15] 8248233: Avoid superfluous Class::isRecord invocations during deserialization

[Chris Hegarty]

> On 24 Jun 2020, at 17:15, Claes Redestad  wrote:
> 
> ...
> It seems ObjectInputStream#isRecord(Class) is now unused. No need for
> a new webrev if you choose to remove it.

Good catch, now removed.

> On 24 Jun 2020, at 17:25, Peter Levart  wrote:
> 
> Hi Chris,
> 
> The patch looks good.
> 
> Before the patch it made sense to have if (cl != null) in line 750 in 
> ObjectStreamClass, but now nothing in the if block depends on cl, so you 
> could use if (osc != null) instead. It is true that:
> 
> (cl != null) == (osc != null) 
> 
> always holds there, but reading the code is easier that way, don't you think? 
> Maybe you could even consider merging the content of this if block into the 
> similar if block that starts in line 771?
> 
Yes, good idea. Done.

Updated webrev:
  https://cr.openjdk.java.net/~chegar/8248233/webrev.01

-Chris.

Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI

[Alexey Semenyuk]

Aleksei,

If I get it right, the fix would not work for the case when there are 
`arguments` properties in `ArgOptions` section of a config file.
Why not just check if the parent process is the same executable as the 
current one and if this is the case don't read data from the config file 
but pass executable arguments as is in JLI_Launch() call?


- Alexey

On 6/24/2020 11:48 AM, Aleksei Voitylov wrote:

Hi,

There are systems that update LD_LIBRARY_PATH or directly return
JNI_TRUE in method RequiresSetenv(const char *jvmpath) from java_md.c
file to request re-execution of the current executable. This leads to
the fact that jpackage application adds some provided arguments twice.

Bug: https://bugs.openjdk.java.net/browse/JDK-8248239
Fix: http://cr.openjdk.java.net/~avoitylov/webrev.8248239.00/

The root cause of the issue is that jpackage application expects one
number of arguments but JLI reexecutes them with another number of
arguments.
  
For example, a jpackage application can be run with arguments:

     ./app/bin/app -Dparam2=Param2 B1 B2 B3
it adds arguments from the config file and calls JLI with arguments:
     app/bin/app -classpath  -Dparam1=Param1 -m com.hello/com.hello.Hello
-Dparam2=Param2 B1 B2 B3
JLI re-executes the app with new arguments so the app adds some
arguments one more time after the re-execution.
     ./app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3

A step by step example that illustrates the issue:

Run jpackage to create an executable application:
   jpackage --dest output --name app --type app-image --module-path
input-modules --module com.hello/com.hello.Hello --arguments "A1 A2 A3"
--verbose --java-options -Dparam1=Param1

Executable application with the app/lib/app/app.cfg config file is created:
 app.cfg  
[Application]
app.name=app
app.version=1.0
app.runtime=$ROOTDIR/lib/runtime
app.identifier=com.hello
app.classpath=
app.mainmodule=com.hello/com.hello.Hello

[JavaOptions]
java-options=-Dparam1=Param1

[ArgOptions]
arguments=A1
arguments=A2
arguments=A3
---

Run the application:
    ./output/app/bin/app -Dparam2=Param2 B1 B2 B3

Chain of JDK methods execution:

LinuxLauncher main()
    args: 5 [./app/bin/app -Dparam2=Param2 B1 B2 B3 ]
AppLauncher createJvmLauncher()
    args: 4 [-Dparam2=Param2 B1 B2 B3 ]
JvmLauncher.cpp Jvm::initFromConfigFile() - adds JavaOptions from app.cfg
    args: 10 [app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ]
AppLauncher Jvm::launch() -  JLI re-executes the app
LinuxLauncher main()
   args: 10 [app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ]
AppLauncher createJvmLauncher()
    args: 9 [-classpath  -Dparam1=Param1 -m com.hello/com.hello.Hello
-Dparam2=Param2 B1 B2 B3 ]
JvmLauncher.cpp Jvm::initFromConfigFile() - adds JavaOptions from app.cfg
    args: 15 [./app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ]
     ^^^

Some arguments like "-classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello" are added twice.

Tested with test/jdk/tools/jpackage/share/jdk/jpackage with no
regressions on Linux, Windows, Mac. On systems affected, the tests now pass.

Thanks,

-Aleksei

Re: [15] RFR JDK-8247785: Small clarification to the javadoc about builtin class loaders

[Roger Riggs]

Hi Mandy,

I'm fine with this.

Roger


On 6/23/20 5:42 PM, Mandy Chung wrote:



On 6/23/20 12:01 PM, Roger Riggs wrote:

Hi Mandy,

There may be a missing "to" in:

+ * Platform classes are visible the platform class loader
++ * Platform classes are visible *via* the platform 
class loader




I caught this accidental change too.

The second change seems to be self referential using "parent" to 
define itself.


And pre-existing in the description of getSystemClassLoader:

* The platform class loader is a parent or an ancestor of the system 
class * loader that all platform classes are visible to it.


Is missing "so" in :

* loader so that all platform classes are visible to it.

Both paragraphs are difficult to read and understand. (  I think the 
originals are more readable).


I made a minor adjustment to break the sentence into two.  That should 
help.


diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java 
b/src/java.base/share/classes/java/lang/ClassLoader.java

--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -133,8 +133,9 @@
  * It is the virtual machine's built-in class loader, typically 
represented

  * as {@code null}, and does not have a parent.
  * {@linkplain #getPlatformClassLoader() Platform class loader}.
- * All platform classes are visible to the platform 
class loader

- * that can be used as the parent of a {@code ClassLoader} instance.
+ * Platform classes are visible to the platform class 
loader.
+ * The platform class loader can be used as the parent of a 
{@code ClassLoader}

+ * instance.
  * Platform classes include Java SE platform APIs, their 
implementation

  * classes and JDK-specific run-time classes that are defined by the
  * platform class loader or its ancestors.
@@ -152,7 +153,7 @@
  * The system class loader is typically used to define classes on the
  * application class path, module path, and JDK-specific tools.
  * The platform class loader is a parent or an ancestor of the 
system class

- * loader that all platform classes are visible to it.
+ * loader.  It searches and loads the platform classes through 
its parent.

  * 
  *
  *  Normally, the Java virtual machine loads classes from the 
local file


Thanks
Mandy

Re: RFR [15] 8248233: Avoid superfluous Class::isRecord invocations during deserialization

[Peter Levart]

Hi Chris,


The patch looks good.


Before the patch it made sense to have if (cl != null) in line 750 in 
ObjectStreamClass, but now nothing in the if block depends on cl, so you 
could use if (osc != null) instead. It is true that:



(cl != null) == (osc != null)


always holds there, but reading the code is easier that way, don't you 
think? Maybe you could even consider merging the content of this if 
block into the similar if block that starts in line 771?



Regards, Peter


On 6/24/20 5:46 PM, Chris Hegarty wrote:

A micro-optimisation noticed when working on JDK-8247532.

For further details see:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067446.html

Webrev:
https://cr.openjdk.java.net/~chegar/8248233/webrev.00/

before:
RecordSerializationBench.deserializeClasses10avgt 10 13.874 ±1.445us/op
 RecordSerializationBench.deserializeClasses       100  avgt   10   
57.839 ±  3.944  us/op
 RecordSerializationBench.deserializeClasses     1000  avgt   10  
515.483 ± 57.275  us/op
 RecordSerializationBench.deserializeRecords       10  avgt   10   
13.563 ±  0.459  us/op
 RecordSerializationBench.deserializeRecords       100  avgt   10   
61.704 ±  2.481  us/op
 RecordSerializationBench.deserializeRecords     1000  avgt   10  
518.671 ± 19.147  us/op

after:
RecordSerializationBench.deserializeClasses10avgt 10 16.021 ±9.091us/op
 RecordSerializationBench.deserializeClasses       100  avgt   10   
58.550 ±  2.164  us/op
 RecordSerializationBench.deserializeClasses     1000  avgt   10  
524.930 ± 49.663  us/op
 RecordSerializationBench.deserializeRecords       10  avgt   10   
12.567 ±  0.711  us/op
 RecordSerializationBench.deserializeRecords       100  avgt   10   
50.235 ±  1.977  us/op
 RecordSerializationBench.deserializeRecords     1000  avgt   10  
421.557 ± 17.348  us/op


-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8247532

Re: RFR [15] 8248233: Avoid superfluous Class::isRecord invocations during deserialization

[Claes Redestad]

Hi Chris,

On 2020-06-24 17:46, Chris Hegarty wrote:

A micro-optimisation noticed when working on JDK-8247532.

For further details see:
   https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067446.html

Webrev:
   https://cr.openjdk.java.net/~chegar/8248233/webrev.00/


This looks good.

It seems ObjectInputStream#isRecord(Class) is now unused. No need for
a new webrev if you choose to remove it.

/Claes



before:
  RecordSerializationBench.deserializeClasses10  avgt   10   13.874 ±  
1.445  us/op
  RecordSerializationBench.deserializeClasses   100  avgt   10   57.839 ±  
3.944  us/op
  RecordSerializationBench.deserializeClasses  1000  avgt   10  515.483 ± 
57.275  us/op
  RecordSerializationBench.deserializeRecords10  avgt   10   13.563 ±  
0.459  us/op
  RecordSerializationBench.deserializeRecords   100  avgt   10   61.704 ±  
2.481  us/op
  RecordSerializationBench.deserializeRecords  1000  avgt   10  518.671 ± 
19.147  us/op
after:
  RecordSerializationBench.deserializeClasses10  avgt   10   16.021 ±  
9.091  us/op
  RecordSerializationBench.deserializeClasses   100  avgt   10   58.550 ±  
2.164  us/op
  RecordSerializationBench.deserializeClasses  1000  avgt   10  524.930 ± 
49.663  us/op
  RecordSerializationBench.deserializeRecords10  avgt   10   12.567 ±  
0.711  us/op
  RecordSerializationBench.deserializeRecords   100  avgt   10   50.235 ±  
1.977  us/op
  RecordSerializationBench.deserializeRecords  1000  avgt   10  421.557 ± 
17.348  us/op

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8247532

RFR [15] 8248233: Avoid superfluous Class::isRecord invocations during deserialization

[Chris Hegarty]

A micro-optimisation noticed when working on JDK-8247532. 

For further details see: 
  https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067446.html

Webrev:
  https://cr.openjdk.java.net/~chegar/8248233/webrev.00/

before:
 RecordSerializationBench.deserializeClasses10  avgt   10   13.874 ±  
1.445  us/op
 RecordSerializationBench.deserializeClasses   100  avgt   10   57.839 ±  
3.944  us/op
 RecordSerializationBench.deserializeClasses  1000  avgt   10  515.483 ± 
57.275  us/op
 RecordSerializationBench.deserializeRecords10  avgt   10   13.563 ±  
0.459  us/op
 RecordSerializationBench.deserializeRecords   100  avgt   10   61.704 ±  
2.481  us/op
 RecordSerializationBench.deserializeRecords  1000  avgt   10  518.671 ± 
19.147  us/op
after:
 RecordSerializationBench.deserializeClasses10  avgt   10   16.021 ±  
9.091  us/op
 RecordSerializationBench.deserializeClasses   100  avgt   10   58.550 ±  
2.164  us/op
 RecordSerializationBench.deserializeClasses  1000  avgt   10  524.930 ± 
49.663  us/op
 RecordSerializationBench.deserializeRecords10  avgt   10   12.567 ±  
0.711  us/op
 RecordSerializationBench.deserializeRecords   100  avgt   10   50.235 ±  
1.977  us/op
 RecordSerializationBench.deserializeRecords  1000  avgt   10  421.557 ± 
17.348  us/op

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8247532

RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI

[Aleksei Voitylov]

Hi,

There are systems that update LD_LIBRARY_PATH or directly return
JNI_TRUE in method RequiresSetenv(const char *jvmpath) from java_md.c
file to request re-execution of the current executable. This leads to
the fact that jpackage application adds some provided arguments twice.

Bug: https://bugs.openjdk.java.net/browse/JDK-8248239
Fix: http://cr.openjdk.java.net/~avoitylov/webrev.8248239.00/

The root cause of the issue is that jpackage application expects one
number of arguments but JLI reexecutes them with another number of
arguments.
 
For example, a jpackage application can be run with arguments:
    ./app/bin/app -Dparam2=Param2 B1 B2 B3
it adds arguments from the config file and calls JLI with arguments:
    app/bin/app -classpath  -Dparam1=Param1 -m com.hello/com.hello.Hello
-Dparam2=Param2 B1 B2 B3
JLI re-executes the app with new arguments so the app adds some
arguments one more time after the re-execution.
    ./app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3

A step by step example that illustrates the issue:

Run jpackage to create an executable application:
  jpackage --dest output --name app --type app-image --module-path
input-modules --module com.hello/com.hello.Hello --arguments "A1 A2 A3"
--verbose --java-options -Dparam1=Param1

Executable application with the app/lib/app/app.cfg config file is created:
 app.cfg  
[Application]
app.name=app
app.version=1.0
app.runtime=$ROOTDIR/lib/runtime
app.identifier=com.hello
app.classpath=
app.mainmodule=com.hello/com.hello.Hello

[JavaOptions]
java-options=-Dparam1=Param1

[ArgOptions]
arguments=A1
arguments=A2
arguments=A3
---

Run the application:
   ./output/app/bin/app -Dparam2=Param2 B1 B2 B3

Chain of JDK methods execution:

LinuxLauncher main()
   args: 5 [./app/bin/app -Dparam2=Param2 B1 B2 B3 ]
AppLauncher createJvmLauncher()
   args: 4 [-Dparam2=Param2 B1 B2 B3 ]
JvmLauncher.cpp Jvm::initFromConfigFile() - adds JavaOptions from app.cfg
   args: 10 [app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ]
AppLauncher Jvm::launch() -  JLI re-executes the app
LinuxLauncher main()
  args: 10 [app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ]
AppLauncher createJvmLauncher()
   args: 9 [-classpath  -Dparam1=Param1 -m com.hello/com.hello.Hello
-Dparam2=Param2 B1 B2 B3 ]
JvmLauncher.cpp Jvm::initFromConfigFile() - adds JavaOptions from app.cfg
   args: 15 [./app/bin/app -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ]
    ^^^

Some arguments like "-classpath  -Dparam1=Param1 -m
com.hello/com.hello.Hello" are added twice.

Tested with test/jdk/tools/jpackage/share/jdk/jpackage with no
regressions on Linux, Windows, Mac. On systems affected, the tests now pass.

Thanks,

-Aleksei

Re: RFR: JDK-8240148: Uncaught exceptions from ScheduledThreadPoolExecutor.execute aren't reported

[Martin Buchholz]

Thanks for your work on this.

Invoking the UEH machinery directly is extremely unusual and would
never have occurred to me (outside of a test).

In a loom-directed world, any attempt to directly manipulate the
current thread is likely to lead to trouble.

This code is primarily maintained separately at Doug's site
http://gee.cs.oswego.edu/dl/concurrency-interest/
I've been handling most of the integration work.

On Wed, Jun 24, 2020 at 6:51 AM Jaikiran Pai  wrote:
>
> Could I please get a review and a sponsor for a fix for
> https://bugs.openjdk.java.net/browse/JDK-8240148?
>
> The patch is available as a webrev at
> https://cr.openjdk.java.net/~jpai/webrev/8240148/1/
>
> The patch updates the ScheduledThreadPoolExecutor.execute(Runnable) to
> wrap the user passed Runnable into a custom one which then catches any
> Throwable and reports it through the Thread's uncaught exception
> handler. After having reviewed the existing code in that class, this was
> the easiest (and perhaps the only feasible?) and cleanest way that I
> could think of, to fix this issue.
>
> A few things about this change:
>
> 1. I decided to use an anonymous class for the Runnable, in the
> execute() method instead of a lambda because I was unsure if this part
> of the JDK code is allowed to use a lambda. In the past I have seen
> discussions where it was recommended not use lambda in certain parts of
> the JDK either because of performance(?) reason or because "this part of
> the code runs too early to be using a lambda" reason. I personally
> prefer an anonymous class anyway, but if others feel using a lambda here
> is relevant, then let me know.
>
> 2. Once the exception is reported using the UncaughtExceptionHandler, I
> just return. Initially, I had considered throwing back the original
> exception/throwable after reporting it via the UncaughtExceptionHandler,
> but having looked at few others places within the JDK which do not throw
> back the original exception, I decided to follow the same semantics.
>
> 3. Now that the passed Runnable is wrapped and submitted for execution,
> I considered whether this would break any (unwritten) contract between
> the ThreadPoolExecutor and the ThreadFactory#newThread(Runnable)
> implementations. If any (JDK internal or user application specific)
> ThreadFactory#newThread method was expecting the passed Runnable to be
> of the same type or the same instance as that was submitted to the
> ScheduledThreadPoolExecutor#execute(Runnable), then this change would
> break such ThreadFactory#newThread implementations. I looked deeper in
> the ThreadPoolExecutor code and from what I could see, this isn't a
> practical concern, since even without this change, The
> ThreadPoolExecutor in its private Worker class already sends a
> "delegate" Runnable (an instance of the
> ThreadPoolExecutor$Worker class) to the ThreadFactory's newThread method
> and not the original Runnable that was submitted to the
> execute(Runnable) method of the executor. So it doesn't look like
> there's any written/unwritten contract that the ThreadFactory is
> expected to receive the same Runnable instance that was passed to the
> execute method of the executor.
>
> 4. The ScheduledThreadPoolExecutor has a different license than some of
> the other files that I am used to, within the JDK code. So I haven't
> edited it for any of the usual copyright year updates.
>
> 5. The patch contains a new (testng based) test case which reproduces
> this issue and verifies the fix.
>
> -Jaikiran
>
>

RFR: JDK-8240148: Uncaught exceptions from ScheduledThreadPoolExecutor.execute aren't reported

[Jaikiran Pai]

Could I please get a review and a sponsor for a fix for
https://bugs.openjdk.java.net/browse/JDK-8240148?

The patch is available as a webrev at
https://cr.openjdk.java.net/~jpai/webrev/8240148/1/

The patch updates the ScheduledThreadPoolExecutor.execute(Runnable) to
wrap the user passed Runnable into a custom one which then catches any
Throwable and reports it through the Thread's uncaught exception
handler. After having reviewed the existing code in that class, this was
the easiest (and perhaps the only feasible?) and cleanest way that I
could think of, to fix this issue.

A few things about this change:

1. I decided to use an anonymous class for the Runnable, in the
execute() method instead of a lambda because I was unsure if this part
of the JDK code is allowed to use a lambda. In the past I have seen
discussions where it was recommended not use lambda in certain parts of
the JDK either because of performance(?) reason or because "this part of
the code runs too early to be using a lambda" reason. I personally
prefer an anonymous class anyway, but if others feel using a lambda here
is relevant, then let me know.

2. Once the exception is reported using the UncaughtExceptionHandler, I
just return. Initially, I had considered throwing back the original
exception/throwable after reporting it via the UncaughtExceptionHandler,
but having looked at few others places within the JDK which do not throw
back the original exception, I decided to follow the same semantics.

3. Now that the passed Runnable is wrapped and submitted for execution,
I considered whether this would break any (unwritten) contract between
the ThreadPoolExecutor and the ThreadFactory#newThread(Runnable)
implementations. If any (JDK internal or user application specific)
ThreadFactory#newThread method was expecting the passed Runnable to be
of the same type or the same instance as that was submitted to the
ScheduledThreadPoolExecutor#execute(Runnable), then this change would
break such ThreadFactory#newThread implementations. I looked deeper in
the ThreadPoolExecutor code and from what I could see, this isn't a
practical concern, since even without this change, The
ThreadPoolExecutor in its private Worker class already sends a
"delegate" Runnable (an instance of the
ThreadPoolExecutor$Worker class) to the ThreadFactory's newThread method
and not the original Runnable that was submitted to the
execute(Runnable) method of the executor. So it doesn't look like
there's any written/unwritten contract that the ThreadFactory is
expected to receive the same Runnable instance that was passed to the
execute method of the executor.

4. The ScheduledThreadPoolExecutor has a different license than some of
the other files that I am used to, within the JDK code. So I haven't
edited it for any of the usual copyright year updates.

5. The patch contains a new (testng based) test case which reproduces
this issue and verifies the fix.

-Jaikiran

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Magnus Ihse Bursie]

On 2020-06-18 08:34, Kim Barrett wrote:

On Jun 15, 2020, at 7:41 AM, Kim Barrett  wrote:


On Jun 14, 2020, at 12:45 AM, Philip Race  wrote:

Kim,


Until it says in "the JDK" and not "in HotSpot" you have not addressed my main 
point.
Please rename the JEP.

After some off-list discussions I have a better idea of what Phil was
asking for and why. In response I've changed the JEP, replacing a few
occurrences of "HotSpot" with "the JDK", as described below.  All
other occurrences of "HotSpot" have been left as-is.

Title:
JEP 347: Adopt C++14 Language Features in HotSpot
=>
JEP 347: Adopt C++14 Language Features in the JDK

Summary:
Allow the use of selected C++14 language features in the HotSpot C++ source 
code.
=>
Allow the use of selected C++14 language features in the JDK C++ source code.

Goals:
The purpose of this JEP is to formally allow C++ source code changes within 
HotSpot to take advantage of some C++14 standard language features.
=>
The purpose of this JEP is to formally allow C++ source code changes within the 
JDK to take advantage of some C++14 standard language features.


This all looks good to me.

/Magnus

RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Patrick Concannon]

Hi,

Could someone please review myself and Julia's RFE and CSR for JDK-8238286 - 
'Add new flatMap stream operation that is more amenable to pushing’?

This proposal is to add a new flatMap-like operation:

` Stream mapMulti(BiConsumer, ? super T> mapper)` 

to the java.util.Stream class. This operation is more receptive to the pushing 
or yielding of values than the current implementation that internally assembles 
values (if any) into one or more streams. This addition includes the primitive 
variations of the operation i.e. mapMultiToInt, IntStream mapMulti, etc.

issue: https://bugs.openjdk.java.net/browse/JDK-8238286 

csr: https://bugs.openjdk.java.net/browse/JDK-8248166 


webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.00/ 

specdiff: 
http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.00/overview-summary.html
  



Kind regards,
Patrick & Julia

Re: RFR: 8247532, 8248135: Records deserialization is slow + Build microbenchmarks with --enable-preview

[Chris Hegarty]

Peter,

> On 24 Jun 2020, at 10:16, Peter Levart  wrote:
> 
> Hi,
> 
> I wonder why "isRecord" was not encoded in class modifier bits, like "isEnum" 
> was for example. Are all 32 bits already taken?
> 
These bits are precious and limited! ;-) The record-ness of a class is already 
implicit by the presence of the Record attribute, we don’t really need another 
way to represent it. I’m sure there was some discussion relating to this on 
amber-dev, but I don’t have it to hand.

> The isEnum() does not have the performance problem since getModifiers() 
> native method is intrinsified.
> 
I don’t think we need anything too “fancy” here. I think there are quite 
straight forward ways to achieve the performance characteristics that we want, 
I’ve already outlined two possibilities (well.. the first is not really a 
serious possibility, just poking the bear!). If we encode these single state 
bits into a modifiers-like value, then we can treat them as such (without 
needing them to be “real” modifiers). And this should address any potential 
bloating concerns.

-Chris.
> Regards, Peter
> 
> 
> 
> On 6/24/20 10:20 AM, Chris Hegarty wrote:
>> 
>> 
>>> On 24 Jun 2020, at 07:03, David Holmes >> > wrote:
>>> 
>>> Hi Chris,
>>> 
>>> On 24/06/2020 2:30 am, Chris Hegarty wrote:
> On 23 Jun 2020, at 14:49, Peter Levart  > wrote:
> 
> ...
> 
> Ok, I'm going to push this to jdk15.
 Thank you Peter. This is a really nice change.
 As a follow on, and not for JDK 15, I observe that Class::isRecord0 / 
 JVM_IsRecord shows up as consuming a significant amount of time, more than 
 10%, in some runs of the deserialization benchmark. The isRecord 
 implementation is a native method in the JVM, so relatively expensive to 
 call.
 This shows an opportunity to improve the Class::isRecord implementation 
 with a simple cache of the record-ness of the j.l.Class, as is done 
 selectively with other particular aspects of a class’s state. There are 
 various ways to implement this, but here is just one [*].
>>> 
>>> There has been reluctance to add more and more fields to Class to cache all 
>>> these new attributes that are being added
>> 
>> Yeah, that seems reasonable. The extra bloat should be given due 
>> consideration.
>> 
>> I’ve not yet counted how many of these isThis and isThat methods that there 
>> are, but I suspect that there are enough that could warrant their state 
>> being encoded into a single int or long value on j.l.Class (that could be 
>> set lazily by the VM). This would setup a convenient and reasonably 
>> efficient location to add future pieces of cached state, like isSealed.
>> 
>>> - but ultimately that is a call for core-libs folk to make. The general 
>>> expectation is/was that the need to ask a class if it is a Record (or 
>>> isSealed etc) would be rare. But (de)serialization is the exception for 
>>> isRecord() as unlike enums a simple instanceof test can't be used.
>> 
>> It is relatively inexpensive to ask a non-record class if it is a record, 
>> but the converse is not the case.
>> 
>> Java Serialization can probably “workaround” this, since it already has a 
>> level of local-class cache state, so we can leverage that [*], which is 
>> probably the right thing to do for Java Serialization anyway, but I still 
>> think that there is a general tractable problem here.
>> 
>> -Chris.
>> 
>> [*]
>> diff -r 3a9521647349 
>> src/java.base/share/classes/java/io/ObjectInputStream.java
>> --- a/src/java.base/share/classes/java/io/ObjectInputStream.java Tue Jun 
>> 23 10:46:39 2020 +0100
>> +++ b/src/java.base/share/classes/java/io/ObjectInputStream.java Wed Jun 
>> 24 09:07:07 2020 +0100
>> @@ -2182,7 +2182,7 @@
>>  handles.markException(passHandle, resolveEx);
>>  }
>>  
>> -final boolean isRecord = cl != null && isRecord(cl);
>> +final boolean isRecord = desc.isRecord();
>>  if (isRecord) {
>>  assert obj == null;
>>  obj = readRecord(desc);
>> 
>> 

Re: RFR: 8247532, 8248135: Records deserialization is slow + Build microbenchmarks with --enable-preview

[Peter Levart]

Hi,


I wonder why "isRecord" was not encoded in class modifier bits, like 
"isEnum" was for example. Are all 32 bits already taken? The isEnum() 
does not have the performance problem since getModifiers() native method 
is intrinsified.



Regards, Peter


On 6/24/20 10:20 AM, Chris Hegarty wrote:



On 24 Jun 2020, at 07:03, David Holmes > wrote:


Hi Chris,

On 24/06/2020 2:30 am, Chris Hegarty wrote:
On 23 Jun 2020, at 14:49, Peter Levart > wrote:


...

Ok, I'm going to push this to jdk15.

Thank you Peter. This is a really nice change.
As a follow on, and not for JDK 15, I observe that Class::isRecord0 
/ JVM_IsRecord shows up as consuming a significant amount of time, 
more than 10%, in some runs of the deserialization benchmark. The 
isRecord implementation is a native method in the JVM, so relatively 
expensive to call.
This shows an opportunity to improve the Class::isRecord 
implementation with a simple cache of the record-ness of the 
j.l.Class, as is done selectively with other particular aspects of a 
class’s state. There are various ways to implement this, but here is 
just one [*].


There has been reluctance to add more and more fields to Class to 
cache all these new attributes that are being added


Yeah, that seems reasonable. The extra bloat should be given due 
consideration.


I’ve not yet counted how many of these isThis and isThat methods that 
there are, but I suspect that there are enough that could warrant 
their state being encoded into a single int or long value on j.l.Class 
(that could be set lazily by the VM). This would setup a convenient 
and reasonably efficient location to add future pieces of cached 
state, like isSealed.


- but ultimately that is a call for core-libs folk to make. The 
general expectation is/was that the need to ask a class if it is a 
Record (or isSealed etc) would be rare. But (de)serialization is the 
exception for isRecord() as unlike enums a simple instanceof test 
can't be used.


It is relatively inexpensive to ask a non-record class if it is a 
record, but the converse is not the case.


Java Serialization can probably “workaround” this, since it already 
has a level of local-class cache state, so we can leverage that [*], 
which is probably the right thing to do for Java Serialization anyway, 
but I still think that there is a general tractable problem here.


-Chris.

[*]
diff -r 3a9521647349 
src/java.base/share/classes/java/io/ObjectInputStream.java
--- a/src/java.base/share/classes/java/io/ObjectInputStream.javaTue 
Jun 23 10:46:39 2020 +0100
+++ b/src/java.base/share/classes/java/io/ObjectInputStream.javaWed 
Jun 24 09:07:07 2020 +0100

@@ -2182,7 +2182,7 @@
handles.markException(passHandle, resolveEx);
         }


-        final boolean isRecord = cl != null && isRecord(cl);
+        final boolean isRecord = desc.isRecord();
         if (isRecord) {
             assert obj == null;
             obj = readRecord(desc);

RFR: JDK-8247592: refactor test/jdk/tools/launcher/Test7029048.java

[Aleksei Voitylov]

Hi,

I'd like to refactor test/jdk/tools/launcher/Test7029048.java, make the
logic easier to follow and remove some magic numbers from the test:

JBS: https://bugs.openjdk.java.net/browse/JDK-8247592
Webrev: http://cr.openjdk.java.net/~avoitylov/webrev.8247592.01/

Testing: the test passes on Linux x86, Linux x86_64, Linux ARM, Linux
AArch64, Linux PPC, Windows x86, Windows x86_64, Mac, AIX. Special
thanks to SAP team for helping test on AIX.

Thanks,
-Aleksei

Re: RFR: 8247532, 8248135: Records deserialization is slow + Build microbenchmarks with --enable-preview

[Chris Hegarty]

> On 24 Jun 2020, at 07:03, David Holmes  wrote:
> 
> Hi Chris,
> 
> On 24/06/2020 2:30 am, Chris Hegarty wrote:
>>> On 23 Jun 2020, at 14:49, Peter Levart  wrote:
>>> 
>>> ...
>>> 
>>> Ok, I'm going to push this to jdk15.
>> Thank you Peter. This is a really nice change.
>> As a follow on, and not for JDK 15, I observe that Class::isRecord0 / 
>> JVM_IsRecord shows up as consuming a significant amount of time, more than 
>> 10%, in some runs of the deserialization benchmark. The isRecord 
>> implementation is a native method in the JVM, so relatively expensive to 
>> call.
>> This shows an opportunity to improve the Class::isRecord implementation with 
>> a simple cache of the record-ness of the j.l.Class, as is done selectively 
>> with other particular aspects of a class’s state. There are various ways to 
>> implement this, but here is just one [*].
> 
> There has been reluctance to add more and more fields to Class to cache all 
> these new attributes that are being added

Yeah, that seems reasonable. The extra bloat should be given due consideration.

I’ve not yet counted how many of these isThis and isThat methods that there 
are, but I suspect that there are enough that could warrant their state being 
encoded into a single int or long value on j.l.Class (that could be set lazily 
by the VM). This would setup a convenient and reasonably efficient location to 
add future pieces of cached state, like isSealed.

> - but ultimately that is a call for core-libs folk to make. The general 
> expectation is/was that the need to ask a class if it is a Record (or 
> isSealed etc) would be rare. But (de)serialization is the exception for 
> isRecord() as unlike enums a simple instanceof test can't be used.

It is relatively inexpensive to ask a non-record class if it is a record, but 
the converse is not the case.

Java Serialization can probably “workaround” this, since it already has a level 
of local-class cache state, so we can leverage that [*], which is probably the 
right thing to do for Java Serialization anyway, but I still think that there 
is a general tractable problem here.

-Chris.

[*]
diff -r 3a9521647349 src/java.base/share/classes/java/io/ObjectInputStream.java
--- a/src/java.base/share/classes/java/io/ObjectInputStream.javaTue Jun 
23 10:46:39 2020 +0100
+++ b/src/java.base/share/classes/java/io/ObjectInputStream.javaWed Jun 
24 09:07:07 2020 +0100
@@ -2182,7 +2182,7 @@
 handles.markException(passHandle, resolveEx);
 }
 
-final boolean isRecord = cl != null && isRecord(cl);
+final boolean isRecord = desc.isRecord();
 if (isRecord) {
 assert obj == null;
 obj = readRecord(desc);

Build error with GCC 10 in NetworkInterface.c and k_standard.c

[Koichi Sakata]

Hi all,

(I've sent a similar e-mail before to this ML, but I extract the part 
only related to core-libs-dev ML from the previous one.)


I tried to build OpenJDK fastdebug with GCC 10.1 on Ubuntu 18.04, but I 
saw some compiler warnings as follows:


/home/jyukutyo/code/jdk/src/java.base/share/native/libfdlibm/: In 
function '__j__kernel_standard':
/home/jyukutyo/code/jdk/src/java.base/share/native/libfdlibm/k_standard.c:743:19: 
error: 'exc.retval' may be used uninitialized in this function 
[-Werror=maybe-uninitialized]

  743 | return exc.retval;
  |~~~^~~

In file included from /usr/include/string.h:494,
 from 
/home/jyukutyo/code/jdk/src/java.base/unix/native/libnet/NetworkInterface.c:30:

In function 'strncpy',
inlined from 'getFlags' at 
/home/jyukutyo/code/jdk/src/java.base/unix/native/libnet/NetworkInterface.c:1362:5,
inlined from 'addif' at 
/home/jyukutyo/code/jdk/src/java.base/unix/native/libnet/NetworkInterface.c:974:13:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: 
'__builtin_strncpy' output may be truncated copying 15 bytes from a 
string of length 15 [-Werror=stringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos 
(__dest));

  | ^~

I can resolve them with the following patch. I believe it fixes those 
potential bugs, so I'd like to contribute it.

(Our company has signed OCA.)

Thanks,
Koichi

= PATCH =
diff -r 20d92fe3ac52 src/java.base/share/native/libfdlibm/k_standard.c
--- a/src/java.base/share/native/libfdlibm/k_standard.c Tue Jun 16 
03:16:41 2020 +
+++ b/src/java.base/share/native/libfdlibm/k_standard.c Thu Jun 18 
07:08:50 2020 +0900

@@ -739,6 +739,10 @@
 errno = EDOM;
 }
 break;
+default:
+exc.retval = zero;
+errno = EINVAL;
+break;
 }
 return exc.retval;
 }
diff -r 20d92fe3ac52 src/java.base/unix/native/libnet/NetworkInterface.c
--- a/src/java.base/unix/native/libnet/NetworkInterface.c   Tue Jun 
16 03:16:41 2020 +
+++ b/src/java.base/unix/native/libnet/NetworkInterface.c   Thu Jun 
18 07:08:50 2020 +0900

@@ -1296,7 +1296,10 @@
 static int getIndex(int sock, const char *name) {
 struct ifreq if2;
 memset((char *), 0, sizeof(if2));
-strncpy(if2.ifr_name, name, sizeof(if2.ifr_name) - 1);
+if (sizeof(if2.ifr_name) < sizeof(name)) {
+return -1;
+}
+strcpy(if2.ifr_name, name);

 if (ioctl(sock, SIOCGIFINDEX, (char *)) < 0) {
 return -1;
@@ -1359,7 +1362,10 @@
 static int getFlags(int sock, const char *ifname, int *flags) {
 struct ifreq if2;
 memset((char *), 0, sizeof(if2));
-strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name) - 1);
+if (sizeof(if2.ifr_name) < sizeof(ifname)) {
+return -1;
+}
+strcpy(if2.ifr_name, ifname);

 if (ioctl(sock, SIOCGIFFLAGS, (char *)) < 0) {
 return -1;

Re: RFR: 8247532, 8248135: Records deserialization is slow + Build microbenchmarks with --enable-preview

[David Holmes]

Hi Chris,

On 24/06/2020 2:30 am, Chris Hegarty wrote:



On 23 Jun 2020, at 14:49, Peter Levart  wrote:

...

Ok, I'm going to push this to jdk15.


Thank you Peter. This is a really nice change.

As a follow on, and not for JDK 15, I observe that Class::isRecord0 / 
JVM_IsRecord shows up as consuming a significant amount of time, more than 10%, 
in some runs of the deserialization benchmark. The isRecord implementation is a 
native method in the JVM, so relatively expensive to call.

This shows an opportunity to improve the Class::isRecord implementation with a 
simple cache of the record-ness of the j.l.Class, as is done selectively with 
other particular aspects of a class’s state. There are various ways to 
implement this, but here is just one [*].


There has been reluctance to add more and more fields to Class to cache 
all these new attributes that are being added - but ultimately that is a 
call for core-libs folk to make. The general expectation is/was that the 
need to ask a class if it is a Record (or isSealed etc) would be rare. 
But (de)serialization is the exception for isRecord() as unlike enums a 
simple instanceof test can't be used.


Cheers,
David
-


Running the deserialization benchmark with this change [*], gives the following 
results:

Benchmark(length)  Mode  CntScore
Error  Units
RecordSerializationBench.deserializeClasses10  avgt   10   14.136 ±  
0.841  us/op
RecordSerializationBench.deserializeClasses   100  avgt   10   61.821 ±  
1.279  us/op
RecordSerializationBench.deserializeClasses  1000  avgt   10  519.473 ±  
7.950  us/op
RecordSerializationBench.deserializeRecords10  avgt   10   13.781 ±  
1.917  us/op
RecordSerializationBench.deserializeRecords   100  avgt   10   54.061 ±  
4.188  us/op
RecordSerializationBench.deserializeRecords  1000  avgt   10  444.538 ± 
13.940  us/op

I think it is worth considering caching the record-ness state of a j.l.Class, 
as I’m sure it will be widely used in third-party serialization libraries, as 
well as by Java Serialization.

-Chris.

[*]
diff -r 3a9521647349 src/java.base/share/classes/java/lang/Class.java
--- a/src/java.base/share/classes/java/lang/Class.java  Tue Jun 23 10:46:39 
2020 +0100
+++ b/src/java.base/share/classes/java/lang/Class.java  Tue Jun 23 17:11:35 
2020 +0100
@@ -3712,9 +3712,17 @@
  
@jdk.internal.PreviewFeature(feature=jdk.internal.PreviewFeature.Feature.RECORDS,
   essentialAPI=false)
  public boolean isRecord() {
-return getSuperclass() == JAVA_LANG_RECORD_CLASS && isRecord0();
+if (!isRecordCalled) {
+isRecord = getSuperclass() == JAVA_LANG_RECORD_CLASS && 
isRecord0();
+isRecordCalled = true;
+}
+return isRecord;
  }
  
+// cached record(ness) status

+private transient boolean isRecord;
+private transient boolean isRecordCalled;
+
  // Fetches the factory for reflective objects
  private static ReflectionFactory getReflectionFactory() {
  if (reflectionFactory == null) {