Re: RFR: 8230301: Re-examine hardcoded defaults in GenerateJLIClassesPlugin

[Mandy Chung]

On 9/6/19 6:29 PM, Claes Redestad wrote:


The original default hardcoded list includes many more forms. What is 
the diff of the jlink-plugin generated classes before and after this 
patch?


I wonder if we should include a few other method types in 
HelloClassList like float parameter.  While they are not used in the 
startup benchmarks, they may be used for some other application 
runtime which has been benefit from jlink-time generated classes.

Yes, there's a reduction of forms pre-generated by this, but that's sort
of the point since the list of defaults was picked by me during JDK 9
development to fit the needs of the lambda metafactory the ISC we had
back then. Since both have been refactored over the last 4 feature
releases, many of the shapes herein now can never be requested by the
current ISC implementation, and are likely dead weight.

So I think of this cleanup as reclaiming some footprint that we can
spend on widening the API coverage in HelloClasslist to include things
that are more likely to be used by a large set of applications.

I do think there's a small risk this cleanup removes a handful of oft-
used shapes, but I'm not sure if it'll ever register as a measurable
regression. It's possible, sure, so I'd prefer to do an exploratory
tuning round to patch this up as a follow-up RFE, and have that effort
be guided by any regressions we (or others) may be able to detect due
this cleanup.


I'm okay with this patch.

It'd be help to add a comment in the plugin what criteria to determine 
the shapes be pre-generated and give examples how to extend the 
pre-generated list in the future.


Mandy

Re: RFR: 8230301: Re-examine hardcoded defaults in GenerateJLIClassesPlugin

[Claes Redestad]

Hi Mandy,

On 2019-09-06 21:07, Mandy Chung wrote:

Hi Claes,

The patch looks much straightforward than I expected.


thanks? :-)



The original default hardcoded list includes many more forms.  What is 
the diff of the jlink-plugin generated classes before and after this patch?


I wonder if we should include a few other method types in HelloClassList 
like float parameter.  While they are not used in the startup 
benchmarks, they may be used for some other application runtime which 
has been benefit from jlink-time generated classes.

Yes, there's a reduction of forms pre-generated by this, but that's sort
of the point since the list of defaults was picked by me during JDK 9
development to fit the needs of the lambda metafactory the ISC we had
back then. Since both have been refactored over the last 4 feature
releases, many of the shapes herein now can never be requested by the
current ISC implementation, and are likely dead weight.

So I think of this cleanup as reclaiming some footprint that we can
spend on widening the API coverage in HelloClasslist to include things
that are more likely to be used by a large set of applications.

I do think there's a small risk this cleanup removes a handful of oft-
used shapes, but I'm not sure if it'll ever register as a measurable
regression. It's possible, sure, so I'd prefer to do an exploratory
tuning round to patch this up as a follow-up RFE, and have that effort
be guided by any regressions we (or others) may be able to detect due
this cleanup.

Thanks!

/Claes

Re: Comments on jpackage (JEP 343)

[Philip Race]

Please see https://bugs.openjdk.java.net/browse/JDK-8211182

IIRC there were also concerns that jpackage was not the right place to 
be defining this,
and also I think Andy wrote some boiler plate code that demonstrated it 
was not needed

for at least some of these use cases. Andy (?)

So not being revived soon.

-phil.

On 9/5/19, 2:53 AM, Lennart Börjeson wrote:

Could you please also revive the UserJvmOptionsService, that (for very unclear 
reasons) was removed together with JavaFX?

Since the functionality provided by the UserJvmOptionsService requires some 
cooperation with the launcher created by jpackage, I can't see how I could 
implement some work-around myself.

(I raised this question already in 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-March/059419.html, 
but without response.)

Best regards,

Lennart Börjeson

Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[Mandy Chung]

On 9/6/19 2:20 PM, Brent Christian wrote:

Update webrev is here:
http://cr.openjdk.java.net/~bchristi/8212117/webrev11/
with changes to jvm.h, MethodHandles.java, and Class.java.


Looks good.

Mandy

Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[Brent Christian]

Hi, Mandy

On 9/5/19 6:03 PM, Mandy Chung wrote:

On 9/5/19 5:09 PM, Brent Christian wrote:

jvm.h

  349  * Link the 'arg' class (unless ClassForNameDeferLinking is set)

I suggest to drop "(unless ...)" phrase because the flag is temporary.


Sure, good idea.


jni.cpp

The implementation of JNI FindClass has the same behavior as 1-arg
Class.forName(name).  The JNI spec needs fixing but it's a separate
issue.


Right - looks like David filed JDK-8230685.


find_class_from_class_loader
   I like David's suggestion to assert that if init == true, link
   must be true.  Alternatively, pass an enum instead of two booleans
   clearly tell linking or initializing.


Yup, assert added.


Lookup::findClass:

"In particular, the method first attempts to load the requested class"

