Re: RFR 8203279 : Faster calculation of power of two

[David Holmes]

On 17/05/2018 1:01 PM, Ivan Gerasimov wrote:



On 5/16/18 7:44 PM, David Holmes wrote:

On 17/05/2018 12:30 PM, Ivan Gerasimov wrote:

Hi David!


On 5/16/18 6:12 PM, David Holmes wrote:

Hi Ivan,

Surely you need to back this up with some performance numbers! And 
verify not assume that numberOfLeadingZeroes is intrinsified!



Yes, good point.
Please find the benchmark with the results here:
http://cr.openjdk.java.net/~igerasim/8203279/00/MyBenchmark.java

It shows that the new version of HashMap. tableSizeFor() was 
(approximately) two times faster.


Impressive. :)


Thanks :)
Of course it's not performance critical code, but why not optimize it by 
a tiny bit if possible?


Do you think it's good to go?


I think I'd rather defer to a more performance oriented reviewer - 
paging Claes! ;-)


David
-



With kind regards,
Ivan


Thanks,
David


With kind regards,
Ivan


Cheers,
David

On 17/05/2018 10:32 AM, Ivan Gerasimov wrote:

Hello!

In a few places we have code that rounds an integer up to the 
nearest power of two.


It is done with a series of RSHOTFs and ORs, but it can possibly be 
done faster with the use of Integer.numberOfLeadingZeros (assuming 
it is intrinsified).


Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from 
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure 
the result is the same.


For TimSort.ensureCapacity() I checked all positive values of 
minCapacity.

Re: RFR 8203279 : Faster calculation of power of two

[David Holmes]

On 17/05/2018 12:55 PM, Martin Buchholz wrote:

Hello Ivan,

I've wondered about this myself.
Probably the performance is architecture dependent.
Probably a new method should be added to Integer and Long with
@HotspotIntrinsicCandidate.
That gives David another chance to practice his blind aarch64 assembly
language coding skills.


Ha! :)


It's hard to give a good name for this.  Hacker's delight calls it dclp2
and we can probably do better than that.
http://www.hackersdelight.org/hdcodetxt/clp2.c.txt
But I approve either way.


Interesting that the existing code is almost, but not quite clp2.

And numberOfLeadingZeroes also comes from HD.

If both forms get optimally compiled I'm very surprised that we would 
see such a performance difference.


David



On Wed, May 16, 2018 at 5:32 PM, Ivan Gerasimov 
wrote:


Hello!

In a few places we have code that rounds an integer up to the nearest
power of two.

It is done with a series of RSHOTFs and ORs, but it can possibly be done
faster with the use of Integer.numberOfLeadingZeros (assuming it is
intrinsified).

Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure the result
is the same.

For TimSort.ensureCapacity() I checked all positive values of minCapacity.

--
With kind regards,
Ivan Gerasimov

Re: RFR 8203279 : Faster calculation of power of two

[Ivan Gerasimov]

On 5/16/18 7:44 PM, David Holmes wrote:

On 17/05/2018 12:30 PM, Ivan Gerasimov wrote:

Hi David!


On 5/16/18 6:12 PM, David Holmes wrote:

Hi Ivan,

Surely you need to back this up with some performance numbers! And 
verify not assume that numberOfLeadingZeroes is intrinsified!



Yes, good point.
Please find the benchmark with the results here:
http://cr.openjdk.java.net/~igerasim/8203279/00/MyBenchmark.java

It shows that the new version of HashMap. tableSizeFor() was 
(approximately) two times faster.


Impressive. :)


Thanks :)
Of course it's not performance critical code, but why not optimize it by 
a tiny bit if possible?


Do you think it's good to go?

With kind regards,
Ivan


Thanks,
David


With kind regards,
Ivan


Cheers,
David

On 17/05/2018 10:32 AM, Ivan Gerasimov wrote:

Hello!

In a few places we have code that rounds an integer up to the 
nearest power of two.


It is done with a series of RSHOTFs and ORs, but it can possibly be 
done faster with the use of Integer.numberOfLeadingZeros (assuming 
it is intrinsified).


Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from 
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure 
the result is the same.


For TimSort.ensureCapacity() I checked all positive values of 
minCapacity.










--
With kind regards,
Ivan Gerasimov

Re: RFR 8203279 : Faster calculation of power of two

[Martin Buchholz]

Hello Ivan,

I've wondered about this myself.
Probably the performance is architecture dependent.
Probably a new method should be added to Integer and Long with
@HotspotIntrinsicCandidate.
That gives David another chance to practice his blind aarch64 assembly
language coding skills.
It's hard to give a good name for this.  Hacker's delight calls it dclp2
and we can probably do better than that.
http://www.hackersdelight.org/hdcodetxt/clp2.c.txt
But I approve either way.


On Wed, May 16, 2018 at 5:32 PM, Ivan Gerasimov 
wrote:

> Hello!
>
> In a few places we have code that rounds an integer up to the nearest
> power of two.
>
> It is done with a series of RSHOTFs and ORs, but it can possibly be done
> faster with the use of Integer.numberOfLeadingZeros (assuming it is
> intrinsified).
>
> Would you please help review this trivial optimization:
>
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8203279
> WEBREV: http://cr.openjdk.java.net/~igerasim/8203279/00/webrev/
>
> For HashMap.tableSizeFor() I created a simple test with a loop from
> Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure the result
> is the same.
>
> For TimSort.ensureCapacity() I checked all positive values of minCapacity.
>
> --
> With kind regards,
> Ivan Gerasimov
>
>

Re: RFR 8203279 : Faster calculation of power of two

[David Holmes]

On 17/05/2018 12:30 PM, Ivan Gerasimov wrote:

Hi David!


On 5/16/18 6:12 PM, David Holmes wrote:

Hi Ivan,

Surely you need to back this up with some performance numbers! And 
verify not assume that numberOfLeadingZeroes is intrinsified!



Yes, good point.
Please find the benchmark with the results here:
http://cr.openjdk.java.net/~igerasim/8203279/00/MyBenchmark.java

