RFR 8206123 : ArrayDeque created with default constructor can only hold 15 elements

[Ivan Gerasimov]

Hello!

The spec states that an ArrayDeque created with the default constructor 
should be able to hold 16 elements.


https://docs.oracle.com/javase/10/docs/api/java/util/ArrayDeque.html#%3Cinit%3E()
"""
Constructs an empty array deque with an initial capacity sufficient to 
hold 16 elements.

"""

In the constructor an array of size 16 is created:
elements = new Object[16];

However, one element must always be null:
/**
 * The array in which the elements of the deque are stored.
 * All array cells not holding deque elements are always null.
 * The array always has at least one null slot (at tail).
 */
transient Object[] elements;

which leaves us space for only 15 elements, contrarily to what the spec 
promises.



Would you please help review a trivial fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8206123
WEBREV: http://cr.openjdk.java.net/~igerasim/8206123/00/webrev/

Thanks in advance!

--
With kind regards,
Ivan Gerasimov

Re: [PATCH] AbstractStringBuilder.append()

[Paul Sandoz]

> On Jun 29, 2018, at 2:50 PM, Jonathan Gibbons  
> wrote:
> 
> In that specific snippet, I would expect the compiler (javac) to fold the 
> constants together.
> 

Yes, and behold the use of indy string concat :-)

Classfile /Users/sandoz/Projects/jdk/jdk-test/target/classes/Test.class
  Last modified Jun 29, 2018; size 873 bytes
  MD5 checksum b6eb6030a1b0840263ecced5880a52e6
  Compiled from "Test.java"
public class Test
  SourceFile: "Test.java"
  InnerClasses:
   public static final #37= #36 of #40; //Lookup=class 
java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles
  BootstrapMethods:
0: #24 invokestatic 
java/lang/invoke/StringConcatFactory.makeConcatWithConstants:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
  Method arguments:
#25 
bazbiz
  minor version: 0
  major version: 54
  flags: ACC_PUBLIC, ACC_SUPER
Constant pool:
...
  #22 = Utf8   barfoo
...
  #25 = String #30//  
