Re: RFR [14] 8217606: LdapContext#reconnect always opens a new connection

[Chris Yin]

> On 9 Aug 2019, at 1:59 AM, Roger Riggs  wrote:

> 
> ...

> Is handling Unbind in the switch needed (as different from the default).

Sorry, I forgot this, it’s a placeholder to handle UNBIND_REQUEST further, per 
ldap protocol, server will close the connection when received unbind request 
from client (no response required), since the test will shutdown server shortly 
and we don’t have special intention for unbind, so it’s ok to remove unbind in 
switch.

Thanks,
Chris

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Mandy Chung]

On 8/12/19 5:13 PM, Frederic Parain wrote:

This looks good to me, with two comments:

I don’t like the static final RETAIN_CLASS_REF for the same
reasons as Aleksey, but I can live with that.


I didn't see Aleksey's comment about RETAIN_CLASS_REF (what is it?).  
Now that it draws my attention.   Since there is no other flag at the 
moment, we can simplify it and rename the flags field to a boolean 
retainClassRef field.



The protocol between the JVM and the Java class is well explained
on the JVM side (javaClasses.cpp:4227). I think it would be valuable
to provide the same description on the Java side, the comment in
StackFrameInfo.java:42 describes only part of the protocol.


Good suggestion.  Added:

96 // VM adds 1 to the code index to StackFrameInfo::bci field such that
97 // zero (and negative values) indicates invalid BCI. So substract 1.


http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.04/

Mandy


No need for another review from me.

Regards,

Fred
  


On Aug 12, 2019, at 16:24, Mandy Chung  wrote:

Having a second thought, I'm keeping @Stable bci field while zero indicates an 
invalid BCI that makes it obvious that this field will be updated.  VM will set 
StackFrameInfo::bci to value+1.

http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.03/

Mandy

Re: RFR: JDK-8226534: combination of windows options cause exception in MsiBundler

[Andy Herrick]

looks good.

/Andy

On 8/12/2019 8:38 PM, Alexander Matveev wrote:

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

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- Fixed by adding --install-dir folders to RemoveFile table as per 
Wixs requirements.


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

[2] http://cr.openjdk.java.net/~almatvee/8226534/webrev.00/

Thanks,
Alexander

RFR: JDK-8226534: combination of windows options cause exception in MsiBundler

[Alexander Matveev]

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

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- Fixed by adding --install-dir folders to RemoveFile table as per Wixs 
requirements.


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

[2] http://cr.openjdk.java.net/~almatvee/8226534/webrev.00/

Thanks,
Alexander

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Frederic Parain]

This looks good to me, with two comments:

I don’t like the static final RETAIN_CLASS_REF for the same
reasons as Aleksey, but I can live with that.

The protocol between the JVM and the Java class is well explained
on the JVM side (javaClasses.cpp:4227). I think it would be valuable
to provide the same description on the Java side, the comment in
StackFrameInfo.java:42 describes only part of the protocol.

No need for another review from me.

Regards,

Fred
 

> On Aug 12, 2019, at 16:24, Mandy Chung  wrote:
> 
> Having a second thought, I'm keeping @Stable bci field while zero indicates 
> an invalid BCI that makes it obvious that this field will be updated.  VM 
> will set StackFrameInfo::bci to value+1.
> 
> http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.03/
> 
> Mandy

Re: RFR: 8226297: Dual-pivot quicksort improvements

[Brent Christian]

Hi,

I received an update from Vladimir:
---
"I investigated approach with adaptive threshold for mixed insertion sort
and have the following results.

New version shows the same gain for large arrays
and has few percents of speed up for small arrays:

total gain:
size = 150, char, was: 0.10 -> new: 0.17
size = 150, int, was: 0.15 -> new: 0.18
size = 150, short, was: 0.14 -> new: 0.16

See new version in attachment. The changes are simple and safety:
initial threshold for insertion sort was increased from 41 to 44,
initial threshold for mixed insertion sort was decreased from 114 to 65,
but the size is compared with adaptive threshold