It shows that the new version of HashMap. tableSizeFor() was 
(approximately) two times faster.


Impressive. :)

Thanks,
David


With kind regards,
Ivan


Cheers,
David

On 17/05/2018 10:32 AM, Ivan Gerasimov wrote:

Hello!

In a few places we have code that rounds an integer up to the nearest 
power of two.


It is done with a series of RSHOTFs and ORs, but it can possibly be 
done faster with the use of Integer.numberOfLeadingZeros (assuming it 
is intrinsified).


Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from 
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure the 
result is the same.


For TimSort.ensureCapacity() I checked all positive values of 
minCapacity.

Re: RFR 8203279 : Faster calculation of power of two

[Ivan Gerasimov]

Hi David!


On 5/16/18 6:12 PM, David Holmes wrote:

Hi Ivan,

Surely you need to back this up with some performance numbers! And 
verify not assume that numberOfLeadingZeroes is intrinsified!



Yes, good point.
Please find the benchmark with the results here:
http://cr.openjdk.java.net/~igerasim/8203279/00/MyBenchmark.java

It shows that the new version of HashMap. tableSizeFor() was 
(approximately) two times faster.


With kind regards,
Ivan


Cheers,
David

On 17/05/2018 10:32 AM, Ivan Gerasimov wrote:

Hello!

In a few places we have code that rounds an integer up to the nearest 
power of two.


It is done with a series of RSHOTFs and ORs, but it can possibly be 
done faster with the use of Integer.numberOfLeadingZeros (assuming it 
is intrinsified).


Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from 
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure the 
result is the same.


For TimSort.ensureCapacity() I checked all positive values of 
minCapacity.






--
With kind regards,
Ivan Gerasimov

RFR: Small cleanups in java.lang.ref

[Martin Buchholz]

I've been confused by NULL vs null for years.

8203327: Small cleanups in java.lang.ref
http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
https://bugs.openjdk.java.net/browse/JDK-8203327

RFR: Rename EFS in java.util.zip internals to something meaningful

[Martin Buchholz]

Hi Xueming, I'd like you to do a code review

8203328: Rename EFS in java.util.zip internals to something meaningful
http://cr.openjdk.java.net/~martin/webrevs/jdk/zip-EFS/
https://bugs.openjdk.java.net/browse/JDK-8203328

Re: RFR 8203279 : Faster calculation of power of two

[David Holmes]

Hi Ivan,

Surely you need to back this up with some performance numbers! And 
verify not assume that numberOfLeadingZeroes is intrinsified!


Cheers,
David

On 17/05/2018 10:32 AM, Ivan Gerasimov wrote:

Hello!

In a few places we have code that rounds an integer up to the nearest 
power of two.


It is done with a series of RSHOTFs and ORs, but it can possibly be done 
faster with the use of Integer.numberOfLeadingZeros (assuming it is 
intrinsified).


Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from 
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure the 
result is the same.


For TimSort.ensureCapacity() I checked all positive values of minCapacity.

Re: 8202076: test/jdk/java/io/File/WinSpecialFiles.java on windows with VS2017

[Brian Burkhalter]

Hi Ivan,

On May 16, 2018, at 2:54 PM, Ivan Gerasimov  wrote:

> Maybe it is better to compare fileData.cFileName with the pathbuf to make 
> sure we're dealing with the correct file?

Certainly making this change to the previous proposal would do no harm:

--- a/src/java.base/windows/native/libjava/WinNTFileSystem_md.c
+++ b/src/java.base/windows/native/libjava/WinNTFileSystem_md.c
@@ -541,10 +541,13 @@
 WIN32_FIND_DATAW fileData;
 HANDLE h = FindFirstFileW(pathbuf, );
 if (h != INVALID_HANDLE_VALUE) {
-ULARGE_INTEGER length;
-length.LowPart = fileData.nFileSizeLow;
-length.HighPart = fileData.nFileSizeHigh;
-rv = (jlong)length.QuadPart;
+size_t off = wcslen(pathbuf) - wcslen(fileData.cFileName);
+if (wcscmp(pathbuf + off, fileData.cFileName) == 0) {
+ULARGE_INTEGER length;
+length.LowPart = fileData.nFileSizeLow;
+length.HighPart = fileData.nFileSizeHigh;
+rv = (jlong)length.QuadPart;
+}
 FindClose(h);
 }
 }

The offset in the string comparison is necessary as “C:\\pagefile.sys” becomes 
“pagefile.sys” in fileData.cFileName. I’m not certain that this will work in 
all cases however but it is certainly no worse than before.

Thanks for the suggestion!

Brian

RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

[Vivek Theeyarath]

Hi All,

 Please review fix for https://bugs.openjdk.java.net/browse/JDK-8177276 
. 

 

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.02/

 

The related CSR is https://bugs.openjdk.java.net/browse/JDK-8202991 .

 

Regards

Vivek

RFR 8203279 : Faster calculation of power of two

[Ivan Gerasimov]

Hello!

In a few places we have code that rounds an integer up to the nearest 
power of two.


It is done with a series of RSHOTFs and ORs, but it can possibly be done 
faster with the use of Integer.numberOfLeadingZeros (assuming it is 
intrinsified).


Would you please help review this trivial optimization:

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

For HashMap.tableSizeFor() I created a simple test with a loop from 
Integer.MIN_VALUE to Integer.MAX_VALUE (including), to make sure the 
result is the same.


For TimSort.ensureCapacity() I checked all positive values of minCapacity.

--
With kind regards,
Ivan Gerasimov

RFR (JDK11/JAXP/java.xml) 8198548: Initialization race in com.sun.org.apache.xerces.internal.impl.xpath.regex.Token.getRange() on Token.categories

[Joe Wang]

Hi,

Please review a fix on synchronization. The real change is the 
double-checked locking, all others are s/Token.categories/tmpMap/g.


All JAXP tests passed. Before the fix, the test attached to JBS failed 
only once for me after 10,000 runs and then a few 1000 runs (impossible 
to get one with only 20 runs as the comment mentioned).  The author of 
the original report put the actual pass rate at 99.994%. After the fix 
then, in theory it should be 100%, I've made a 100,000 run without 
seeing a failure.


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