I think this sentence is no longer needed together with your change.  What 
about:

  /**
- * Looks up a class by name from the lookup context defined by this 
{@code Lookup} object. The static
- * initializer of the class is not run.
+ * Looks up a class by name from the lookup context defined by this 
{@code Lookup} object.
+ * This method attempts to locate, load, and link the class, and then 
determines whether
+ * the class is accessible to this {@code Lookup} object.
+ * The static initializer of the class is not run.
   * 
   * The lookup context here is determined by the {@linkplain 
#lookupClass() lookup class}, its class
- * loader, and the {@linkplain #lookupModes() lookup modes}. In 
particular, the method first attempts to
- * load the requested class, and then determines whether the class is 
accessible to this lookup object.
+ * loader, and the {@linkplain #lookupModes() lookup modes}.


Looks good to me.


The note you added in this method should also be added to 2-arg
Class::forName for consistency.


OK, sure.


Update webrev is here:
  http://cr.openjdk.java.net/~bchristi/8212117/webrev11/
with changes to jvm.h, MethodHandles.java, and Class.java.

I went ahead and put together a specdiff[1] for the changed methods 
([2][3][4]).  I've updated and finalized the CSR.


Thanks for the review,
-Brent

1. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/specdiff-summary.html


2. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/Class-report.html#forName(java.lang.String,boolean,java.lang.ClassLoader)


3. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/Class-report.html#forName(java.lang.Module,java.lang.String)


4. 
http://cr.openjdk.java.net/~bchristi/8212117/webrev11-specdiff/invoke/MethodHandles.Lookup-report.html#findClass(java.lang.String)

Re: jpackage on MacOs: app from pkg cannot write files and create directories inside itself

[Alexander Matveev]

Hi Andrey,

Writing to  /Application on macOS or "Program Files" on Windows requires 
admin privileges. It is very uncommon when application stores temp files 
or setting files modifiable by user inside installation folder. For temp 
files I will recommend to use system provided temp folder. For setting 
it will depend on OS type.


Thanks,
Alexander

On 9/6/2019 5:49 AM, Andrey Volkov wrote:

Hello.

I use jpackage (Build 14-jpackage+1-35) to bundle my Swing-based app for
MacOS. Whereas DMG format works pretty well, PGK bundle has a serious
issue. When I install the app from PKG, it cannot write files or create
directories inside itself.
I have /resource directory for my app that has a few settings and temp
directory for file processing. For the app from PKG I get "File not Found
(Permission Denied)" when the app tries to create a directory like
"/Application/MyApp.app/Contents/Java/resources/temp" or any write/create
any file inside  "/Application/MyApp.app/Contents/Java/resources/"
Do you have any clue how to fix it for PKG bundle? I would prefer to use it
because it has a more user-friendly installation.

BTW, Windows has a similar issue when per-user installation works fine, but
a system-wide installation in "Program Files" lack permissions to write
files inside "Program File/MyApp".

Re: JDK 14 RFR of JDK-8230723: Remove default constructors from java.lang and java.io

[Brian Burkhalter]

Hi Joe,

> On Sep 6, 2019, at 11:12 AM, Joe Darcy  wrote:
> 
> First part of a multi-part cleanup effort please review
> 
> JDK-8230723: Remove default constructors from java.lang and java.io 
> 
> webrev: http://cr.openjdk.java.net/~darcy/8230723.0/ 
> 
> CSR: https://bugs.openjdk.java.net/browse/JDK-8230724 
> 

CSR reviewed.

> Patch below.
> 
> The default constructors on Modifier and ConstantBootstraps seem clearly 
> accidental so I deprecated them for removal.
> 
> The PrimitiveSlot and Factory classes are public classes within non-public 
> classes so I made the constructors package private rather than public. 
> However, this is not a spec change for the Java SE API.

On Factory I think the doc might be better as “Contructs” instead of 
“Construct” (like for ThreadDeath).

> I'll update the any copyright years as appropriate before pushing.

+1

Brian

Re: RFR: 8230301: Re-examine hardcoded defaults in GenerateJLIClassesPlugin

[Mandy Chung]

Hi Claes,

The patch looks much straightforward than I expected.

The original default hardcoded list includes many more forms.  What is 
the diff of the jlink-plugin generated classes before and after this patch?


I wonder if we should include a few other method types in HelloClassList 
like float parameter.  While they are not used in the startup 
benchmarks, they may be used for some other application runtime which 
has been benefit from jlink-time generated classes.


Mandy

On 9/2/19 7:02 AM, Claes Redestad wrote:

Hi,

we should avoid hard-coding the set of dynamically generated j.l.invoke
classes to pre-generate into the jlink plugin, instead favoring
generating the set of code to use at build-time.

Webrev: http://cr.openjdk.java.net/~redestad/8230301/webrev.00/

Several of the hard-coded defaults predate optimizations to remove MH
usage in lambda bootstrap and were generated using earlier iterations of
ISC. This means many classes/methods generated are actualy never used,
and that dropping all the defaults had relatively minimal effect on our
set of startup tests.

It's easy to create increasingly synthetic applications that suffer
marginally. I did some experiments and identified a few small API calls
we can add to HelloClasslist to recuperate a fair amount on some key
applications, while still significantly reducing size of pre-generated
code.

Testing: tier1-3

Thanks!

/Claes

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Martin Buchholz]

Martin's rules for test methods:
- never have more than 100 millisecond total expected runtime
- never have more than 12 millisecond blocking time for any single operation
- yet be able to survive a one-time 5 second thread suspension at any time

---

adjusting wait time looks like an anti-pattern to me.  You can't stop a
rare 5-second thread suspension at line 387.

 385 final long waitTime = hiMillis -
 386 NANOSECONDS.toMillis(System.nanoTime() - startNanos);
// (2) adjust wait time
 387
 388 try {
 389 T r = task.get(waitTime, MILLISECONDS); // (3) wait for
the task to complete


On Fri, Sep 6, 2019 at 9:33 AM Pavel Rappo  wrote:

> > On 6 Sep 2019, at 15:59, Martin Buchholz  wrote:
> >
> > I took another look at LdapTimeoutTest.java.
>
> Many thanks!
>
> > I was surprised to see RIGHT_MARGIN.  In jsr166 we succeed in having one
> timeout that is long enough to "never happen".  I'm still advocating the 10
> second value.
> >
> > I was surprised to see LEFT_MARGIN.  We just fixed Thread.sleep, so we
> have no known problems with JDK methods returning early - you can trust
> timed get!
> > You start measuring, by calling nanoTime, before you start the activity
> you are measuring, so there should be no need for LEFT_MARGIN.
>
> You raised many good points. Let me try to address them.
>
> 1. RIGHT_MARGIN is not used for checking that the activity has stuck
> infinitely (assertIncompletion). INFINITY_MILLIS is used for that.
> RIGHT_MARGIN is used for checking that the activity takes some predefined
> amount of time (roughly).
>
> 2. As for the numeric value of INFINITY_MILLIS, the reason I chose 20
> seconds is twofold. Firstly, the code under test is subject to different
> timeouts. Every timeout value should be distinctive. Otherwise, how would I
> differentiate between them? For example, if I chose INFINITY_MILLIS to be
> 10 seconds how would I know if the code is stuck due to the read timeout of
> 10 seconds or the "infinite timeout"? Secondly, I must take into account
> slow machines. Really slow virtual machines. Hence, minimal timeouts
> (read/connect) have a magnitude of seconds and tens of seconds and the
> "rightmost", infinite timeout, is 20 seconds.
>
> 3. LEFT_MARGIN might no longer be needed due to the fact that no timed
> methods return early (actually there is a comment about it inside those two
> assert methods).
>
> > We have some fresh thread-awaiting code here:
> >
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestCase.java?view=markup#l1443
>
> Interesting.
>
> > Instead of communicating startTime from the test thread back to the main
> thread, I would do my loMillis checking in the test thread, and hiMillis
> checking in the main thread, like e.g. compare with a fresh test method
> testTimedOffer
> >
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/ArrayBlockingQueueTest.java?view=markup#l394
>
> Understood. However, that might be a matter of taste. I prefer to have all
> the calculations and error handling in one place. Unless there's a good
> reason I wouldn't change it.
>
> > Timeouts should be adjusted via Utils.adjustTimeout
>
> That makes perfect sense. I never knew this method existed. Thanks!
>
> > On Fri, Sep 6, 2019 at 4:31 AM Pavel Rappo 
> wrote:
> > Martin, thanks for having a look at it.
> >
> > I'd appreciate if you could have a look at the timeout measuring
> mechanics in assertCompletion/assertIncompletion specifically, maybe to
> spot something that is grossly inadequate.
> >
> > I tried to accommodate some usual suspects of timeout measurements
> failures. I understand that since we're not working with real-time systems,
> my attempts to build bullet-proof measurement mechanics are futile.
> >
> > -Pavel
> >
> > > On 30 Aug 2019, at 18:19, Martin Buchholz  wrote:
> > >
> > > Not really a review, but:
> > >
> > > For many years we've been using 10 seconds (scaled by timeout factor)
> as a duration long enough that a timeout is a real failure.
> > > Which is close to your own 20 seconds.  Of course, no value is surely
> safe.
> > >
> > > Probably, parallel testing infrastructure for timeouts should be a
> test library method.  I do something similar in JSR166TestCase
> > >
> > > /**
> > >  * Runs all the given actions in parallel, failing if any fail.
> > >  * Useful for running multiple variants of tests that are
> > >  * necessarily individually slow because they must block.
> > >  */
> > > void testInParallel(Action ... actions) {
> > > ExecutorService pool = Executors.newCachedThreadPool();
> > > try (PoolCleaner cleaner = cleaner(pool)) {
> > >
> > > On Fri, Aug 30, 2019 at 6:23 AM Daniel Fuchs 
> wrote:
> > > On 30/08/2019 13:54, Pavel Rappo wrote:
> > > > Updated,
> > > >
> > > >  http://cr.openjdk.java.net/~prappo/8151678/webrev.01/
> > > >
> > >
> > > Changes look good!
> > >
> > > best regards,
> > >
> > > -- daniel
> >
>
>

Re: RFR: 8230662: Remove dead code from MethodTypeForm

[Mandy Chung]

Looks good.  This is a good simplification.

thanks
Mandy

On 9/5/19 7:41 AM, Claes Redestad wrote:

Hi,

I noticed some unused methods in java.lang.invoke.MethodTypeForm and
ended up with a rather substantial cleanup after pulling that particular
thread for a bit:

http://cr.openjdk.java.net/~redestad/8230662/jdk.00/
https://bugs.openjdk.java.net/browse/JDK-8230662

Testing: tier1-3

Thanks!

/Claes

JDK 14 RFR of JDK-8230723: Remove default constructors from java.lang and java.io

[Joe Darcy]

Hello,

First part of a multi-part cleanup effort please review

    JDK-8230723: Remove default constructors from java.lang and java.io
    webrev: http://cr.openjdk.java.net/~darcy/8230723.0/
    CSR: https://bugs.openjdk.java.net/browse/JDK-8230724

Patch below.

The default constructors on Modifier and ConstantBootstraps seem clearly 
accidental so I deprecated them for removal.


The PrimitiveSlot and Factory classes are public classes within 
non-public classes so I made the constructors package private rather 
than public. However, this is not a spec change for the Java SE API.


I'll update the any copyright years as appropriate before pushing.

Thanks,

-Joe

--- old/src/java.base/share/classes/java/io/InputStream.java 2019-09-06 
11:11:22.330589001 -0700
+++ new/src/java.base/share/classes/java/io/InputStream.java 2019-09-06 
11:11:22.146589001 -0700

@@ -56,6 +56,11 @@
 private static final int DEFAULT_BUFFER_SIZE = 8192;

 /**
+ * Constructor for subclasses to call.
+ */
+    public InputStream() {}
+
+    /**
  * Returns a new {@code InputStream} that reads no bytes. The returned
  * stream is initially open.  The stream is closed by calling the
  * {@code close()} method.  Subsequent calls to {@code close()} 
have no
--- old/src/java.base/share/classes/java/io/ObjectInputStream.java 
2019-09-06 11:11:22.838589001 -0700
+++ new/src/java.base/share/classes/java/io/ObjectInputStream.java 
2019-09-06 11:11:22.646589001 -0700

@@ -1321,6 +1321,10 @@
  * Provide access to the persistent fields read from the input stream.
  */
 public abstract static class GetField {
+    /**
+ * Constructor for subclasses to call.
+ */
+    public GetField() {}

 /**
  * Get the ObjectStreamClass that describes the fields in the 
stream.
--- old/src/java.base/share/classes/java/io/ObjectOutputStream.java 
2019-09-06 11:11:23.346589001 -0700
+++ new/src/java.base/share/classes/java/io/ObjectOutputStream.java 
2019-09-06 11:11:23.162589001 -0700

@@ -879,6 +879,10 @@
  * @since 1.2
  */
 public abstract static class PutField {
+    /**
+ * Constructor for subclasses to call.
+ */
+    public PutField() {}

 /**
  * Put the value of the named boolean field into the 
persistent field.
--- old/src/java.base/share/classes/java/io/OutputStream.java 2019-09-06 
11:11:23.866589001 -0700
+++ new/src/java.base/share/classes/java/io/OutputStream.java 2019-09-06 
11:11:23.674589001 -0700

@@ -47,6 +47,11 @@
  */
 public abstract class OutputStream implements Closeable, Flushable {
 /**
+ * Constructor for subclasses to call.
+ */
+    public OutputStream() {}
+
+    /**
  * Returns a new {@code OutputStream} which discards all bytes.  The
  * returned stream is initially open.  The stream is closed by calling
  * the {@code close()} method.  Subsequent calls to {@code 
close()} have
--- 
old/src/java.base/share/classes/java/lang/InheritableThreadLocal.java 
2019-09-06 11:11:24.394589001 -0700
+++ 
new/src/java.base/share/classes/java/lang/InheritableThreadLocal.java 
2019-09-06 11:11:24.182589001 -0700

@@ -52,6 +52,11 @@

 public class InheritableThreadLocal extends ThreadLocal {
 /**
+ * Creates an inheritable thread local variable.
+ */
+    public InheritableThreadLocal() {}
+
+    /**
  * Computes the child's initial value for this inheritable 
thread-local

  * variable as a function of the parent's value at the time the child
  * thread is created.  This method is called from within the parent
--- old/src/java.base/share/classes/java/lang/LiveStackFrame.java 
2019-09-06 11:11:24.894589001 -0700
+++ new/src/java.base/share/classes/java/lang/LiveStackFrame.java 
2019-09-06 11:11:24.690589001 -0700

@@ -116,6 +116,11 @@
  */
 public abstract class PrimitiveSlot {
 /**
+ * Constructor.
+ */
+    PrimitiveSlot() {}
+
+    /**
  * Returns the size, in bytes, of the slot.
  */
 public abstract int size();
--- old/src/java.base/share/classes/java/lang/ThreadDeath.java 
2019-09-06 11:11:25.486589001 -0700
+++ new/src/java.base/share/classes/java/lang/ThreadDeath.java 
2019-09-06 11:11:25.234589001 -0700

@@ -49,4 +49,9 @@
 public class ThreadDeath extends Error {
 @java.io.Serial
 private static final long serialVersionUID = -4417128565033088268L;
+
+    /**
+ * Constructs a {@code ThreadDeath}.
+ */
+    public ThreadDeath() {}
 }
--- 
old/src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java 
2019-09-06 11:11:26.150589001 -0700
+++ 
new/src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java 
2019-09-06 11:11:25.882589001 -0700

@@ -459,6 +459,11 @@
  */
 public class Factory {
 /**
+ * Construct a factory.
+ */
+    Factory() {}
+
+    /**
  * Get a concrete subclass of the top class for a given 
com

[14] RFR: 8230284: Accounting currency format support does not cope with explicit number system.

[naoto . sato]

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8230284

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8230284/webrev.00/

The original enhancement for the accounting currency format support 
(https://bugs.openjdk.java.net/browse/JDK-8215181) did not account for 
the explicit/implicit numbering script. The above change made it to 
share the same code with NumberElements which properly deals with the 
numbering script.


Naoto

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Milan Mimica]

On Thu, 5 Sep 2019 at 18:59, Florian Weimer  wrote:
>
> But I think in the UDP case, the client will retry.  I think the total
> timeout in the TCP case should equal the total timeout in the UDP case.
> That's what I'm going to implement for glibc.  The difference is that in
> the TCP case, the TCP stack will take care of the retries, not the
> application code.

I understand that, and it does make sense, but we have to put it in
context of how current DnsClient.java works:
//
// The UDP retry strategy is to try the 1st server, and then
// each server in order. If no answer, double the timeout
// and try each server again.
//

Fallback to TCP happens within this process. Going immediately with
timeout*2^maxRetry could yield significantly larger delays, if there
happens to be some other server on the list that works better.
I would rather look into reusing TCP connections, not to close them immediately.

What about read() and non-handshake TCP retransmissions? Do those
usually happen faster?


-- 
Milan Mimica

jpackage on MacOs: app from pkg cannot write files and create directories inside itself

[Andrey Volkov]

Hello.

I use jpackage (Build 14-jpackage+1-35) to bundle my Swing-based app for
MacOS. Whereas DMG format works pretty well, PGK bundle has a serious
issue. When I install the app from PKG, it cannot write files or create
directories inside itself.
I have /resource directory for my app that has a few settings and temp
directory for file processing. For the app from PKG I get "File not Found
(Permission Denied)" when the app tries to create a directory like
"/Application/MyApp.app/Contents/Java/resources/temp" or any write/create
any file inside  "/Application/MyApp.app/Contents/Java/resources/"
Do you have any clue how to fix it for PKG bundle? I would prefer to use it
because it has a more user-friendly installation.

BTW, Windows has a similar issue when per-user installation works fine, but
a system-wide installation in "Program Files" lack permissions to write
files inside "Program File/MyApp".

-- 
Best regards,
Andrey Volkov

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Pavel Rappo]

> On 6 Sep 2019, at 17:11, Martin Buchholz  wrote:
> 
> Bigger picture: all of this timeout-fiddling infrastructure should be in a 
> test library.  It's not easy!
> 
> test/jdk/java/util/concurrent/tck effectively has its own test library, for 
> historical reasons.

Agreed. It is not an easy task. I'd address this concern of having library 
level support for measuring timeouts and asserting threads blocking in a 
separate bug. For now, I'm afraid I have to reinvent the wheel. 

Re: 8229333: java/io/File/SetLastModified.java timed out

[Brian Burkhalter]

An alternative would be to increase the required memory:

@@ -23,6 +23,7 @@
 
 /* @test
@bug 4091757 6652379 8177809
+   @requires os.maxMemory >= 16G
@summary Basic test for setLastModified method
  */

Thanks,

Brian

> On Aug 28, 2019, at 10:51 AM, Lance Andersen  
> wrote:
> 
> Looks fine Brian, fingers crossed the little bugger does not come back :-)
> 
>> On Aug 27, 2019, at 2:00 PM, Brian Burkhalter > > wrote:
>> 
>> The failure reported in [1] was observed to occur on two Mac minis with 
>> HDDs. Given the large file access it is possibly due to slow disk access. 
>> The suggested fix is to increase the timeout [2]. If it is still observed at 
>> a later date a new issue can be filed.
>> 
>> Thanks,
>> 
>> Brian
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8229333 
>> 
>> [2] diff:
>> 
>> @@ -22,11 +22,13 @@
>>  */
>> 
>> /* @test
>> -   @bug 4091757 6652379 8177809
>> -   @summary Basic test for setLastModified method
>> + * @bug 4091757 6652379 8177809
>> + * @summary Basic test for setLastModified method
>> + * @run main/timeout=180 SetLastModified
>>  */
>> 
>> -import java.io .*;
>> +import java.io.File;
>> +import java.io.FileOutputStream;
>> import java.nio.ByteBuffer;
>> import java.nio.channels.FileChannel;
> 
>  
> 
>   
> 
>  Lance Andersen| 
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 
> 

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Pavel Rappo]

> On 6 Sep 2019, at 15:59, Martin Buchholz  wrote:
> 
> I took another look at LdapTimeoutTest.java.

Many thanks!

> I was surprised to see RIGHT_MARGIN.  In jsr166 we succeed in having one 
> timeout that is long enough to "never happen".  I'm still advocating the 10 
> second value.
> 
> I was surprised to see LEFT_MARGIN.  We just fixed Thread.sleep, so we have 
> no known problems with JDK methods returning early - you can trust timed get!
> You start measuring, by calling nanoTime, before you start the activity you 
> are measuring, so there should be no need for LEFT_MARGIN.

You raised many good points. Let me try to address them.

1. RIGHT_MARGIN is not used for checking that the activity has stuck infinitely 
(assertIncompletion). INFINITY_MILLIS is used for that. RIGHT_MARGIN is used 
for checking that the activity takes some predefined amount of time (roughly).

2. As for the numeric value of INFINITY_MILLIS, the reason I chose 20 seconds 
is twofold. Firstly, the code under test is subject to different timeouts. 
Every timeout value should be distinctive. Otherwise, how would I differentiate 
between them? For example, if I chose INFINITY_MILLIS to be 10 seconds how 
would I know if the code is stuck due to the read timeout of 10 seconds or the 
"infinite timeout"? Secondly, I must take into account slow machines. Really 
slow virtual machines. Hence, minimal timeouts (read/connect) have a magnitude 
of seconds and tens of seconds and the "rightmost", infinite timeout, is 20 
seconds.

3. LEFT_MARGIN might no longer be needed due to the fact that no timed methods 
return early (actually there is a comment about it inside those two assert 
methods).

> We have some fresh thread-awaiting code here:
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestCase.java?view=markup#l1443

Interesting.

> Instead of communicating startTime from the test thread back to the main 
> thread, I would do my loMillis checking in the test thread, and hiMillis 
> checking in the main thread, like e.g. compare with a fresh test method 
> testTimedOffer
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/ArrayBlockingQueueTest.java?view=markup#l394

Understood. However, that might be a matter of taste. I prefer to have all the 
calculations and error handling in one place. Unless there's a good reason I 
wouldn't change it.

> Timeouts should be adjusted via Utils.adjustTimeout

That makes perfect sense. I never knew this method existed. Thanks!

> On Fri, Sep 6, 2019 at 4:31 AM Pavel Rappo  wrote:
> Martin, thanks for having a look at it.
> 
> I'd appreciate if you could have a look at the timeout measuring mechanics in 
> assertCompletion/assertIncompletion specifically, maybe to spot something 
> that is grossly inadequate.
> 
> I tried to accommodate some usual suspects of timeout measurements failures. 
> I understand that since we're not working with real-time systems, my attempts 
> to build bullet-proof measurement mechanics are futile.
> 
> -Pavel
> 
> > On 30 Aug 2019, at 18:19, Martin Buchholz  wrote:
> > 
> > Not really a review, but:
> > 
> > For many years we've been using 10 seconds (scaled by timeout factor) as a 
> > duration long enough that a timeout is a real failure.
> > Which is close to your own 20 seconds.  Of course, no value is surely safe.
> > 
> > Probably, parallel testing infrastructure for timeouts should be a test 
> > library method.  I do something similar in JSR166TestCase
> > 
> > /**
> >  * Runs all the given actions in parallel, failing if any fail.
> >  * Useful for running multiple variants of tests that are
> >  * necessarily individually slow because they must block.
> >  */
> > void testInParallel(Action ... actions) {
> > ExecutorService pool = Executors.newCachedThreadPool();
> > try (PoolCleaner cleaner = cleaner(pool)) {
> > 
> > On Fri, Aug 30, 2019 at 6:23 AM Daniel Fuchs  
> > wrote:
> > On 30/08/2019 13:54, Pavel Rappo wrote:
> > > Updated,
> > > 
> > >  http://cr.openjdk.java.net/~prappo/8151678/webrev.01/
> > > 
> > 
> > Changes look good!
> > 
> > best regards,
> > 
> > -- daniel
> 

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Martin Buchholz]

Bigger picture: all of this timeout-fiddling infrastructure should be in a
test library.  It's not easy!

test/jdk/java/util/concurrent/tck effectively has its own test library, for
historical reasons.

On Fri, Sep 6, 2019 at 7:59 AM Martin Buchholz  wrote:

> I took another look at LdapTimeoutTest.java.
>
> I was surprised to see RIGHT_MARGIN.  In jsr166 we succeed in having one
> timeout that is long enough to "never happen".  I'm still advocating the 10
> second value.
>
> I was surprised to see LEFT_MARGIN.  We just fixed Thread.sleep, so we
> have no known problems with JDK methods returning early - you can trust
> timed get!
> You start measuring, by calling nanoTime, before you start the activity
> you are measuring, so there should be no need for LEFT_MARGIN.
>
> We have some fresh thread-awaiting code here:
>
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestCase.java?view=markup#l1443
>
> Instead of communicating startTime from the test thread back to the main
> thread, I would do my loMillis checking in the test thread, and hiMillis
> checking in the main thread, like e.g. compare with a fresh test
> method testTimedOffer
>
> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/ArrayBlockingQueueTest.java?view=markup#l394
>
> Timeouts should be adjusted via Utils.adjustTimeout
>
>
> On Fri, Sep 6, 2019 at 4:31 AM Pavel Rappo  wrote:
>
>> Martin, thanks for having a look at it.
>>
>> I'd appreciate if you could have a look at the timeout measuring
>> mechanics in assertCompletion/assertIncompletion specifically, maybe to
>> spot something that is grossly inadequate.
>>
>> I tried to accommodate some usual suspects of timeout measurements
>> failures. I understand that since we're not working with real-time systems,
>> my attempts to build bullet-proof measurement mechanics are futile.
>>
>> -Pavel
>>
>> > On 30 Aug 2019, at 18:19, Martin Buchholz  wrote:
>> >
>> > Not really a review, but:
>> >
>> > For many years we've been using 10 seconds (scaled by timeout factor)
>> as a duration long enough that a timeout is a real failure.
>> > Which is close to your own 20 seconds.  Of course, no value is surely
>> safe.
>> >
>> > Probably, parallel testing infrastructure for timeouts should be a test
>> library method.  I do something similar in JSR166TestCase
>> >
>> > /**
>> >  * Runs all the given actions in parallel, failing if any fail.
>> >  * Useful for running multiple variants of tests that are
>> >  * necessarily individually slow because they must block.
>> >  */
>> > void testInParallel(Action ... actions) {
>> > ExecutorService pool = Executors.newCachedThreadPool();
>> > try (PoolCleaner cleaner = cleaner(pool)) {
>> >
>> > On Fri, Aug 30, 2019 at 6:23 AM Daniel Fuchs 
>> wrote:
>> > On 30/08/2019 13:54, Pavel Rappo wrote:
>> > > Updated,
>> > >
>> > >  http://cr.openjdk.java.net/~prappo/8151678/webrev.01/
>> > >
>> >
>> > Changes look good!
>> >
>> > best regards,
>> >
>> > -- daniel
>>
>>

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Martin Buchholz]

I took another look at LdapTimeoutTest.java.

I was surprised to see RIGHT_MARGIN.  In jsr166 we succeed in having one
timeout that is long enough to "never happen".  I'm still advocating the 10
second value.

I was surprised to see LEFT_MARGIN.  We just fixed Thread.sleep, so we have
no known problems with JDK methods returning early - you can trust timed
get!
You start measuring, by calling nanoTime, before you start the activity you
are measuring, so there should be no need for LEFT_MARGIN.

We have some fresh thread-awaiting code here:
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestCase.java?view=markup#l1443

Instead of communicating startTime from the test thread back to the main
thread, I would do my loMillis checking in the test thread, and hiMillis
checking in the main thread, like e.g. compare with a fresh test
method testTimedOffer
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/ArrayBlockingQueueTest.java?view=markup#l394

Timeouts should be adjusted via Utils.adjustTimeout


On Fri, Sep 6, 2019 at 4:31 AM Pavel Rappo  wrote:

> Martin, thanks for having a look at it.
>
> I'd appreciate if you could have a look at the timeout measuring mechanics
> in assertCompletion/assertIncompletion specifically, maybe to spot
> something that is grossly inadequate.
>
> I tried to accommodate some usual suspects of timeout measurements
> failures. I understand that since we're not working with real-time systems,
> my attempts to build bullet-proof measurement mechanics are futile.
>
> -Pavel
>
> > On 30 Aug 2019, at 18:19, Martin Buchholz  wrote:
> >
> > Not really a review, but:
> >
> > For many years we've been using 10 seconds (scaled by timeout factor) as
> a duration long enough that a timeout is a real failure.
> > Which is close to your own 20 seconds.  Of course, no value is surely
> safe.
> >
> > Probably, parallel testing infrastructure for timeouts should be a test
> library method.  I do something similar in JSR166TestCase
> >
> > /**
> >  * Runs all the given actions in parallel, failing if any fail.
> >  * Useful for running multiple variants of tests that are
> >  * necessarily individually slow because they must block.
> >  */
> > void testInParallel(Action ... actions) {
> > ExecutorService pool = Executors.newCachedThreadPool();
> > try (PoolCleaner cleaner = cleaner(pool)) {
> >
> > On Fri, Aug 30, 2019 at 6:23 AM Daniel Fuchs 
> wrote:
> > On 30/08/2019 13:54, Pavel Rappo wrote:
> > > Updated,
> > >
> > >  http://cr.openjdk.java.net/~prappo/8151678/webrev.01/
> > >
> >
> > Changes look good!
> >
> > best regards,
> >
> > -- daniel
>
>

Re: RFR: 8230662: Remove dead code from MethodTypeForm

[Claes Redestad]

Vladimir, thanks for reviewing!

/Claes

On 2019-09-06 15:48, Vladimir Ivanov wrote:

Looks good.

Best regards,
Vladimir Ivanov

On 05/09/2019 17:41, Claes Redestad wrote:

Hi,

I noticed some unused methods in java.lang.invoke.MethodTypeForm and
ended up with a rather substantial cleanup after pulling that particular
thread for a bit:

http://cr.openjdk.java.net/~redestad/8230662/jdk.00/
https://bugs.openjdk.java.net/browse/JDK-8230662

Testing: tier1-3

Thanks!

/Claes

Re: RFR: 8230662: Remove dead code from MethodTypeForm

[Vladimir Ivanov]

Looks good.

Best regards,
Vladimir Ivanov

On 05/09/2019 17:41, Claes Redestad wrote:

Hi,

I noticed some unused methods in java.lang.invoke.MethodTypeForm and
ended up with a rather substantial cleanup after pulling that particular
thread for a bit:

http://cr.openjdk.java.net/~redestad/8230662/jdk.00/
https://bugs.openjdk.java.net/browse/JDK-8230662

Testing: tier1-3

Thanks!

/Claes

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Pavel Rappo]

Milan,

In order to simulate the "Address already in use" scenario I did this:

TcpDnsServer(int port) {
try {
*   new ServerSocket(port, 0, InetAddress.getLoopbackAddress());
serverSocket = new ServerSocket(port, 0, 
InetAddress.getLoopbackAddress());
}
catch (BindException ex) {

The test failed, instead of being skipped. That's what I saw in the logs:


jtreg.SkippedException: Cannot start TCP server
at TcpTimeout$TcpDnsServer.(TcpTimeout.java:97)
at TcpTimeout.initTest(TcpTimeout.java:71)
at TestBase.run(TestBase.java:48)
at TcpTimeout.main(TcpTimeout.java:47)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at 
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.net.BindException: Address already in use: bind
at java.base/sun.nio.ch.Net.bind0(Native Method)
at java.base/sun.nio.ch.Net.bind(Net.java:469)
at java.base/sun.nio.ch.Net.bind(Net.java:458)
at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
at java.base/java.net.ServerSocket.bind(ServerSocket.java:355)
at java.base/java.net.ServerSocket.(ServerSocket.java:241)
at TcpTimeout$TcpDnsServer.(TcpTimeout.java:91)
... 9 more

JavaTest Message: Test threw exception: jtreg.SkippedException
JavaTest Message: shutting down test


JavaTest Message: Problem cleaning up the following threads:
Thread-0
  at 
java.base@14-internal/java.net.DualStackPlainDatagramSocketImpl.socketReceiveOrPeekData(Native
 Method)
  at 
java.base@14-internal/java.net.DualStackPlainDatagramSocketImpl.peekData(DualStackPlainDatagramSocketImpl.java:113)
  at 
java.base@14-internal/java.net.DatagramSocket.receive(DatagramSocket.java:746)
  at DNSServer.receiveQuery(DNSServer.java:178)
  at DNSServer.run(DNSServer.java:119)


What's going on here? It looks like the test didn't call `cleanupTest()` (and 
therefore `server.stopServer()`) because the exception was thrown before it had 
reached `runTest()`. I think we must address this, otherwise our 
jtreg.SkippedException is only half measure.

I thought about how to do that. I'm thinking of going the full way up to 
TestBase as I believe it makes the most sense. Having said that, we'd better 
ask the original author (cc'ed), Chris Yin,  what he thinks about it. Here's 
the proposal:


diff -r 53e139e55605 test/jdk/com/sun/jndi/dns/lib/TestBase.java
--- a/test/jdk/com/sun/jndi/dns/lib/TestBase.java   Thu Sep 05 14:59:29 
2019 +0100
+++ b/test/jdk/com/sun/jndi/dns/lib/TestBase.java   Fri Sep 06 14:08:48 
2019 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -44,10 +44,14 @@
  * @throws Exception if any exception
  */
 public void run(String[] args) throws Exception {
-initRes();
-initTest(args);
-setupTest();
-launch();
+try {
+initRes();
+initTest(args);
+setupTest();
+launch();
+} finally {
+cleanupTest();
+}
 }
 
 /**
@@ -84,8 +88,6 @@
 if (!handleException(e)) {
 throw e;
 }
-} finally {
-cleanupTest();
 }
 }
 
@@ -108,6 +110,11 @@
 
 /**
  * Cleanup test.
+ *
+ * This method may be called by the test at any time, including before all
+ * the resources, initialization, and setup have been completed. This might
+ * require careful attention. For example, before closing a resource this
+ * method should check that resource for being {@code null}.
  */
 public abstract void cleanupTest();
 }


-Pavel

> On 5 Sep 2019, at 11:20, Milan Mimica  wrote:
> 
> On Wed, 4 Sep 2019 at 20:32, Florian Weimer  wrote:
>> 
>> If you use the initial UDP timeout (one second, I think), the kernel
>> will not complete the TCP handshake if the initial SYN segment is lost
>> because the retransmit delay during the handshake is longer than that.
> 
> We could set a higher timeout value, but I would not prefer that.
> After all, 1 second is just default value, and can be changed. If the
> user wants us to give up on DNS query after the specified timeout then
> lets do just that. True, some queries that previously worked might
> start failing, b

Re: RFR: 8230648: Replace @exception tag with @throws in java.base

[Pavel Rappo]

If you ran the specdiff and saw nothing, I'm fine with that. Looks good.

> On 5 Sep 2019, at 19:28, Julia Boes  wrote:
> 
> Hi,
> 
> Thanks for your comments, Lance and Pavel.
> 
> The copyright will be updated before pushing, as Daniel suggested.
> 
> To address the tag alignment, I adjusted the replacement from '@exception' -> 
> '@throws' to '@exception' -> 'throws   ', where the added whitespace 
> preserves the original alignment. This doesn't improve the alignment (which 
> is not consistent in many places) but at least doesn't make it worse.
> 
> Updated webrev: http://cr.openjdk.java.net/~dfuchs/jboes/8230648/webrev.02/
> 
> Regarding Pavel's comment:
> 
>8157682: @inheritDoc doesn't work with @exception
> 
> I ran specdiff on the whole JDK and it didn't flag any differences but I'll 
> look into additional comparison options.
> 
> Cheers,
> 
> Julia
> 

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Pavel Rappo]

Martin, thanks for having a look at it.

I'd appreciate if you could have a look at the timeout measuring mechanics in 
assertCompletion/assertIncompletion specifically, maybe to spot something that 
is grossly inadequate.

I tried to accommodate some usual suspects of timeout measurements failures. I 
understand that since we're not working with real-time systems, my attempts to 
build bullet-proof measurement mechanics are futile.

-Pavel

> On 30 Aug 2019, at 18:19, Martin Buchholz  wrote:
> 
> Not really a review, but:
> 
> For many years we've been using 10 seconds (scaled by timeout factor) as a 
> duration long enough that a timeout is a real failure.
> Which is close to your own 20 seconds.  Of course, no value is surely safe.
> 
> Probably, parallel testing infrastructure for timeouts should be a test 
> library method.  I do something similar in JSR166TestCase
> 
> /**
>  * Runs all the given actions in parallel, failing if any fail.
>  * Useful for running multiple variants of tests that are
>  * necessarily individually slow because they must block.
>  */
> void testInParallel(Action ... actions) {
> ExecutorService pool = Executors.newCachedThreadPool();
> try (PoolCleaner cleaner = cleaner(pool)) {
> 
> On Fri, Aug 30, 2019 at 6:23 AM Daniel Fuchs  wrote:
> On 30/08/2019 13:54, Pavel Rappo wrote:
> > Updated,
> > 
> >  http://cr.openjdk.java.net/~prappo/8151678/webrev.01/
> > 
> 
> Changes look good!
> 
> best regards,
> 
> -- daniel

Re: RFR: 8230648: Replace @exception tag with @throws in java.base

[Lance Andersen]

Hi Julia,

It looks fine., thank you for doing this


Best
Lance
> On Sep 5, 2019, at 2:28 PM, Julia Boes  wrote:
> 
> Hi,
> 
> Thanks for your comments, Lance and Pavel.
> 
> The copyright will be updated before pushing, as Daniel suggested.
> 
> To address the tag alignment, I adjusted the replacement from '@exception' -> 
> '@throws' to '@exception' -> 'throws   ', where the added whitespace 
> preserves the original alignment. This doesn't improve the alignment (which 
> is not consistent in many places) but at least doesn't make it worse.
> 
> Updated webrev: http://cr.openjdk.java.net/~dfuchs/jboes/8230648/webrev.02/
> 
> Regarding Pavel's comment:
> 
>8157682: @inheritDoc doesn't work with @exception
> 
> I ran specdiff on the whole JDK and it didn't flag any differences but I'll 
> look into additional comparison options.
> 
> Cheers,
> 
> Julia
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: Request for sponsor : 8230436: String.stripIndent() javadoc unclear about LF/CR at end of string

[Andrew Leonard]

Hi,
Anyone able to review please?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Andrew Leonard/UK/IBM
To: core-libs-dev@openjdk.java.net
Cc: james.las...@oracle.com
Date:   02/09/2019 17:10
Subject:RFR: Request for sponsor : 8230436: String.stripIndent() 
javadoc unclear about LF/CR at end of string


Hi,
Can I get people's views and sponsor on a small update to the javadoc for 
String.stripIndent() to make the \n end of block special case more clear 
please?
I've done a proposed patch here: 
http://cr.openjdk.java.net/~aleonard/8230436/webrev.00/
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU



Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU