RFR: [15,docs] JDK-8247959,doclint errors in NIO code

[Jonathan Gibbons]

Please review some fixes to address issues found by doclint.

In a couple of places, imports were required in order to resolve symbols.
In another case, the name also had a typo in it.
In the last case, the incorrect syntax was used for a type parameter in 
@param.


-- Jon

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

Patch inline:

diff -r 086c7f077fc6 
src/jdk.nio.mapmode/share/classes/jdk/nio/mapmode/ExtendedMapMode.java
--- 
a/src/jdk.nio.mapmode/share/classes/jdk/nio/mapmode/ExtendedMapMode.java 
Fri Jun 19 15:22:19 2020 -0400
+++ 
b/src/jdk.nio.mapmode/share/classes/jdk/nio/mapmode/ExtendedMapMode.java 
Fri Jun 19 20:24:02 2020 -0700

@@ -25,6 +25,8 @@

 package jdk.nio.mapmode;

+import java.nio.MappedByteBuffer;
+import java.nio.channels.FileChannel;
 import java.nio.channels.FileChannel.MapMode;

 /**
@@ -52,7 +54,7 @@

 /**
  * File mapping mode for a read-write mapping of a file backed by
- * non-volatile RAM. {@linkplain MappedByteBufefr#force force}
+ * non-volatile RAM. {@linkplain MappedByteBuffer#force force}
  * operations on a buffer created with this mode will be performed
  * using cache line writeback rather than proceeding via a file
  * device flush.
diff -r 086c7f077fc6 
src/jdk.sctp/share/classes/com/sun/nio/sctp/NotificationHandler.java
--- 
a/src/jdk.sctp/share/classes/com/sun/nio/sctp/NotificationHandler.java 
Fri Jun 19 15:22:19 2020 -0400
+++ 
b/src/jdk.sctp/share/classes/com/sun/nio/sctp/NotificationHandler.java 
Fri Jun 19 20:24:02 2020 -0700

@@ -45,7 +45,7 @@
  * this handler interface as the type for parameters, return type, 
etc. rather

  * than the abstract class.
  *
- * @param T The type of the object attached to the receive operation
+ * @param  The type of the object attached to the receive operation
  *
  * @since 1.7
  */

Re: RFR: 8225251: Minimise import statements in jpackage sources

[alexander . matveev]

http://cr.openjdk.java.net/~almatvee/8225251/webrev.01/

- Expended "import static org.junit.Assert.*" and "import static 
jdk.incubator.jpackage.internal.StandardBundlerParam.*".
- Fixed inconsistency between imported StandardBundlerParam or when used 
directly. See 
http://cr.openjdk.java.net/~almatvee/8225251/webrev.01/src/jdk.incubator.jpackage/macosx/classes/jdk/incubator/jpackage/internal/MacAppImageBuilder.java.frames.html 
for example.


Thanks,
Alexander

On 6/19/20 3:36 PM, Andy Herrick wrote:

I'm fine with it either way - the main thing is what you have done.

/Andy

On 6/19/2020 6:02 PM, alexander.matv...@oracle.com wrote:

Hi Alexey,

Andy mentioned in issue description not to expend static imports, 
this is why all static imports were not expended. I can expend them 
or we can keep as is. I am fine with either way. Andy any comments 
why we do not need to expend static imports?


Thanks,
Alexander

On 6/19/20 10:49 AM, Alexey Semenyuk wrote:
This is how they were ut in unit tests initially. I'm OK either way, 
just wanted to double check these imports were not overlooked.


- Alexey

On 6/19/2020 12:30 PM, Kevin Rushforth wrote:
That a reasonably common pattern in JUnit tests, but expanding them 
to individual static imports is, of course, fine as well.


-- Kevin


On 6/19/2020 9:08 AM, Alexey Semenyuk wrote:

Alexander,

There is still
---
import static org.junit.Assert.*;
---
in unit tests. Is this intended?

- Alexey

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: [PATCH] 8247402: Rewrite the implementation requirements for Map::compute()

[林自均]

Hi Martin,

Any update on this? Thanks!

Best,
John Lin