Thanks,
Joe

Re: 8202076: test/jdk/java/io/File/WinSpecialFiles.java on windows with VS2017

[Brian Burkhalter]

Hi Ivan,

On May 16, 2018, at 2:54 PM, Ivan Gerasimov  wrote:

> I wonder if FileSystem.getLength() will start to return the file size when 
> the file name contains wildcards?

Interesting point. A simple program verifies that this is the case:

import java.io.File;

public class FileLength {
public static void main(String[] args) {
String name = args.length == 0 ? "C:\\pagefile.sys" : args[1];
File file = new File(name);
long length = file.length();
System.out.format("%s length: %d%n", name, length);
}
}

> Maybe it is better to compare fileData.cFileName with the pathbuf to make 
> sure we're dealing with the correct file?

I am doubtlessly missing something, but if pathbuf contains wildcards it’s not 
obvious to me exactly how that would help.

Thanks,

Brian

Re: RFR: jsr166 jdk integration 2018-05

[Doug Lea]

Sorry Stuart, but I'm joining the no-modCount barrage.
To pick on the main issue...

On 05/16/2018 05:21 PM, Stuart Marks wrote:

> Suppose that a replaceAll() on another thread occurs, and that this is
> allowed. Does the application care whether the eventual printout
> contains partly new values and partly old values? How can you tell? It
> seems to me that this is more likely a programming error than a valid
> use case.

There are many data-races that are never detected via CME. CME was
designed to trap only some of them (and even at that, only heuristically
because modCount operations are not strictly ordered). ModCount
mechanics only deal with insertions and removals. Failing to catch a
race in replaceAll is no different than failing to catch one with
multiple concurrent setValues. It might be "nice" to adjust modCount on
any operation that might contribute to a datarace, but no reason to
single out replaceAll.

Having said this, I don't think that anything in the spec (vs default
implementation) prohibits an implementation of replaceAll from removing
and then re-inserting each element, in which case an implementation
would modify modCounts.

-Doug

Re: RFR: jsr166 jdk integration 2018-05

[David Holmes]

On 17/05/2018 4:25 AM, Stuart Marks wrote:

On 5/15/18 7:37 PM, David Holmes wrote:
I'm still with Martin on this. It makes no sense to me to allow 
replacing one element to not cause CME, but replacing more than one 
does cause CME. This is inconsistent and confusing and the end result 
is the programmer won't know what to expect when or why.


This is the fallacy of composition, that if a single operation has some 
property, then an arbitrary composition of those operations must also 
have that property. This is decidedly not true for many things. Consider 
atomicity for example. My claim is that the setting of an individual 
element is qualitatively different from a bulk operation.


No it's not "the fallacy of composition". Just because some operations 
can not compose it does not follow that no operations can ever compose.


The original intent of CME, from my recollections back in 
lead-up-to-Java-5 days, was to prevent iterators from breaking i.e. 
throwing exceptions, due to the occurrence of the "concurrent" 
operation that changed the structure. It was not intended as an 
indicator of a semantic programming error. Replacing one element 
whilst there is a live iterator can be just as semantically wrong as 
replacing them all.


CME, iterators, and the fail-fast concept were introduced in JDK 1.2 


Yes but believe me it was discussed a LOT when we introduced the 
concurrency utilities! :)


with the Collections Framework. The platform has evolved a LOT since 
then, so I don't think it's worthwhile to make a stand on original 
intent. 


That seems an attempt at legitimising straying from the original intent, 
rather than recognizing that straying away from that intent should have 
been considered a bug.


Besides, ArrayList clearly documents that original intent: "if the list 
is structurally modified ...".


The wording around CME and fail-fast is very pragmatic, so it 
sense to be pragmatic about this issue.


I think I am being pragmatic. A change in content is not a change in 
structure. CME is inappropriate in such circumstances IMHO.


It seems to me that inconsistencies have been allowed to creep in wrt. 
concurrent "modifications" and throwing CME. Now that is recognized we 
have the dilemma of undoing past mistakes or embracing a stronger 
(undocumented) notion of "concurrent modification" and applying it 
uniformly. Neither path is smooth unfortunately.


David
-


s'marks

Re: RFR: jsr166 jdk integration 2018-05

[Martin Buchholz]

On Wed, May 16, 2018 at 2:21 PM, Stuart Marks 
wrote:

>
>
> On 5/15/18 8:02 PM, Martin Buchholz wrote:
>
>>
>> How many times the lock is acquired is an implementation detail, a
>> non-obvious tradeoff even.
>> vector.replaceAll holds the lock throughout, but vector.subList(0,
>> size).replaceAll holds the lock for each element (sigh ... racily (really a
>> bug!)).
>>
>
> In Vector's case it's specified, not an implementation detail. You can't
> perform certain bulk operations on a Vector without holding the lock
> externally.
>

What is specified?  Vector is synchronized, but it's unspecified whether
external synchronization via synchronized (vector) {...} works

Vector.replaceAll simply inherits spec from List.replaceAll - it seems
unspecified whether bulk operations like replaceAll on Vector are atomic.
(In fact they are not atomic on the subList).


> Again, imagine this use case: there is a periodic background task
>> that
>> optimizes all the elements of a Vector
>> vector.replaceAll(x -> optimized(x))
>> That should not break any iterations in progress.
>>
>> I don't think it's possible to do that correctly without holding a
>> lock
>> around the entire iteration.
>>
>> I don't see why.
>>
>> If the lock is held, CME can't occur, as a concurrent replaceAll()
>> will
>> occur before or after the iteration, never in the middle.
>>
>> I don't see why - iteration is inherently non-atomic, so replaceAll could
>> be called in the middle.
>>
>> If an iteration over a Vector doesn't hold a lock, any
>> read-modify-write
>> operations (consider a loop with a ListIterator on which set() is
>> called)
>> can be interleaved with bulk operations (like replaceAll) which is
>> clearly
>> incorrect. In such cases, CME should be thrown.
>>
>> Imagine your iterators are all read-only, and don't care whether they see
>> an element or its replacement.
>>
>
> You're imagining an incredibly narrow use case. You're choosing Vector,
> not ArrayList; the replaceAll() operation must an optimization that doesn't
> affect the semantics of the object (say, the outcome of any logic); the
> iteration must be read-only, not read-modify-write; and the logic within
> the iteration "doesn't care" whether it gets old or new values.
>
> I don't find it compelling that it's possible to imagine such a case. Most
> code won't conform to it. And in fact it's really hard to tell whether it
> does.
>
> Consider an example like this:
>
> for (T t : vectorOfT) {
> print(t);
> }
>
> Suppose that a replaceAll() on another thread occurs, and that this is
> allowed. Does the application care whether the eventual printout contains
> partly new values and partly old values? How can you tell? It seems to me
> that this is more likely a programming error than a valid use case.
>