bazbiz
{
  public Test();
...
  public static void main(java.lang.String[]);
descriptor: ([Ljava/lang/String;)V
flags: ACC_PUBLIC, ACC_STATIC
Code:
  stack=1, locals=2, args_size=1
 0: ldc   #2  // String barfoo
 2: astore_1  
 3: aload_1   
 4: invokedynamic #3,  0  // InvokeDynamic 
#0:makeConcatWithConstants:(Ljava/lang/String;)Ljava/lang/String;
 9: astore_1  
10: return   


Paul.

> -- Jon
> 
> On 06/29/2018 01:02 PM, Isaac Levy wrote:
>> Would this snippet end up merging two StringBuilders?
>> 
>> String x = "bar" + "foo";
>> x += "baz" + "biz";
>> 
>> I had trouble reading stringopts.cpp. I was thinking this append might
>> be common due to implicit StringBuilder creation.
>> 
>> Isaac
>> 
>> 
>> On Fri, Jun 29, 2018 at 12:48 PM, Paul Sandoz  wrote:
>>> Hard to reconstruct the culture memory around this. I suspect such a new 
>>> method was not considered necessary when StringBuilder was added to Java 
>>> 1.5 given the CharSequence accepting method, and performance was not 
>>> considered a concern, and I doubt it's a concern now.
>>> 
>>> (I don’t think there is any circularity that would result from bridge 
>>> methods if such a method was added.)
>>> 
>>> Paul.
>>> 
 On Jun 27, 2018, at 10:22 PM, Martin Buchholz  wrote:
 
 I'm fairly sure the append(StringBuilder) overloads were left out
 intentionally.
 
 On Wed, Jun 27, 2018 at 8:57 PM, Isaac Levy  wrote:
 
> AbstractStringBuilder already has append(). This patch
> adds append().
> 
> The new method improves parity between the two classes. In addition,
> combining StringBuilders is presumably common. append()
> has a couple insteadof checks, which this new method skips.
> 
> -Isaac
> 
> 
> 
> 
> diff --git a/src/java.base/share/classes/java/lang/
> AbstractStringBuilder.java
> b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
> index 2ef3e53256..1fe89bb92a 100644
> --- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
> +++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
> @@ -543,6 +543,11 @@ abstract class AbstractStringBuilder implements
> Appendable, CharSequence {
> return this.append((AbstractStringBuilder)sb);
> }
> 
> +// Documentation in subclasses because of synchro difference
> +public AbstractStringBuilder append(StringBuilder sb) {
> +return this.append((AbstractStringBuilder)sb);
> +}
> +
> /**
>  * @since 1.8
>  */
> diff --git a/src/java.base/share/classes/java/lang/StringBuffer.java
> b/src/java.base/share/classes/java/lang/StringBuffer.java
> index e597a8112e..613ba90c25 100644
> --- a/src/java.base/share/classes/java/lang/StringBuffer.java
> +++ b/src/java.base/share/classes/java/lang/StringBuffer.java
> @@ -313,6 +313,33 @@ import jdk.internal.HotSpotIntrinsicCandidate;
> return this;
> }
> 
> +/**
> + * Appends the specified {@code StringBuilder} to this sequence.
> + * 
> + * The characters of the {@code StringBuilder} argument are appended,
> + * in order, to the contents of this {@code StringBuffer}, increasing
> the
> + * length of this {@code StringBuffer} by the length of the argument.
> + * If {@code sb} is {@code null}, then the four characters
> + * {@code "null"} are appended to this {@code StringBuffer}.
> + * 
> + * Let n be the length of the old character sequence, the one
> + * contained in the {@code StringBuffer} just prior to execution of
> the
> + * {@code append} method. Then the character at index k in
> + * the new 

Re: [PATCH] AbstractStringBuilder.append()

[Jonathan Gibbons]

In that specific snippet, I would expect the compiler (javac) to fold 
the constants together.


-- Jon

On 06/29/2018 01:02 PM, Isaac Levy wrote:

Would this snippet end up merging two StringBuilders?

String x = "bar" + "foo";
x += "baz" + "biz";

I had trouble reading stringopts.cpp. I was thinking this append might
be common due to implicit StringBuilder creation.

Isaac


On Fri, Jun 29, 2018 at 12:48 PM, Paul Sandoz  wrote:

Hard to reconstruct the culture memory around this. I suspect such a new method 
was not considered necessary when StringBuilder was added to Java 1.5 given the 
CharSequence accepting method, and performance was not considered a concern, 
and I doubt it's a concern now.

(I don’t think there is any circularity that would result from bridge methods 
if such a method was added.)

Paul.


On Jun 27, 2018, at 10:22 PM, Martin Buchholz  wrote:

I'm fairly sure the append(StringBuilder) overloads were left out
intentionally.

On Wed, Jun 27, 2018 at 8:57 PM, Isaac Levy  wrote:


AbstractStringBuilder already has append(). This patch
adds append().

The new method improves parity between the two classes. In addition,
combining StringBuilders is presumably common. append()
has a couple insteadof checks, which this new method skips.

-Isaac




diff --git a/src/java.base/share/classes/java/lang/
AbstractStringBuilder.java
b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
index 2ef3e53256..1fe89bb92a 100644
--- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
+++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
@@ -543,6 +543,11 @@ abstract class AbstractStringBuilder implements
Appendable, CharSequence {
 return this.append((AbstractStringBuilder)sb);
 }

+// Documentation in subclasses because of synchro difference
+public AbstractStringBuilder append(StringBuilder sb) {
+return this.append((AbstractStringBuilder)sb);
+}
+
 /**
  * @since 1.8
  */
diff --git a/src/java.base/share/classes/java/lang/StringBuffer.java
b/src/java.base/share/classes/java/lang/StringBuffer.java
index e597a8112e..613ba90c25 100644
--- a/src/java.base/share/classes/java/lang/StringBuffer.java
+++ b/src/java.base/share/classes/java/lang/StringBuffer.java
@@ -313,6 +313,33 @@ import jdk.internal.HotSpotIntrinsicCandidate;
 return this;
 }

