Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Erik Joelsson]

On 2018-09-06 10:29, Severin Gehwolf wrote:

On Thu, 2018-09-06 at 09:55 -0700, Erik Joelsson wrote:
Thanks, Erik. GCC supports -ffp-contract since 4.6. Clang has -ffp-
contract too. Question is beginning from which version. That's why I'd
expect for those flags to work on linux. Is there anything else I need
to check?

Would it be preferred if I moved this into a block like this?

ifeq ($(TOOLCHAIN_TYPE), gcc)
   [...]
endif
Yes, making it conditional on toolchain type is what David was after. 
You can also consider adding a capability check if you know that 
versions of the compiler that should still work don't have the feature.


/Erik

Thanks,
Severin


/Erik

Thanks,
Severin


Thanks,
David

On 5/09/2018 11:12 PM, Severin Gehwolf wrote:

Hi,

Cross-posting this review-thread on core-libs-dev and build-dev as
this
is a build change, but affects fdlibm which is core-libs.

With JDK-8170153 optimization for fdlibm code has been turned on
for
ppc64 s390 and aarch64. This patch intends to turn it on on all
arches
on Linux. I've not observed any precision issues. Is there a good
reason to not use -O3 -ffp-contract=off everywhere?

Bug:https://bugs.openjdk.java.net/browse/JDK-8210416
webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.01/

Testing: - java/lang/Math, java/lang/StrictMath tests (all pass).
- Currently running through submit repo.

A simple micro benchmark from JDK-8170153[1] shows these numbers
for
StrictMath:

Function  | before | after
--
sin   | 0m33.382s  | 0m18.731s
cos   | 0m31.562s  | 0m18.796s
tan   | 0m33.657s  | 0m21.093s
atan  | 0m5.714s   | 0m4.902s
log   | 0m6.212s   | 0m4.439s
log10 | 0m7.946s   | 0m5.543s
sqrt  | 0m0.481s   | 0m0.449s
cbrt  | 0m5.295s   | 0m5.214s
tanh  | 0m1.404s   | 0m1.307s
log1p | 0m6.457s   | 0m5.131s
IEEEremainder | 0m10.629s  | 0m6.048s
atan2 | 0m8.037s   | 0m5.668s
hypot | 0m2.171s   | 0m2.147s

Thoughts?

Thanks,
Severin

Re: [12] RFR: 8210142: java.util.Calendar.clone() doesn't respect sharedZone flag

[Roger Riggs]

Looks good, thanks for the update.

Roger


On 9/5/18 5:10 PM, naoto.s...@oracle.com wrote:

Hi Roger,

I updated the fix to share the zone only if the sharedZone is true. 
Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8210142/webrev.01/

Naoto

On 9/5/18 7:07 AM, Roger Riggs wrote:

Hi Naoto,

That sounds like a test issue.  I would not expect a cloned Calendar 
or TimeZone to have a different hashCode.
None of the fields involved in the hashCode/equals should be 
different. (Or I'm missing something about them).


Thanks, Roger


On 9/4/18 5:19 PM, Naoto Sato wrote:

Hi Roger,

I tentatively tried to return a shared zone within a cloned 
Calendar. One test failed: 
java/util/Calendar/CalendarRegression.Test4136399, where it makes 
sure that the cloned Calendar object hash code should be different 
for the better distribution. Although the comment does not reflect 
the current implementation (getTimeZone() returning cloned zone), 
the intention here seems to have the better hash distribution for 
cloned objects.


Naoto

On 9/4/18 1:41 PM, Roger Riggs wrote:

Hi Naoto,

That access via reflection is going to go away sometime; so I'm not 
too concerned about

maintaining compatibility of the internal implementation.
I think I'd rather see the memory savings, however small.
Let see if anyone else has a recommendation.

Roger


On 9/4/18 4:12 PM, Naoto Sato wrote:

Hi Roger,

Yes, I considered that too, but did not change the behavior and 
just to maintain the field consistent. I agree that it would not 
be observable via the public Calendar API, but some apps (like how 
the submitter found it) may be doing something using reflection.


Naoto

On 9/4/18 12:31 PM, Roger Riggs wrote:

Hi Naoto,

The spec for clone doesn't say whether the clone should share or 
not share the TimeZone.
Did you consider that if sharedZone was true , *not* to clone the 
TimeZone?

It would still get cloned when requested from getTimeZone().

This does seem somewhat safer not to change the cloning behavior 
but I don't think the behavior would be observable.


The current code and test is fine, except for reducing the 
potential for sharing the TimeZone.


Thanks, Roger

On 9/4/2018 2:14 PM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8210142/webrev.00/

The fix is a simple one line change, which is to make the 
sharedZone field consistent with the cloned TimeZone instance in 
Calendar.clone().


Naoto

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

On Thu, 2018-09-06 at 09:55 -0700, Erik Joelsson wrote:
> On 2018-09-06 03:12, Severin Gehwolf wrote:
> > Hi David,
> > 
> > On Thu, 2018-09-06 at 07:32 +1000, David Holmes wrote:
> > > Hi Severin,
> > > 
> > > Might as well raise this here too as it's really a build philosophy
> > > issue. Shouldn't flags like -ffp-contract=off (and the existing AIX
> > > -qfloat=nomaf) be toolchain specific rather than platform specific?
> > 
> > Looks like Clang has -ffp-contract:
> > https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffp-contract
> > 
> > Is there any other (supported) toolchain other than gcc and clang on
> > linux? As for AIX I suppose there is only on supported toolchain?
> 
> We don't have a big variety of toolchain/platform combinations that we 
> actively support no, but we still try to be conscious of when a flag is 
> toolchain specific and when it's platform specific. There is certainly 
> interest in using other compilers on Linux, and GCC could potentially be 
> used on other platforms as well. Until we actually try it, it can be 
> hard to know for sure if a flag actually applies in other cases for the 
> toolchain and/or platform, but we can at least try our best guess.