Vector is unloved and there are few good uses for it at all - the List
interface is concurrency-hostile.  BUT one good use case for concurrency
here seems to be when there are no structural modifications, when an index
unambiguously identifies a "place".  Sort of like AtomicReferenceArray.  We
could aim towards making Vector and AtomicReferenceArray and
ConcurrentHashMap more useful and cross-compatible (e.g.
Vector.accumulateAndGet0


> Also, this use case cannot be written today, because CME is thrown.
>>
>> ??  Imagine there's only one writer thread, with some Iterating reader
>> threads.  Every midnight, the writer thread optimizes all the elements
>>for (int i = 0; i < size; i++) vector.set(i, optimized(vector.get(i))
>> This code has worked well since the '90s without CME.  One day the
>> maintainer notices shiny new Vector.replaceAll and "fixes" the code.
>> """It's perfectly safe""".
>> The CME is rare and so only caught when the maintainer has gone on to the
>> next job.  The CMEs only happen at midnight!
>>
>
> By "cannot be written today" I mean that the following:
>
> for (T t: arrayList) {
> if (condition) {
> list.replaceAll();
> }
> }
>
> has ALWAYS thrown CME, since the introduction of replaceAll().
>

But that's a bug (!) and the variant below has never thrown CME, and I
don't want to compound the existing bug.

for (T t: arrayList) {
if (condition) {
list.subList(0, list.size()).replaceAll();
}
}

Sure, there are cases such as you describe where CME gets thrown only
> rarely. That's preferable to getting incorrect results equally rarely.
> That's the point of fail-fast.
>
> **
>
> (subsequent email)
>
> (We don't seem to be moving towards consensus ...)
>>
>
> Apparently
>
> At the very least, having replaceAll increment modCount (inconsistently!)
>> is surprising to users and is not documented anywhere.
>>
>
> Maybe it should be documented then.
>
> **
>
> Why are you placing so much emphasis on *removing* the CME behavior? It's
> been this way since Java 8 was 

Re: 8202076: test/jdk/java/io/File/WinSpecialFiles.java on windows with VS2017

[Ivan Gerasimov]

Hi Brian!

I wonder if FileSystem.getLength() will start to return the file size 
when the file name contains wildcards?


Maybe it is better to compare fileData.cFileName with the pathbuf to 
make sure we're dealing with the correct file?


With kind regards,
Ivan


On 5/16/18 2:37 PM, Brian Burkhalter wrote:

https://bugs.openjdk.java.net/browse/JDK-8202076
http://cr.openjdk.java.net/~bpb/8202076/webrev.00/

If all else fails, obtain the file length via the FindFirstFileW function [1].

Thanks,

Brian

[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa364418(v=vs.85).aspx


--
With kind regards,
Ivan Gerasimov

8202076: test/jdk/java/io/File/WinSpecialFiles.java on windows with VS2017

[Brian Burkhalter]

https://bugs.openjdk.java.net/browse/JDK-8202076
http://cr.openjdk.java.net/~bpb/8202076/webrev.00/

If all else fails, obtain the file length via the FindFirstFileW function [1].

Thanks,

Brian

[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa364418(v=vs.85).aspx

Re: Bug Request: Please remove out-of-date files from bug

[David Holmes]

Done.

Though you sent this to the wrong mailing list for a hotspot bug.

David

On 17/05/2018 1:36 AM, Adam Farley8 wrote:

Hi All,

In bug JDK-8190187, hotspot_hg_diff and jdk_hg_diff are out-of-date.

Please can someone delete these files?

Best Regards

Adam Farley
OpenJDK Team
Runtimes
IBM


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

Re: RFR: jsr166 jdk integration 2018-05

[Stuart Marks]

On 5/15/18 8:02 PM, Martin Buchholz wrote:


How many times the lock is acquired is an implementation detail, a non-obvious 
tradeoff even.
vector.replaceAll holds the lock throughout, but vector.subList(0, 
size).replaceAll holds the lock for each element (sigh ... racily (really a bug!)).


In Vector's case it's specified, not an implementation detail. You can't perform 
certain bulk operations on a Vector without holding the lock externally.



Again, imagine this use case: there is a periodic background task that
optimizes all the elements of a Vector
vector.replaceAll(x -> optimized(x))
That should not break any iterations in progress.

I don't think it's possible to do that correctly without holding a lock
around the entire iteration.

I don't see why.

If the lock is held, CME can't occur, as a concurrent replaceAll() will
occur before or after the iteration, never in the middle.

I don't see why - iteration is inherently non-atomic, so replaceAll could be 
called in the middle.


If an iteration over a Vector doesn't hold a lock, any read-modify-write
operations (consider a loop with a ListIterator on which set() is called)
can be interleaved with bulk operations (like replaceAll) which is clearly
incorrect. In such cases, CME should be thrown.

Imagine your iterators are all read-only, and don't care whether they see an 
element or its replacement.


You're imagining an incredibly narrow use case. You're choosing Vector, not 
ArrayList; the replaceAll() operation must an optimization that doesn't affect 
the semantics of the object (say, the outcome of any logic); the iteration must 
be read-only, not read-modify-write; and the logic within the iteration "doesn't 
care" whether it gets old or new values.


I don't find it compelling that it's possible to imagine such a case. Most code 
won't conform to it. And in fact it's really hard to tell whether it does.


Consider an example like this:

for (T t : vectorOfT) {
print(t);
}

Suppose that a replaceAll() on another thread occurs, and that this is allowed. 
Does the application care whether the eventual printout contains partly new 
values and partly old values? How can you tell? It seems to me that this is more 
likely a programming error than a valid use case.



Also, this use case cannot be written today, because CME is thrown.

??  Imagine there's only one writer thread, with some Iterating reader threads.  
Every midnight, the writer thread optimizes all the elements

   for (int i = 0; i < size; i++) vector.set(i, optimized(vector.get(i))
This code has worked well since the '90s without CME.  One day the maintainer 
notices shiny new Vector.replaceAll and "fixes" the code.

"""It's perfectly safe""".
The CME is rare and so only caught when the maintainer has gone on to the next 
job.  The CMEs only happen at midnight!


By "cannot be written today" I mean that the following:

for (T t: arrayList) {
if (condition) {
list.replaceAll();
}
}

has ALWAYS thrown CME, since the introduction of replaceAll().

Sure, there are cases such as you describe where CME gets thrown only rarely. 
That's preferable to getting incorrect results equally rarely. That's the point 
of fail-fast.


**

(subsequent email)


(We don't seem to be moving towards consensus ...)


Apparently


At the very least, having replaceAll increment modCount (inconsistently!) is 
surprising to users and is not documented anywhere.


Maybe it should be documented then.

**

Why are you placing so much emphasis on *removing* the CME behavior? It's been 
this way since Java 8 was delivered. Is this causing a problem? What will be 
solved by removing it?


s'marks

Re: RFR: jsr166 jdk integration 2018-05

[Martin Buchholz]

On Wed, May 16, 2018 at 11:25 AM, Stuart Marks 
wrote:

>
>
> On 5/15/18 7:37 PM, David Holmes wrote:
>
>> I'm still with Martin on this. It makes no sense to me to allow replacing
>> one element to not cause CME, but replacing more than one does cause CME.
>> This is inconsistent and confusing and the end result is the programmer
>> won't know what to expect when or why.
>>
>
> This is the fallacy of composition, that if a single operation has some
> property, then an arbitrary composition of those operations must also have
> that property. This is decidedly not true for many things. Consider
> atomicity for example. My claim is that the setting of an individual
> element is qualitatively different from a bulk operation.
>

(We don't seem to be moving towards consensus ...)

atomicity is "obviously" a non-composable property of an operation, but
contrarily "does not modify" is "obviously" composable.

Having replaceAll increment modCount is almost as wrong as having forEach
increment modCount, even though """forEach is qualitatively different from
get()."""

Incrementing modCount is introducing a subtle race into plausible user code
that would otherwise be correct.

At the very least, having replaceAll increment modCount (inconsistently!)
is surprising to users and is not documented anywhere.

Re: RFR: jsr166 jdk integration 2018-05

[Stuart Marks]

On 5/15/18 7:37 PM, David Holmes wrote:
I'm still with Martin on this. It makes no sense to me to allow replacing one 
element to not cause CME, but replacing more than one does cause CME. This is 
inconsistent and confusing and the end result is the programmer won't know what 
to expect when or why.


This is the fallacy of composition, that if a single operation has some 
property, then an arbitrary composition of those operations must also have that 
property. This is decidedly not true for many things. Consider atomicity for 
example. My claim is that the setting of an individual element is qualitatively 
different from a bulk operation.


The original intent of CME, from my recollections back in lead-up-to-Java-5 
days, was to prevent iterators from breaking i.e. throwing exceptions, due to 
the occurrence of the "concurrent" operation that changed the structure. It was 
not intended as an indicator of a semantic programming error. Replacing one 
element whilst there is a live iterator can be just as semantically wrong as 
replacing them all.


CME, iterators, and the fail-fast concept were introduced in JDK 1.2 with the 
Collections Framework. The platform has evolved a LOT since then, so I don't 
think it's worthwhile to make a stand on original intent. The wording around CME 
and fail-fast is very pragmatic, so it sense to be pragmatic about this issue.


s'marks

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[Paul Sandoz]

HI,

Nice thorough work on this, surprisingly tricky in some areas esp. MHs.

Class
—

3857  * If there is any error accessing the nest host, or the nest host 
is
3858  * in any way invalid, then {@code this} is returned.

I am curious under what conditions this can arise. As a caller this makes me 
nervous :-) Are there conditions that are worth calling it. This is related to 
Alan’s comment about primitive or array classes. Its clearer when looking at 
the implementation: primitive/array classes, linkage error, and the more 
general nest membership validation.


3883  * @throws SecurityException
3884  * If the returned class is not the current class, and
3885  * if a security manager, s, is present and the caller's
3886  * class loader is not the same as or an ancestor of the class
3887  * loader for the returned class and invocation of {@link
3888  * SecurityManager#checkPackageAccess s.checkPackageAccess()}
3889  * denies access to the package of the returned class
3890  * @since 11
3891  * @jvms 4.7.28 and 4.7.29 NestHost and NestMembers attributes
3892  */
3893 @CallerSensitive
3894 public Class getNestHost() {

Maybe i need more coffee, but I am struggling to see in the implementation the 
checks for the case of "and the caller’s class loader is not the same as or an 
ancestor of the class loader for the returned class”. Is it implied that all 
classes in the nest have to be loaded from the same or from a common ancestor 
class loader? so you only need to check one class in the nest against the 
calling class.


3984 public Class[] getNestMembers() {

I still think not removing dups is a mistake as it could be a source of subtle 
bugs. But i doubt at this point i can persuade you or others to change it :-)


3989 // Can't actually enable this due to bootstrapping issues
3990 // assert(members.length != 1 || members[0] == this); // expected 
invariant from VM

That's interesting and frustrating!


Reflection.java
—

 146 // Check for nestmate access if member is private
 147 if (Modifier.isPrivate(modifiers)) {
 148   // assert: isSubclassof(targetClass, memberClass)
 149   // Note: targetClass may be outside the nest, but that is okay
 150   //   as long as memberClass is in the nest.
 151   boolean nestmates = areNestMates(currentClass, memberClass);
 152   if (nestmates) {
 153 return true;
 154   }
 155 }

Trivially, you don’t need the local variable “nestmates”.


VerifyAccess.java
—

 134 case PRIVATE:
 135 // Rules for privates follows access rules for nestmates.
 136 boolean canAccess = ((allowedModes & PRIVATE) != 0 &&
 137  Reflection.areNestMates(defc, 
lookupClass));
 138 // FIX ME: Sanity check refc == defc. Either remove or convert 
to
 139 // plain assert before integration.
 140 myassert((canAccess && refc == defc) || !canAccess);
 141 return canAccess;
 142 default:
 143 throw new IllegalArgumentException("bad modifiers: 
"+Modifier.toString(mods));
 144 }
 145 }
 146 static void myassert(boolean cond) {
 147 if (!cond) throw new Error("Assertion failed");
 148 }

Do you plan to chase up the FIX ME now or later?


I agree with Alan about the current location of the reflection API tests. If 
these tests don’t need to be hammered with various and exotic HotSpot flags i 
think they are better placed to be under test/jdk/java/lang/reflect.


Thanks,
Paul.



> On May 14, 2018, at 5:52 PM, David Holmes  wrote:
> 
> This review is being spread across four groups: langtools, core-libs, hotspot 
> and serviceability. This is the specific review thread for core-libs - webrev:
> 
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
> 
> See below for full details - including annotated full webrev guiding the 
> review.
> 
> The intent is to have JEP-181 targeted and integrated by the end of this 
> month.
> 
> Thanks,
> David
> -
> 
> The nestmates project (JEP-181) introduces new classfile attributes to 
> identify classes and interfaces in the same nest, so that the VM can perform 
> access control based on those attributes and so allow direct private access 
> between nestmates without requiring javac to generate synthetic accessor 
> methods. These access control changes also extend to core reflection and the 
> MethodHandle.Lookup contexts.
> 
> Direct private calls between nestmates requires a more general calling 
> context than is permitted by invokespecial, and so the JVMS is updated to 
> allow, and javac updated to use, invokevirtual and invokeinterface for 
> private class and interface method calls respectively. These changed 
> semantics also extend to MethodHandle 

Re: Discussion: Efficient ByteBuffer -> String that avoids additional copying

[Xueming Shen]

On 5/16/18, 10:03 AM, Claes Redestad wrote:

Hi,

I'd just like to point out there's been some recent effort on this, see:

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

.. which was RFR'd here:

 http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051401.html 



I'm not sure what the current status of that is, but seems there's 
more than

a bit of overlap with what's being suggested here.


It started to appear Charset.De/Encoder might be a better place for 
String/ByteBuffer
decoding/encoding operations, convenient methods included, given the 
"complicated"
nature of decoding/encoding lifecircle management (shift-in, shfit-out, 
or even the
utf16 bom status, when the passed in output ByteBuffer does not have 
enough space,

for example). No final decision/proposal yet.

-Sherman




/Claes

On 2018-05-16 18:42, Jacob Glickman wrote:

Paul,

Would you mind explaining more about what you're looking for regarding
this?  I'm curious if `ByteBuffer#getString` is what you're after, or
rather something similar to `String.from(ByteBuffer)`.  I'll definitely
have a look at jdk.internal.misc.SharedSecrets in the meantime.  Thanks!

- Jacob


Hi Jacob,
I do have one idea (that i don’t think is currently represented as a 
bug,
though i have not searched JBS), if you are willing to take it on. It 
will

require some investigation, and careful testing, it’s not necessarily a
starter bug :-), but i can help guide.
Investigate new methods to more efficiently support ByteBuffer -> 
String
method on ByteBuffer that can avoid additional copying and 
makeappropriate
use of charsets given String’s compact string support. You need to 
look at
jdk.internal.misc.SharedSecrets for clues on how to trampoline 
between the

nio and lang packages.

Paul.

Re: Discussion: Efficient ByteBuffer -> String that avoids additional copying

[Claes Redestad]

Hi,

I'd just like to point out there's been some recent effort on this, see:

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

.. which was RFR'd here:

 http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051401.html

I'm not sure what the current status of that is, but seems there's more than
a bit of overlap with what's being suggested here.

/Claes

On 2018-05-16 18:42, Jacob Glickman wrote:

Paul,

Would you mind explaining more about what you're looking for regarding
this?  I'm curious if `ByteBuffer#getString` is what you're after, or
rather something similar to `String.from(ByteBuffer)`.  I'll definitely
have a look at jdk.internal.misc.SharedSecrets in the meantime.  Thanks!

- Jacob


Hi Jacob,
I do have one idea (that i don’t think is currently represented as a bug,

though i have not searched JBS), if you are willing to take it on. It will
require some investigation, and careful testing, it’s not necessarily a
starter bug :-), but i can help guide.

Investigate new methods to more efficiently support ByteBuffer -> String

method on ByteBuffer that can avoid additional copying and makeappropriate
use of charsets given String’s compact string support. You need to look at
jdk.internal.misc.SharedSecrets for clues on how to trampoline between the
nio and lang packages.

Paul.

Re: RFR JDK-8191533,jar --describe-module prints service provider class names in lower case

[Alan Bateman]

On 16/05/2018 17:11, Xueming Shen wrote:

:

http://cr.openjdk.java.net/~sherman/8191533/webrev

Rename as suggested. The empty set change suggested seems to be an 
"incompatible"
change for the output when the set indeed is empty :-) I'm not sure if 
anyone has already
built dependency on it since 9. So if you are fine with it I'd like to 
leave it asis for now.
Aside from the modifier sets, the others can't be empty in the context 
that toString and toLowerCaseString is used. However, what you have is 
fine too.


-Alan

Discussion: Efficient ByteBuffer -> String that avoids additional copying

[Jacob Glickman]

Paul,

Would you mind explaining more about what you're looking for regarding
this?  I'm curious if `ByteBuffer#getString` is what you're after, or
rather something similar to `String.from(ByteBuffer)`.  I'll definitely
have a look at jdk.internal.misc.SharedSecrets in the meantime.  Thanks!

- Jacob

>Hi Jacob,
>I do have one idea (that i don’t think is currently represented as a bug,
though i have not searched JBS), if you are willing to take it on. It will
require some investigation, and careful testing, it’s not necessarily a
starter bug :-), but i can help guide.
>Investigate new methods to more efficiently support ByteBuffer -> String
method on ByteBuffer that can avoid additional copying and makeappropriate
use of charsets given String’s compact string support. You need to look at
jdk.internal.misc.SharedSecrets for clues on how to trampoline between the
nio and lang packages.
>Paul.

Re: RFR JDK-8191533,jar --describe-module prints service provider class names in lower case

[Xueming Shen]

On 5/16/18, 12:19 AM, Alan Bateman wrote:

On 16/05/2018 00:51, Xueming Shen wrote:

Hi,

Please help review the change for JDK-8191533

issue: https://bugs.openjdk.java.net/browse/JDK-8191533
webrev: http://cr.openjdk.java.net/~sherman/8191533/webrev/

The jmod change has been verify by applying jmod on those modules at 
jdk home.
This looks okay but I think it could be improved if you have 
toLowerCaseString and toString rather than toStringWithCase and 
toString. Also they can both drop the isEmpty check and prefixing the 
string with " ". Instead I think the usages should be fixed to add the 
trailing space that they are missing, e.g. " with" to " with ", " to" 
to " to " 


-Alan

Thanks Alan.

http://cr.openjdk.java.net/~sherman/8191533/webrev

Rename as suggested. The empty set change suggested seems to be an 
"incompatible"
change for the output when the set indeed is empty :-) I'm not sure if 
anyone has already
built dependency on it since 9. So if you are fine with it I'd like to 
leave it asis for now.


-Shrman

Bug Request: Please remove out-of-date files from bug

[Adam Farley8]

Hi All,

In bug JDK-8190187, hotspot_hg_diff and jdk_hg_diff are out-of-date.

Please can someone delete these files?

Best Regards

Adam Farley 
OpenJDK Team 
Runtimes 
IBM 


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

RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java - Final Email

[Adam Farley8]

Hi All, 

The code to resolve JDK-8190187 has been added to the bug, in hg_diff.txt.

Could a committer please take the fix and: 

1) Complete the CSR process for the new JNI Return code. 
2) Commit the changes that contain the Return code.

And, optionally, 3) Perform 1 and 2 for the jdwp agent changes as well, so 
it can use this new functionality.