if (size < MAX_MIXED_INSERTION_SORT_SIZE + bits && 
(was: if (size < MAX_MIXED_INSERTION_SORT_SIZE && )

Variable bits is increased by 6 instead of 2."
---

I've incorporated this change in an updated webrev, here:
http://cr.openjdk.java.net/~bchristi/8226297/webrev05-adaptive/webrev/

For convenience, I made .diffs from the previous version (webrev04) for 
DualPivotQuickSort.java[1] and Arrays.java[2 - change to trailing 
white-space, only].


-Brent
1. 
http://cr.openjdk.java.net/~bchristi/8226297/webrev05-adaptive/DualPivotQuicksort.java.diff


2. 
http://cr.openjdk.java.net/~bchristi/8226297/webrev05-adaptive/Arrays.java.diff

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[David Holmes]

On 13/08/2019 8:55 am, Mandy Chung wrote:

On 8/12/19 3:28 PM, David Holmes wrote:

Hi Mandy,

On 13/08/2019 6:24 am, Mandy Chung wrote:
Having a second thought, I'm keeping @Stable bci field while zero 
indicates an invalid BCI that makes it obvious that this field will 
be updated.  VM will set StackFrameInfo::bci to value+1.


I don't know this code but why have the VM set the value one too many 
and then have the Java code subtract one again. ???


I keep it as @Stable field be initialized once by VM and it means that 0 
indicates an invalid bci.   It could be made as final field but 
initialized in the constructor to -1 and then set by VM.  I opt for 
webrev.03 to make it clear it's initialized later by the VM once.


I see, so the zero is a constraint of it being @Stable. So we offset by 
1 at both ends so that we return -1 when not set. Otherwise we'd need to 
special-case for zero.


Okay I get it. Thanks. Not a review though as I do not know this code.

David


Mandy

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Mandy Chung]

On 8/12/19 3:28 PM, David Holmes wrote:

Hi Mandy,

On 13/08/2019 6:24 am, Mandy Chung wrote:
Having a second thought, I'm keeping @Stable bci field while zero 
indicates an invalid BCI that makes it obvious that this field will 
be updated.  VM will set StackFrameInfo::bci to value+1.


I don't know this code but why have the VM set the value one too many 
and then have the Java code subtract one again. ???


I keep it as @Stable field be initialized once by VM and it means that 0 
indicates an invalid bci.   It could be made as final field but 
initialized in the constructor to -1 and then set by VM.  I opt for 
webrev.03 to make it clear it's initialized later by the VM once.


Mandy

Re: RFR: 8211990: DateTimeException thrown when calculating duration between certain dates

[Lance Andersen]

Hi Naoto
> On Aug 12, 2019, at 6:34 PM, naoto.s...@oracle.com wrote:
> 
> Hi Lance,
> 
> Yes, I would like the style, but AFAIK, all java.time tests are testng, and 
> controlled with the java/time/{test/tck}/TEST.properties file so that each 
> test file won't need jtreg tags (it cannot override them either).

Yes the @build,@run, @library would be handled via the TEST.properties and are 
not needed for the test because of TEST.properties.

The other tags are informational and do not impact the running of the test such 
as @test, @bug, @summary….

Anyways, no big deal either way, just thought I would ask.

have a good evening 

Best
lance
> 
> Naoto
> 
> On 8/12/19 3:17 PM, Lance Andersen wrote:
>> Hi Naoto,
>>> On Aug 12, 2019, at 6:01 PM, naoto.s...@oracle.com 
>>>  wrote:
>>> 
>>> Thank you for the review, Lance.
>>> 
>>> On 8/12/19 2:37 PM, Lance Andersen wrote:
 Looks good Naoto.
 One question I had which is not relevant to your fix, but should the tests 
 as we modify them include the JTReg tags such as @bug, @summary…. etc…  
 just for consistency….
>>> 
>>> I put @bug tags to each of the modified test, but not @summary, et.al. It 
>>> seems that each test file corresponds to the java.time class, so adding 
>>> @summary for this bug might not fit. I could add @summary to existing 
>>> "Tests " statement, but keep it consistent with other java.time test 
>>> case files.
>> I was thinking more similar to:
>> open/test/jdk/jdk/nio/zipfs/TestPosix.java or 
>> open/test/jdk/java/nio/file/Files/StreamTest.java
>> ——
>> /* @test
>> * @bug 8006884 8019526 8132539
>> * @library ..
>> * @build PassThroughFileSystem FaultyFileSystem
>> * @run testng StreamTest
>> * @summary Unit test for java.nio.file.Files methods that return a Stream
>> */
>> 
>> You won’t need the @run for these tests but each test is for a specific 
>> class such as OffsetDateTime and ZonedOffSetDateTime and all of the bugs are 
>> listed at the top of the file.
>> I am not sure we have agreed to standardize this historically, but I tend to 
>> when I update a test if applicable.
>> Anyways, just a suggestion… Feel free to ignore ;-)
>> Have a good rest of your evening :-)
>>> 
>>> Naoto
>>> 
 Best
 Lance