林自均  於 2020年6月13日 週六 上午11:08寫道：
>
> Hi Martin,
>
> I see your point. Thank you for demostrating this for me.
>
> Here's my updated patch:
>
> # HG changeset patch
> # User John Lin 
> # Date 1591923561 -28800
> #  Fri Jun 12 08:59:21 2020 +0800
> # Node ID e01d9d020506a88d3d585bd3264594a26450c659
> # Parent  49a68abdb0ba68351db0f140ddac793b1c391bd5
> 8247402: Rewrite the implementation requirements for Map::compute()
>
> diff --git a/src/java.base/share/classes/java/util/Map.java
> b/src/java.base/share/classes/java/util/Map.java
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1113,17 +1113,12 @@ public interface Map {
>   *  {@code
>   * V oldValue = map.get(key);
>   * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - *if (newValue != null)
> - *   map.put(key, newValue);
> - *else
> - *   map.remove(key);
> + * if (newValue != null) {
> + * map.put(key, newValue);
>   * } else {
> - *if (newValue != null)
> - *   map.put(key, newValue);
> - *else
> - *   return null;
> + * map.remove(key);
>   * }
> + * return newValue;
>   * }
>   *
>   * The default implementation makes no guarantees about detecting if 
> the
>
> By the way, I saw everyone in this mailing list is using webrev.
> However, the tutorial https://openjdk.java.net/guide/codeReview.html
> says that only the users with push access to the OpenJDK Mercurial
> server can use webrev. Can I apply for push access to the OpenJDK
> Mercurial server too?
>
> Best,
> John Lin
>
> Martin Buchholz  於 2020年6月13日 週六 上午12:36寫道：
> >
> > If I rub that program, I get:
> >
> > HashMap false
> > HashMap1 false
> > HashMap2 true
> >
> > which suggests that HashMap2's implementation is wrong.

Re: RFR: 8225251: Minimise import statements in jpackage sources

[Andy Herrick]

I'm fine with it either way - the main thing is what you have done.

/Andy

On 6/19/2020 6:02 PM, alexander.matv...@oracle.com wrote:

Hi Alexey,

Andy mentioned in issue description not to expend static imports, this 
is why all static imports were not expended. I can expend them or we 
can keep as is. I am fine with either way. Andy any comments why we do 
not need to expend static imports?


Thanks,
Alexander

On 6/19/20 10:49 AM, Alexey Semenyuk wrote:
This is how they were ut in unit tests initially. I'm OK either way, 
just wanted to double check these imports were not overlooked.


- Alexey

On 6/19/2020 12:30 PM, Kevin Rushforth wrote:
That a reasonably common pattern in JUnit tests, but expanding them 
to individual static imports is, of course, fine as well.


-- Kevin


On 6/19/2020 9:08 AM, Alexey Semenyuk wrote:

Alexander,

There is still
---
import static org.junit.Assert.*;
---
in unit tests. Is this intended?

- Alexey

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: RFR: 8225251: Minimise import statements in jpackage sources

[alexander . matveev]

Hi Alexey,

Andy mentioned in issue description not to expend static imports, this 
is why all static imports were not expended. I can expend them or we can 
keep as is. I am fine with either way. Andy any comments why we do not 
need to expend static imports?


Thanks,
Alexander

On 6/19/20 10:49 AM, Alexey Semenyuk wrote:
This is how they were ut in unit tests initially. I'm OK either way, 
just wanted to double check these imports were not overlooked.


- Alexey

On 6/19/2020 12:30 PM, Kevin Rushforth wrote:
That a reasonably common pattern in JUnit tests, but expanding them 
to individual static imports is, of course, fine as well.


-- Kevin


On 6/19/2020 9:08 AM, Alexey Semenyuk wrote:

Alexander,

There is still
---
import static org.junit.Assert.*;
---
in unit tests. Is this intended?

- Alexey

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: RFR [15] 8247696: Incorrect tail computation for large segments in AbstractMemorySegmentImpl::mismatch

[Maurizio Cimadamore]

Looks good!

Thanks
Maurizio

On 19/06/2020 11:56, Chris Hegarty wrote:

Paul, Maurizio,

This version incorporates all feedback so far.

https://cr.openjdk.java.net/~chegar/8247696/webrev.01/
Results on my machine:

Benchmark  Mode  Cnt       Score        Error  Units
BulkOps.mismatch_large_bytebuffer  avgt   30   88266.728 
?   4083.476  ns/op
BulkOps.mismatch_large_segment     avgt   30   86141.343 
?   2450.450  ns/op

BulkOps.mismatch_small_bytebuffer  avgt   30       6.360 ?   0.425  ns/op
BulkOps.mismatch_small_segment     avgt   30       4.582 ?   1.040  ns/op

-Chris.

On 19 Jun 2020, at 00:35, Paul Sandoz > wrote:


Thanks Chris.

On Jun 18, 2020, at 2:57 AM, Maurizio Cimadamore 
> wrote:


Thanks for looking at this Chris

On 17/06/2020 21:56, Paul Sandoz wrote:

Hi Chris,

AbstractMemorySegmentImpl
—

In vectorizedMismatchLarge:

163             if (remaining > 7)
164                 throw new InternalError("remaining greater than 
7: " + remaining);

165             i = length - remaining;
166         }

Should this check be an assert?
I suggested that to Chris, since sometimes asserts are enabled when 
running tests, sometimes are not - in langtools we moved away from 
using asserts as we realized that in certain cases they were 
silently failing. I'm ok with whatever standard you feel comfortable 
with though.


Automated running of tests enable assertions (-ea -esa), perhaps the 
use is more commonly accepted in library code than compiler code. I 
would favor so in this case if necessary (sometimes they are avoided 
to reduce inline thresholds).





—

This fix prompted me to think more deeply about the implementation 
of vectorizedMismatchLarge and its use of 
vectorizedMismatch. Sorry, I should have thought about this more 
throughly earlier on.


We need to refine the approach, not for this patch, but something 
to follow up after. I think there are two issues.


1) The intrinsic method vectorizedMismatch could potentially bomb 
out at any point and return the bitwise compliment of the 
remaining number of elements to check.


Obviously, there is no point doing that arbitrarily but a stub 
implementation for, say, x86 AVX-512 might decide bomb out for  < 
64 remaining elements, rather than apply vector operations on 
smaller vector sizes or use a scalar loop. It does not today, but I 
think we should guard against that potentially happening, otherwise 
bad things can happen.


So your worry here is that we'll end up with an infinite loop, right?



Or more likely that an incorrect result is returned since tail 
elements will be skipped over as the offset and size is updated.



If so, we could check remaining against previous remaining and bomb 
out too if no further progress seem to be made?




I think it's better to always terminate the loop after the last 
sub-range is checked, rather than unnecessarily calling twice.


I am not confident the vectorizedMismatch intrinsic stub has been 
tested properly on very small lengths, since it's never directly 
called in such cases, so also keeping "remaining > 7" is good too.


Paul.



I think the loop should exit when the last sub-range has been 
checked. We should rely on other tests to ensure the intrinsic 
method is operating efficiently.



2) This method only works when operating on byte arrays. It will 
not work correctly if operating on short or long arrays, since we 
are not adjusting the length and offsets accordingly by the scale. 
It's probably easier to just rename this as 
vectorizedMismatchLargeForBytes and drop the log2ArrayIndexScale 
argument. Then expand later if need be. I still think the method 
rightly belongs in ArraysSupport.


Yep - probably good idea to restrict on bytes, for now.

Maurizio



Paul.

On Jun 17, 2020, at 8:33 AM, Chris Hegarty 
mailto:chris.hega...@oracle.com>> wrote:


The MemorySegment::mismatch implementation added vectorized 
mismatch of long sizes. The implementation is trivial, but 
the starting point for a more optimized implementation, if needed. 
ArraysSupport::vectorizedMismatchLarge incorrectly returns the 
bitwise complement of the offset of the first mismatch, where it 
should return the bitwise complement of the number of remaining 
pairs of elements to be checked in the tail of the two arrays. The 
AbstractMemorySegmentImpl::mismatch masked this problem, since 
it seamlessly compared the remaining tail, which is larger than it 
should be.


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

I updated the exiting BulkOps micro-benchmark to cover mismatch. 
Here are the results, compared to ByteBuffer::mismatch, on my machine:


Benchmark                          Mode  Cnt  Score        Error 
 Units
BulkOps.mismatch_large_bytebuffer  avgt   30 740186.973 ? 
119314.207  ns/op
BulkOps.mismatch_large_segment     avgt   30 683475.305 ? 
 76355.043  ns/op
BulkOps.mismatch_small_bytebuffer  avgt   30  7.367 ? 

Re: RFR: 8225251: Minimise import statements in jpackage sources

[Alexey Semenyuk]

This is how they were ut in unit tests initially. I'm OK either way, 
just wanted to double check these imports were not overlooked.


- Alexey

On 6/19/2020 12:30 PM, Kevin Rushforth wrote:
That a reasonably common pattern in JUnit tests, but expanding them to 
individual static imports is, of course, fine as well.


-- Kevin


On 6/19/2020 9:08 AM, Alexey Semenyuk wrote:

Alexander,

There is still
---
import static org.junit.Assert.*;
---
in unit tests. Is this intended?

- Alexey

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: RFR: 8225251: Minimise import statements in jpackage sources

[Kevin Rushforth]

That a reasonably common pattern in JUnit tests, but expanding them to 
individual static imports is, of course, fine as well.


-- Kevin


On 6/19/2020 9:08 AM, Alexey Semenyuk wrote:

Alexander,

There is still
---
import static org.junit.Assert.*;
---
in unit tests. Is this intended?

- Alexey

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: RFR: 8225251: Minimise import statements in jpackage sources

[Alexey Semenyuk]

Alexander,

There is still
---
import static org.junit.Assert.*;
---
in unit tests. Is this intended?

- Alexey

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: [PATCH] 8246633: Improve the performance of ObjectInputStream.resolveClass(ObjectStreamClass)

[Roger Riggs]

Hi Peter,

User-defined is a bit ambiguous but includes the appClassLoader since 
its contents
are controlled by classpath and module path properties and command line 
arguments.


Thanks, Roger


On 6/18/20 2:24 PM, Peter Kessler (Open Source) wrote:

Roger,

You are right that the existing code does not let me avoid the stack
walk. I withdraw the proposed patch.

I think at least part of the problem is the comment on
VM.latestUserDefinedLoader().  That says

 /*
  * Returns the first user-defined class loader up the execution stack,
  * or the platform class loader if only code from the platform or
  * bootstrap class loader is on the stack.
  */

I thought that a "user-defined class loader" was a *class loader*
defined by the user. I now think VM.latestUserDefinedLoader returns
the class loader of the first user-defined *method* up the execution
stack, whether that class loader is user-defined or is, for example,
the application class loader. I do not consider the application class
loader to be user-defined, though obviously methods loaded by the
application class loader can be user-defined.

   ... peter

-Original Message-
From: Roger Riggs 
Organization: Java Products Group, Oracle
Date: Tuesday, June 16, 2020 at 7:14 AM
To: "Peter Kessler (Open Source)" , 
"core-libs-dev@openjdk.java.net" 
Subject: Re: [PATCH] 8246633: Improve the performance of 
ObjectInputStream.resolveClass(ObjectStreamClass)

 Hi Peter,

 I think you've hardwired an assumption about the contents of the stack
 into VM.latestUserDefineLoader...

 On 6/12/20 7:19 PM, Peter Kessler (Open Source) wrote:
 > Roger,
 >
 > I did think about confining the changes to ObjectInputStream.  Maybe I
 > did not think hard enough about it.
 >
 > Keeping a cache of the result of a first stack walk might work for the
 > recursive calls.  One might still do a useless stack walk for each
 > top-level readObject call.  No concurrent class loading can invalidate
 > the cache for any particular stack walk.  But it seems fraught to
 > reuse a cached value in the face of concurrent class loading, or in
 > the presence of overloaded ObjectInputStream methods that might create
 > new ClassLoaders on this stack.  I'm sure I don't want to debug that.
 >
 > My experiments have shown that the average stack walk is about 20
 > frames.  I do not have the data on how many of those frames are
 > recursive calls within ObjectInputStream methods and how many are in
 > other code.
 I'd be interested in a histogram of the depths and in which applications.
 > Dragging in the StackWalker API would further complicate things, in my
 > opinion.  It might appear to keep the code out of platform native
 > code, but we know that walking thread stacks involves native code.
 >
 > I settled on a system-wide one-bit cache: There have been no
 > user-defined ClassLoaders constructed, so I can not find one on any
 > particular stack walk.  It may not be the best possible solution, but
 > it addresses the issue of useless stack walks in the common case where
 > there are no user-defined ClassLoaders.
 >
 > I agree that both ClassLoader and ObjectInputStream are complex.  In
 > fact, the proposed patch makes no changes to ObjectInputStream.  There
 > is one line added to each of the system-defined ClassLoaders to
 > identify them as not user-defined ClassLoaders, one line in the base
 > ClassLoader constructor, and the bulk of the change is around the
 > guard in jdk.internal.misc.VM.latestUserDefinedLoader() to avoid the
 > stack walk if possible.

 The spec and implementation of OIS.resolveClass do not make any assumptions
 about whether serialization is invoked from a class on the
 PlatformLoader, AppClassLoader,
 or another loader.

 In the case of a 'system' thread or class calling OIS.readObject, the
 change you are proposing
 will force class loading in resolveClass to the AppClassLoader.

 In VM.latestUserDefinedLoader:390 caught my eye because it forces the
 AppClassLoader.
 It assumes that scanning the stack would find and return the
 AppClassLoader, not so in all cases.

 Regards, Roger


 >
 >  ... peter
 >
 > -Original Message-
 > From: Roger Riggs 
 > Organization: Java Products Group, Oracle
 > Date: Friday, June 12, 2020 at 11:50 AM
 > To: "Peter Kessler (Open Source)" , 
"core-libs-dev@openjdk.java.net" 
 > Subject: Re: [PATCH] 8246633: Improve the performance of 
ObjectInputStream.resolveClass(ObjectStreamClass)
 >
 >  Hi Peter,
 >
 >  The hazard to avoid is the cross package coupling that makes both
 >  ClassLoader and
 >  ObjectInputStream more complex; both are more than sufficiently 
complex
 >  

Re: [PATCH] remove redundant initialization of volatile fields with default values

[Peter Levart]

Hi, Sergey,


On 6/19/20 11:33 AM, Сергей Цыпанов wrote:

Hello Remi,

thanks for pointing this out, I didn't take this into account.

As I understand, the volatile semantics here covers all the fields of an object,
no matter whether they are declared before or after volatile field (probably
actual object layout might be different from declaration in source code).


The layout of the fields doesn't matter. The order of assignment and the 
order of reading and volatileness/finallness of fields matter.





If that's true I think we still can apply this optimization at least for

1) classes with single field and without super-classes



I think that even removing assignment of default value to a field in a 
singular-field class (either volatile or not) in constructor can be 
observed. But I don't believe any program is relying on the observed 
behavior of such assignment - quite the contrary this might be a hidden bug.




2) classes with all non-volatile fields declared final



Final fields have to be explicitly assigned in the constructor even if 
they need to contain default value(s). But they have special JMM 
semantics and such assignment is not problematic.




3) classes with not-initialized non-volatile fields



This is similar to point 1). The additional non-explicitly assigned 
non-volatile fields don't matter here.




4) classes with super-classes where non-volatile fields are not initialized (or 
inialized with default values)



Where fields are declared (in super class or in sub class) does not 
matter. At runtime we are dealing with a single object. So if both 
non-volatile fields and volatile fields are assigned it depends on the 
order of assignments and on the order of reading those fields in the 
"whole program" whether removing volatile assignment of default value 
can have an effect on the program semantics. But as said, relying on the 
effects of the default value volatile write in constructor is fragile as 
it is not guaranteed by JMM. So such writes need to be studied and 
eventually removed.





Looking at source code I see that we could keep java.security.KeyStore and



KeyStore does not explicitly assign non-volatile non-final fields in 
constructor so the assignment of false to a volatile 'initialized' field 
in constructor can safely be removed.




its nested class PasswordProtection along with java.util.ListResourceBundle.



PasswordProtection has just final fields and one volatile field 
'destroyed' which is explicitly assigned to 'false' in constructor. This 
assignment can safely be removed.



ListResourceBundle is more complicated and would need to be studied more 
carefully.



Regards, Peter




Please correct me if I miss anything in my speculation.

Regards,
Sergey Tsypanov

19.06.2020, 10:04, "Remi Forax" :

Hi Sergei,
the problem is that you are changing the semantics if there are several fields.

By example with the code below, you have the guarantee that the code will print 
4 (if it prints something),
if you remove the assignment field = false, the code can print 0 or 4.

   class A {
 int i = 4;
 volatile boolean field = false;
   }

thread 1:
   global = new A()

thread 2:
   var a = global;
   if (a != null) {
 System.out.println(a.i);
   }

regards,
Rémi

- Mail original -

  De: "Сергей Цыпанов" 
  À: "core-libs-dev" 
  Envoyé: Vendredi 19 Juin 2020 06:57:25
  Objet: [PATCH] remove redundant initialization of volatile fields with 
default values
  Hello,

  while investigating an issue I've found out that assignment of default value 
to
  volatile fields slows down object instantiation.

  Consider the benchmark:

  @State(Scope.Thread)
  @OutputTimeUnit(TimeUnit.NANOSECONDS)
  @BenchmarkMode(value = Mode.AverageTime)
  @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
  public class VolatileFieldBenchmark {
   @Benchmark
   public Object explicitInit() {
 return new ExplicitInit();
   }

   @Benchmark
   public Object noInit() {
 return new NoInit();
   }

   private static class ExplicitInit {
 private volatile boolean field = false;
   }
   private static class NoInit {
 private volatile boolean field;
   }
  }

  This gives the following results as of my machine:

  Benchmark Mode Cnt Score Error Units
  VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
  VolatileFieldBenchmark.noInit avgt 40 3.367 ± 0.131 ns/op

  I've looked into source code of java.base and found out several cases where 
the
  default value is assigned to volatile field.

  Getting rid of such assignements demonstates improvement as of object
  instantiation, e.g. javax.security.auth.Subject:

    Mode Cnt Score Error Units
  before avgt 40 35.933 ± 2.647 ns/op
  after avgt 40 30.817 ± 2.384 ns/op

  As of testing tier1 and tier2 are both ok after the changes.

  Best regards,
  Sergey Tsypanov

Re: [PATCH] remove redundant initialization of volatile fields with default values

[David Holmes]

On 19/06/2020 11:25 pm, David Holmes wrote:

Hi Remi,

On 19/06/2020 6:03 pm, Remi Forax wrote:

Hi Sergei,
the problem is that you are changing the semantics if there are 
several fields.


By example with the code below, you have the guarantee that the code 
will print 4 (if it prints something),

if you remove the assignment field = false, the code can print 0 or 4.

   class A {
 int i = 4;
 volatile boolean field = false;
   }

thread 1:
   global = new A()

thread 2:
   var a = global;
   if (a != null) {
 System.out.println(a.i);
   }


Surely you intended to read field to get the volatile read to pair with 
the volatile write of the initializer? Otherwise there is no 
happens-before ordering.


And as Peter points out that may not suffice as you can't distinguish 
the default initialization of field with the actual volatile write in 
the initializer.


David


David


regards,
Rémi

- Mail original -

De: "Сергей Цыпанов" 
À: "core-libs-dev" 
Envoyé: Vendredi 19 Juin 2020 06:57:25
Objet: [PATCH] remove redundant initialization of volatile fields 
with default values



Hello,

while investigating an issue I've found out that assignment of 
default value to

volatile fields slows down object instantiation.

Consider the benchmark:

@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(value = Mode.AverageTime)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class VolatileFieldBenchmark {
  @Benchmark
  public Object explicitInit() {
    return new ExplicitInit();
  }

  @Benchmark
  public Object noInit() {
    return new NoInit();
  }

  private static class ExplicitInit {
    private volatile boolean field = false;
  }
  private static class NoInit {
    private volatile boolean field;
  }
}

This gives the following results as of my machine:

Benchmark    Mode  Cnt   Score   Error  Units
VolatileFieldBenchmark.explicitInit  avgt   40  11.087 ± 0.140  ns/op
VolatileFieldBenchmark.noInit    avgt   40   3.367 ± 0.131  ns/op


I've looked into source code of java.base and found out several cases 
where the

default value is assigned to volatile field.

Getting rid of such assignements demonstates improvement as of object
instantiation, e.g. javax.security.auth.Subject:

   Mode  Cnt   Score   Error  Units
before avgt   40  35.933 ± 2.647  ns/op
after  avgt   40  30.817 ± 2.384  ns/op

As of testing tier1 and tier2 are both ok after the changes.

Best regards,
Sergey Tsypanov

Re: [PATCH] remove redundant initialization of volatile fields with default values

[David Holmes]

Hi Remi,

On 19/06/2020 6:03 pm, Remi Forax wrote:

Hi Sergei,
the problem is that you are changing the semantics if there are several fields.

By example with the code below, you have the guarantee that the code will print 
4 (if it prints something),
if you remove the assignment field = false, the code can print 0 or 4.

   class A {
 int i = 4;
 volatile boolean field = false;
   }

thread 1:
   global = new A()

thread 2:
   var a = global;
   if (a != null) {
 System.out.println(a.i);
   }


Surely you intended to read field to get the volatile read to pair with 
the volatile write of the initializer? Otherwise there is no 
happens-before ordering.


David


regards,
Rémi

- Mail original -

De: "Сергей Цыпанов" 
À: "core-libs-dev" 
Envoyé: Vendredi 19 Juin 2020 06:57:25
Objet: [PATCH] remove redundant initialization of volatile fields with default 
values



Hello,

while investigating an issue I've found out that assignment of default value to
volatile fields slows down object instantiation.

Consider the benchmark:

@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(value = Mode.AverageTime)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class VolatileFieldBenchmark {
  @Benchmark
  public Object explicitInit() {
return new ExplicitInit();
  }

  @Benchmark
  public Object noInit() {
return new NoInit();
  }

  private static class ExplicitInit {
private volatile boolean field = false;
  }
  private static class NoInit {
private volatile boolean field;
  }
}

This gives the following results as of my machine:

BenchmarkMode  Cnt   Score   Error  Units
VolatileFieldBenchmark.explicitInit  avgt   40  11.087 ± 0.140  ns/op
VolatileFieldBenchmark.noInitavgt   40   3.367 ± 0.131  ns/op


I've looked into source code of java.base and found out several cases where the
default value is assigned to volatile field.

Getting rid of such assignements demonstates improvement as of object
instantiation, e.g. javax.security.auth.Subject:

   Mode  Cnt   Score   Error  Units
before avgt   40  35.933 ± 2.647  ns/op
after  avgt   40  30.817 ± 2.384  ns/op

As of testing tier1 and tier2 are both ok after the changes.

Best regards,
Sergey Tsypanov

Re: [PATCH] remove redundant initialization of volatile fields with default values

[Peter Levart]

Hi Remi,

On 6/19/20 10:03 AM, Remi Forax wrote:

Hi Sergei,
the problem is that you are changing the semantics if there are several fields.

By example with the code below, you have the guarantee that the code will print 
4 (if it prints something),
if you remove the assignment field = false, the code can print 0 or 4.

   class A {
 int i = 4;
 volatile boolean field = false;
   }

thread 1:
   global = new A()

thread 2:
   var a = global;
   if (a != null) {
 System.out.println(a.i);
   }



I don't think this is guaranteed by the JMM. Unless you also read the 
'field' in thread 2 and observe the effects of write to 'field' before 
reading 'i' :



thread 2:

    var a = global;

    if (a != null && !a.field) {

        System.out.println(a.i);

    }


And even that might not work as you have to "observe" the effects of 
write to 'field' before reading 'i' and if the value of 'field' didn't 
change by the write, nothing guarantees that you have observed the 
effects of write to 'field' if you read false from it. So we could argue 
that any program that relies on similar unexistent guarantees is wrong, 
but works because the implementation usually exhibits stronger semantics 
than required. So removing such volatile write might break some programs 
in practice that are already broken in theory.


Anyway, assigning default value to a field in constructor is always sign 
of bed smell and it is almost always possible to restructure code so 
that it is not needed. You have to be careful and see the "whole 
program" since other places might have to be modified too to preserve 
the semantics.


Regards, Peter



regards,
Rémi

- Mail original -

De: "Сергей Цыпанов" 
À: "core-libs-dev" 
Envoyé: Vendredi 19 Juin 2020 06:57:25
Objet: [PATCH] remove redundant initialization of volatile fields with default 
values
Hello,

while investigating an issue I've found out that assignment of default value to
volatile fields slows down object instantiation.

Consider the benchmark:

@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(value = Mode.AverageTime)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class VolatileFieldBenchmark {
  @Benchmark
  public Object explicitInit() {
return new ExplicitInit();
  }

  @Benchmark
  public Object noInit() {
return new NoInit();
  }

  private static class ExplicitInit {
private volatile boolean field = false;
  }
  private static class NoInit {
private volatile boolean field;
  }
}

This gives the following results as of my machine:

BenchmarkMode  Cnt   Score   Error  Units
VolatileFieldBenchmark.explicitInit  avgt   40  11.087 ± 0.140  ns/op
VolatileFieldBenchmark.noInitavgt   40   3.367 ± 0.131  ns/op


I've looked into source code of java.base and found out several cases where the
default value is assigned to volatile field.