Note: the lack of response to this issue over the past few months has been 

interpreted as a lack of community interest in resolving this defect, so, 
should 
this email spark no response, I will not be pursuing the issue further. If 
and when 
community interest does resume, hg_diff contains the most up-to-date code.

Best Regards

Adam Farley


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

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Severin Gehwolf]

On Wed, 2018-05-16 at 12:39 +0100, Alan Bateman wrote:
> On 16/05/2018 09:02, Severin Gehwolf wrote:
> > :
> > New webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/
> > 
> 
> This version looks okay to me.
> 
> -Alan

Thanks for the review!

Cheers,
Severin

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Alan Bateman]

On 16/05/2018 09:02, Severin Gehwolf wrote:

:
New webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/


This version looks okay to me.

-Alan

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Andrew Haley]

On 05/16/2018 09:10 AM, Aleksey Shipilev wrote:
> On 05/16/2018 10:02 AM, Severin Gehwolf wrote:
>> $ bash imageFile.o.cmdline 
>> /disk/openjdk/upstream-sources/openjdk-hs/src/java.base/share/native/libjimage/imageFile.cpp:60:33:
>>  error: macro "assert" passed 2 arguments, but takes just 1
>>  assert(seed > 0, "invariant");
> 
> Oh, WTF! So it is the trick to print the assert message with C asserts.