+/**
+ * Appends the specified {@code StringBuilder} to this sequence.
+ * 
+ * The characters of the {@code StringBuilder} argument are appended,
+ * in order, to the contents of this {@code StringBuffer}, increasing
the
+ * length of this {@code StringBuffer} by the length of the argument.
+ * If {@code sb} is {@code null}, then the four characters
+ * {@code "null"} are appended to this {@code StringBuffer}.
+ * 
+ * Let n be the length of the old character sequence, the one
+ * contained in the {@code StringBuffer} just prior to execution of
the
+ * {@code append} method. Then the character at index k in
+ * the new character sequence is equal to the character at index
k
+ * in the old character sequence, if k is less than n;
+ * otherwise, it is equal to the character at index k-n in the
+ * argument {@code sb}.
+ * 
+ *
+ * @param   sb   the {@code StringBuilder} to append.
+ * @return  a reference to this object.
+ */
+public synchronized StringBuffer append(StringBuilder sb) {
+toStringCache = null;
+super.append(sb);
+return this;
+}
+
 /**
  * Appends the specified {@code StringBuffer} to this sequence.
  * 
diff --git a/src/java.base/share/classes/java/lang/StringBuilder.java
b/src/java.base/share/classes/java/lang/StringBuilder.java
index 40da2083c2..5ddd4fb5f9 100644
--- a/src/java.base/share/classes/java/lang/StringBuilder.java
+++ b/src/java.base/share/classes/java/lang/StringBuilder.java
@@ -199,6 +199,30 @@ public final class StringBuilder
 return this;
 }

+/**
+ * Appends the specified {@code StringBuilder} to this sequence.
+ * 
+ * The characters of the {@code StringBuilder} argument are appended,
+ * in order, to this sequence, increasing the
+ * length of this sequence by the length of the argument.
+ * If {@code sb} is {@code null}, then the four characters
+ * {@code "null"} are appended to this sequence.
+ * 
+ * Let n be the length of this character sequence just prior to
+ * execution of the {@code append} method. Then the character at index
+ * k in the new character sequence is equal to the character at
+ * index k in the old character sequence, if k is less
than
+ * n; otherwise, it is equal to the character at index
k-n
+ * in the argument {@code sb}.
+ *
+ * @param   sb   the {@code StringBuilder} to append.
+ * @return  a reference to this object.
+ */
+public StringBuilder append(StringBuilder sb) {
+

Re: [PATCH] AbstractStringBuilder.append()

[Isaac Levy]

Would this snippet end up merging two StringBuilders?

String x = "bar" + "foo";
x += "baz" + "biz";

I had trouble reading stringopts.cpp. I was thinking this append might
be common due to implicit StringBuilder creation.

Isaac


On Fri, Jun 29, 2018 at 12:48 PM, Paul Sandoz  wrote:
> Hard to reconstruct the culture memory around this. I suspect such a new 
> method was not considered necessary when StringBuilder was added to Java 1.5 
> given the CharSequence accepting method, and performance was not considered a 
> concern, and I doubt it's a concern now.
>
> (I don’t think there is any circularity that would result from bridge methods 
> if such a method was added.)
>
> Paul.
>
>> On Jun 27, 2018, at 10:22 PM, Martin Buchholz  wrote:
>>
>> I'm fairly sure the append(StringBuilder) overloads were left out
>> intentionally.
>>
>> On Wed, Jun 27, 2018 at 8:57 PM, Isaac Levy  wrote:
>>
>>> AbstractStringBuilder already has append(). This patch
>>> adds append().
>>>
>>> The new method improves parity between the two classes. In addition,
>>> combining StringBuilders is presumably common. append()
>>> has a couple insteadof checks, which this new method skips.
>>>
>>> -Isaac
>>>
>>>
>>>
>>>
>>> diff --git a/src/java.base/share/classes/java/lang/
>>> AbstractStringBuilder.java
>>> b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>>> index 2ef3e53256..1fe89bb92a 100644
>>> --- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>>> +++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>>> @@ -543,6 +543,11 @@ abstract class AbstractStringBuilder implements
>>> Appendable, CharSequence {
>>> return this.append((AbstractStringBuilder)sb);
>>> }
>>>
>>> +// Documentation in subclasses because of synchro difference
>>> +public AbstractStringBuilder append(StringBuilder sb) {
>>> +return this.append((AbstractStringBuilder)sb);
>>> +}
>>> +
>>> /**
>>>  * @since 1.8
>>>  */
>>> diff --git a/src/java.base/share/classes/java/lang/StringBuffer.java
>>> b/src/java.base/share/classes/java/lang/StringBuffer.java
>>> index e597a8112e..613ba90c25 100644
>>> --- a/src/java.base/share/classes/java/lang/StringBuffer.java
>>> +++ b/src/java.base/share/classes/java/lang/StringBuffer.java
>>> @@ -313,6 +313,33 @@ import jdk.internal.HotSpotIntrinsicCandidate;
>>> return this;
>>> }
>>>
>>> +/**
>>> + * Appends the specified {@code StringBuilder} to this sequence.
>>> + * 
>>> + * The characters of the {@code StringBuilder} argument are appended,
>>> + * in order, to the contents of this {@code StringBuffer}, increasing
>>> the
>>> + * length of this {@code StringBuffer} by the length of the argument.
>>> + * If {@code sb} is {@code null}, then the four characters
>>> + * {@code "null"} are appended to this {@code StringBuffer}.
>>> + * 
>>> + * Let n be the length of the old character sequence, the one
>>> + * contained in the {@code StringBuffer} just prior to execution of
>>> the
>>> + * {@code append} method. Then the character at index k in
>>> + * the new character sequence is equal to the character at index
>>> k
>>> + * in the old character sequence, if k is less than n;
>>> + * otherwise, it is equal to the character at index k-n in the
>>> + * argument {@code sb}.
>>> + * 
>>> + *
>>> + * @param   sb   the {@code StringBuilder} to append.
>>> + * @return  a reference to this object.
>>> + */
>>> +public synchronized StringBuffer append(StringBuilder sb) {
>>> +toStringCache = null;
>>> +super.append(sb);
>>> +return this;
>>> +}
>>> +
>>> /**
>>>  * Appends the specified {@code StringBuffer} to this sequence.
>>>  * 
>>> diff --git a/src/java.base/share/classes/java/lang/StringBuilder.java
>>> b/src/java.base/share/classes/java/lang/StringBuilder.java
>>> index 40da2083c2..5ddd4fb5f9 100644
>>> --- a/src/java.base/share/classes/java/lang/StringBuilder.java
>>> +++ b/src/java.base/share/classes/java/lang/StringBuilder.java
>>> @@ -199,6 +199,30 @@ public final class StringBuilder
>>> return this;
>>> }
>>>
>>> +/**
>>> + * Appends the specified {@code StringBuilder} to this sequence.
>>> + * 
>>> + * The characters of the {@code StringBuilder} argument are appended,
>>> + * in order, to this sequence, increasing the
>>> + * length of this sequence by the length of the argument.
>>> + * If {@code sb} is {@code null}, then the four characters
>>> + * {@code "null"} are appended to this sequence.
>>> + * 
>>> + * Let n be the length of this character sequence just prior to
>>> + * execution of the {@code append} method. Then the character at index
>>> + * k in the new character sequence is equal to the character at
>>> + * index k in the old character sequence, if k is less
>>> than
>>> + * n; otherwise, it is equal to the 

Re: RFR(JDK12/JAXP/java.xml) 8190835: Subtraction with two javax.xml.datatype.Duration gives incorrect result

[Joe Wang]

Thanks Lance!  Pushed.

-Joe

On 6/28/18, 5:22 PM, Lance Andersen wrote:

Looks OK joe.


On Jun 28, 2018, at 5:06 PM, Joe Wang > wrote:


Hi,

Please review a quick fix for the first of JDK12/JAXP.

JBS: https://bugs.openjdk.java.net/browse/JDK-8190835
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8190835/webrev/ 



Thanks,
Joe




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: [PATCH] AbstractStringBuilder.append()

[Paul Sandoz]

Hard to reconstruct the culture memory around this. I suspect such a new method 
was not considered necessary when StringBuilder was added to Java 1.5 given the 
CharSequence accepting method, and performance was not considered a concern, 
and I doubt it's a concern now.

(I don’t think there is any circularity that would result from bridge methods 
if such a method was added.)

Paul.

> On Jun 27, 2018, at 10:22 PM, Martin Buchholz  wrote:
> 
> I'm fairly sure the append(StringBuilder) overloads were left out
> intentionally.
> 
> On Wed, Jun 27, 2018 at 8:57 PM, Isaac Levy  wrote:
> 
>> AbstractStringBuilder already has append(). This patch
>> adds append().
>> 
>> The new method improves parity between the two classes. In addition,
>> combining StringBuilders is presumably common. append()
>> has a couple insteadof checks, which this new method skips.
>> 
>> -Isaac
>> 
>> 
>> 
>> 
>> diff --git a/src/java.base/share/classes/java/lang/
>> AbstractStringBuilder.java
>> b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> index 2ef3e53256..1fe89bb92a 100644
>> --- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> +++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> @@ -543,6 +543,11 @@ abstract class AbstractStringBuilder implements
>> Appendable, CharSequence {
>> return this.append((AbstractStringBuilder)sb);
>> }
>> 
>> +// Documentation in subclasses because of synchro difference
>> +public AbstractStringBuilder append(StringBuilder sb) {
>> +return this.append((AbstractStringBuilder)sb);
>> +}
>> +
>> /**
>>  * @since 1.8
>>  */
>> diff --git a/src/java.base/share/classes/java/lang/StringBuffer.java
>> b/src/java.base/share/classes/java/lang/StringBuffer.java
>> index e597a8112e..613ba90c25 100644
>> --- a/src/java.base/share/classes/java/lang/StringBuffer.java
>> +++ b/src/java.base/share/classes/java/lang/StringBuffer.java
>> @@ -313,6 +313,33 @@ import jdk.internal.HotSpotIntrinsicCandidate;
>> return this;
>> }
>> 
>> +/**
>> + * Appends the specified {@code StringBuilder} to this sequence.
>> + * 
>> + * The characters of the {@code StringBuilder} argument are appended,
>> + * in order, to the contents of this {@code StringBuffer}, increasing
>> the
>> + * length of this {@code StringBuffer} by the length of the argument.
>> + * If {@code sb} is {@code null}, then the four characters
>> + * {@code "null"} are appended to this {@code StringBuffer}.
>> + * 
>> + * Let n be the length of the old character sequence, the one
>> + * contained in the {@code StringBuffer} just prior to execution of
>> the
>> + * {@code append} method. Then the character at index k in
>> + * the new character sequence is equal to the character at index
>> k
>> + * in the old character sequence, if k is less than n;
>> + * otherwise, it is equal to the character at index k-n in the
>> + * argument {@code sb}.
>> + * 
>> + *
>> + * @param   sb   the {@code StringBuilder} to append.
>> + * @return  a reference to this object.
>> + */
>> +public synchronized StringBuffer append(StringBuilder sb) {
>> +toStringCache = null;
>> +super.append(sb);
>> +return this;
>> +}
>> +
>> /**
>>  * Appends the specified {@code StringBuffer} to this sequence.
>>  * 
>> diff --git a/src/java.base/share/classes/java/lang/StringBuilder.java
>> b/src/java.base/share/classes/java/lang/StringBuilder.java
>> index 40da2083c2..5ddd4fb5f9 100644
>> --- a/src/java.base/share/classes/java/lang/StringBuilder.java
>> +++ b/src/java.base/share/classes/java/lang/StringBuilder.java
>> @@ -199,6 +199,30 @@ public final class StringBuilder
>> return this;
>> }
>> 
>> +/**
>> + * Appends the specified {@code StringBuilder} to this sequence.
>> + * 
>> + * The characters of the {@code StringBuilder} argument are appended,
>> + * in order, to this sequence, increasing the
>> + * length of this sequence by the length of the argument.
>> + * If {@code sb} is {@code null}, then the four characters
>> + * {@code "null"} are appended to this sequence.
>> + * 
>> + * Let n be the length of this character sequence just prior to
>> + * execution of the {@code append} method. Then the character at index
>> + * k in the new character sequence is equal to the character at
>> + * index k in the old character sequence, if k is less
>> than
>> + * n; otherwise, it is equal to the character at index
>> k-n
>> + * in the argument {@code sb}.
>> + *
>> + * @param   sb   the {@code StringBuilder} to append.
>> + * @return  a reference to this object.
>> + */
>> +public StringBuilder append(StringBuilder sb) {
>> +super.append(sb);
>> +return this;
>> +}
>> +
>> @Override
>> public StringBuilder append(CharSequence s) {
>>  

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

I've introduced a new test helper class in the jdk/test/lib/jfr 
directory to help with the dual test nature of the new tests. It's 
helped alot with test code duplication.


Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's 
fits. The output is displayed in units like "KiB" - not the normal when 
examining key lengths used in X509Certificates. i.e a 2048 bit key gets 
displayed as "2 KiB" - I'd prefer to keep the 2048 display version.


new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/

Regards,
Sean.

On 28/06/18 17:59, Seán Coffey wrote:

Comments inline.


On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method 
(regardless if JFR is enabled or not).


Some more feedback:

Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to serialNumber

all above done.

Rename field algId to AlgorithmicId or AlgorithmicID

maybe "algorithm" works here also ?

Rename @Label("Ciphersuite") to @Label("Cipher Suite")
Rename @Label("Cert Chain") to @Label("Certificate Chain")
Rename @Label("Property Name") to "Name" or "Key" if that makes sense 
in the context?

Rename @Label("Property Value") to "Value".
Put events in a subcategory, i.e  @Category({"Java Development Kit", 
"Security"})

done.

Make classes final.

done. I had thought that the JFR mechanism required non-final classes.
What is the unit of the key in X509Certificate event? Bits? If that 
is the case, use @DataAmount(DataAmount.BITS)
Yes - it's essentially the bit length of the keys used. Let me look 
into that annotation usage.
@Label("Serial numbers forming chain of trust") should not be a 
sentence. How about "Serial Numbers"?


I think the tests are hard to read when two things are tested at the 
same time. It is also likely that if a test fail due to logging 
issues, it will be assigned to JFR because of the test name, even 
thought the issues is not JFR related.
I think we're always going to have some ownership issues when tests 
serve a dual purpose. I still think it makes sense to keep the test 
logic in one place. Any failures in these tests will most likely be 
owned by security team. (moving the tests to security directory is 
also an option)


If you want to reuse code between tests, I would put it in testlibrary.
Let me check if there's any common patterns that could be added to the 
testlibary.


Thanks,
Sean.


Erik

Thanks for the update Erik. By default I'm proposing that the new 
JFR Events and Logger be disabled. As a result the event class 
shouldn't escape. If performance metrics highlight an issue, we 
should revisit.


regards,
Sean.


On 27/06/2018 20:57, Erik Gahlin wrote:

On 2018-06-27 21:14, Seán Coffey wrote:



On 27/06/2018 19:57, Xuelei Fan wrote:

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+.collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the 
above code will create a new instance.  Is the return value of 
cce.isEnabled() dynamically changed or static?
This is a requirement from the JFR framework. All their 
event.isEnabled calls are instance methods and follow a similar 
pattern. The JFR team assure me that the JIT can optimize away 
such calls.


The JIT will most likely not be able to optimize if the event class 
escapes.


From a JFR perspective, this would be the preferred layout:

EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();

and then do logging separately:

System.Logger.log(DEBUG, this.value)

With this layout, the JIT will remove the allocation and dead store.

If it is expensive to gather the data for the event, like the 
CertChainEvent above, the following pattern should be used.


EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
   eventB.value = calculateValue();
   eventB .commit();
}