Getting rid of such assignements demonstates improvement as of object
instantiation, e.g. javax.security.auth.Subject:

   Mode  Cnt   Score   Error  Units
before avgt   40  35.933 ± 2.647  ns/op
after  avgt   40  30.817 ± 2.384  ns/op

As of testing tier1 and tier2 are both ok after the changes.

Best regards,
Sergey Tsypanov

Re: RFR: 8225251: Minimise import statements in jpackage sources

[Andy Herrick]

all looks good.

/Andy

On 6/19/2020 4:46 AM, alexander.matv...@oracle.com wrote:

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

Cleanup import statements.

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

Thanks,
Alexander

Re: RFR [15] 8247696: Incorrect tail computation for large segments in AbstractMemorySegmentImpl::mismatch

[Chris Hegarty]

Paul, Maurizio,

This version incorporates all feedback so far.

https://cr.openjdk.java.net/~chegar/8247696/webrev.01/
Results on my machine:

Benchmark  Mode  Cnt   ScoreError  Units
BulkOps.mismatch_large_bytebuffer  avgt   30   88266.728 ?   4083.476  ns/op
BulkOps.mismatch_large_segment avgt   30   86141.343 ?   2450.450  ns/op
BulkOps.mismatch_small_bytebuffer  avgt   30   6.360 ?  0.425  ns/op
BulkOps.mismatch_small_segment avgt   30   4.582 ?  1.040  ns/op

-Chris.

> On 19 Jun 2020, at 00:35, Paul Sandoz  wrote:
> 
> Thanks Chris.
> 
>> On Jun 18, 2020, at 2:57 AM, Maurizio Cimadamore 
>>  wrote:
>> 
>> Thanks for looking at this Chris
>> 
>> On 17/06/2020 21:56, Paul Sandoz wrote:
>>> Hi Chris,
>>> 
>>> AbstractMemorySegmentImpl
>>> —
>>> 
>>> In vectorizedMismatchLarge:
>>> 
>>> 163 if (remaining > 7)
>>> 164 throw new InternalError("remaining greater than 7: " + 
>>> remaining);
>>> 165 i = length - remaining;
>>> 166 }
>>> 
>>> Should this check be an assert?
>> I suggested that to Chris, since sometimes asserts are enabled when running 
>> tests, sometimes are not - in langtools we moved away from using asserts as 
>> we realized that in certain cases they were silently failing. I'm ok with 
>> whatever standard you feel comfortable with though.
> 
> Automated running of tests enable assertions (-ea -esa), perhaps the use is 
> more commonly accepted in library code than compiler code. I would favor so 
> in this case if necessary (sometimes they are avoided to reduce inline 
> thresholds).
> 
> 
>>> 
>>> —
>>> 
>>> This fix prompted me to think more deeply about the implementation of 
>>> vectorizedMismatchLarge and its use of vectorizedMismatch. Sorry, I should 
>>> have thought about this more throughly earlier on.
>>> 
>>> We need to refine the approach, not for this patch, but something to follow 
>>> up after. I think there are two issues.
>>> 
>>> 1) The intrinsic method vectorizedMismatch could potentially bomb out at 
>>> any point and return the bitwise compliment of the remaining number of 
>>> elements to check.
>>> 
>>> Obviously, there is no point doing that arbitrarily but a stub 
>>> implementation for, say, x86 AVX-512 might decide bomb out for  < 64 
>>> remaining elements, rather than apply vector operations on smaller vector 
>>> sizes or use a scalar loop. It does not today, but I think we should guard 
>>> against that potentially happening, otherwise bad things can happen.
>> 
>> So your worry here is that we'll end up with an infinite loop, right?
>> 
> 
> Or more likely that an incorrect result is returned since tail elements will 
> be skipped over as the offset and size is updated.
> 
> 
>> If so, we could check remaining against previous remaining and bomb out too 
>> if no further progress seem to be made?
>> 
> 
> I think it's better to always terminate the loop after the last sub-range is 
> checked, rather than unnecessarily calling twice.
> 
> I am not confident the vectorizedMismatch intrinsic stub has been tested 
> properly on very small lengths, since it's never directly called in such 
> cases, so also keeping "remaining > 7" is good too. 
> 
> Paul.
> 
>>> 
>>> I think the loop should exit when the last sub-range has been checked. We 
>>> should rely on other tests to ensure the intrinsic method is operating 
>>> efficiently.
>>> 
>>> 
>>> 2) This method only works when operating on byte arrays. It will not work 
>>> correctly if operating on short or long arrays, since we are not adjusting 
>>> the length and offsets accordingly by the scale. It's probably easier to 
>>> just rename this as vectorizedMismatchLargeForBytes and drop the 
>>> log2ArrayIndexScale argument. Then expand later if need be. I still think 
>>> the method rightly belongs in ArraysSupport.
>> 
>> Yep - probably good idea to restrict on bytes, for now.
>> 
>> Maurizio
>> 
>>> 
>>> Paul.
>>> 
 On Jun 17, 2020, at 8:33 AM, Chris Hegarty  
 wrote:
 
 The MemorySegment::mismatch implementation added vectorized mismatch of 
 long sizes. The implementation is trivial, but the starting point for a 
 more optimized implementation, if needed. 
 ArraysSupport::vectorizedMismatchLarge incorrectly returns the bitwise 
 complement of the offset of the first mismatch, where it should return the 
 bitwise complement of the number of remaining pairs of elements to be 
 checked in the tail of the two arrays. The 
 AbstractMemorySegmentImpl::mismatch masked this problem, since it 
 seamlessly compared the remaining tail, which is larger than it should be.
 
 Webrev:
 https://cr.openjdk.java.net/~chegar/8247696/webrev.00/
 
 I updated the exiting BulkOps micro-benchmark to cover mismatch. Here are 
 the results, compared to ByteBuffer::mismatch, on my machine:
 
 Benchmark  Mode  Cnt

Re: RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Rahul Yadav]

Hi Daniel,

Thank you for the feedback.