Yikes!  WTF indeed.  :-)

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Aleksey Shipilev]

On 05/16/2018 10:02 AM, Severin Gehwolf wrote:
> $ bash imageFile.o.cmdline 
> /disk/openjdk/upstream-sources/openjdk-hs/src/java.base/share/native/libjimage/imageFile.cpp:60:33:
>  error: macro "assert" passed 2 arguments, but takes just 1
>  assert(seed > 0, "invariant");

Oh, WTF! So it is the trick to print the assert message with C asserts.

> New webrev:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/

Looks good!

-Aleksey

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Severin Gehwolf]

Hi David, Aleksey,

Thanks for the reviews! Comments inline.

On Wed, 2018-05-16 at 12:01 +1000, David Holmes wrote:
> +1 on both comments.
> 
> In addition I'd prefer
> 
> + u4 useed = (u4)seed;
> 
> for clarity, rather than just 's'.

Fixed.

> Thanks,
> David
> 
> On 16/05/2018 2:16 AM, Aleksey Shipilev wrote:
> > On 05/15/2018 06:11 PM, Severin Gehwolf wrote:
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8203223
> > > webrev: 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.01/
> > 
> > *) Um:
> >assert(seed > 0 && "invariant");
> > 
> > This should be this, right?
> > 
> >assert(seed > 0, "invariant");