Thanks, Erik. GCC supports -ffp-contract since 4.6. Clang has -ffp-
contract too. Question is beginning from which version. That's why I'd
expect for those flags to work on linux. Is there anything else I need
to check?

Would it be preferred if I moved this into a block like this?

ifeq ($(TOOLCHAIN_TYPE), gcc)
  [...]
endif

Thanks,
Severin

> /Erik
> > Thanks,
> > Severin
> > 
> > > Thanks,
> > > David
> > > 
> > > On 5/09/2018 11:12 PM, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Cross-posting this review-thread on core-libs-dev and build-dev as
> > > > this
> > > > is a build change, but affects fdlibm which is core-libs.
> > > > 
> > > > With JDK-8170153 optimization for fdlibm code has been turned on
> > > > for
> > > > ppc64 s390 and aarch64. This patch intends to turn it on on all
> > > > arches
> > > > on Linux. I've not observed any precision issues. Is there a good
> > > > reason to not use -O3 -ffp-contract=off everywhere?
> > > > 
> > > > Bug:https://bugs.openjdk.java.net/browse/JDK-8210416
> > > > webrev:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.01/
> > > > 
> > > > Testing: - java/lang/Math, java/lang/StrictMath tests (all pass).
> > > >- Currently running through submit repo.
> > > > 
> > > > A simple micro benchmark from JDK-8170153[1] shows these numbers
> > > > for
> > > > StrictMath:
> > > > 
> > > > Function  | before | after
> > > > --
> > > > sin   | 0m33.382s  | 0m18.731s
> > > > cos   | 0m31.562s  | 0m18.796s
> > > > tan   | 0m33.657s  | 0m21.093s
> > > > atan  | 0m5.714s   | 0m4.902s
> > > > log   | 0m6.212s   | 0m4.439s
> > > > log10 | 0m7.946s   | 0m5.543s
> > > > sqrt  | 0m0.481s   | 0m0.449s
> > > > cbrt  | 0m5.295s   | 0m5.214s
> > > > tanh  | 0m1.404s   | 0m1.307s
> > > > log1p | 0m6.457s   | 0m5.131s
> > > > IEEEremainder | 0m10.629s  | 0m6.048s
> > > > atan2 | 0m8.037s   | 0m5.668s
> > > > hypot | 0m2.171s   | 0m2.147s
> > > > 
> > > > Thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > 
> 
> 

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Erik Joelsson]

On 2018-09-06 03:12, Severin Gehwolf wrote:

Hi David,

On Thu, 2018-09-06 at 07:32 +1000, David Holmes wrote:

Hi Severin,

Might as well raise this here too as it's really a build philosophy
issue. Shouldn't flags like -ffp-contract=off (and the existing AIX
-qfloat=nomaf) be toolchain specific rather than platform specific?

Looks like Clang has -ffp-contract:
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffp-contract

Is there any other (supported) toolchain other than gcc and clang on
linux? As for AIX I suppose there is only on supported toolchain?
We don't have a big variety of toolchain/platform combinations that we 
actively support no, but we still try to be conscious of when a flag is 
toolchain specific and when it's platform specific. There is certainly 
interest in using other compilers on Linux, and GCC could potentially be 
used on other platforms as well. Until we actually try it, it can be 
hard to know for sure if a flag actually applies in other cases for the 
toolchain and/or platform, but we can at least try our best guess.


/Erik

Thanks,
Severin


Thanks,
David

On 5/09/2018 11:12 PM, Severin Gehwolf wrote:

Hi,

Cross-posting this review-thread on core-libs-dev and build-dev as
this
is a build change, but affects fdlibm which is core-libs.

With JDK-8170153 optimization for fdlibm code has been turned on
for
ppc64 s390 and aarch64. This patch intends to turn it on on all
arches
on Linux. I've not observed any precision issues. Is there a good
reason to not use -O3 -ffp-contract=off everywhere?

Bug:https://bugs.openjdk.java.net/browse/JDK-8210416
webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.01/

Testing: - java/lang/Math, java/lang/StrictMath tests (all pass).
   - Currently running through submit repo.

A simple micro benchmark from JDK-8170153[1] shows these numbers
for
StrictMath:

Function  | before | after
--
sin   | 0m33.382s  | 0m18.731s
cos   | 0m31.562s  | 0m18.796s
tan   | 0m33.657s  | 0m21.093s
atan  | 0m5.714s   | 0m4.902s
log   | 0m6.212s   | 0m4.439s
log10 | 0m7.946s   | 0m5.543s
sqrt  | 0m0.481s   | 0m0.449s
cbrt  | 0m5.295s   | 0m5.214s
tanh  | 0m1.404s   | 0m1.307s
log1p | 0m6.457s   | 0m5.131s
IEEEremainder | 0m10.629s  | 0m6.048s
atan2 | 0m8.037s   | 0m5.668s
hypot | 0m2.171s   | 0m2.147s

Thoughts?

Thanks,
Severin

Re: RFR(L/M) : 8210112 : remove jdk.testlibrary.ProcessTools

[Igor Ignatyev]

JC,
thanks for your review!

Core-libs team,
as the majority of changed tests are core-libs tests, I'd really appreciate if 
someone from core-libs (preferably a Reviewer) could review the patch. 

for the record, [1] is the latest version of webrev.

[1] http://cr.openjdk.java.net/~iignatyev//8210112/webrev.02/index.html 

> 2433 lines changed: 334 ins; 1677 del; 422 mod;

Cheers,
-- Igor