If JFR is not enabled, shouldCommit returns false and the JIT 
should be able to remove the allocation.  This will be best from a 
performance point of view, and also in my opinion from a 
maintenance and readability perspective. Others may disagree.


Erik



Is there a need to support both logging and JFR?  I'm new to 
record events.  I did not get the point to use both.
I was initially hoping to concentrate on just JFR events but I got 
strong feedback to also consider use of Logger (esp. in cases 
where the jdk.jfr module is not available)


regards,
Sean.



Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't 

RFR(M): 8203826: Chain class initialization exceptions into later NoClassDefFoundErrors

[Volker Simonis]

Hi,

can I please have a review for the following change which saves
ExceptionInInitializerError thrown during class initialization and
chains them as cause into potential NoClassDefFoundErrors for the same
class. We are using this features since years in our commercial SAP
JVM and it proved extremely useful for detecting and fixing errors
especially in big deployments.

This is a follow-up on a discussion previously started by Goetz [1].
His first proposal (which is close to our current, internal
implementation) inserted an additional field into java.lang.Class
objects to save potential ExceptionInInitializerErrors. This was
considered to much overhead in the initial discussion [1].

http://cr.openjdk.java.net/~simonis/webrevs/2018/8203826.v2/
https://bugs.openjdk.java.net/browse/JDK-8203826