This was throwing me off too, but I've used the same model than other
assert() calls in this file. For example line 161 in imageFile.cpp
reads like this:
 161 assert(replaced != NULL && "allocation failed");

If I changed it to your suggestion I'm getting this compile failure:

$ bash imageFile.o.cmdline 
/disk/openjdk/upstream-sources/openjdk-hs/src/java.base/share/native/libjimage/imageFile.cpp:60:33:
 error: macro "assert" passed 2 arguments, but takes just 1
 assert(seed > 0, "invariant");
 ^
Seems to be a difference between hotspot and core-libs. So I've kept
this as is.

> > *) I would also write:
> > 
> >return (s4)s & 0x7FFF;
> > 
> > as
> > 
> >return (s4)(s & 0x7FFF);

Fixed.

New webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.02/

Thanks,
Severin

Re: RFR JDK-8191533,jar --describe-module prints service provider class names in lower case

[Alan Bateman]

On 16/05/2018 00:51, Xueming Shen wrote:

Hi,

Please help review the change for JDK-8191533

issue: https://bugs.openjdk.java.net/browse/JDK-8191533
webrev: http://cr.openjdk.java.net/~sherman/8191533/webrev/

The jmod change has been verify by applying jmod on those modules at 
jdk home.
This looks okay but I think it could be improved if you have 
toLowerCaseString and toString rather than toStringWithCase and 
toString. Also they can both drop the isEmpty check and prefixing the 
string with " ". Instead I think the usages should be fixed to add the 
trailing space that they are missing, e.g. " with" to " with ", " to" to 
" to " 