> On Sep 5, 2018, at 5:03 PM, JC Beyler  wrote:
> 
> Hi Igor,
> 
> Looks good to me then. I agree most are nits, personal preferences, and just 
> I noticed them as you were cleaning them up!
> 
> Awesome clean up :-)
> Jc
> 
> On Wed, Sep 5, 2018 at 3:20 PM Igor Ignatyev  > wrote:
> Hi JC,
> 
> 
>> On Sep 5, 2018, at 2:59 PM, JC Beyler > > wrote:
>> 
>> Hi Igor,
>> 
>> I like this much better! A few more comments:
>> 
>> - 
>> http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/lib/testlibrary/OutputAnalyzerTest.java.udiff.html
>>  
>> 
>>   -> If the shouldMatch call fails, it throws an exception, why not just let 
>> that fail test, why are you catching and then rethrowing (like you do for 
>> http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html
>>  
>> )
> just to follow the style used by this tests, e.g.
> 
>>   54 try {
>>   55 output.shouldContain(stdout);
>>   56 output.stdoutShouldContain(stdout);
>>   57 output.shouldContain(stderr);
>>   58 output.stderrShouldContain(stderr);
>>   59 } catch (RuntimeException e) {
>>   60 throw new Exception("shouldContain() failed", e);
>>   61 }
> 
>>   86 try {
>>   87 output.shouldNotContain(nonExistingString);
>>   88 output.stdoutShouldNotContain(nonExistingString);
>>   89 output.stderrShouldNotContain(nonExistingString);
>>   90 } catch (RuntimeException e) {
>>   91 throw new Exception("shouldNotContain() failed", e);
>>   92 }
>>   93 
> 
> 
>> - 
>> http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdDefaults.java.udiff.html
>>  
>> 
>>There is now only a 1-liner for this method and it is called only once, 
>> should we inline and remove the method?
> 
> we can, but I ain't sure we should. from my point of view
> 
>   80 output.shouldHaveExitValue(0);
>   81 output.shouldContain("sun.tools.jcmd.JCmd");
>   82 matchListedProcesses(output);
> is a bit easier to understand than
>   80 output.shouldHaveExitValue(0);
>   81 output.shouldContain("sun.tools.jcmd.JCmd");
>   82 output.shouldMatchByLine(JCMD_LIST_REGEX);
> 
> however, I don't have strong preference here and if serviceability team 
> wants, I can inline matchListedProcesses.
> 
>> - Same for (we could inline): 
>> http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/jdk/sun/tools/jcmd/TestJcmdSanity.java.udiff.html
>>  
>> same
>>  here
> 
>> 
>> - 
>> http://cr.openjdk.java.net/~iignatyev//8210112/webrev.0-1/test/lib/jdk/test/lib/process/OutputAnalyzer.java.udiff.html
>>  
>> 
>> "There is no lines" -> "There are no lines"
> fixed. thanks for spotting.
> 
>> - What is the advantage of having the return at all now for the 
>> shouldMatch methods, if it fails it throws, the test fails; otherwise it 
>> doesn't return anything, the test can move on, no? I saw no moment when you 
>> get the return to do something more with it
> OutputAnalazyer is supposed to be a fluent interface, and in some cases you 
> might find it used that way, so I'd prefer to have possibility to use these 
> methods in a method chain, as w/ we already have for the the most of other 
> should* method. I've fixed a few more should* methods to return this.
> 
> 
>> Thanks for the incremental webrev, that made looking at the changes so much 
>> easier!
> 
> here is the next one -- 
> http://cr.openjdk.java.net/~iignatyev//8210112/webrev.1-2/index.html 
> 
> 
> -- Igor
>> Jc
>> 
>> On Wed, Sep 5, 2018 at 2:32 PM Igor Ignatyev > > wrote:
>> Hi JC,
>> 
>> thanks for reviewing this! I agree w/ both your 

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

On 06/09/18 1:24 PM, Bernd Eckenfels wrote:
> Yes you are right @apinote is aproperiate section (was confusing it with 
> implnote).
>
> Still think a ‚literal specified list‘ is no longer a good (as in canonical) 
> usecase for that method.
>
> I used it in the past often to get a List for using toString() on it, but I 
> guess even for that case List.of can be used instead now.
>
> So @see List#of and let the reader figure out when to use them?

That sounds good to me. I've now updated it to reflect this change and
the javadoc now looks as below. I've also attached the new updated patch.

    /**
 * Returns a fixed-size list backed by the specified array. The passed
 * array is held as a reference in the returned list. Any subsequent
 * changes made to the array contents will be visible in the returned
 * list. Similarly any changes that happen in the returned list will
 * also be visible in the array. The returned list is serializable and
 * implements {@link RandomAccess}.
 *
 * The returned list can be changed only in certain ways. Operations
 * like {@code add}, {@code remove}, {@code clear} and other such, that
 * change the size of the list aren't allowed. Operations like
 * {@code replaceAll}, {@code set}, that change the elements in the list
 * are permitted.
 *
 * This method acts as bridge between array-based and
collection-based
 * APIs, in combination with {@link Collection#toArray}.
 *
 * @apiNote
 * The returned list throws a {@link UnsupportedOperationException} for
 * operations that aren't permitted. Certain implementations of the
returned
 * list might choose to throw the exception only if the call to such
methods
 * results in an actual change, whereas certain other
implementations may
 * always throw the exception when such methods are called.
 *
 * @param  the class of the objects in the array
 * @param a the array by which the list will be backed
 * @return a list view of the specified array
 * @see List#of()
 */



-Jaikiran

# HG changeset patch
# User Jaikiran Pai 
# Date 1535208403 -19800
#  Sat Aug 25 20:16:43 2018 +0530
# Node ID e4d5e71a20f7c80196f211f62440d3cccb69e8f3
# Parent  a716460217ed180972b568e28cf332e37e07a3ae
JDK-7033681 Improve the javadoc of Arrays#toList()