> On Aug 12, 2019, at 4:43 PM, naoto.s...@oracle.com 
>   wrote:
> 
> Hello,
> 
> Please review the fix to the following issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8211990
> 
> The proposed changeset is located at:
> 
> https://cr.openjdk.java.net/~naoto/8211990/webrev.00/
> 
> The DateTimeException was thrown due to unconditional conversion beyond 
> the valid range of the internal LocalDateTime value. If it happens, 
> normalize two instants with the offset of "start" instant. The same kind 
> of exception is observed with ZonedDateTime.until(), which is also fixed 
> in this changeset.
> 
> Naoto
 
 
 Lance Andersen| 
 Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com  
 
>> 
>> 
>> Lance Andersen| 
>> Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering
>> 1 Network Drive
>> Burlington, MA 01803
>> lance.ander...@oracle.com 

 
  

 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: 8211990: DateTimeException thrown when calculating duration between certain dates

[naoto . sato]

Hi Lance,

Yes, I would like the style, but AFAIK, all java.time tests are testng, 
and controlled with the java/time/{test/tck}/TEST.properties file so 
that each test file won't need jtreg tags (it cannot override them either).


Naoto

On 8/12/19 3:17 PM, Lance Andersen wrote:

Hi Naoto,
On Aug 12, 2019, at 6:01 PM, naoto.s...@oracle.com 
 wrote:


Thank you for the review, Lance.

On 8/12/19 2:37 PM, Lance Andersen wrote:

Looks good Naoto.
One question I had which is not relevant to your fix, but should the 
tests as we modify them include the JTReg tags such as @bug, 
@summary…. etc…  just for consistency….


I put @bug tags to each of the modified test, but not @summary, et.al. 
It seems that each test file corresponds to the java.time class, so 
adding @summary for this bug might not fit. I could add @summary to 
existing "Tests " statement, but keep it consistent with other 
java.time test case files.


I was thinking more similar to:

open/test/jdk/jdk/nio/zipfs/TestPosix.java or 
open/test/jdk/java/nio/file/Files/StreamTest.java


——

/* @test
* @bug 8006884 8019526 8132539
* @library ..
* @build PassThroughFileSystem FaultyFileSystem
* @run testng StreamTest
* @summary Unit test for java.nio.file.Files methods that return a Stream
*/



You won’t need the @run for these tests but each test is for a specific 
class such as OffsetDateTime and ZonedOffSetDateTime and all of the bugs 
are listed at the top of the file.


I am not sure we have agreed to standardize this historically, but I 
tend to when I update a test if applicable.


Anyways, just a suggestion… Feel free to ignore ;-)

Have a good rest of your evening :-)



Naoto


Best
Lance
On Aug 12, 2019, at 4:43 PM, naoto.s...@oracle.com 
  wrote:


Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

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

The DateTimeException was thrown due to unconditional conversion 
beyond the valid range of the internal LocalDateTime value. If it 
happens, normalize two instants with the offset of "start" instant. 
The same kind of exception is observed with ZonedDateTime.until(), 
which is also fixed in this changeset.