I have added the new case for thread id > Integer.MAX_VALUE.
have updated comments/names and string to JDK 16.
I will follow-up with a new JBS issue for the backport

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html


- rahul

On 19/06/2020 09:58, Daniel Fuchs wrote:

Hi Rahul,

This looks very good.

I have some comments regarding the SerializeLogRecord test:

- the test speaks of JDK 15 at several places: is it really JDK 15, or
  should it be JDK 16? I mean - was the serialized bytes generated
  before your fix or after?

- the generate() method (no args) should probably have a test case
  where a log record has a long thread id > Integer.MAX_VALUE.

- it might be good to work on a version of this test that could
  be backported to JDK 15 (and 11) to verify that a LogRecord
  serialized with JDK 16 can be deserialized in those versions.
  You should probably log a JBS issue to follow-up on that.

best regards,

-- daniel


On 18/06/2020 23:37, Rahul Yadav wrote:

Hi Alan,

Thank you for the feedback.I have updated the webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html


- rahul

Re: [PATCH] remove redundant initialization of volatile fields with default values

[Сергей Цыпанов]

Hello Remi,

thanks for pointing this out, I didn't take this into account.

As I understand, the volatile semantics here covers all the fields of an 
object, 
no matter whether they are declared before or after volatile field (probably
actual object layout might be different from declaration in source code).

If that's true I think we still can apply this optimization at least for

1) classes with single field and without super-classes
2) classes with all non-volatile fields declared final
3) classes with not-initialized non-volatile fields
4) classes with super-classes where non-volatile fields are not initialized (or 
inialized with default values)

Looking at source code I see that we could keep java.security.KeyStore and 
its nested class PasswordProtection along with java.util.ListResourceBundle.

Please correct me if I miss anything in my speculation.

Regards,
Sergey Tsypanov

19.06.2020, 10:04, "Remi Forax" :
> Hi Sergei,
> the problem is that you are changing the semantics if there are several 
> fields.
>
> By example with the code below, you have the guarantee that the code will 
> print 4 (if it prints something),
> if you remove the assignment field = false, the code can print 0 or 4.
>
>   class A {
> int i = 4;
> volatile boolean field = false;
>   }
>
> thread 1:
>   global = new A()
>
> thread 2:
>   var a = global;
>   if (a != null) {
> System.out.println(a.i);
>   }
>
> regards,
> Rémi
>
> - Mail original -
>>  De: "Сергей Цыпанов" 
>>  À: "core-libs-dev" 
>>  Envoyé: Vendredi 19 Juin 2020 06:57:25
>>  Objet: [PATCH] remove redundant initialization of volatile fields with 
>> default values
>
>>  Hello,
>>
>>  while investigating an issue I've found out that assignment of default 
>> value to
>>  volatile fields slows down object instantiation.
>>
>>  Consider the benchmark:
>>
>>  @State(Scope.Thread)
>>  @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>  @BenchmarkMode(value = Mode.AverageTime)
>>  @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>>  public class VolatileFieldBenchmark {
>>   @Benchmark
>>   public Object explicitInit() {
>> return new ExplicitInit();
>>   }
>>
>>   @Benchmark
>>   public Object noInit() {
>> return new NoInit();
>>   }
>>
>>   private static class ExplicitInit {
>> private volatile boolean field = false;
>>   }
>>   private static class NoInit {
>> private volatile boolean field;
>>   }
>>  }
>>
>>  This gives the following results as of my machine:
>>
>>  Benchmark Mode Cnt Score Error Units
>>  VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
>>  VolatileFieldBenchmark.noInit avgt 40 3.367 ± 0.131 ns/op
>>
>>  I've looked into source code of java.base and found out several cases where 
>> the
>>  default value is assigned to volatile field.
>>
>>  Getting rid of such assignements demonstates improvement as of object
>>  instantiation, e.g. javax.security.auth.Subject:
>>
>>    Mode Cnt Score Error Units
>>  before avgt 40 35.933 ± 2.647 ns/op
>>  after avgt 40 30.817 ± 2.384 ns/op
>>
>>  As of testing tier1 and tier2 are both ok after the changes.
>>
>>  Best regards,
>>  Sergey Tsypanovdiff --git a/src/java.base/share/classes/java/security/KeyStore.java b/src/java.base/share/classes/java/security/KeyStore.java
--- a/src/java.base/share/classes/java/security/KeyStore.java
+++ b/src/java.base/share/classes/java/security/KeyStore.java
@@ -219,7 +219,7 @@
 private KeyStoreSpi keyStoreSpi;
 
 // Has this keystore been initialized (loaded)?