diff --git a/src/java.base/share/classes/java/util/Arrays.java 
b/src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java
+++ b/src/java.base/share/classes/java/util/Arrays.java
@@ -4288,21 +4288,33 @@
 // Misc
 
 /**
- * Returns a fixed-size list backed by the specified array.  (Changes to
- * the returned list "write through" to the array.)  This method acts
- * as bridge between array-based and collection-based APIs, in
- * combination with {@link Collection#toArray}.  The returned list is
- * serializable and implements {@link RandomAccess}.
- *
- * This method also provides a convenient way to create a fixed-size
- * list initialized to contain several elements:
- * 
- * ListString stooges = Arrays.asList("Larry", "Moe", "Curly");
- * 
+ * Returns a fixed-size list backed by the specified array. The passed
+ * array is held as a reference in the returned list. Any subsequent
+ * changes made to the array contents will be visible in the returned
+ * list. Similarly any changes that happen in the returned list will
+ * also be visible in the array. The returned list is serializable and
+ * implements {@link RandomAccess}.
+ *
+ * The returned list can be changed only in certain ways. Operations
+ * like {@code add}, {@code remove}, {@code clear} and other such, that
+ * change the size of the list aren't allowed. Operations like
+ * {@code replaceAll}, {@code set}, that change the elements in the list
+ * are permitted.
+ *
+ * This method acts as bridge between array-based and collection-based
+ * APIs, in combination with {@link Collection#toArray}.
+ *
+ * @apiNote
+ * The returned list throws a {@link UnsupportedOperationException} for
+ * operations that aren't permitted. Certain implementations of the 
returned
+ * list might choose to throw the exception only if the call to such 
methods
+ * results in an actual change, whereas certain other implementations may
+ * always throw the exception when such methods are called.
  *
  * @param  the class of the objects in the array
  * @param a the array by which the list will be backed
  * @return a list view of the specified array
+ * @see List#of()
  */
 @SafeVarargs
 @SuppressWarnings("varargs")

Re: RFR - JDK-8200434 - String::align, String::indent (code review)

[Jim Laskey]

Revised webrev at: 
http://cr.openjdk.java.net/~jlaskey/8200434/webrev-01/index.html 


Thank you Roger,

— Jim

> On Sep 5, 2018, at 5:07 PM, Roger Riggs  wrote:
> 
> Hi Jim,
> 
> Overall it looks fine. Some quibbles on the wording and the test.
> 
> 
> The spec for the align() and align(n) methods in String.java show a possibly 
> misleading inconsistency.
> The first line says:
> 
>   Removes vertical and horizontal white space margins.
> 
> But later align() says:
> 
>   Trailing spaces are  preserved.
> 
> The former implies all 4 margins but then it seems only to apply to 3 of the 
> 4.  (top, left, bottom).
> I'm not sure if there some wording that can clear that up in the first line.

There has been some debate on whether to drop trailing blanks or not. I’m in 
favour of dropping trailing blanks, but need to see some use cases.

> 
> 
> AlignIndent.java:
> 
>  - Line 28: this test should not need /othervm nor the explicit @compile
> 
>  - There should be tests of align(n) with negative values.
> 
>  - The for for for for structure doesn't follow the style guide.

Fixed, see updated webrev.

> 
> 
> Thanks, Roger
> 
> (Sorry for the duplicates, I missed core-libs the first time)
> 
> On 8/29/18 10:00 AM, Jim Laskey wrote:
>> Please review the code  for  String::align and String::indent at the link 
>> below.
>> 
>> Notes:
>>  Includes a private version of String::isMultiline() which may be made 
>> into a public method at some future date
>>  Includes minor correctness clean up of StringLatin1.java, 
>> StringUTF16.java
>> 
>> webrev: http://cr.openjdk.java.net/~jlaskey/8200434/webrev/index.html
>> jbs: https://bugs.openjdk.java.net/browse/JDK-8200434
>> 
>> Cheers,
>> 
>> — Jim
>> 
> 

RE: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Lindenmaier, Goetz]

Yes, that's fine. 

I can sponsor it tomorrow. I'm not in the office today.

Best regards,
  Goetz.