So in this change, I've completely re-implemented the feature by using
a java.lang.Hashtable which is attached to the ClassLoaderData object.
The Hashtable is lazily created when the first
ExceptionInInitializerError is thrown and maps the Class which
triggered the ExceptionInInitializerError during the execution of its
static initializer to the corresponding ExceptionInInitializerError.

If the same class will be accessed once again, this will directly lead
to a plain NoClassDefFoundError (as per the JVMS, 5.5 Initialization)
because the static initializer won't be executed a second time. Until
now, this NoClassDefFoundError wasn't linked in any way to the root
cause of the problem (i.e. the first ExceptionInInitializerError
together with the chained exception that happened during the execution
of the static initializer). With this change, the NoClassDefFoundError
will now chain the initial ExceptionInInitializerError as cause,
making it much easier to detect the problem which lead to the
NoClassDefFoundError.

Following is an example from the new JTreg tests which comes which
this change to demonstrate the feature. Until know, a typical stack
trace from a NoClassDefFoundError looked as follows:

java.lang.NoClassDefFoundError: Could not initialize class
NoClassDefFound$ClassWithFailedInitializer
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:291)
at NoClassDefFound.main(NoClassDefFound.java:38)

With this change, the same stack trace now looks as follows:

java.lang.NoClassDefFoundError: Could not initialize class
NoClassDefFound$ClassWithFailedInitializer
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:315)
at NoClassDefFound.main(NoClassDefFound.java:38)
Caused by: java.lang.ExceptionInInitializerError
at 
java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native
Method)
at 
java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
at java.base/java.lang.Class.newInstance(Class.java:584)
at 
NoClassDefFound$ClassWithFailedInitializer.(NoClassDefFound.java:20)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:315)
at NoClassDefFound.main(NoClassDefFound.java:30)
Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 2 out of
bounds for length 1
at NoClassDefFound$A.(NoClassDefFound.java:9)
... 9 more