Naoto



Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com  





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 JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[David Holmes]

Hi Mandy,

On 13/08/2019 6:24 am, Mandy Chung wrote:
Having a second thought, I'm keeping @Stable bci field while zero 
indicates an invalid BCI that makes it obvious that this field will be 
updated.  VM will set StackFrameInfo::bci to value+1.


I don't know this code but why have the VM set the value one too many 
and then have the Java code subtract one again. ???


David



http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.03/

Mandy

Re: RFR: 8211990: DateTimeException thrown when calculating duration between certain dates

[Lance Andersen]

Hi Naoto,
> On Aug 12, 2019, at 6:01 PM, naoto.s...@oracle.com wrote:
> 
> Thank you for the review, Lance.
> 
> On 8/12/19 2:37 PM, Lance Andersen wrote:
>> Looks good Naoto.
>> One question I had which is not relevant to your fix, but should the tests 
>> as we modify them include the JTReg tags such as @bug, @summary…. etc…  just 
>> for consistency….
> 
> I put @bug tags to each of the modified test, but not @summary, et.al. It 
> seems that each test file corresponds to the java.time class, so adding 
> @summary for this bug might not fit. I could add @summary to existing "Tests 
> " statement, but keep it consistent with other java.time test case files.

I was thinking more similar to:

open/test/jdk/jdk/nio/zipfs/TestPosix.java or 
open/test/jdk/java/nio/file/Files/StreamTest.java

——
/* @test
 * @bug 8006884 8019526 8132539
 * @library ..
 * @build PassThroughFileSystem FaultyFileSystem
 * @run testng StreamTest
 * @summary Unit test for java.nio.file.Files methods that return a Stream
 */


You won’t need the @run for these tests but each test is for a specific class 
such as OffsetDateTime and ZonedOffSetDateTime and all of the bugs are listed 
at the top of the file.

I am not sure we have agreed to standardize this historically, but I tend to 
when I update a test if applicable.

Anyways, just a suggestion… Feel free to ignore ;-)

Have a good rest of your evening :-)

> 
> Naoto
> 
>> Best
>> Lance
>>> On Aug 12, 2019, at 4:43 PM, naoto.s...@oracle.com 
>>>  wrote:
>>> 
>>> Hello,
>>> 
>>> Please review the fix to the following issue:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8211990
>>> 
>>> The proposed changeset is located at:
>>> 
>>> https://cr.openjdk.java.net/~naoto/8211990/webrev.00/
>>> 
>>> The DateTimeException was thrown due to unconditional conversion beyond the 
>>> valid range of the internal LocalDateTime value. If it happens, normalize 
>>> two instants with the offset of "start" instant. The same kind of exception 
>>> is observed with ZonedDateTime.until(), which is also fixed in this 
>>> changeset.
>>> 
>>> Naoto
>> 
>> 
>> Lance Andersen| 
>> Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering
>> 1 Network Drive
>> Burlington, MA 01803
>> lance.ander...@oracle.com 

 
  

 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: 8211990: DateTimeException thrown when calculating duration between certain dates

[naoto . sato]

Thank you for the review, Lance.

On 8/12/19 2:37 PM, Lance Andersen wrote:

Looks good Naoto.

One question I had which is not relevant to your fix, but should the 
tests as we modify them include the JTReg tags such as @bug, @summary…. 
etc…  just for consistency….


I put @bug tags to each of the modified test, but not @summary, et.al. 
It seems that each test file corresponds to the java.time class, so 
adding @summary for this bug might not fit. I could add @summary to 
existing "Tests " statement, but keep it consistent with other 
java.time test case files.


Naoto



Best
Lance
On Aug 12, 2019, at 4:43 PM, naoto.s...@oracle.com 
 wrote:


Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

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

The DateTimeException was thrown due to unconditional conversion 
beyond the valid range of the internal LocalDateTime value. If it 
happens, normalize two instants with the offset of "start" instant. 
The same kind of exception is observed with ZonedDateTime.until(), 
which is also fixed in this changeset.