> -Original Message-
> From: Andrew Leonard 
> Sent: Donnerstag, 6. September 2018 12:29
> To: Magnus Ihse Bursie ; Lindenmaier, Goetz
> 
> Cc: 2d-dev <2d-...@openjdk.java.net>; Brian Burkhalter
> ; build-dev ; core-
> libs-dev ; Philip Race
> 
> Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on
> zLinux
> 
> Thanks Magnus,
> 
> Hi Goetz, we have agreement from both library owners, so I think we're good
> now?
> Thanks
> Andrew
> 
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: andrew_m_leon...@uk.ibm.com
> 
> 
> 
> 
> From:Magnus Ihse Bursie 
> To:Andrew Leonard 
> Cc:Brian Burkhalter , 2d-dev <2d-
> d...@openjdk.java.net>, build-dev , core-libs-dev
> , "Lindenmaier, Goetz"
> , Philip Race 
> Date:04/09/2018 16:41
> Subject:Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors 
> on
> zLinux
> 
> 
> 
> 
> 
> 
> Looks good to me.
> 
> /Magnus
> 
> 4 sep. 2018 kl. 12:11 skrev Andrew Leonard   >:
> 
> Hi Brian/Goetz,
> Yes, that seems sensible. I have created a new webrev with fdlibm compiler
> option disabled, and mediaLib code fixed:
> http://cr.openjdk.java.net/~aleonard/8209786/webrev.02/
> 
> I've built and tested on xLinux and zLinux, with gcc 4.8.4 and 7.3.
> 
> Are we good to go?
> Thanks
> Andrew
> 
> hg export:
> # HG changeset patch
> # User aleonard
> # Date 1536055438 -3600
> #  Tue Sep 04 11:03:58 2018 +0100
> # Node ID c2523f285c503e8f82f1212b38de1cb54093255e
> # Parent  3ee91722550680c18b977f0e00b1013323b5c9ef
> 8209786: JDK12 fails to build on s390x with gcc 7.3
> 
> diff -r 3ee917225506 -r c2523f285c50 make/lib/CoreLibraries.gmk
> --- a/make/lib/CoreLibraries.gmkTue Sep 04 14:47:55 2018 +0800
> +++ b/make/lib/CoreLibraries.gmkTue Sep 04 11:03:58 2018 +0100
> @@ -68,7 +68,7 @@
>   CFLAGS_linux_ppc64le := -ffp-contract=off, \
>   CFLAGS_linux_s390x := -ffp-contract=off, \
>   CFLAGS_linux_aarch64 := -ffp-contract=off, \
> -  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation, \
> +  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation array-
> bounds, \
>   DISABLED_WARNINGS_microsoft := 4146 4244 4018, \
>   ARFLAGS := $(ARFLAGS), \
>   OBJECT_DIR := $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libfdlibm, \
> diff -r 3ee917225506 -r c2523f285c50
> src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c
> --- a/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c
> Tue Sep 04 14:47:55 2018 +0800
> +++ b/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c
> Tue Sep 04 11:03:58 2018 +0100
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2003, 2018, 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
> @@ -259,18 +259,18 @@
>   }
> 
> #ifdef _LITTLE_ENDIAN
> -  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8);
> +  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8);
> #else
> -  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8);
> +  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8);
> #endif /* _LITTLE_ENDIAN */
>   ((mlib_u32*)da)[0] = (val1 & emask) | (((mlib_u32*)da)[0] &~ emask);
> 
> #else /* _NO_LONGLONG */
> 
> #ifdef _LITTLE_ENDIAN
> -  mlib_u64 emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 8);
> +  mlib_u64 emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8);
> #else
> -  mlib_u64 emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8);
> +  mlib_u64 emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8);
> #endif /* _LITTLE_ENDIAN */
> 
>   ((mlib_u64*)da)[0] = (((mlib_u64*)dd_array)[sa[0]] & emask) |
> (((mlib_u64*)da)[0] &~ emask);
> @@ -395,9 +395,9 @@
>   }
> 
> #ifdef _LITTLE_ENDIAN
> -  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8);
> +  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8);
> #else
> -  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8);
> +  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8);
> #endif /* _LITTLE_ENDIAN */
>   ((mlib_u32*)da)[0] = (dd1 & emask) | (((mlib_u32*)da)[0] &~ emask);
> 
> @@ -413,9 +413,9 @@
>   }
> 
> #ifdef _LITTLE_ENDIAN
> -  emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 8);
> +  emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8);
> #else
> -  emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8);
> +  emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8);
> #endif /* _LITTLE_ENDIAN */
>   ((mlib_u64*)da)[0] = (dd 

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Andrew Leonard]

Thanks Magnus,

Hi Goetz, we have agreement from both library owners, so I think we're 
good now?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   Magnus Ihse Bursie 
To: Andrew Leonard 
Cc: Brian Burkhalter , 2d-dev 
<2d-...@openjdk.java.net>, build-dev , 
core-libs-dev , "Lindenmaier, Goetz" 
, Philip Race 
Date:   04/09/2018 16:41
Subject:Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler 
errors on zLinux



Looks good to me. 

/Magnus

4 sep. 2018 kl. 12:11 skrev Andrew Leonard :

Hi Brian/Goetz, 
Yes, that seems sensible. I have created a new webrev with fdlibm compiler 
option disabled, and mediaLib code fixed: 
http://cr.openjdk.java.net/~aleonard/8209786/webrev.02/ 
I've built and tested on xLinux and zLinux, with gcc 4.8.4 and 7.3. 

Are we good to go? 
Thanks 
Andrew 

hg export: 
# HG changeset patch 
# User aleonard 
# Date 1536055438 -3600 
#  Tue Sep 04 11:03:58 2018 +0100 
# Node ID c2523f285c503e8f82f1212b38de1cb54093255e 
# Parent  3ee91722550680c18b977f0e00b1013323b5c9ef 
8209786: JDK12 fails to build on s390x with gcc 7.3 

diff -r 3ee917225506 -r c2523f285c50 make/lib/CoreLibraries.gmk 
--- a/make/lib/CoreLibraries.gmkTue Sep 04 14:47:55 2018 +0800 
+++ b/make/lib/CoreLibraries.gmkTue Sep 04 11:03:58 2018 +0100 
@@ -68,7 +68,7 @@ 
   CFLAGS_linux_ppc64le := -ffp-contract=off, \ 
   CFLAGS_linux_s390x := -ffp-contract=off, \ 
   CFLAGS_linux_aarch64 := -ffp-contract=off, \ 
-  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation, \ 
+  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation 
array-bounds, \ 
   DISABLED_WARNINGS_microsoft := 4146 4244 4018, \ 
   ARFLAGS := $(ARFLAGS), \ 
   OBJECT_DIR := $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libfdlibm, \ 
diff -r 3ee917225506 -r c2523f285c50 
src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c 
--- a/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c   
  Tue Sep 04 14:47:55 2018 +0800 
+++ b/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c   
  Tue Sep 04 11:03:58 2018 +0100 
@@ -1,5 +1,5 @@ 
 /* 
- * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved. 

+ * Copyright (c) 2003, 2018, 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 
@@ -259,18 +259,18 @@ 
   } 
  
 #ifdef _LITTLE_ENDIAN 
-  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8); 
+  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8); 
 #else 
-  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8); 
+  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8); 
 #endif /* _LITTLE_ENDIAN */ 
   ((mlib_u32*)da)[0] = (val1 & emask) | (((mlib_u32*)da)[0] &~ 
emask); 
  
 #else /* _NO_LONGLONG */ 
  
 #ifdef _LITTLE_ENDIAN 
-  mlib_u64 emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 
8); 
+  mlib_u64 emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8); 
 #else 
-  mlib_u64 emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8); 
+  mlib_u64 emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8); 
 #endif /* _LITTLE_ENDIAN */ 
  
   ((mlib_u64*)da)[0] = (((mlib_u64*)dd_array)[sa[0]] & emask) | 
(((mlib_u64*)da)[0] &~ emask); 
@@ -395,9 +395,9 @@ 
   } 
  
 #ifdef _LITTLE_ENDIAN 
-  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8); 
+  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8); 
 #else 