-private boolean initialized = false;
+private boolean initialized;
 
 /**
  * A marker interface for {@code KeyStore}
@@ -264,7 +264,7 @@
 private final char[] password;
 private final String protectionAlgorithm;
 private final AlgorithmParameterSpec protectionParameters;
-private volatile boolean destroyed = false;
+private volatile boolean destroyed;
 
 /**
  * Creates a password parameter.
diff --git a/src/java.base/share/classes/java/util/ListResourceBundle.java b/src/java.base/share/classes/java/util/ListResourceBundle.java
--- a/src/java.base/share/classes/java/util/ListResourceBundle.java
+++ b/src/java.base/share/classes/java/util/ListResourceBundle.java
@@ -206,5 +206,5 @@
 lookup = temp;
 }
 
-private volatile Map lookup = null;
+private volatile Map lookup;
 }

Re: RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Rahul Yadav]

Thank you Alan, updated webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html


- rahul

On 19/06/2020 08:43, Alan Bateman wrote:

On 18/06/2020 23:37, Rahul Yadav wrote:

Hi Alan,

Thank you for the feedback.I have updated the webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html

This looks quite good.

The comment in shortShortID has "any positive long less than 
Integer.MAX_VALUE" but it's actually <= MAX_VALUE.


I don't think MIN_SEQUENTIAL_THREAD_ID is used so I assume it can be 
removed.


The @return for setLongThreadID has a description "Log Record" but 
this should "this LogRecord".


Can you update SerializeLogRecordTest with clear instructions on how 
to generate the stream? This will help future maintainers that may 
have to update this test.


-Alan

Re: RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Daniel Fuchs]

Hi Rahul,

This looks very good.

I have some comments regarding the SerializeLogRecord test:

- the test speaks of JDK 15 at several places: is it really JDK 15, or
  should it be JDK 16? I mean - was the serialized bytes generated
  before your fix or after?

- the generate() method (no args) should probably have a test case
  where a log record has a long thread id > Integer.MAX_VALUE.

- it might be good to work on a version of this test that could
  be backported to JDK 15 (and 11) to verify that a LogRecord
  serialized with JDK 16 can be deserialized in those versions.
  You should probably log a JBS issue to follow-up on that.

best regards,

-- daniel


On 18/06/2020 23:37, Rahul Yadav wrote:

Hi Alan,

Thank you for the feedback.I have updated the webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html


- rahul

RFR: 8225251: Minimise import statements in jpackage sources

[alexander . matveev]

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

Cleanup import statements.

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

Thanks,
Alexander

Re: [PATCH] remove redundant initialization of volatile fields with default values

[Remi Forax]

Hi Sergei,
the problem is that you are changing the semantics if there are several fields.

By example with the code below, you have the guarantee that the code will print 
4 (if it prints something),
if you remove the assignment field = false, the code can print 0 or 4.

  class A {
int i = 4;
volatile boolean field = false;
  }

thread 1:
  global = new A()

thread 2:
  var a = global;
  if (a != null) {
System.out.println(a.i);
  }

regards,
Rémi

- Mail original -
> De: "Сергей Цыпанов" 
> À: "core-libs-dev" 
> Envoyé: Vendredi 19 Juin 2020 06:57:25
> Objet: [PATCH] remove redundant initialization of volatile fields with 
> default values

> Hello,
> 
> while investigating an issue I've found out that assignment of default value 
> to
> volatile fields slows down object instantiation.
> 
> Consider the benchmark:
> 
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class VolatileFieldBenchmark {
>  @Benchmark
>  public Object explicitInit() {
>return new ExplicitInit();
>  }
> 
>  @Benchmark
>  public Object noInit() {
>return new NoInit();
>  }
> 
>  private static class ExplicitInit {
>private volatile boolean field = false;
>  }
>  private static class NoInit {
>private volatile boolean field;
>  }
> }
> 
> This gives the following results as of my machine:
> 
> BenchmarkMode  Cnt   Score   Error  Units
> VolatileFieldBenchmark.explicitInit  avgt   40  11.087 ± 0.140  ns/op
> VolatileFieldBenchmark.noInitavgt   40   3.367 ± 0.131  ns/op
> 
> 
> I've looked into source code of java.base and found out several cases where 
> the
> default value is assigned to volatile field.
> 
> Getting rid of such assignements demonstates improvement as of object
> instantiation, e.g. javax.security.auth.Subject:
> 
>   Mode  Cnt   Score   Error  Units
> before avgt   40  35.933 ± 2.647  ns/op
> after  avgt   40  30.817 ± 2.384  ns/op
> 
> As of testing tier1 and tier2 are both ok after the changes.
> 
> Best regards,
> Sergey Tsypanov

Re: [PATCH] remove redundant initialization of volatile fields with default values

[Alan Bateman]

On 19/06/2020 05:57, Сергей Цыпанов wrote:

Hello,

while investigating an issue I've found out that assignment of default value to 
volatile fields slows down object instantiation.

Yes, there's been several efforts over the years to eliminate the 
initializing volatile fields to their default values. It's good to keep 
these in check. It's probably best to get a sponsor on the security-libs 
list as all by one of these in the security code.


-Alan

Re: RFR 8245302: Upgrade LogRecord to support long thread ids and remove its usage of ThreadLocal

[Alan Bateman]

On 18/06/2020 23:37, Rahul Yadav wrote:

Hi Alan,

Thank you for the feedback.I have updated the webrev.

webrev : 
http://cr.openjdk.java.net/~ryadav/webrev_8245302/webrev.00/index.html

This looks quite good.

The comment in shortShortID has "any positive long less than 
Integer.MAX_VALUE" but it's actually <= MAX_VALUE.


I don't think MIN_SEQUENTIAL_THREAD_ID is used so I assume it can be 
removed.


The @return for setLongThreadID has a description "Log Record" but this 
should "this LogRecord".


Can you update SerializeLogRecordTest with clear instructions on how to 
generate the stream? This will help future maintainers that may have to 
update this test.


-Alan

Re: RFR: [15,docs] JDK-8247899,HTML errors and warnings in threadPrimitiveDeprecation.html

[David Holmes]

Looks good and trivial!

Thanks,
David

On 19/06/2020 10:18 am, Jonathan Gibbons wrote:

Please review this trivial fix to some minor issues reported by doclint.

In the first change, the ``  contained a hangover of some HTML 4 
attributes

which are not supported in HTML5. They are simply deleted.

In the second change, a paragraph just contained a comment. While it might
be reasonable to just remove the paragraph tags surrounding the comment,
the comment itself ("Body text ends here") is somewhat redundant, appearing
as it does right before `` and so the entire line is deleted.

-- Jon

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

Inline patch:

diff -r f80508232687 
src/java.base/share/classes/java/lang/doc-files/threadPrimitiveDeprecation.html 

--- 
a/src/java.base/share/classes/java/lang/doc-files/threadPrimitiveDeprecation.html 
Thu Jun 18 16:21:34 2020 -0700
+++ 
b/src/java.base/share/classes/java/lang/doc-files/threadPrimitiveDeprecation.html 
Thu Jun 18 17:10:24 2020 -0700

@@ -282,7 +282,7 @@
  }
  }
  
-
+
  Can I combine the two techniques to produce a thread that may
  be safely "stopped" or "suspended"?
  Yes, it's reasonably straightforward. The one subtlety is that the
@@ -324,6 +324,5 @@
  described above, it needn't call notify as well, but it
  still must be synchronized. This ensures that the target thread
  won't miss an interrupt due to a race condition.
-