-Alan

RE: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

[Langer, Christoph]

Hi Matthias,

yes, reviewed.

Best regards
Christoph

From: Baesken, Matthias
Sent: Mittwoch, 16. Mai 2018 09:06
To: Langer, Christoph ; 'build-...@openjdk.java.net' 
; ppc-aix-port-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Cc: Lindenmaier, Goetz 
Subject: RE: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

Hi  Christoph can I add you as second reviewer  (other reviewer was Erik 
Joelsson) can push the change ?

Best regards, Matthias



From: Langer, Christoph
Sent: Donnerstag, 26. April 2018 16:38
To: Baesken, Matthias 
>; 
'build-...@openjdk.java.net' 
>; 
ppc-aix-port-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Cc: Simonis, Volker >
Subject: RE: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

Hi Matthias,

to me the change in principal looks good.

I'm wondering if it is possible to do a comparison like xlc < 13 (e.g. extract 
major number before the first dot, then compare numerically) - but maybe it is 
too complicated and the current single version compare suits our needs ?

Best regards
Christoph

From: Baesken, Matthias
Sent: Donnerstag, 26. April 2018 16:14
To: 'build-...@openjdk.java.net' 
>; 
ppc-aix-port-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Cc: Langer, Christoph 
>; Simonis, Volker 
>
Subject: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

Hello  ,  could you please review this small adjustment to  the symbol 
visibility compilation settings on AIX ?
Currently  we use  XLC 12.1  to compile  JDK on AIX .

However XLC 12.1   does not support  the "-qvisibility=hidden"  setting 
currently set on AIX.
It was introduced with  XLC 13.1 . Christoph found some info about it here :

https://www.ibm.com/developerworks/aix/library/au-aix-symbol-visibility-part2/index.html

Setting it  only generates  hundreds  of warnings  in the build log , warnings 
look like this :
XlC12.1

bash-4.4$ xlC -qversion
IBM XL C/C++ for AIX, V12.1 (5765-J02, 5725-C72)
Version: 12.01..0019

bash-4.4$ xlC -qvisibility=hidden sizeof.c -o sizeof_aixxlc
1506-173 (W) Option visibility=hidden is not valid. Enter xlC for list of valid 
options.

Compare to XLC13.1

bash-3.00$ xlC -qversion
IBM XL C/C++ for AIX, V13.1 (5725-C72, 5765-J07)
Version: 13.01..0008
bash-3.00$ xlC -qvisibility=default sizeof.c -o sizeof_aixxlc
bash-3.00$ xlC -qvisibility=hidden sizeof.c -o sizeof_aixxlc


So it is better to avoid  setting these flags when using xlc12.1   .
Please review :

Bug :

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

Change :

http://cr.openjdk.java.net/~mbaesken/webrevs/8202322/


Best regards, Matthias

RE: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

[Baesken, Matthias]

Hi  Christoph can I add you as second reviewer  (other reviewer was Erik 
Joelsson) can push the change ?

Best regards, Matthias



From: Langer, Christoph
Sent: Donnerstag, 26. April 2018 16:38
To: Baesken, Matthias ; 'build-...@openjdk.java.net' 
; ppc-aix-port-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Cc: Simonis, Volker 
Subject: RE: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

Hi Matthias,

to me the change in principal looks good.

I'm wondering if it is possible to do a comparison like xlc < 13 (e.g. extract 
major number before the first dot, then compare numerically) - but maybe it is 
too complicated and the current single version compare suits our needs ?

Best regards
Christoph

From: Baesken, Matthias
Sent: Donnerstag, 26. April 2018 16:14
To: 'build-...@openjdk.java.net' 
>; 
ppc-aix-port-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Cc: Langer, Christoph 
>; Simonis, Volker 
>
Subject: RFR : 8202322: AIX: symbol visibility flags not support on xlc 12.1

Hello  ,  could you please review this small adjustment to  the symbol 
visibility compilation settings on AIX ?
Currently  we use  XLC 12.1  to compile  JDK on AIX .

However XLC 12.1   does not support  the "-qvisibility=hidden"  setting 
currently set on AIX.
It was introduced with  XLC 13.1 . Christoph found some info about it here :

https://www.ibm.com/developerworks/aix/library/au-aix-symbol-visibility-part2/index.html

Setting it  only generates  hundreds  of warnings  in the build log , warnings 
look like this :
XlC12.1

bash-4.4$ xlC -qversion
IBM XL C/C++ for AIX, V12.1 (5765-J02, 5725-C72)
Version: 12.01..0019

bash-4.4$ xlC -qvisibility=hidden sizeof.c -o sizeof_aixxlc
1506-173 (W) Option visibility=hidden is not valid. Enter xlC for list of valid 
options.

Compare to XLC13.1

bash-3.00$ xlC -qversion
IBM XL C/C++ for AIX, V13.1 (5725-C72, 5765-J07)
Version: 13.01..0008
bash-3.00$ xlC -qvisibility=default sizeof.c -o sizeof_aixxlc
bash-3.00$ xlC -qvisibility=hidden sizeof.c -o sizeof_aixxlc


So it is better to avoid  setting these flags when using xlc12.1   .
Please review :

Bug :

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

Change :

http://cr.openjdk.java.net/~mbaesken/webrevs/8202322/


Best regards, Matthias