-  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8); 
+  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8); 
 #endif /* _LITTLE_ENDIAN */ 
   ((mlib_u32*)da)[0] = (dd1 & emask) | (((mlib_u32*)da)[0] &~ emask); 

  
@@ -413,9 +413,9 @@ 
   } 
  
 #ifdef _LITTLE_ENDIAN 
-  emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 8); 
+  emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8); 
 #else 
-  emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8); 
+  emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8); 
 #endif /* _LITTLE_ENDIAN */ 
   ((mlib_u64*)da)[0] = (dd & emask) | (((mlib_u64*)da)[0] &~ emask); 
  
@@ -565,9 +565,9 @@ 
   } 
  
 #ifdef _LITTLE_ENDIAN 
-  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8); 
+  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8); 
 #else 
-  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8); 
+  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8); 
 #endif /* _LITTLE_ENDIAN */ 
   da[0] = (dd & emask) | (da[0] &~ emask); 
 } 







Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:Brian Burkhalter  
To:Magnus Ihse Bursie  
Cc:Andrew Leonard , 

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

Hi Joe,

On Wed, 2018-09-05 at 12:15 -0700, joe darcy wrote:
> On 9/5/2018 6:12 AM, Severin Gehwolf wrote:
> > Hi,
> > 
> > Cross-posting this review-thread on core-libs-dev and build-dev as
> > this
> > is a build change, but affects fdlibm which is core-libs.
> > 
> > With JDK-8170153 optimization for fdlibm code has been turned on
> > for
> > ppc64 s390 and aarch64. This patch intends to turn it on on all
> > arches
> > on Linux. I've not observed any precision issues. Is there a good
> > reason to not use -O3 -ffp-contract=off everywhere?
> > 
> > Bug:https://bugs.openjdk.java.net/browse/JDK-8210416
> > webrev: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.01/
> > 
> > Testing: - java/lang/Math, java/lang/StrictMath tests (all pass).
> >   - Currently running through submit repo.
> > 
> > A simple micro benchmark from JDK-8170153[1] shows these numbers
> > for
> > StrictMath:
> > 
> > Function  | before | after
> > --
> > sin   | 0m33.382s  | 0m18.731s
> > cos   | 0m31.562s  | 0m18.796s
> > tan   | 0m33.657s  | 0m21.093s
> > atan  | 0m5.714s   | 0m4.902s
> > log   | 0m6.212s   | 0m4.439s
> > log10 | 0m7.946s   | 0m5.543s
> > sqrt  | 0m0.481s   | 0m0.449s
> > cbrt  | 0m5.295s   | 0m5.214s
> > tanh  | 0m1.404s   | 0m1.307s
> > log1p | 0m6.457s   | 0m5.131s
> > IEEEremainder | 0m10.629s  | 0m6.048s
> > atan2 | 0m8.037s   | 0m5.668s
> > hypot | 0m2.171s   | 0m2.147s
> > 
> > 
> 
> Note that pow (JDK-8134795), hypot (JDK-7130085), cbrt (JDK-8136799), 
> and exp (JDK-8139688), have been ported to Java as of JDK 9. The sqrt 
> method is commonly handled as an intrinsic.

OK thanks. Since ppc64/s390x/aarch64 uses this already on Linux do you
anticipate the same being applied to x86/x86_64 causing issues (modulo
compiler bugs of course)?

> Testing that was not geared toward finding precision/rounding issues 
> would be unlikely to find them.

Would running the TCK be geared towards precision/rounding issues? I
could ask someone to run the TCK on a test build on x86_64/x86 to find
out.

> I don't see the sources of the microbenchmark in JDK-8170153.

https://github.com/gromero/strictmath/

Thanks,
Severin

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

Hi David,

On Thu, 2018-09-06 at 07:32 +1000, David Holmes wrote:
> Hi Severin,
> 
> Might as well raise this here too as it's really a build philosophy 
> issue. Shouldn't flags like -ffp-contract=off (and the existing AIX 
> -qfloat=nomaf) be toolchain specific rather than platform specific?

Looks like Clang has -ffp-contract:
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffp-contract

Is there any other (supported) toolchain other than gcc and clang on
linux? As for AIX I suppose there is only on supported toolchain?

Thanks,
Severin

> Thanks,
> David
> 
> On 5/09/2018 11:12 PM, Severin Gehwolf wrote:
> > Hi,
> > 
> > Cross-posting this review-thread on core-libs-dev and build-dev as
> > this
> > is a build change, but affects fdlibm which is core-libs.
> > 
> > With JDK-8170153 optimization for fdlibm code has been turned on
> > for
> > ppc64 s390 and aarch64. This patch intends to turn it on on all
> > arches
> > on Linux. I've not observed any precision issues. Is there a good
> > reason to not use -O3 -ffp-contract=off everywhere?
> > 
> > Bug:https://bugs.openjdk.java.net/browse/JDK-8210416
> > webrev: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.01/
> > 
> > Testing: - java/lang/Math, java/lang/StrictMath tests (all pass).
> >   - Currently running through submit repo.
> > 
> > A simple micro benchmark from JDK-8170153[1] shows these numbers
> > for
> > StrictMath:
> > 
> > Function  | before | after
> > --
> > sin   | 0m33.382s  | 0m18.731s
> > cos   | 0m31.562s  | 0m18.796s
> > tan   | 0m33.657s  | 0m21.093s
> > atan  | 0m5.714s   | 0m4.902s
> > log   | 0m6.212s   | 0m4.439s
> > log10 | 0m7.946s   | 0m5.543s
> > sqrt  | 0m0.481s   | 0m0.449s
> > cbrt  | 0m5.295s   | 0m5.214s
> > tanh  | 0m1.404s   | 0m1.307s
> > log1p | 0m6.457s   | 0m5.131s
> > IEEEremainder | 0m10.629s  | 0m6.048s
> > atan2 | 0m8.037s   | 0m5.668s
> > hypot | 0m2.171s   | 0m2.147s
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
> > 