Naoto




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: JDK-8227172: revert JDK-8225569 on windows

[Alexander Matveev]

Looks good.

On 8/10/2019 3:44 AM, Andy Herrick wrote:

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

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


This change will remove the "bin" directory on windows and revert to 
putting the executable(s) directly in output root dir. (dir>/)


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

[2] http://cr.openjdk.java.net/~herrick/8227172/webrev.01/

/Andy

Re: RFR: 8211990: DateTimeException thrown when calculating duration between certain dates

[Lance Andersen]

Looks good Naoto.

One question I had which is not relevant to your fix, but should the tests as 
we modify them include the JTReg tags such as @bug, @summary…. etc…  just for 
consistency….

Best
Lance
> On Aug 12, 2019, at 4:43 PM, naoto.s...@oracle.com wrote:
> 
> Hello,
> 
> Please review the fix to the following issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8211990
> 
> The proposed changeset is located at:
> 
> https://cr.openjdk.java.net/~naoto/8211990/webrev.00/
> 
> The DateTimeException was thrown due to unconditional conversion beyond the 
> valid range of the internal LocalDateTime value. If it happens, normalize two 
> instants with the offset of "start" instant. The same kind of exception is 
> observed with ZonedDateTime.until(), which is also fixed in this changeset.
> 
> Naoto

 
  

 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: 8211990: DateTimeException thrown when calculating duration between certain dates

[Stephen Colebourne]

+1

On Mon, 12 Aug 2019 at 21:44,  wrote:
>
> Hello,
>
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8211990
>
> The proposed changeset is located at:
>
> https://cr.openjdk.java.net/~naoto/8211990/webrev.00/
>
> The DateTimeException was thrown due to unconditional conversion beyond
> the valid range of the internal LocalDateTime value. If it happens,
> normalize two instants with the offset of "start" instant. The same kind
> of exception is observed with ZonedDateTime.until(), which is also fixed
> in this changeset.
>
> Naoto

RFR: 8211990: DateTimeException thrown when calculating duration between certain dates

[naoto . sato]

Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

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

The DateTimeException was thrown due to unconditional conversion beyond 
the valid range of the internal LocalDateTime value. If it happens, 
normalize two instants with the offset of "start" instant. The same kind 
of exception is observed with ZonedDateTime.until(), which is also fixed 
in this changeset.


Naoto

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Mandy Chung]

Having a second thought, I'm keeping @Stable bci field while zero 
indicates an invalid BCI that makes it obvious that this field will be 
updated.  VM will set StackFrameInfo::bci to value+1.


http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.03/

Mandy

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Aleksey Shipilev]

On 8/12/19 9:01 PM, Mandy Chung wrote:
>  import jdk.internal.access.JavaLangInvokeAccess;
>  import jdk.internal.access.SharedSecrets;
> +import jdk.internal.vm.annotation.Stable;

Not needed anymore.

> +    private final int bci = -1; // BCI set by VM

AFAIU, this sets up the field to be "constant variable", which does run afoul 
the later VM
injection. See for example: 
https://shipilev.net/jvm/anatomy-quarks/14-constant-variables/

>  StackFrameInfo(StackWalker walker) {
>  this.flags = walker.retainClassRef ? RETAIN_CLASS_REF : 0;
> -    this.bci = -1;

I'd keep initialization here instead of the field, see above why.

-- 
Thanks,
-Aleksey

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Mandy Chung]

On 8/12/19 11:33 AM, Aleksey Shipilev wrote:

Alternatively, we can make bci an int (as Alekey suggests) that does not 
increase the object size:

    http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.02/

@Stable seems meaningless here, for the reasons Daniel points out. I believe "final 
int bci" is just as good and clean.


I am happy to keep it final field and the comment already indicates that 
the field is set by the VM.  It's initialized to -1 in case any accident 
change that VM didn't set this field.



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

--- a/src/java.base/share/classes/java/lang/StackFrameInfo.java
+++ b/src/java.base/share/classes/java/lang/StackFrameInfo.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2015, 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
@@ -26,28 +26,31 @@

 import jdk.internal.access.JavaLangInvokeAccess;
 import jdk.internal.access.SharedSecrets;