As you can see, the reason for the NoClassDefFoundError when accessing
the class 'NoClassDefFound$ClassWithFailedInitializer' is actually not
even in the class or its static initializer itself, but in the class
'NoClassDefFound$A' which is a base class of
'NoClassDefFound$ClassWithFailedInitializer'. This is not easily
detectible from the old, plain NoClassDefFoundError.

As I wrote, the only overhead we have with the new implementation is
an additional OopHandle field per ClassLoaderData which I think is
acceptable. The Hashtable object itself is only created lazily, after
the first occurrence of an ExceptionInInitializerError in the
corresponding class loader. The whole Hashtable creation and
storing/quering of ExceptionInInitializerErrors in
ClassLoaderData::record_init_exception()/ClassLoaderData::query_init_exception()
is optional in the sense that any errors/exceptions occurring during
the execution of these functions are ignored and cleared.

Finally, we also take care to recursively convert all native
backtraces in the stored ExceptionInInitializerErrors (and their
suppressed and chained exceptions) into symbolic stack traces in order
to avoid holding references to classes and prevent their unloading.
This is implemented in the new private, static method
java.lang.Throwable::removeNativeBacktrace() which is called for each
ExceptionInInitializerError before it is stored in the Hashtable.

Thank you and best 

Re: 8143850: retrofit ArrayDeque to implement List

[Martin Buchholz]

On Thu, Jun 28, 2018 at 11:42 PM, Alex Foster  wrote:

> Another question is whether or not it should allow null elements.
> ArrayDeque currently doesn't but I think it would be useful to support
> them, which makes having an asList() view or sharing code harder.
>

JDK has moved away from null elements in collections.  We want the new
collection to implement both List and Deque, and the spec says

"""

While Deque implementations are not strictly required to prohibit the
insertion of null elements, they are strongly encouraged to do so. Users of
any Dequeimplementations that do allow null elements are strongly encouraged
 *not* to take advantage of the ability to insert nulls. This is so because
null is used as a special return value by various methods to indicate that
the deque is empty.

"""


---

Another idea is to simply add all of those List-like methods to ArrayDeque
without actually implementing List.  Then we have maximal efficiency (no
wrappers), we don't have to come up with a new name for the class, and a
user who wants a genuine 100% List view can do subList(deq, 0,
deq.size()).  In the past we've been reluctant to do such a strange thing
...