Re: [12] RFR 8042902: Test java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java fails intermittently

[Chris Yin]

Thank you, Chris H.  :)

Regards,
Chris Y.

> On 6 Sep 2018, at 4:05 PM, Chris Hegarty  wrote:
> 
> 
>> On 6 Sep 2018, at 09:00, Chris Yin  wrote:
>> 
>> Please have a review for below minor change to test 
>> java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java, thanks
>> 
>> The bug has a long history, so a quick summary and explanation here, the 
>> issue which caused test failure before 2016 should already been fixed, so 
>> this fix change is for new observed failures in 2018. I checked all failures 
>> happened on Mac OS platform.
>> The failure is weird that we got different value by call 
>> InetAddress.getHostName() on same address one after one in very short time, 
>> cannot make sure whether any network configuration change or Mac OS system 
>> caused the difference for multiple name reverse lookup, but from this test 
>> aspect, guess the focus on address serialization should be enough, so below 
>> fix change is simple, we will call InetAddress.getHostName() first to set 
>> holder() ’s hostname field in testAllNetworkInterfaces() before 
>> serialization test (as Chris Hegarty commented in 2014. And per Mark's 
>> comments, "display call was commented out as it was too verbose" so we just 
>> simple print hostname instead of get whole display call function back), then 
>> when we deserialized the address object and do comparison with original one 
>> later, getHostName() call will retrieve holder() ’s hostname value directly, 
>> that should eliminate the possible difference when doing name reverse lookup 
>> twice.
>> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8042902
>> 
>> changes:
>> 
>> diff -r b51d348698c2 
>> test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java
>> --- 
>> a/test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java
>> Wed Sep 05 19:40:52 2018 -0700
>> +++ 
>> b/test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java
>> Thu Sep 06 14:29:38 2018 +0800
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (c) 2015, 2018, 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
>> @@ -192,6 +192,7 @@
>>System.err.println("Testing with " + iadr);
>>System.err.println(" scoped iface: "
>>+ i6adr.getScopedInterface());
>> +System.err.println(" hostname: " + i6adr.getHostName());
>>testInet6AddressSerialization(i6adr, null);
>>}
>>}
> 
> Thanks Chris Y.I think the change looks fine.
> 
> -Chris H.

Re: [12] RFR 8042902: Test java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java fails intermittently

[Chris Hegarty]

> On 6 Sep 2018, at 09:00, Chris Yin  wrote:
> 
> Please have a review for below minor change to test 
> java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java, thanks
> 
> The bug has a long history, so a quick summary and explanation here, the 
> issue which caused test failure before 2016 should already been fixed, so 
> this fix change is for new observed failures in 2018. I checked all failures 
> happened on Mac OS platform.
> The failure is weird that we got different value by call 
> InetAddress.getHostName() on same address one after one in very short time, 
> cannot make sure whether any network configuration change or Mac OS system 
> caused the difference for multiple name reverse lookup, but from this test 
> aspect, guess the focus on address serialization should be enough, so below 
> fix change is simple, we will call InetAddress.getHostName() first to set 
> holder() ’s hostname field in testAllNetworkInterfaces() before serialization 
> test (as Chris Hegarty commented in 2014. And per Mark's comments, "display 
> call was commented out as it was too verbose" so we just simple print 
> hostname instead of get whole display call function back), then when we 
> deserialized the address object and do comparison with original one later, 
> getHostName() call will retrieve holder() ’s hostname value directly, that 
> should eliminate the possible difference when doing name reverse lookup twice.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8042902
> 
> changes:
> 
> diff -r b51d348698c2 
> test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java
> --- 
> a/test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java 
> Wed Sep 05 19:40:52 2018 -0700
> +++ 
> b/test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java 
> Thu Sep 06 14:29:38 2018 +0800
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2015, 2018, 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
> @@ -192,6 +192,7 @@
> System.err.println("Testing with " + iadr);
> System.err.println(" scoped iface: "
> + i6adr.getScopedInterface());
> +System.err.println(" hostname: " + i6adr.getHostName());
> testInet6AddressSerialization(i6adr, null);
> }
> }

Thanks Chris Y.I think the change looks fine.

-Chris H.

[12] RFR 8042902: Test java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java fails intermittently

[Chris Yin]

Please have a review for below minor change to test 
java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java, thanks

The bug has a long history, so a quick summary and explanation here, the issue 
which caused test failure before 2016 should already been fixed, so this fix 
change is for new observed failures in 2018. I checked all failures happened on 
Mac OS platform.
The failure is weird that we got different value by call 
InetAddress.getHostName() on same address one after one in very short time, 
cannot make sure whether any network configuration change or Mac OS system 
caused the difference for multiple name reverse lookup, but from this test 
aspect, guess the focus on address serialization should be enough, so below fix 
change is simple, we will call InetAddress.getHostName() first to set holder() 
’s hostname field in testAllNetworkInterfaces() before serialization test (as 
Chris Hegarty commented in 2014. And per Mark's comments, "display call was 
commented out as it was too verbose" so we just simple print hostname instead 
of get whole display call function back), then when we deserialized the address 
object and do comparison with original one later, getHostName() call will 
retrieve holder() ’s hostname value directly, that should eliminate the 
possible difference when doing name reverse lookup twice.

bug: https://bugs.openjdk.java.net/browse/JDK-8042902

changes:

diff -r b51d348698c2 
test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java
--- 
a/test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java   
Wed Sep 05 19:40:52 2018 -0700
+++ 
b/test/jdk/java/net/Inet6Address/serialize/Inet6AddressSerializationTest.java   
Thu Sep 06 14:29:38 2018 +0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2018, 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
@@ -192,6 +192,7 @@
 System.err.println("Testing with " + iadr);
 System.err.println(" scoped iface: "
 + i6adr.getScopedInterface());