+import jdk.internal.vm.annotation.Stable;

 import java.lang.StackWalker.StackFrame;
 import java.lang.invoke.MethodType;

 class StackFrameInfo implements StackFrame {
-    private final byte RETAIN_CLASS_REF = 0x01;
+    private final static byte RETAIN_CLASS_REF = 0x01;

 private final static JavaLangInvokeAccess JLIA =
 SharedSecrets.getJavaLangInvokeAccess();

 private final byte flags;
-    private final Object memberName;
-    private final short bci;
+    private final Object memberName;    // MemberName initialized by VM
+    private final int bci = -1; // BCI set by VM
 private volatile StackTraceElement ste;

 /*
- * Create StackFrameInfo for StackFrameTraverser and 
LiveStackFrameTraverser

- * to use
+ * Construct an empty StackFrameInfo object that will be filled by 
the VM

+ * during stack walking.
+ *
+ * @see StackStreamFactory.AbstractStackWalker#callStackWalk
+ * @see StackStreamFactory.AbstractStackWalker#fetchStackFrames
  */
 StackFrameInfo(StackWalker walker) {
 this.flags = walker.retainClassRef ? RETAIN_CLASS_REF : 0;
-    this.bci = -1;
 this.memberName = JLIA.newMemberName();
 }

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Aleksey Shipilev]

On 8/12/19 8:12 PM, Mandy Chung wrote:
> On 8/11/19 9:49 PM, David Holmes wrote:
>> On 11/08/2019 2:50 pm, Mandy Chung wrote:
>>> On 8/10/19 12:30 AM, Aleksey Shipilev wrote:
 I wonder if bci=-1 is meaningful, and should be returned when BCI is not 
 available. After this
 patch, it would be converted to 65536?
>>
>> This is my query as well. It is not obvious to me that the VM will never set 
>> a BCI of -1
> 
> Frederic adds an assert to ensure that bci is a valid value [1] in the fix 
> for JDK-8229375.  This
> would help catching the odd case with invalid BCI.

But the initializing value from Java code is still -1, so 65535 would be 
printed if BCI is not
initialized by VM on some path?

> Alternatively, we can make bci an int (as Alekey suggests) that does not 
> increase the object size:
> 
>    http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.02/

@Stable seems meaningless here, for the reasons Daniel points out. I believe 
"final int bci" is just
as good and clean.

-- 
Thanks,
-Aleksey

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Daniel Fuchs]

Hi Mandy,

I thought the contract for @Stable was that the value would
not change after it's been assigned a non default value.
And the default for int is '0', not '-1'.
In view of that - isn't using 'final' a better alternative,
even though it's also lying?

best regards,

-- daniel

On 12/08/2019 19:12, Mandy Chung wrote:
Alternatively, we can make bci an int (as Alekey suggests) that does not 
increase the object size:


    http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.02/

This means that this will replace Frederic's proposed patch for 
JDK-8229375.


What do you think?

Mandy
[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-August/035596.html 

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Mandy Chung]

On 8/11/19 9:49 PM, David Holmes wrote:

On 11/08/2019 2:50 pm, Mandy Chung wrote:

On 8/10/19 12:30 AM, Aleksey Shipilev wrote:

On 8/9/19 10:19 PM, Mandy Chung wrote:

An earlier version of this patch was reviewed [1] but I
didn't get back to explore the other approach.  I rebase
the patch and take out the hotspot change which will be
covered by JDK-8229375:

http://cr.openjdk.java.net/~mchung/jdk14//8193325/webrev.01
I wonder if bci=-1 is meaningful, and should be returned when BCI is 
not available. After this patch, it would be converted to 65536?


This is my query as well. It is not obvious to me that the VM will 
never set a BCI of -1




Frederic adds an assert to ensure that bci is a valid value [1] in the 
fix for JDK-8229375.  This would help catching the odd case with invalid 
BCI.


Alternatively, we can make bci an int (as Alekey suggests) that does not 
increase the object size:


   http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.02/