+System.err.println(" hostname: " + i6adr.getHostName());
 testInet6AddressSerialization(i6adr, null);
 }
 }

Regards,
Chris

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Bernd Eckenfels]

Yes you are right @apinote is aproperiate section (was confusing it with 
implnote).

Still think a ‚literal specified list‘ is no longer a good (as in canonical) 
usecase for that method.

I used it in the past often to get a List for using toString() on it, but I 
guess even for that case List.of can be used instead now.

So @see List#of and let the reader figure out when to use them?

Gruss
Bernd
--
http://bernd.eckenfels.net


Von: Jaikiran Pai 
Gesendet: Donnerstag, September 6, 2018 9:13 AM
An: Bernd Eckenfels; core-libs-dev@openjdk.java.net
Betreff: Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList



Hello Bernd,

Thank you for the review and sorry about the delayed response. Comments inline.

On 29/08/18 4:26 PM, Bernd Eckenfels wrote:
Hello,

Not an Reviewer But just wanted to give a short Feedback: I like the new 
Version it is really helpful.

However I wonder if the usage example should be outside of the apinote.


+ * @apiNote
+ * This method also provides a convenient way to create a fixed-size
  * list initialized to contain several elements:
  * 
  * List stooges = Arrays.asList("Larry", "Moe", "Curly");
  * 
  *

My limited understanding of the @apiNote is that it is supposed to be used for 
additional details of the API and/or its usage examples and the javadoc itself 
(outside of the @apiNote) should be a specification of the API. In this case, I 
moved that section to @apiNote since that part appears to mention how/when to 
use that API in. Having said that, I can move it out of @apiNote and let it 
stay the way it previously was, if you and others feel that's the way to go.


Given the existence of List.of() I wonder if you either mention it as a 
alternative to the example (with slightly different semantic) or just remove 
the sample completely?

I'm not too sure mentioning List.of() construct here will be useful, but I do 
see why you mention that. I think the existing example does seem like a useful 
usage example, irrespective of whether or not we decide to have it in or 
outside of an @apiNote.

-Jaikiran

Re: [PATCH] Simplification of TreeMap

[Michael Kuhlmann]

Hi Sergey and Sergey ;),

that's totally correct! Serialization really is a beast sometimes, you
have to take care that instances that have been serialized even with
Java 1.2 still get deserialized correctly in the current version.

So it's not sufficient to only look at the constructors when the class
is Serializable. In the case of TreeMap, the only solutions I see are
either making 'comparator' mutable and setting it in readObject(), or
again always check for null values when accessing it - which would make
your idea rather useless. As Martin already mentioned, "if it's not
broken, don't fix it" - either option is not worth the change.

What would make sense is to remove valEquals() in favor of
Objects::equals, so at least a minor patch would remain. ;)

-Michael


Am 05.09.2018 um 21:22 schrieb Sergey:
> Hi Sergey,
> 
> Michael might correct me if I’ve missed something, but 
> problem with your test case is that you’re serializing already 
> patched version. That makes sense if you want to test current 
> behavior. However the case you truly want to test is how your
> patched TreeMap deserializes pre-patched TreeMaps.
> 
> What you have currently just tests if patched map could be
> deserialized without any problems.
> 
> Cheers,
> su -
> 
> On Wed, 5 Sep 2018 at 20:30, Сергей Цыпанов  > wrote:
> 
> Hi Michael,
> 
> thanks for pointing out this serialization concern, I didn't think
> about it at all.
> 
> I've wrote a simple test for serialization of patched TreeMap and it
> works without errors for both no-args constructor and constructor
> with comparator:
> 
> public class TreeMapSerialization {
>     public static void main(String[] args) throws Exception {
>         TreeMap serialized = new
> TreeMap<>(Comparator.reverseOrder());
>         serialized.put(1, "1");
>         serialized.put(2, "2");
> 
>         ByteArrayOutputStream baos = new ByteArrayOutputStream();
>         ObjectOutputStream oos = new ObjectOutputStream(baos);
> 
>         oos.writeObject(serialized);
>         oos.flush();
>         baos.flush();
>         oos.close();
>         baos.close();
> 
>         ByteArrayInputStream bais = new
> ByteArrayInputStream(baos.toByteArray());
>         ObjectInputStream ois = new ObjectInputStream(bais);
> 
>         TreeMap deserialized = (TreeMap String>) ois.readObject();
>         deserialized.put(3, "3");
> 
>         System.out.println(deserialized);
>     }
> }
> 
> 
> I hope I don't miss anything, so there shouldn't be any
> serialization issues.
> 
> Regards,
> Sergey Tsypanov
> 

Re: [PATCH] JDK-7033681 - Improve the documentation of Arrays.asList

[Jaikiran Pai]

Hello Bernd,

Thank you for the review and sorry about the delayed response. Comments
inline.

On 29/08/18 4:26 PM, Bernd Eckenfels wrote:
> Hello,
>
> Not an Reviewer But just wanted to give a short Feedback: I like the
> new Version it is really helpful. 
>
> However I wonder if the usage example should be outside of the apinote.

+ * @apiNote
+ * This method also provides a convenient way to create a fixed-size
  * list initialized to contain several elements:
  * 
  * List stooges = Arrays.asList("Larry", "Moe", "Curly");
  * 
  *

My limited understanding of the @apiNote is that it is supposed to be
used for additional details of the API and/or its usage examples and the
javadoc itself (outside of the @apiNote) should be a specification of
the API. In this case, I moved that section to @apiNote since that part
appears to mention how/when to use that API in. Having said that, I can
move it out of @apiNote and let it stay the way it previously was, if
you and others feel that's the way to go.

>
> Given the existence of List.of() I wonder if you either mention it as
> a alternative to the example (with slightly different semantic) or
> just remove the sample completely?

I'm not too sure mentioning List.of() construct here will be useful, but
I do see why you mention that. I think the existing example does seem
like a useful usage example, irrespective of whether or not we decide to
have it in or outside of an @apiNote.

-Jaikiran