This means that this will replace Frederic's proposed patch for JDK-8229375.

What do you think?

Mandy
[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-August/035596.html

Re: RFR [14] 8217606: LdapContext#reconnect always opens a new connection

[Pavel Rappo]

Comments are inline.

> On 8 Aug 2019, at 18:59, Roger Riggs  wrote:
> 
> Hi Pavel,
> 
> To your questions...
> 
> 
> On 8/8/19 11:07 AM, Pavel Rappo wrote:
> ...
>>> 109: "template method" doesn't describe the method well, the method is 
>>> private and not overridable.
>>>update the javadoc.
>>> 
>> I can see several questions here. Correct me if I'm wrong. The first one is 
>> about the use of the term "Template Method". This is the name of the 
>> behavioral pattern that has been applied here. We can try to describe what 
>> that method does a bit better, however... here goes the second question. 
>> There is no Javadoc generated for classes like this one. So it's very likely 
>> that the documentation we are talking about will be read in conjunction with 
>> the source code in an editor. Combine that with the fact that this is a test 
>> supporting infrastructure and you might see why I think it would be easier 
>> to just outline the intent of the method. People will read the source code 
>> anyway. Which is not to say that documentation can be avoided altogether, 
>> it's just that I wouldn't sweat it.
> Since its a private method, it is not a template for anything and the term 
> doesn't apply.
>> The third question if that method should be overridable. The canonical 
>> version [1] of the Template Method pattern says: 
>> 
>> "...Primitive operations that _must_ be overridden are declared pure 
>> virtual. The template method itself should not be overridden..."
>> 
> Refering the 'Template" pattern doesn't make the documentation clearer.
>> I don't have a strong opinion about this, only a preference. I prefer that 
>> we will make it overridable only if we find out that the required degree of 
>> extensibility cannot be achieved by introducing additional extension/hook 
>> methods (the ones that the template method calls). Think about it this way. 
>> If that method were overridable, then why would that class be called 
>> `BaseLdapServer`? That method is responsible for that class' "LDAPness"!
> Private is fine.
> 
> ...
>>> 178:  logger() should be final. The subclass has no reason to override.  
>>> And it can be "public final".
>>> 
>> final? Yes. public? I'd rather leave it as it is. I can't think of a case 
>> where an *internal* logger should be sticking out of that class. Hence it's 
>> not public. It might be required when subclassing though, hence it's not 
>> private or of default (package) access.
> final is enough to avoid hacking around without giving it some thought.

I will update the documentation accordingly.

>>> 238: isRunning should be synchronized(lock) to make sure each Thread sees 
>>> the same value threads that update it in start() and close()
>>> 
>> We have already discussed it with you in this thread. The `state` is 
>> `volatile` which is enough for visibility in this query-method.
> yes, but there are multiple locking/synchronization constructs that make the 
> code harder to read
> as unnecessary complexity.

Fine. I will remove `volatile` and synchronize on `lock` in the `isRunning` 
method.

>>> 105:  Tests that sleep are prone to intermittent failures on slow or 
>>> delayed systems.
>>> 
>> True, however, I'm not aware of any general solution of this problem.
> General is not needed, and sleeps waste time time and resources.
> In this particular test, if there is no possiblity of multiple connections 
> then CountDownLatch is a good mechanism.

I would prefer to test for a particular number of connections.

>>> It would be more reliable to countdown *after* the Connection was handled.
>>> As is, it might report success even if handleRequest failed for some reason.
>>> 
>> Hm... Let me think. What we are interested in here is whether the connection 
>> is attempted or not. We may do what you are suggesting just to be sure the 
>> server is not failing. That is, the client is served successfully.
> The test description is not specific about that.  
> And it raises questions about the purpose of the environment setup.  
> Is it all needed. 

I will update the documentation.

> Is handling Unbind in the switch needed (as different from the default).

Chris might want to answer that.

>>> 
>>> 
>>> If there as some suspecion of too many connections
>>> that could be checked after the beforeConnectionHandled called countDown.
>>> 
>> Wouldn't that make problems of its own? If I got you right, a time window 
>> for the LDAP client to create more *unwanted* connections could be really 
>> small. A hybrid approach might be even better. We wait for the first 
>> connection with a generous timeout and then give the client some extra time 
>> to create more connections.
>> 
>> It really is fine-tuning. You can't shield completely from arbitrarily slow 
>> systems.
>> 
> That's fine, a Semaphore can be used to wait for the first connection and 
> then check with a different timeout for unexpected connections.

I've sketched some possible implementations of the hybrid 

Re: RFR: JDK-8227172: revert JDK-8225569 on windows

[Alexey Semenyuk]

Looks good.

- Alexey

On 8/10/2019 6:44 AM, Andy Herrick wrote:

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

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


This change will remove the "bin" directory on windows and revert to 
putting the executable(s) directly in output root dir. (dir>/)


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

[2] http://cr.openjdk.java.net/~herrick/8227172/webrev.01/

/Andy

Re: RFR(XS): 8229407: Avoid ConcurrentHashMap resizes during bootstrap

[Claes Redestad]

Alan, Aleksey, Daniel,

thanks for your reviews!

On 2019-08-12 13:03, Daniel Fuchs wrote:

Hi Claes,

I'd suggest adding a comment such as:

// sized to 32 to avoid resizing during bootstrap


I think this would be superfluous, so unless there is
popular demand I'll leave it as is.

Thanks!

/Claes

Re: RFR(XS): 8229407: Avoid ConcurrentHashMap resizes during bootstrap

[Daniel Fuchs]

Hi Claes,

I'd suggest adding a comment such as:

// sized to 32 to avoid resizing during bootstrap

best regards (and no need for a new review if you
decide to take in my suggestion).

-- daniel

On 12/08/2019 11:13, Claes Redestad wrote:

Hi core-libs,

analysis shows that a handful of small ConcurrentHashMaps used during
bootstrap is subject to some amount of resizing. Initializing these to
slightly larger values improves startup metrics measurably on default
images without regressing performance on minimal images.

Bug:    https://bugs.openjdk.java.net/browse/JDK-8229407
Webrev: http://cr.openjdk.java.net/~redestad/8229407/webrev.00/

Thanks!

/Claes

Re: RFR(XS): 8229407: Avoid ConcurrentHashMap resizes during bootstrap

[Aleksey Shipilev]

On 8/12/19 12:13 PM, Claes Redestad wrote:
> Hi core-libs,
> 
> analysis shows that a handful of small ConcurrentHashMaps used during
> bootstrap is subject to some amount of resizing. Initializing these to
> slightly larger values improves startup metrics measurably on default
> images without regressing performance on minimal images.
> 
> Bug:    https://bugs.openjdk.java.net/browse/JDK-8229407
> Webrev: http://cr.openjdk.java.net/~redestad/8229407/webrev.00/

Looks fine.

-Aleksey

Re: RFR(XS): 8229407: Avoid ConcurrentHashMap resizes during bootstrap

[Alan Bateman]

On 12/08/2019 11:13, Claes Redestad wrote:

Hi core-libs,

analysis shows that a handful of small ConcurrentHashMaps used during
bootstrap is subject to some amount of resizing. Initializing these to
slightly larger values improves startup metrics measurably on default
images without regressing performance on minimal images.

Bug:    https://bugs.openjdk.java.net/browse/JDK-8229407
Webrev: http://cr.openjdk.java.net/~redestad/8229407/webrev.00/

This looks okay.

-Alan

RFR(XS): 8229407: Avoid ConcurrentHashMap resizes during bootstrap

[Claes Redestad]

Hi core-libs,

analysis shows that a handful of small ConcurrentHashMaps used during
bootstrap is subject to some amount of resizing. Initializing these to
slightly larger values improves startup metrics measurably on default
images without regressing performance on minimal images.

Bug:https://bugs.openjdk.java.net/browse/JDK-8229407
Webrev: http://cr.openjdk.java.net/~redestad/8229407/webrev.00/

Thanks!

/Claes