date:20181211

I used to believe that,  but apparently I was wrong.
https://openjdk.markmail.org/thread/rfqfultw35i2az45

On Tue, Dec 11, 2018 at 8:14 PM Zheka Kozlov  wrote:

> Would be better to add @Stable to the fields instead? (`n` and `element`
> are final, so @Stable is OK here)
>
> ср, 12 дек. 2018 г. в 11:02, Martin Buchholz :
>
>> In performance critical code, we don't trust hotspot to not reload final
>>> fields.  Other forEach methods do this, e.g.
>>
>>
>> final Object[] es = queue;
>> for (int i = 0, n = size; i < n; i++)
>> action.accept((E) es[i]);
>>
>>
>

Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

2018-12-11 Thread Zheka Kozlov

Would be better to add @Stable to the fields instead? (`n` and `element`
are final, so @Stable is OK here)

ср, 12 дек. 2018 г. в 11:02, Martin Buchholz :

> In performance critical code, we don't trust hotspot to not reload final
>> fields.  Other forEach methods do this, e.g.
>
>
> final Object[] es = queue;
> for (int i = 0, n = size; i < n; i++)
> action.accept((E) es[i]);
>
>

Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

>
> In performance critical code, we don't trust hotspot to not reload final
> fields.  Other forEach methods do this, e.g.


final Object[] es = queue;
for (int i = 0, n = size; i < n; i++)
action.accept((E) es[i]);

Re: JDK-8210280 - Unnecessary reallocation when invoking HashMap.putAll()


Hi,

fyi, jmh tests appropriate for JDK apis are located in the 
test/micro/... directories.


The benchmarks are built with:
  % make build-microbenchmark

The results are in the build//images/test/micro/benchmarks.jar

$.02, Roger

On 12/11/2018 A 9:55 PM, Martin Buchholz wrote:

Hi Michal, pleased to meet you.  I'll be your sponsor.

The test will need a legal header, presumably similar to others authored by
redhatters.

It is now possible to check in jmh microbenchmarks (but I've never done so
myself).

The current coding style is non-standard, but deliberate; avoid gratuitous
changes like
-Node e;
+Node e;

As author of concurrent/ConcurrentHashMap/WhiteBox.java ...
- I thought you'd need a @modules ... did this test actually pass?
- you should have some kind of @summary that includes the word "whitebox"
- why not "throws ReflectiveOperationException" ?
- your reviewer is a bit on the autistic side, so please change /** to /*
on the @test comment (it's not javadoc!)


On Tue, Dec 11, 2018 at 11:06 AM Michal Vala  wrote:


Hi,

I've been looking into this bug in HashMap where resize() is called
multiple
times when putting whole map into another.

I come up with following patch:
http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8210280/webrev.00/

I've tried to do it as little invasive as possible. New resize(int) method
is
called just from putMapEntries for now. Notice that method is called with
size
of the new map. I was experimenting with calling it with 'size()+s', but
this
leads to unwanted space allocation when inserted map rewrites a lot of
existing
keys.

I've benchmarked this with adding 10millions elements into existing map
and it
gets ~50% improvement:

unpatched
BenchmarkMode  Cnt  Score   Error  Units
MyBenchmark.testMethod  thrpt   10  1.248 ± 0.058  ops/s

patched
BenchmarkMode  Cnt  Score   Error  Units
MyBenchmark.testMethod  thrpt   10  1.872 ± 0.109  ops/s

public class MyBenchmark {
  static Map mapTocopy = IntStream.range(1,
10_000_000).boxed().collect(Collectors.toMap(k -> k,
  k -> k));

  @Benchmark
  public int testMethod() {
  var map = new HashMap();
  map.put(-5, -5);
  map.putAll(mapTocopy);
  return map.size();
  }
}

Any ideas for missed corner-cases or some good tests?

--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

2018-12-11 Thread Zheka Kozlov

Hi Sergey.

`n` and `element` are final fields. Extracting it into local vars will not
make any difference.

ср, 12 дек. 2018 г. в 02:25, Сергей Цыпанов :

> Hi Zheka and Tagir,
>
> @Override
> public void forEach(final Consumer action) {
>   Objects.requireNonNull(action);
>   for (int i = 0; i < this.n; i++) {
> action.accept(this.element);
>   }
> }
>
> I think here we should extract this.element and this.n into a local vars,
> as Consumer may have interaction with volatile field inside (e.g.
> AtomicInteger::addAndGet) which would cause a load of consumed field at
> each iteration. See original post by Nitsan Wakart
> https://psy-lob-saw.blogspot.com/2014/08/the-volatile-read-suprise.html
>
> Regards,
> Sergey Tsypanov
>
>
> 07.12.2018, 15:22, "Zheka Kozlov" :
> > What about forEach()?
> >
> > @Override
> > public void forEach(final Consumer action) {
> > Objects.requireNonNull(action);
> > for (int i = 0; i < this.n; i++) {
> > action.accept(this.element);
> > }
> > }
> >
> > I think this version has better chances to be faster than the default
> > implementation (did not measure though).
> >
> > вт, 4 дек. 2018 г. в 14:40, Tagir Valeev :
> >
> >>  Hello!
> >>
> >>  > In CopiesList.equals() please use eq() instead of Objects.equal()
> (see a
> >>  comment at the line 5345).
> >>
> >>  Ok
> >>
> >>  > I think you should use iterator() instead of listIterator(). See the
> >>  explanation here:
> >>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052472.html
> >>
> >>  Ok. I wonder why this message received no attention.
> >>
> >>  Here's updated webrev:
> >>  http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r3/
> >>
> >>  With best regards,
> >>  Tagir Valeev
> >>  On Tue, Dec 4, 2018 at 1:10 PM Zheka Kozlov 
> wrote:
> >>  >
> >>  > I think you should use iterator() instead of listIterator(). See the
> >>  explanation here:
> >>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052472.html
> >>  >
> >>  > вт, 4 дек. 2018 г. в 12:23, Tagir Valeev :
> >>  >>
> >>  >> Hello!
> >>  >>
> >>  >> Thank you for your comments!
> >>  >>
> >>  >> Yes, deserialization will be broken if we assume that size is never
> 0.
> >>  >> Also we'll introduce referential identity Collections.nCopies(0, x)
> ==
> >>  >> Collections.nCopies(0, y) which might introduce slight semantics
> >>  >> change in existing programs. Once I suggested to wire
> Arrays.asList()
> >>  >> (with no args) to Collections.emptyList(), but it was rejected for
> the
> >>  >> same reason: no need to introduce a risk of possible semantics
> change.
> >>  >>
> >>  >> I updated webrev with equals implementation and test:
> >>  >> http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r2/
> >>  >> Comparing two CopiesList is much faster now indeed. Also we can
> spare
> >>  >> an iterator in the common case and hoist the null-check out of the
> >>  >> loop. Not sure whether we can rely that JIT will always do this for
> >>  >> us, but if you think that it's unnecessary, I can merge the loops
> >>  >> back. Note that now copiesList.equals(arrayList) could be faster
> than
> >>  >> arrayList.equals(copiesList). I don't think it's an issue. On the
> >>  >> other hand we could keep simpler and delegate to
> super-implementation
> >>  >> if the other object is not a CopiesList (like it's implemented in
> >>  >> java.util.RegularEnumSet::equals for example). What do you think?
> >>  >>
> >>  >> With best regards,
> >>  >> Tagir Valeev.
> >>  >> On Tue, Dec 4, 2018 at 10:56 AM Stuart Marks <
> stuart.ma...@oracle.com>
> >>  wrote:
> >>  >> >
> >>  >> >
> >>  >> > >> I believe it makes sense to override CopiesList.equals()
> >>  >> > > Also: contains(), iterator(), listIterator()
> >>  >> >
> >>  >> > equals(): sure
> >>  >> >
> >>  >> > contains() is already overridden. Not sure there's much benefit to
> >>  overriding
> >>  >> > the iterators.
> >>  >> >
> >>  >> > s'marks
>

Re: JDK-8210280 - Unnecessary reallocation when invoking HashMap.putAll()

Hi Michal, pleased to meet you.  I'll be your sponsor.

The test will need a legal header, presumably similar to others authored by
redhatters.

It is now possible to check in jmh microbenchmarks (but I've never done so
myself).

The current coding style is non-standard, but deliberate; avoid gratuitous
changes like
-Node e;
+Node e;

As author of concurrent/ConcurrentHashMap/WhiteBox.java ...
- I thought you'd need a @modules ... did this test actually pass?
- you should have some kind of @summary that includes the word "whitebox"
- why not "throws ReflectiveOperationException" ?
- your reviewer is a bit on the autistic side, so please change /** to /*
on the @test comment (it's not javadoc!)


On Tue, Dec 11, 2018 at 11:06 AM Michal Vala  wrote:

> Hi,
>
> I've been looking into this bug in HashMap where resize() is called
> multiple
> times when putting whole map into another.
>
> I come up with following patch:
> http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8210280/webrev.00/
>
> I've tried to do it as little invasive as possible. New resize(int) method
> is
> called just from putMapEntries for now. Notice that method is called with
> size
> of the new map. I was experimenting with calling it with 'size()+s', but
> this
> leads to unwanted space allocation when inserted map rewrites a lot of
> existing
> keys.
>
> I've benchmarked this with adding 10millions elements into existing map
> and it
> gets ~50% improvement:
>
> unpatched
> BenchmarkMode  Cnt  Score   Error  Units
> MyBenchmark.testMethod  thrpt   10  1.248 ± 0.058  ops/s
>
> patched
> BenchmarkMode  Cnt  Score   Error  Units
> MyBenchmark.testMethod  thrpt   10  1.872 ± 0.109  ops/s
>
> public class MyBenchmark {
>  static Map mapTocopy = IntStream.range(1,
> 10_000_000).boxed().collect(Collectors.toMap(k -> k,
>  k -> k));
>
>  @Benchmark
>  public int testMethod() {
>  var map = new HashMap();
>  map.put(-5, -5);
>  map.putAll(mapTocopy);
>  return map.size();
>  }
> }
>
> Any ideas for missed corner-cases or some good tests?
>
> --
> Michal Vala
> OpenJDK QE
> Red Hat Czech
>

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

2018-12-11 Thread Stuart Marks





On 12/11/18 1:21 PM, Vincent Ryan wrote:
My preference is for PrintStream rather than Writer, for the same reason as 
Roger: it’s more convenient

for handling System.out. Does that address your concern?


PrintStream is fine with me.

I cannot simply cast 8859-1 characters into UTF-8 because UTF-8 is multi-byte 
charset so some 0x8X characters
Will trigger the multi-byte sequence and will end up being misinterpreted. Hence 
my rather awkward conversion to a String.

Is there a better way?


In toPrintableString(),

 259 StringBuilder printable = new StringBuilder(toIndex - fromIndex);
 260 for (int i = fromIndex; i < toIndex; i++) {
 261 if (bytes[i] > 0x1F && bytes[i] < 0x7F) {
 262 printable.append((char) bytes[i]);
 263 } else if (bytes[i] > (byte)0x9F && bytes[i] <= (byte)0xFF) {
 264 printable.append(new String(new byte[]{bytes[i]}, 
ISO_8859_1));

 265
 266 } else {
 267 printable.append('.');
 268 }
 269 }

It works to cast ASCII bytes char, because the 7-bit ASCII range overlaps the 
low 7 bits of the UTF-16 char range. The bytes values of ISO 8859-1 overlap the 
low 8 bits of UTF-16, so casts work for them too.


For any other charset, you'd need to do codeset conversion. But you're cleverly 
supporting only ISO 8859-1, so you don't have to do any conversion. :-)



I’m not sure I’ve addressed your concern regarding IOExceptions - can you 
elaborate?


Taking out the OutputStream overloads addressed my concerns. In at least one 
case the code would wrap the OutputStream into a PrintStream, print stuff to it, 
and then throw away the PrintStream. If an output error occurred, any error 
state in the PrintStream would also be thrown away. The creation of the 
PrintStream wrapper would also use the system's default charset instead of 
letting the caller control it.


The dump() overloads now all take PrintStream, so it's the caller's 
responsibility to ensure that the PrintStream is using the right charset and to 
check for errors after. So this is all OK now.


Note that the internal getPrintStream(), to wrap an OutputStream in a 
PrintStream, is now obsolete and can be removed.


(Oh, I see Roger has said much the same things. Oh well, the peril of parallel 
reviews.)


**


BTW updated webrev/javadoc available:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html


Now we have a somewhat unsatisfying asymmetry in the APIs.

There are four kinds of inputs:

1. byte[]
2. byte[] subrange
3. InputStream
4. ByteBuffer

and two kinds of outputs:

1. PrintStream
2. Stream

and two variations of formatters:

1. default formatter
2. custom formatter + chunk size

This is a total of 16 combinations. But there are only eight methods: three 
PrintStream methods with choice of input, two stream-output methods using the 
default formatter, and three stream-output methods using custom chunk+formatter.


You don't have to provide ALL combinations, but what's here is an odd subset 
with some apparently arbitrary choices. For example, if I have a ByteBuffer and 
I want to dump it to System.out using default formatting, I have to go the 
Stream.forEachOrdered route AND provide the default chunk size and formatter.


HexFormat.dumpAsStream(buf, DEFAULT_CHUNK_SIZE, HEXDUMP_FORMATTER)
 .forEachOrdered(System.out::println);

These aren't huge deals, but they're easily stumbled over.

One approach to organizing this is to have a HexFormat instance that contains 
the setup information and then to have instance methods that either update the 
setup or perform conversion and output. I'd use static factory methods instead 
of constructors. For example, you could do this:


static factories methods:
 - from(byte[])
 - from(byte[], fromIndex, toIndex)
 - from(InputStream)
 - from(ByteBuffer)

formatter setup instance methods:
 - format(chunksize, formatter)

output:
 - void dump(PrintStream)
 - Stream stream()

Using this approach my example from above could be performed as follows:

   HexFormat.from(buf).dump(System.out);

Note, I'm not saying that you HAVE to do it this way. (In particular, the naming 
could use work.) This is quite possibly overkill. But it's something you might 
consider, as it gets you all 16 combinations using seven methods, compared to 
the eight static methods in the current proposal that cover only half of the 
combinations.


Alternatively, pare down the set of static methods to a bare minimum. Provide 
one that can do everything, and then provide one or two more that are 
essentially the same as the first but with some hardwired defaults. For example, 
to help minimize things, you can wrap a ByteBuffer around a byte array subrange, 
or get an InputStream from a byte array subrange. But you can't get an 
InputStream from a ByteBuffer or vice-versa, without a lot of

Re: RFR: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace

2018-12-11 Thread Gustavo Romero


Hi Michi,

On 12/11/2018 11:12 PM, Michihiro Horie wrote:

Thank you for finding the issue on Power8. You do not need a check with 
has_darn in the ppc.ad. It is better to add a check in vm_versoin_ppc.


I agree.


I uploaded webrev.08 based on your webrev.07. (Thanks for the enhancement of 
opto assembly and removing trailing spaces!)
http://cr.openjdk.java.net/~mhorie/8213754/webrev.08/ 



Thanks for the updated webrev. Looks good!
I've just pushed webrev.08 to jdk/submit expecting no failures as .07 passed 
fine.

Once I get the jdk/submit results tomorrow I'll push.

Best regards,
Gustavo



Best regards,
--
Michihiro,
IBM Research - Tokyo

Inactive hide details for "Gustavo Romero" ---2018/12/12 07:57:21---From: "Gustavo Romero"  To: 
"Do"Gustavo Romero" ---2018/12/12 07:57:21---From: "Gustavo Romero"  To: "Doerr, 
Martin" , Michihiro Horie/Japan/IBM@IBMJP

From: "Gustavo Romero" 
To: "Doerr, Martin" , Michihiro Horie/Japan/IBM@IBMJP
Cc: "core-libs-dev@openjdk.java.net" , "hotspot-compiler-...@openjdk.java.net" 
, "Roger Riggs" , "Vladimir Kozlov" 
, "Simonis, Volker" 
Date: 2018/12/12 07:57
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for 
isDigit/isLowerCase/isUpperCase/isWhitespace

--



Hi Michi and Martin,

On 12/11/2018 01:30 PM, Doerr, Martin wrote:
 > thanks for updating. I think it can get shipped when Gustavo is fine with it 
and jdk/submit tests have passed.

webrev.06 removed has_darn from match_rule_supported and the JVM now crashes
with SIGILL (cmprb) on CPUs <= POWER8. I think that what we want is both
UseCharacterCompareIntrinsics and has_darn in the predicate, so:

diff -r 01b519fcb8a8 -r f3bd7a422a6c src/hotspot/cpu/ppc/ppc.ad
--- a/src/hotspot/cpu/ppc/ppc.ad        Tue Dec 11 11:01:02 2018 -0500
+++ b/src/hotspot/cpu/ppc/ppc.ad        Tue Dec 11 12:29:41 2018 -0500
@@ -2257,6 +2257,11 @@
      return SuperwordUseVSX;
    case Op_PopCountVI:
      return (SuperwordUseVSX && UsePopCountInstruction);
+  case Op_Digit:
+  case Op_LowerCase:
+  case Op_UpperCase:
+  case Op_Whitespace:
+    return (UseCharacterCompareIntrinsics && VM_Version::has_darn());
    }

Martin, is what you meant on your last comment about it?

I tested the change on POWER9 and it looks good (from webrev.06 and also with
webrev.06 + the fix above)!

I think that the Opto assembly could be improved a little bit around 'done:'
label, like:

diff -r 89aab62d10cd src/hotspot/cpu/ppc/ppc.ad
--- a/src/hotspot/cpu/ppc/ppc.ad        Tue Dec 11 12:29:41 2018 -0500
+++ b/src/hotspot/cpu/ppc/ppc.ad        Tue Dec 11 15:55:34 2018 -0500
@@ -12440,7 +12440,8 @@
              "LIS     $src2, (signed short)0xAAB5\n\t"
              "ORI     $src2, $src2, 0xBABA\n\t"
              "INSRDI  $src2, $src2, 32, 0\n\t"
-            "CMPEQB  $crx, 1, $src1, $src2\n\t"
+            "CMPEQB  $crx, 1, $src1, $src2\n"
+            "done:\n\t"
              "SETB    $dst, $crx" %}

    size(48);
@@ -12484,7 +12485,8 @@
              "BGT     $crx, done\n\t"
              "LIS     $src2, (signed short)0xD6C0\n\t"
              "ORI     $src2, $src2, 0xDED8\n\t"
-            "CMPRB   $crx, 1, $src1, $src2\n\t"
+            "CMPRB   $crx, 1, $src1, $src2\n"
+            "done:\n\t"
              "SETB    $dst, $crx" %}

so the output would be:

024   B2: #     B5 B3 <- B1  Freq: 1
024     LI      R14, 0x5A41
         CMPRB   CCR6, 0, R3, R14
         BGT     CCR6, done
         LIS     R14, (signed short)0xD6C0
         ORI     R14, R14, 0xDED8
         CMPRB   CCR6, 1, R3, R14
done:
         SETB    R15, CCR6
040     CMPWI   CCR6, R15, #0
044     Beq     CCR6, B5  P=0.00 C=5377.00

instead of:

024   B2: #     B5 B3 <- B1  Freq: 1
024     LI      R14, 0x5A41
         CMPRB   CCR6, 0, R3, R14
         BGT     CCR6, done
         LIS     R14, (signed short)0xD6C0
         ORI     R14, R14, 0xDED8
         CMPRB   CCR6, 1, R3, R14
         SETB    R15, CCR6
040     CMPWI   CCR6, R15, #0
044     Beq

Re: JDK 13 RFR of core libs portions of JDK-8205626: Start of release updates for JDK 13

2018-12-11 Thread Joseph D. Darcy


Hi Andrew,

Way back when JSR 269 which added the javax.lang.model API was running, 
we did consider having a "LATEST" enum constant that would be an alias 
for the most current release. In the end, we decided against this 
approach and use a pair of "latest" methods on the SourceVersion enum to 
provide analogous functionality.


The use of an enum value in an annotation has to be a compile-time 
constant, which for enum-typed methods means the enum value has to be 
referred directly as an enum constant, a final or static final variable 
is not enough (JLS 9.7.1. Normal Annotations).


Therefore,

public static final CURRENT_RELEASE = SourceVersion.RELEASE_13;

would *not* allow

   @SupportedSourceVersion(SourceVersion.CURRENT_RELEASE)

to be used.

Thanks,

-Joe

On 12/7/2018 6:18 PM, Andrew Luo wrote:

Not a reviewer - looks good anyways - however, I would if some of those 
constants be refactored to fewer locations?

For example, I see many copies of:

@SupportedSourceVersion(SourceVersion.RELEASE_13)

Perhaps if you declare SourceVersion as:

RELEASE_12,
RELEASE_13,
CURRENT_RELEASE = RELEASE_13;

Then you could use:

@SupportedSourceVersion(SourceVersion.CURRENT_RELEASE)

Thanks,

-Andrew

-Original Message-
From: core-libs-dev  On Behalf Of joe 
darcy
Sent: Friday, December 7, 2018 9:52 AM
To: Alan Bateman ; Core-Libs-Dev 

Subject: Re: JDK 13 RFR of core libs portions of JDK-8205626: Start of release 
updates for JDK 13

Hi Alan,

On 12/7/2018 12:16 AM, Alan Bateman wrote:

On 07/12/2018 02:31, Joseph D. Darcy wrote:

Hello,

With the start of JDK 13 around the corner, please review the core
libs portions of:

 JDK-8205626: Start of release updates for JDK 13
 http://cr.openjdk.java.net/~darcy/jdk13-fork.2



[snip]


Looks okay (and the same as what we reviewed on build-dev yesterday?).


Yes, same changes as on build-dev (other than the update of several more 
TEST.ROOT files to require jtreg 4.2 b13 rather than b12.)

I wanted to have a bit more of the testing complete before sending the core 
libs portion out for the review.

Thanks,

-Joe

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Stuart Marks





On 12/11/18 1:52 PM, mark.reinh...@oracle.com wrote:

2018/12/11 7:03:57 -0800, adam.far...@uk.ibm.com:

I've spotted 12 instances of swear words in OpenJDK source comments, and
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217


(webrev: http://cr.openjdk.java.net/~afarley/8215217/webrev/)

I can certainly see removing the f-word, and other words of a sexual
nature.  Those are clearly inappropriate.

Removing lesser words, and continuing to police their use henceforth,
strikes me as overkill.

What do other Committers think?


I think it is faintly ridiculous to consider words like "crap" and "damn" as 
vulgarities to be removed from source code. None of my dictionaries describe 
"crap" or "damn" as vulgar, while they do describe other words as vulgar.


The "Where's that damn torpedo?" quote came from "Star Trek VI", which was rated 
PG. [1] In the MPAA rating system, PG is the second-least restrictive rating, 
above only the all-ages G rating. More-restrictive ratings include PG-13, R, and 
NC-17. [2] The word "damn" or a variant occurs about five times in that movie. [3]


The MPAA takes language and swearing very seriously, with fairly strict 
guidelines on the number of f-bombs and their context to determine a rating of 
PG-13 or R. [4] Film producers will edit very carefully in order to achieve a 
particular rating. [5]


Given that the producers of "The Martian" had to edit carefully in order to 
preserve its PG-13 rating, whereas "Star Trek VI" and its five "damns" sailed 
through with a PG rating, I think it's safe to say that the notoriously Puritan 
MPAA [6] doesn't concern itself with the word "damn." Neither should we.


**

Adam,

Starting from your patch, I've removed changes relating to "crap" and "damn" and 
the changes to upstream jszip.js. This leaves the patches appended below. The 
SoftChannel.java change is most likely a typo that should be fixed. The 
BitArray.java change is part of Xalan. We don't actually keep our sources in 
sync with Xalan, but I note that upstream [7] has made the same change, so we 
might as well change it too.


Would you like me to sponsor this change for you?

s'marks

[1] https://www.imdb.com/title/tt0102975/

[2] https://filmratings.com/Tips

[3] http://www.chakoteya.net/movies/movie6.html

[4] https://filmratings.com/Content/Downloads/rating_rules.pdf

[5] https://www.polygon.com/2015/10/22/9592366/The-martian-rating-fuck [warning, 
language]


[6] No link, but a web search for "mpaa puritanism" will reveal an unbounded 
number of articles railing against the arbitrary moralizing imposed by the MPAA 
via its ratings system.


[7] 
http://svn.apache.org/viewvc/xalan/java/tags/xalan-j_2_7_2/src/org/apache/xalan/xsltc/dom/BitArray.java?view=markup


**


# HG changeset patch
# User afarley
# Date 1544574289 28800
#  Tue Dec 11 16:24:49 2018 -0800
# Node ID 0c40c78b6d139eb05b0718d0b524a465419ee9cb
# Parent  b75a44aad06cd93c823159265a1f200bf0ce390c
8215217: OpenJDK Source Has Too Many Swear Words

diff -r b75a44aad06c -r 0c40c78b6d13 
src/java.desktop/share/classes/com/sun/media/sound/SoftChannel.java
--- a/src/java.desktop/share/classes/com/sun/media/sound/SoftChannel.java	Tue 
Dec 11 13:10:14 2018 -0800
+++ b/src/java.desktop/share/classes/com/sun/media/sound/SoftChannel.java	Tue 
Dec 11 16:24:49 2018 -0800

@@ -1472,7 +1472,7 @@
 }
 for (int controller : co_midi_nrpn_nrpn.keySet())
 nrpnChange(controller, 0);
-rpnChange(0, 2 << 7);   // Bitch Bend sensitivity
+rpnChange(0, 2 << 7);   // Pitch Bend sensitivity
 rpnChange(1, 64 << 7);  // Channel fine tunning
 rpnChange(2, 64 << 7);  // Channel Coarse Tuning
 rpnChange(5, 64);   // Modulation Depth, +/- 50 cent
diff -r b75a44aad06c -r 0c40c78b6d13 
src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/BitArray.java
--- 
a/src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/BitArray.java 
Tue Dec 11 13:10:14 2018 -0800
+++ 
b/src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/BitArray.java 
Tue Dec 11 16:24:49 2018 -0800

@@ -133,7 +133,7 @@
  * This method returns the Nth bit that is set in the bit array. The
  * current position is cached in the following 4 variables and will
  * help speed up a sequence of next() call in an index iterator. This
- * method is a mess, but it is fast and it works, so don't fuck with it.
+ * method is a mess, but it is fast and it works, so don't change it.
  */
 private int _pos = Integer.MAX_VALUE;
 private int _node = 0;

Re: Review Request JDK-8215238: (jdeps) update jdk8_internals.txt per the removal of javafx, corba, EE modules

2018-12-11 Thread Lance Andersen

+1
> On Dec 11, 2018, at 6:57 PM, Mandy Chung  wrote:
> 
> Several modules including Java EE and CORBA modules [1] and
> JavaFX and a few other modules have been removed from JDK.
> 
> jdeps maintains a list of JDK 8 internal API packages that
> `jdeps -jdkinternals` can properly flag that a reference to
> JDK 8 internal API that was renamed or removed while the
> feature is still supported.  For these removed modules,
> their internal API packages should be handled as non-JDK
> API and reports missing dependency if not found.  The patch
> is simple and the main change is to remove them from the
> `jdk8_internals.txt` resource file.
> 
> Webrev:
> http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8215238/webrev.00/
> 
> Thanks
> Mandy
> [1] http://openjdk.java.net/jeps/320

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Review Request JDK-8215238: (jdeps) update jdk8_internals.txt per the removal of javafx, corba, EE modules

2018-12-11 Thread Mandy Chung


Several modules including Java EE and CORBA modules [1] and
JavaFX and a few other modules have been removed from JDK.

jdeps maintains a list of JDK 8 internal API packages that
`jdeps -jdkinternals` can properly flag that a reference to
JDK 8 internal API that was renamed or removed while the
feature is still supported.  For these removed modules,
their internal API packages should be handled as non-JDK
API and reports missing dependency if not found.  The patch
is simple and the main change is to remove them from the
`jdk8_internals.txt` resource file.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8215238/webrev.00/

Thanks
Mandy
[1] http://openjdk.java.net/jeps/320

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread joe darcy


On 12/11/2018 1:52 PM, mark.reinh...@oracle.com wrote:

2018/12/11 7:03:57 -0800, adam.far...@uk.ibm.com:

I've spotted 12 instances of swear words in OpenJDK source comments, and
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

(webrev: http://cr.openjdk.java.net/~afarley/8215217/webrev/)


I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in more
excusable locations. It would be good to get the community's take on
those.

It also would be good to discuss the instances that you’ve proposed
to change in your patch.

I can certainly see removing the f-word, and other words of a sexual
nature.  Those are clearly inappropriate.

Removing lesser words, and continuing to police their use henceforth,
strikes me as overkill.

What do other Committers think?


I don't think our sensibilities need be so delicate as to be compelled 
to purge mild exclamations from test files.


As noted elsewhere on this thread, the words altered in the proposed 
patch which intersect with the words in George Carin's list of "Seven 
Words You Can Never Say on Television," originate in up-stream sources 
and would be better addressed there.


-Joe

Re: RFR: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace

2018-12-11 Thread Gustavo Romero


Hi Michi and Martin,

On 12/11/2018 01:30 PM, Doerr, Martin wrote:

thanks for updating. I think it can get shipped when Gustavo is fine with it 
and jdk/submit tests have passed.


webrev.06 removed has_darn from match_rule_supported and the JVM now crashes
with SIGILL (cmprb) on CPUs <= POWER8. I think that what we want is both
UseCharacterCompareIntrinsics and has_darn in the predicate, so:

diff -r 01b519fcb8a8 -r f3bd7a422a6c src/hotspot/cpu/ppc/ppc.ad
--- a/src/hotspot/cpu/ppc/ppc.adTue Dec 11 11:01:02 2018 -0500
+++ b/src/hotspot/cpu/ppc/ppc.adTue Dec 11 12:29:41 2018 -0500
@@ -2257,6 +2257,11 @@
 return SuperwordUseVSX;
   case Op_PopCountVI:
 return (SuperwordUseVSX && UsePopCountInstruction);
+  case Op_Digit:
+  case Op_LowerCase:
+  case Op_UpperCase:
+  case Op_Whitespace:
+return (UseCharacterCompareIntrinsics && VM_Version::has_darn());
   }
  
Martin, is what you meant on your last comment about it?


I tested the change on POWER9 and it looks good (from webrev.06 and also with
webrev.06 + the fix above)!

I think that the Opto assembly could be improved a little bit around 'done:'
label, like:

diff -r 89aab62d10cd src/hotspot/cpu/ppc/ppc.ad
--- a/src/hotspot/cpu/ppc/ppc.adTue Dec 11 12:29:41 2018 -0500
+++ b/src/hotspot/cpu/ppc/ppc.adTue Dec 11 15:55:34 2018 -0500
@@ -12440,7 +12440,8 @@
 "LIS $src2, (signed short)0xAAB5\n\t"
 "ORI $src2, $src2, 0xBABA\n\t"
 "INSRDI  $src2, $src2, 32, 0\n\t"
-"CMPEQB  $crx, 1, $src1, $src2\n\t"
+"CMPEQB  $crx, 1, $src1, $src2\n"
+"done:\n\t"
 "SETB$dst, $crx" %}
 
   size(48);

@@ -12484,7 +12485,8 @@
 "BGT $crx, done\n\t"
 "LIS $src2, (signed short)0xD6C0\n\t"
 "ORI $src2, $src2, 0xDED8\n\t"
-"CMPRB   $crx, 1, $src1, $src2\n\t"
+"CMPRB   $crx, 1, $src1, $src2\n"
+"done:\n\t"
 "SETB$dst, $crx" %}

so the output would be:
 
024   B2: # B5 B3 <- B1  Freq: 1

024 LI  R14, 0x5A41
CMPRB   CCR6, 0, R3, R14
BGT CCR6, done
LIS R14, (signed short)0xD6C0
ORI R14, R14, 0xDED8
CMPRB   CCR6, 1, R3, R14
done:
SETBR15, CCR6
040 CMPWI   CCR6, R15, #0
044 Beq CCR6, B5  P=0.00 C=5377.00

instead of:

024   B2: # B5 B3 <- B1  Freq: 1
024 LI  R14, 0x5A41
CMPRB   CCR6, 0, R3, R14
BGT CCR6, done
LIS R14, (signed short)0xD6C0
ORI R14, R14, 0xDED8
CMPRB   CCR6, 1, R3, R14
SETBR15, CCR6
040 CMPWI   CCR6, R15, #0
044 Beq CCR6, B5  P=0.00 C=5379.00

If nobody opposes to that tiny enhancement I would like to keep it.

... and a nit: several trailing spaces here and there :)

I generated a webrev with has_darn in match_rule_supported, with the change to
the Opto asm, and with the trailing spaces removed. I uploaded it to:

http://cr.openjdk.java.net/~gromero/8213754/webrev.07/

I pushed that same version to jdk/submit [1] not expecting any failure and once
I get the fine results and if you, Martin are ok with the version in webrev.07
I'll push it to jdk/jdk.

I'll update this thread a soon as I get the results back from jdk/submit repo.
I think we have time until Thurday 16:00 UTC, even taking into account the
3+ different timezones working on that change :)

Thank you and best regards,
Gustavo

[1] 
http://mail.openjdk.java.net/pipermail/jdk-submit-changes/2018-December/004418.html


Note that changes pushed before Thursday 16:00 UTC will go into jdk12.

Best regards,

Martin

*From:*Michihiro Horie 
*Sent:* Dienstag, 11. Dezember 2018 14:08
*To:* Doerr, Martin 
*Cc:* core-libs-dev@openjdk.java.net; Gustavo Romero ; 
hotspot-compiler-...@openjdk.java.net; Roger Riggs ; Vladimir Kozlov 
; Simonis, Volker 
*Subject:* RE: RFR: 8213754: PPC64: Add Intrinsics for 
isDigit/isLowerCase/isUpperCase/isWhitespace

Hi Martin,

Thank you so much for your review and pointing out the issue on flag enablement 
on PPC64.

Latest webrev enables the flag on PPC64 using has_darn in vm_version_ppc and it 
is used in match_rule_supported in the ad file.
http://cr.openjdk.java.net/~mhorie/8213754/webrev.06/ 



Best regards,
--
Michihiro,
IBM Research - Tokyo

Inactive hide details for "Doerr, Martin" ---2018/12/11 20:31:40---Hi Michihiro, the 
shared code looks good to me, now."Doerr, Martin" ---2018/12/11 20:31:40---Hi Michihiro, 
the shared code looks good to me, now.

From: "Doerr, Martin" mailto:martin.do...@sap.com>>
To: Vladimir Kozlov mailto:vladimir.koz...@oracle.com>>, Michihiro Horie 
mailto:ho...@jp.ibm.com>>, Gustavo Romero mailto:grom...@linux.vnet.ibm.com>>
Cc: "core-libs-dev@openjdk.java.net " mailto:core-libs-dev@openjdk.java.net>>,

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

2018-12-11 Thread David Holmes


On 12/12/2018 12:34 am, Magnus Ihse Bursie wrote:



On 2018-12-11 00:23, David Holmes wrote:

Hi Magnus,

On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK native 
files (Hotspot included), called JDK_EXPORT. The behavior of this 
symbol will be very similar (as of now, in fact identical) to 
JNIEXPORT; however, the semantics will not.


Currently, we "mis-use" the JNIEXPORT define to mark a function for 
exporting from the library. The problem with this is that JNIEXPORT 
is part of the JNI interface, and is supposed to be used when C 
programs interact with Java. And, when doing this, the function 
should be fully decorated like this: "JNIEXPORT foo JNICALL".


I've seen a lot of the emails on this issue and I don't fully 
understand what has been going wrong. But the intent is obviously the 
JNIEXPORT represents what is needed to export this function for use by 
JNI, while JNICALL defines the calling convention. I agree there may 
be some mistmatch when functions are actually not intended for general 
export outside the JDK but are only for internal JDK use.


We do have many such JNI exports in our native libraries, but we also 
have a lot of other, non-JNI exports, where one native library just 
provides an interface to other libraries. In these cases, we have 
still used JNIEXPORT for the functionality of getting the function 
exported, but we have not been consistent in our use of JNICALL. This 
has caused us way too much trouble for something that should Just 
Work.


Are you suggesting that the interface between different libraries in 
the JDK should not be a JNI interface? Is this because you think the 
functions in these libraries are only for JDK internal use or ... ??


I therefore propose that we define "JDK_EXPORT", with the same 
behavior as JNIEXPORT (that is, flagging the function for external 
visibility in the resulting native library), but which is *not* 
supposed to be exported to Java code using JNI, nor supposed to be 
decorated with 


Just a clarification there. JNI functions are not exported to Java 
code, they are exported to native code. Java code can declare native 
methods and those native methods must be written as JNI functions, but 
that's not what we are discussing. Libraries expose a JNI interface (a 
set of functions in the library) that can be called by application 
native code, using JNI.
We're apparently looking at "JNI" and "exporting" from two opposite 
sides here. :-) Just to make everything clear: If I have a Java class

class MyClass {
   public static void native myNativeFunc();
}

then I have one half of the JNI function, the Java half. This must be 
matched by a corresponding implementation in native, like this:

JNIEXPORT void JNICALL
Java_MyClass_myNativeFunc(void) {
// ... do stuff
}

And this is the native half of the JNI function. Right? Let's leave 
aside which side is "exporting" to the other for now. :-)


This way of setting up native functions that can be called from Java is 
what I refer to as JNI. And when you declare a native JNI function, you 
*must* use both JNIEXPORT and JNICALL. Alright?


We do have a lot of those functions in our native libraries. And they 
are correct just the way they are.


Yep all well and good. A function declared native in Java must have an 
implementation as you describe. But not all native functions exist to 
provide the native-half of a Java native function!


However, we also have *other* native functions, that are exported, not 
as JNI functions that should be called from Java, but as normal native 
library functions that should be called by other native code. Okay so 
far? And *those* functions have been problematic in how we decorate 


But there are again two cases. Those functions exported from a library 
that are expected to be called from external code using the JNI 
interface mechanism - such as all the JNI functions and JVM TI functions 
we export from the JVM - and those "exported" for access between 
libraries within the JDK (such as all the JVM_* functions in libjvm).


I think it is only the second group that should be addressed by your 
JDK_EXPORT proposal - though I'm not completely clear exactly how to 
identify them.


them. My proposal is that we *refrain* from using JNIEXPORT for those 
functions, and instead use JDK_EXPORT as name for the macro that 
decorates them as exported. That way, we can clearly see that a function 
like this:


JDK_EXPORT void
JLI_ReadEnv(char* env);

is correctly declared, and will be exported to other native libraries, 
but not to Java.


The issue is not whether it is "exported to Java"** but whether it is 
exported using the JNI mechanism such that other native code calls it 
using the JNI mechanism.


** There is no way to write a native method declaration in Java such 
that it would be linked to the JLI_ReadEnv function. The naming is all 
wrong, as is the signature.


Just to clarify, this has

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Claes,

I don't see any particular reason for that. In fact, I was wondering the 
same thing, but just left it as it was. Since I've already pushed the 
changeset, I will fix it on the next occasion.


Naoto

On 12/11/18 2:31 PM, Claes Redestad wrote:

Hi Naoto,

while the here fix looks good, I can't help wondering why the map isn't
final?

/Claes

On 2018-12-11 19:59, Naoto Sato wrote:

Hi Ivan,

Thank you for the review.

On 12/11/18 10:49 AM, Ivan Gerasimov wrote:
OptimalCapacity.ofHashMap uses the initialCapacity to verify that a 
HashMap created with that initialCapacity will not reallocate its 
internal array after inserting the factual number of elements.


So, if one day the number of entities is increased, but the constant 
NUM_ENTITIES is not updated, then the test will catch it.


Actually the reason I am fixing it is that, the test did not detect 
the discrepancy with Unicode 11 upgrade, where it just increased the 
entry numbers by the true block increases (11 blocks), without adding 
their aliases.


---

--- a/src/java.base/share/classes/java/lang/Character.java    Wed Nov 
14 13:15:54 2018 +0100
+++ b/src/java.base/share/classes/java/lang/Character.java    Wed Nov 
21 14:24:31 2018 +0530

@@ -43,7 +43,7 @@
   * a character's category (lowercase letter, digit, etc.) and for 
converting

   * characters from uppercase to lowercase and vice versa.
   * 
- * Character information is based on the Unicode Standard, version 
10.0.0.
+ * Character information is based on the Unicode Standard, version 
11.0.0.

   * 
   * The methods and data of class {@code Character} are defined by
   * the information in the UnicodeData file that is part of the
@@ -681,11 +681,11 @@
   */
  public static final class UnicodeBlock extends Subset {
  /**
- * 638  - the expected number of entities
+ * 649  - the expected number of entities
   * 0.75 - the default load factor of HashMap
   */
  private static Map map =
-    new HashMap<>((int)(638 / 0.75f + 1.0f));
+    new HashMap<>((int)(649 / 0.75f + 1.0f));

  /**
   * Creates a UnicodeBlock with the given identifier name.
---

However, the HashMap allocates the entry size to the nearest power of 
two number, i.e., 1024 for both numbers 649 and 667. Thus the test 
passes even if there is discrepancy.


Naoto

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Naoto,

while the here fix looks good, I can't help wondering why the map isn't
final?

/Claes

On 2018-12-11 19:59, Naoto Sato wrote:

Hi Ivan,

Thank you for the review.

On 12/11/18 10:49 AM, Ivan Gerasimov wrote:
OptimalCapacity.ofHashMap uses the initialCapacity to verify that a 
HashMap created with that initialCapacity will not reallocate its 
internal array after inserting the factual number of elements.


So, if one day the number of entities is increased, but the constant 
NUM_ENTITIES is not updated, then the test will catch it.


Actually the reason I am fixing it is that, the test did not detect the 
discrepancy with Unicode 11 upgrade, where it just increased the entry 
numbers by the true block increases (11 blocks), without adding their 
aliases.


---

--- a/src/java.base/share/classes/java/lang/Character.java    Wed Nov 14 
13:15:54 2018 +0100
+++ b/src/java.base/share/classes/java/lang/Character.java    Wed Nov 21 
14:24:31 2018 +0530

@@ -43,7 +43,7 @@
   * a character's category (lowercase letter, digit, etc.) and for 
converting

   * characters from uppercase to lowercase and vice versa.
   * 
- * Character information is based on the Unicode Standard, version 10.0.0.
+ * Character information is based on the Unicode Standard, version 11.0.0.
   * 
   * The methods and data of class {@code Character} are defined by
   * the information in the UnicodeData file that is part of the
@@ -681,11 +681,11 @@
   */
  public static final class UnicodeBlock extends Subset {
  /**
- * 638  - the expected number of entities
+ * 649  - the expected number of entities
   * 0.75 - the default load factor of HashMap
   */
  private static Map map =
-    new HashMap<>((int)(638 / 0.75f + 1.0f));
+    new HashMap<>((int)(649 / 0.75f + 1.0f));

  /**
   * Creates a UnicodeBlock with the given identifier name.
---

However, the HashMap allocates the entry size to the nearest power of 
two number, i.e., 1024 for both numbers 649 and 667. Thus the test 
passes even if there is discrepancy.


Naoto

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread David Holmes


On 12/12/2018 7:52 am, mark.reinh...@oracle.com wrote:

2018/12/11 7:03:57 -0800, adam.far...@uk.ibm.com:

I've spotted 12 instances of swear words in OpenJDK source comments, and
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217


(webrev: http://cr.openjdk.java.net/~afarley/8215217/webrev/)


I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in more
excusable locations. It would be good to get the community's take on
those.


It also would be good to discuss the instances that you’ve proposed
to change in your patch.

I can certainly see removing the f-word, and other words of a sexual
nature.  Those are clearly inappropriate.

Removing lesser words, and continuing to police their use henceforth,
strikes me as overkill.

What do other Committers think?


I completely agree. I was surprised to see the F-word but then somewhat 
relieved to see it came from external sources!


I'd also advocate, as a matter of technical style, not using emotive 
commentary in the code, and avoiding colloquialisms. That should avoid 
any future issues.


Cheers,
David
-


- Mark

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Jonathan Gibbons




On 12/11/2018 01:52 PM, mark.reinh...@oracle.com wrote:

2018/12/11 7:03:57 -0800, adam.far...@uk.ibm.com:

I've spotted 12 instances of swear words in OpenJDK source comments, and
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

(webrev: http://cr.openjdk.java.net/~afarley/8215217/webrev/)


I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in more
excusable locations. It would be good to get the community's take on
those.

It also would be good to discuss the instances that you’ve proposed
to change in your patch.

I can certainly see removing the f-word, and other words of a sexual
nature.  Those are clearly inappropriate.

Removing lesser words, and continuing to police their use henceforth,
strikes me as overkill.

What do other Committers think?

- Mark


The f-bombs in this file are from an upstream library:

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/jquery/jszip/dist/jszip.js

Are we committing to removing these words every time we update the library?

-- Jon

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread mark . reinhold

2018/12/11 7:03:57 -0800, adam.far...@uk.ibm.com:
> I've spotted 12 instances of swear words in OpenJDK source comments, and 
> it seems appropriate to remove them.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

(webrev: http://cr.openjdk.java.net/~afarley/8215217/webrev/)

> I've created a webrev and attached to the bug.
> 
> Also, I've mentioned in the bug that there are additional swears in more 
> excusable locations. It would be good to get the community's take on 
> those.

It also would be good to discuss the instances that you’ve proposed
to change in your patch.

I can certainly see removing the f-word, and other words of a sexual
nature.  Those are clearly inappropriate.

Removing lesser words, and continuing to police their use henceforth,
strikes me as overkill.

What do other Committers think?

- Mark

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Aleksey Shipilev

On 12/11/18 10:44 PM, David Holmes wrote:
> No issue with fixing F-bomb (though one comes from upstream sources I think) 
> and the Pitch typo, 
> but seriously "damn" is not a swear word.
Exactly my comment as well.

Thanks,
-Aleksey

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

Hi Vincent,

I have no new inclinations about relative vs absolute, maybe someone
else has

use cases that will tip the balance.

HexFormat.java:

- line 264: I think you can safely just append the (char) (byte[i] &
0xff). (to avoid sign extension).

Creating a string for each char seems like a waste.

- 759: The getPrintStream(out) method can be removed since its always a
PrintStream

- 733: The exception containing the entire input hex string might be a
bit daunting.

Can it return just the bad character and perhaps the offset?

- The 'infinite' in the @return of two of the dumpAsStream methods could
be removed.
The bound on the stream depends on the input and its not important to
say its infinite.

Otherwise, looking very good.

Thanks, Roger

On 12/11/2018 04:21 PM, Vincent Ryan wrote:

Thanks Roger.

I believe that all but one of your concerns are addressed in this
latest webrev/javadoc:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/

http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html

The remaining issue is how best to define the dumpAsStream() method
that takes a ByteBuffer.
The current approach has dropped the to/from parameters, consumes all
the buffer and advances

Its position to the buffer limit.
This places the burden on the caller to size the buffer appropriately.

However I also see the merit in a ‘read-only’ approach that does not
advance the buffer position.

Any further thoughts?

On 11 Dec 2018, at 16:54, Roger Riggs > wrote:

Ho Vincent,

On 12/11/2018 11:34 AM, Vincent Ryan wrote:

Responses in-line.

Its really nice/necessary that examples can be copy/pasted and
compile.

- dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be
confusing because it
is using the relative methods of ByteBuffer, but the from and
to offsets are within

the position .. limit buffer. That should be explicit.
(Or consider switching to the absolute accesses in the
ByteBuffer and not affect the position)

Is the additional javadoc line added earlier sufficient?
I would bear some reinforcing that the entire remainder of the
buffer is read

and the from and to are indexes within that remainder.
And I'm not sure that's the desirable semantics.

It would make more sense if the from bytes from the buffer are skipped
and only the (to - from) bytes are dumped. That leaves the ByteBuffer
reading only the requested bytes. A natural usage such as:
dumpAsStream(ByteBuffer buffer, 0, 256, 16, formatter)
would dump the next 256bytes of the ByteBuffer and position would be
moved by 256.

[As suggested by Alan]
How about dropping the fromIndex and toIndex parameters and
requiring the caller

to provide a suitably sized buffer?

public static Stream dumpAsStream(ByteBuffer
buffer, int chunkSize, Formatter formatter) {

I'd go the other direction and make dump use absolute offset and limit.
The current values for position and limit are readily available from
the buffer.

If the dumping code is being added to perform some diagnostics then
it would not
modify the position and not disturb the existing code that is using
the buffer.

Absolute is simpler to explain and implement and has fewer side effects.

- dump(byte[], OutputStream) - I don't think the example is
correct, there is no reference
to a stream, only the PrintStream::println method, which is not
static.

The code is just illustrative. Real values would have to be
supplied for the input bytes and the
chosen print method on the output stream. Typically, that print
method will be System.out::println

Examples that don't compile are really confusing and annoying.

Updated the ‘as if’ code to:

* byte[] bytes = ...
* PrintStream out = ...
* HexFormat.dumpAsStream(bytes, 16,
* (offset, chunk, from, to) ->
* String.format("%08x %s |%s|",
* offset,
* HexFormat.toFormattedString(chunk, from, to),
* HexFormat.toPrintableString(chunk, from, to)))
* .forEachOrdered(out::println);

Looks fine, thanks

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread David Holmes

No issue with fixing F-bomb (though one comes from upstream sources I 
think) and the Pitch typo, but seriously "damn" is not a swear word.


I have to suspect you ran a corporate word checker over the sources.

David
-

damn
/dam/
verb
past participle: damned

1.
(in Christian belief) be condemned by God to suffer eternal 
punishment in hell.

"I treated her badly and I'll be damned to hell for it"
be doomed to misfortune or failure.
verb: damn; 3rd person present: damns; past tense: damned; 
gerund or present participle: damning

"the enterprise was damned"
2.
criticize strongly.
"the book damns her husband"
synonyms:	condemn, censure, criticize, attack, denounce, deplore, 
decry, revile, inveigh against; blame, chastise, castigate, berate, 
upbraid, reprimand, rebuke, reprove, reprehend, take to task, find fault 
with, give someone/something a bad press; deprecate, disparage; 
informal: slam, hammer, lay into, cane, blast; informal: slate, slag 
off, have a go at; archaic: slash, reprobate; rare: excoriate, 
vituperate, arraign, objurgate, anathematize
"we are certainly not going to damn a product just because it is 
non-traditional"

antonyms:   acclaim, praise
curse (someone or something).
"she cleared her throat, damning it for its huskiness"
synonyms:	curse, put a curse on, put the evil eye on, execrate, 
imprecate, hoodoo; More

anathematize, excommunicate;
hex;
informal put a jinx on, jinx;
rare accurse
"if we did not believe in God, we would be damned"
antonyms:   bless

exclamation informal
exclamation: damn

1.
expressing anger or frustration.
"Damn! I completely forgot!"

adjective informal
adjective: damn

1.
used for emphasis, especially to express anger or frustration.
"turn that damn thing off!"

Phrases
—— be damned
used to express defiance or rejection of someone or something previously 
mentioned. "glory be damned!"

damn all
nothing at all. "there's damn all you can do about it"
damn someone/thing with faint praise
praise someone or something so unenthusiastically as to imply 
condemnation. "it was a wretched review, damning poor Lisa with faint 
praise"

I'm damned if
used to express a strong negative. "I'm damned if I know"
not be worth a damn
have no value at all. "your evidence isn't worth a damn"
not give a damn
not care at all. "people who don't give a damn about the environment"
well I'll be damned
used to express surprise. "Well, I'll be damned! What brings you here?"
Origin

On 12/12/2018 2:45 am, Adam Farley8 wrote:

Sure thing:

http://cr.openjdk.java.net/~afarley/8215217/webrev/

Best Regards

Adam Farley
IBM Runtimes


Volker Simonis  wrote on 11/12/2018 15:46:44:


From: Volker Simonis 
To: adam.far...@uk.ibm.com
Cc: Java Core Libs 
Date: 11/12/2018 15:47
Subject: Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

Hi Adam,

in order to prevent me from using swear words, could you please upload
your webrev to cr.openjdk.java.net :)

As you may have realized webrevs are a collection of HTML files and it
makes no big sense to provide them as a zip file.

Thank you and best regards,
Volker
On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8 

wrote:


Hey All,

I've spotted 12 instances of swear words in OpenJDK source comments,

and

it seems appropriate to remove them.

Bug: INVALID URI REMOVED



u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8215217=DwIBaQ=jf_iaSHvJObTbx-

siA1ZOg=P5m8KWUXJf-


CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=GfAb5QlDParO6DVrhdvPZTSafShnFACNF3JgqF-

_RkM=Qscaf2tTpPcZKpIelJ6SrP0uRYSFoKaCNATns0FX7_Y=


I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in

more

excusable locations. It would be good to get the community's take on
those.

Reviews and opinions welcome. :)

Best Regards

Adam Farley
IBM Runtimes

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




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: 8170769 Provide a simple hexdump facility for binary data

Thanks for your review Alan.

I believe I’ve addressed your concerns in this latest webrev/javadoc:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 

http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
 



> On 11 Dec 2018, at 13:04, Alan Bateman  wrote:
> 
> On 08/12/2018 01:18, Vincent Ryan wrote:
>> Here’s the latest version that addresses all the current open issues:
>> 
>> webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/ 
>> 
>> javadoc: 
>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html
>>  
>> 
>> 
>> CSR: https://bugs.openjdk.java.net/browse/CCC-8170769 
>> 
>> 
> I read through the latest javadoc corresponding to webrev.07.
> 
> Overall I think it looks very good except for dumpAsStream(ByteBuffer, 
> fromIndex, toIndex, chunkSize, Formatter) as it's not clear if fromIndex is 
> from the buffer position or an absolute index. If the former then there are a 
> couple of other issues. I see Roger has touched on the advancement of the 
> buffer position to its limit which isn't right unless all remaining bytes in 
> the  buffer are consumer. There is also an issue with the exception as 
> attempting to consume beyond the limit is a BufferUnderflowException. It 
> might be simpler to replace this method with a dumpAsStream(ByteBuffer, 
> chunkSize, Formatter) that can lazily (rather than eagerly) consume the 
> remaining bytes. Additional overloads could be added in the future if needed.
> 
> dumpAsStream(InputStream) specifies "On return, the input stream will be at 
> end-of-stream" but I assume this isn't right as the method is lazy.
> 
> The 3-arg dumpAsStream doesn't specify the exception/behavior for when 
> chunkSize is <= 0.
> 
> fromString(CharSequence) may need clarification on how it behaves when the CS 
> is a CharBuffer. Does it consume all the remaining chars in the buffer so its 
> position be advanced to the limit? The 3-arg version is also not clear on 
> this point.
> 
> In Formatter interface it may be useful to expand the description of the 
> "offset" parameter as readers may not immediately understand that it's just 
> an input for the formatted string rather than a real offset, an example might 
> help. It also doesn't say if the offset can be specified as <= 0L or not.
> 
> A minor comment is that several places refer to a byte array as a "byte 
> buffers. Is this deliberate or can these be replaced with "byte array" to 
> avoid confusion. Another minor comment is that one of the dumpAsStream 
> methods is missing @throws NPE - maybe they should all be removed and 
> replaced with a statement in the class description.
> 
> -Alan

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

Thanks Roger. 

I believe that all but one of your concerns are addressed in this latest 
webrev/javadoc:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 

http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
 


The remaining issue is how best to define the dumpAsStream() method that takes 
a ByteBuffer.
The current approach has dropped the to/from parameters, consumes all the 
buffer and advances
Its position to the buffer limit.
This places the burden on the caller to size the buffer appropriately.

However I also see the merit in a ‘read-only’ approach that does not advance 
the buffer position.

Any further thoughts?



> On 11 Dec 2018, at 16:54, Roger Riggs  wrote:
> 
> Ho Vincent,
> 
> On 12/11/2018 11:34 AM, Vincent Ryan wrote:
>> Responses in-line.
>> 
>>> 
 Its really nice/necessary that examples can be copy/pasted and compile.
> 
> 
>  - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be confusing 
> because it
>is using the relative methods of ByteBuffer, but the from and to 
> offsets are within
>the position .. limit buffer.  That should be explicit.
>(Or consider switching to the absolute accesses in the ByteBuffer and 
> not affect the position)
 
 Is the additional javadoc line added earlier sufficient?
>>> I would bear some reinforcing that the entire remainder of the buffer is 
>>> read
>>> and the from and to are indexes within that remainder.
>>> And I'm not sure that's the desirable semantics.
>>> 
>>> It would make more sense if the from bytes from the buffer are skipped
>>> and only the (to - from) bytes are dumped.  That leaves the ByteBuffer 
>>> reading only the requested bytes.  A natural usage such as:
>>>  dumpAsStream(ByteBuffer buffer, 0, 256, 16, formatter)
>>> would dump the next 256bytes of the ByteBuffer and position would be
>>> moved by 256.
>> 
>> [As suggested by Alan]
>> How about dropping the fromIndex and toIndex parameters and requiring the 
>> caller
>> to provide a suitably sized buffer? 
>> 
>> public static Stream dumpAsStream(ByteBuffer buffer, int chunkSize, 
>> Formatter formatter) {
>> 
> I'd go the other direction and make dump use absolute offset and limit.
> The current values for position and limit are readily available from the 
> buffer.
> 
> If the dumping code is being added to perform some diagnostics then it would 
> not
> modify the position and not disturb the existing code that is using the 
> buffer.
> 
> Absolute is simpler to explain and implement and has fewer side effects.
> 
>> 
>>> 
 
 
> 
> - dump(byte[], OutputStream) - I don't think the example is correct, 
> there is no reference
>   to a stream, only the PrintStream::println method, which is not static.
 
 The code is just illustrative. Real values would have to be supplied for 
 the input bytes and the
 chosen print method on the output stream. Typically, that print method 
 will be System.out::println
>>> Examples that don't compile are really confusing and annoying.
>> 
>> Updated the ‘as if’ code to:
>> 
>>  * byte[] bytes = ...
>>  * PrintStream out = ...
>>  * HexFormat.dumpAsStream(bytes, 16,
>>  * (offset, chunk, from, to) ->
>>  * String.format("%08x  %s  |%s|",
>>  * offset,
>>  * HexFormat.toFormattedString(chunk, from, to),
>>  * HexFormat.toPrintableString(chunk, from, to)))
>>  * .forEachOrdered(out::println);
>> 
>> 
> Looks fine, thanks

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

Thanks for your review Stuart.

My preference is for PrintStream rather than Writer, for the same reason as 
Roger: it’s more convenient
for handling System.out. Does that address your concern?

I cannot simply cast 8859-1 characters into UTF-8 because UTF-8 is multi-byte 
charset so some 0x8X characters
Will trigger the multi-byte sequence and will end up being misinterpreted. 
Hence my rather awkward conversion to a String.
Is there a better way?

I’m not sure I’ve addressed your concern regarding IOExceptions - can you 
elaborate?


BTW updated webrev/javadoc available:
http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 

http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
 




> On 11 Dec 2018, at 02:11, Stuart Marks  wrote:
> 
> On 12/7/18 10:22 AM, Vincent Ryan wrote:
>>> I'm not convinced that the overloads that send output to an OutputStream 
>>> pull their weight. They basically wrap the OutputStream in a PrintStream, 
>>> which conveniently doesn't declare IOException, making it easy to use from 
>>> a lambda passed to forEachOrdered(). If an error writing the output occurs, 
>>> this is recorded by the PrintStream wrapper; however, the wrapper is then 
>>> thrown away, making it impossible for the caller to check its error status.
>> The intent is to support a trivial convenience method call that generates 
>> the well-known hexdump format.
>> Especially for users that are interested in the hexdump data rather than the 
>> low-level details of how to terminate a stream.
>> The dumpAsStream methods are available to support cases that differ from 
>> that format.
>> Have you a suggestion to improve the dump() methods, or you’d like to see 
>> them omitted?
>>> The PrintStream wrapper also uses the platform default charset, and doesn't 
>>> provide any way for the caller to override the charset.
>> Is there a need for that? Originally the requirement was driven by the 
>> hexdump format which is ASCII-only.
>> Recently the class has been enhanced to also support the printable 
>> characters from ISO 8859-1.
>> A custom formatter be supplied to dumpAsStream() to cater for all other 
>> cases?
> 
> OK, let's step back from this a bit. I see this hexdump as a little subsystem 
> that has the following facets:
> 
> 1) a source of bytes
> 2) a converter to hex
> 3) a destination
> 
> The converter is HexDump.Formatter, which converts and formats a subrange of 
> byte[] to a String. Since the user can supply the Formatter function, the 
> result String can contain any unicode character. Thus, the destination needs 
> to handle any unicode character. It can be a Writer, which accepts String 
> data. Or if you want it to write bytes, it can be an OutputStream, which 
> raises the issue of encoding (charset). I would recommend against relying on 
> the platform default charset, as this has been a source of subtle bugs. The 
> preferred approach these days is to default to UTF-8 and provide an overload 
> that takes an explicit charset.
> 
> An alternative is PrintStream. (This overlaps somewhat with your recent 
> exchange with Roger on this topic.) PrintStream also does charset encoding, 
> and the charset it uses depends on how it's created. I think the same 
> approach should be applied as I described above with OutputStream, namely 
> avoid the platform default charset; default to UTF-8; and provide an overload 
> that takes an explicit charset.
> 
> I'm not sure which of these is the right thing. You should decide which is 
> the most convenient for the use cases you expect to see. However, the 
> solution needs to handle charset encoding. (And it should also properly deal 
> with I/O exceptions, per my previous message.)
> 
> **
> 
> ISO 8859-1 comes up in a different place. The toPrintableString() method 
> (used by the default formatter) considers a byte "printable" if it encodes a 
> valid ISO 8859-1 character. The byte is properly decoded to a String, so this 
> is ok. Note this is a distinct issue from the encoding of the OutputStream or 
> PrintStream as described above.
> 
> (As an aside I think that the encoding of ISO 8859-1 matches the 
> corresponding code units of UTF-16, so you don't have to do the new 
> String(..., ISO_8859_1) jazz. You can just cast the byte to a char and append 
> it to the StringBuilder.)
> 
> s'marks

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Stefan Reich

What is this, the thought police?

Cheers

Stefan
BotCompany.de

On Tue, 11 Dec 2018 at 20:47, Phil Race  wrote:

> 1) Thanks for uploading the webrev. much better for one person to do
> this than make
> everyone who wants to look at it go through a tedious and off-putting
> set of steps.
>
> 2) I've added some client lists since you are touching UI client files,
> not just core-libs.
> To me the client ones look OK, one looks more like it was a typo
> than anything intentional,
> and the other was pretty mild.
>
> 3) Regarding the comment in the bug report about hb-private.hh and the
> use of
> /* CRAP pool: Common Region for Access Protection. */
> since it not only is in an upstream library, but also used 14 times in
> variable names,
> then I can't possibly agree with your comment that an argument for
> leaving them
> would be "shaky". Take this up with the upstream library ... I have no
> interest in
> renaming these every time we upgrade this library.
>
> -phil.
>
> On 12/11/18 8:45 AM, Adam Farley8 wrote:
> > Sure thing:
> >
> > http://cr.openjdk.java.net/~afarley/8215217/webrev/
> >
> > Best Regards
> >
> > Adam Farley
> > IBM Runtimes
> >
> >
> > Volker Simonis  wrote on 11/12/2018 15:46:44:
> >
> >> From: Volker Simonis 
> >> To: adam.far...@uk.ibm.com
> >> Cc: Java Core Libs 
> >> Date: 11/12/2018 15:47
> >> Subject: Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words
> >>
> >> Hi Adam,
> >>
> >> in order to prevent me from using swear words, could you please upload
> >> your webrev to cr.openjdk.java.net :)
> >>
> >> As you may have realized webrevs are a collection of HTML files and it
> >> makes no big sense to provide them as a zip file.
> >>
> >> Thank you and best regards,
> >> Volker
> >> On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8 
> > wrote:
> >>> Hey All,
> >>>
> >>> I've spotted 12 instances of swear words in OpenJDK source comments,
> > and
> >>> it seems appropriate to remove them.
> >>>
> >>> Bug: INVALID URI REMOVED
> >
> u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8215217=DwIBaQ=jf_iaSHvJObTbx-
> >> siA1ZOg=P5m8KWUXJf-
> >>
> >
> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=GfAb5QlDParO6DVrhdvPZTSafShnFACNF3JgqF-
> >> _RkM=Qscaf2tTpPcZKpIelJ6SrP0uRYSFoKaCNATns0FX7_Y=
> >>> I've created a webrev and attached to the bug.
> >>>
> >>> Also, I've mentioned in the bug that there are additional swears in
> > more
> >>> excusable locations. It would be good to get the community's take on
> >>> those.
> >>>
> >>> Reviews and opinions welcome. :)
> >>>
> >>> Best Regards
> >>>
> >>> Adam Farley
> >>> IBM Runtimes
> >>>
> >>> 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
> > 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
>
>

-- 
Stefan Reich
BotCompany.de // Java-based operating systems

Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

2018-12-11 Thread Sean Mullan


On 12/11/18 10:38 AM, Baesken, Matthias wrote:

File paths are, in general, always something that demands extra scrutiny
as it can be the source of security issues (privacy leaks, traversal
attacks, etc). It's not just me that thinks that way, you can do a
search on the Internet and find lots of references.

...


It might be perfectly fine and your usage might be ok too. But I'll need
some time to evaluate it further. I am not familiar with the code in
this part of the JDK.

I would also see if you can think about the security issues as well.
Where do these paths come from? What are the application use cases that
invoke these lower methods? From application code or elsewhere? Are
relative paths used? Are paths containing ".." canonicalized? Are they
arbitrary paths or a fixed set of paths? Do they ever contain sensitive
information like home directory user names, etc?

Once we understand if there are any security issues, then we can decide
if allowing that via a system property is acceptable or not, and if so
the security risks that we would have to document for that property.



Hello, the file paths potentially printed in the enhanced exceptions  in 
*canonicalize0, *createFileExclusively,
Java_java_io_WinNTFileSystem_canonicalizeWithPrefix0 could potentially come 
from  more or less arbitrary paths.


That to me, is an issue that warrants much more careful analysis before 
deciding whether to proceed with this fix.



This means, in some cases, there is a chance that information (like 
user-ids/user-names often found in paths below  HOME dirs )
might be in the printed paths that people do not want to have in log files or 
other output containing the exception messages.

For such potentially sensitive info in exception messages, we have already the  
jdk.includeInExceptions
security property, see the java.security file :

1064 # Enhanced exception message information
1065 #
1066 # By default, exception messages should not include potentially sensitive
1067 # information such as file names, host names, or port numbers. This 
property
1068 # accepts one or more comma separated values, each of which represents a
1069 # category of enhanced exception message information to enable. Values are
1070 # case-insensitive. Leading and trailing whitespaces, surrounding each 
value,
1071 # are ignored. Unknown values are ignored.

I think this approach fits well for 8211752 (and is used already for some other 
categories).


I am concerned that a single property controls whether or not 
potentially sensitive pathnames appear in Exceptions.



I created an updated webrev,  it uses the jdk.includeInExceptions security 
property
(had to do it via JNI because it did not work from the static class 
initializers of UnixFileSystem/WinNTFileSystem) :

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


These exceptions are generated from a very low level part of the native 
JDK Windows or Unix FileSystem implementations. That is a concern. The 
previous usages of this property were more focused and confined to 
smaller parts of the code resulting in fewer code paths to analyze.


I think we need to take a step back. I think we need to reconsider 
whether the jdk.includeInExceptions security property is appropriate for 
this type of enhancement.


Therefore, I oppose this change as-is. I'm happy to participate in a 
more involved discussion where we start with use cases and motivation 
before jumping to solutions.


Thanks,
Sean

Re: RFR(s): JDK-8214687 Optimize Collections.nCopies().hashCode()

2018-12-11 Thread Сергей Цыпанов

Hi Zheka and Tagir,

@Override
public void forEach(final Consumer action) {
  Objects.requireNonNull(action);
  for (int i = 0; i < this.n; i++) {
action.accept(this.element);
  }
}

I think here we should extract this.element and this.n into a local vars, as 
Consumer may have interaction with volatile field inside (e.g. 
AtomicInteger::addAndGet) which would cause a load of consumed field at each 
iteration. See original post by Nitsan Wakart 
https://psy-lob-saw.blogspot.com/2014/08/the-volatile-read-suprise.html

Regards,
Sergey Tsypanov


07.12.2018, 15:22, "Zheka Kozlov" :
> What about forEach()?
>
> @Override
> public void forEach(final Consumer action) {
> Objects.requireNonNull(action);
> for (int i = 0; i < this.n; i++) {
> action.accept(this.element);
> }
> }
>
> I think this version has better chances to be faster than the default
> implementation (did not measure though).
>
> вт, 4 дек. 2018 г. в 14:40, Tagir Valeev :
>
>>  Hello!
>>
>>  > In CopiesList.equals() please use eq() instead of Objects.equal() (see a
>>  comment at the line 5345).
>>
>>  Ok
>>
>>  > I think you should use iterator() instead of listIterator(). See the
>>  explanation here:
>>  http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052472.html
>>
>>  Ok. I wonder why this message received no attention.
>>
>>  Here's updated webrev:
>>  http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r3/
>>
>>  With best regards,
>>  Tagir Valeev
>>  On Tue, Dec 4, 2018 at 1:10 PM Zheka Kozlov  wrote:
>>  >
>>  > I think you should use iterator() instead of listIterator(). See the
>>  explanation here:
>>  http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052472.html
>>  >
>>  > вт, 4 дек. 2018 г. в 12:23, Tagir Valeev :
>>  >>
>>  >> Hello!
>>  >>
>>  >> Thank you for your comments!
>>  >>
>>  >> Yes, deserialization will be broken if we assume that size is never 0.
>>  >> Also we'll introduce referential identity Collections.nCopies(0, x) ==
>>  >> Collections.nCopies(0, y) which might introduce slight semantics
>>  >> change in existing programs. Once I suggested to wire Arrays.asList()
>>  >> (with no args) to Collections.emptyList(), but it was rejected for the
>>  >> same reason: no need to introduce a risk of possible semantics change.
>>  >>
>>  >> I updated webrev with equals implementation and test:
>>  >> http://cr.openjdk.java.net/~tvaleev/webrev/8214687/r2/
>>  >> Comparing two CopiesList is much faster now indeed. Also we can spare
>>  >> an iterator in the common case and hoist the null-check out of the
>>  >> loop. Not sure whether we can rely that JIT will always do this for
>>  >> us, but if you think that it's unnecessary, I can merge the loops
>>  >> back. Note that now copiesList.equals(arrayList) could be faster than
>>  >> arrayList.equals(copiesList). I don't think it's an issue. On the
>>  >> other hand we could keep simpler and delegate to super-implementation
>>  >> if the other object is not a CopiesList (like it's implemented in
>>  >> java.util.RegularEnumSet::equals for example). What do you think?
>>  >>
>>  >> With best regards,
>>  >> Tagir Valeev.
>>  >> On Tue, Dec 4, 2018 at 10:56 AM Stuart Marks 
>>  wrote:
>>  >> >
>>  >> >
>>  >> > >> I believe it makes sense to override CopiesList.equals()
>>  >> > > Also: contains(), iterator(), listIterator()
>>  >> >
>>  >> > equals(): sure
>>  >> >
>>  >> > contains() is already overridden. Not sure there's much benefit to
>>  overriding
>>  >> > the iterators.
>>  >> >
>>  >> > s'marks

JDK-8210280 - Unnecessary reallocation when invoking HashMap.putAll()

2018-12-11 Thread Michal Vala


Hi,

I've been looking into this bug in HashMap where resize() is called multiple 
times when putting whole map into another.


I come up with following patch: 
http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8210280/webrev.00/


I've tried to do it as little invasive as possible. New resize(int) method is 
called just from putMapEntries for now. Notice that method is called with size 
of the new map. I was experimenting with calling it with 'size()+s', but this 
leads to unwanted space allocation when inserted map rewrites a lot of existing 
keys.


I've benchmarked this with adding 10millions elements into existing map and it 
gets ~50% improvement:


unpatched
BenchmarkMode  Cnt  Score   Error  Units
MyBenchmark.testMethod  thrpt   10  1.248 ± 0.058  ops/s

patched
BenchmarkMode  Cnt  Score   Error  Units
MyBenchmark.testMethod  thrpt   10  1.872 ± 0.109  ops/s

public class MyBenchmark {
static Map mapTocopy = IntStream.range(1, 
10_000_000).boxed().collect(Collectors.toMap(k -> k,

k -> k));

@Benchmark
public int testMethod() {
var map = new HashMap();
map.put(-5, -5);
map.putAll(mapTocopy);
return map.size();
}
}

Any ideas for missed corner-cases or some good tests?

--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Looks fine.

Thanks, Roger

On 12/11/2018 01:33 PM, Naoto Sato wrote:

Hi Roger,

On 12/11/18 10:12 AM, Roger Riggs wrote:

Hi Naoto,

Looks ok,

I intended only the number of elements to be defined as a constant.
The other factors can be hard coded.


OK, I revised it again:

http://cr.openjdk.java.net/~naoto/8215194/webrev.02/



In the test, you will still have to edit the test when the number 
changes.
I meant to avoid that edit.  Though then may be there is not need for 
the test at all.


Yes, if we just reflectively use the constant in 
Character.UnicodeBlock in the test, it is guaranteed to succeed no 
matter what. Thus I added the assertion. Still, the test ofHashMap() 
succeeds till the block additions surpasses that 1024 capacity (thus 
this was overlooked on Unicode 11 upgrade).


Naoto



Roger


On 12/11/2018 12:59 PM, Naoto Sato wrote:

Hi Roger,

Thanks. I updated it as suggested (incl. test using reflection):

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

Naoto

On 12/11/18 7:57 AM, Roger Riggs wrote:

Hi Naoto,

Since the value changes from time to time, it would give it some 
visibility

if it were defined using a private final int  (or float)
  private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using 
reflection.

But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size 
of Character.UnicodeBlock.


Naoto

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Ivan,

Thank you for the review.

On 12/11/18 10:49 AM, Ivan Gerasimov wrote:
OptimalCapacity.ofHashMap uses the initialCapacity to verify that a 
HashMap created with that initialCapacity will not reallocate its 
internal array after inserting the factual number of elements.


So, if one day the number of entities is increased, but the constant 
NUM_ENTITIES is not updated, then the test will catch it.


Actually the reason I am fixing it is that, the test did not detect the 
discrepancy with Unicode 11 upgrade, where it just increased the entry 
numbers by the true block increases (11 blocks), without adding their 
aliases.


---

--- a/src/java.base/share/classes/java/lang/Character.java	Wed Nov 14 
13:15:54 2018 +0100
+++ b/src/java.base/share/classes/java/lang/Character.java	Wed Nov 21 
14:24:31 2018 +0530

@@ -43,7 +43,7 @@
  * a character's category (lowercase letter, digit, etc.) and for 
converting

  * characters from uppercase to lowercase and vice versa.
  * 
- * Character information is based on the Unicode Standard, version 10.0.0.
+ * Character information is based on the Unicode Standard, version 11.0.0.
  * 
  * The methods and data of class {@code Character} are defined by
  * the information in the UnicodeData file that is part of the
@@ -681,11 +681,11 @@
  */
 public static final class UnicodeBlock extends Subset {
 /**
- * 638  - the expected number of entities
+ * 649  - the expected number of entities
  * 0.75 - the default load factor of HashMap
  */
 private static Map map =
-new HashMap<>((int)(638 / 0.75f + 1.0f));
+new HashMap<>((int)(649 / 0.75f + 1.0f));

 /**
  * Creates a UnicodeBlock with the given identifier name.
---

However, the HashMap allocates the entry size to the nearest power of 
two number, i.e., 1024 for both numbers 649 and 667. Thus the test 
passes even if there is discrepancy.


Naoto

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect

2018-12-11 Thread Ivan Gerasimov


Hi Naoto!

The fix looks good, thank you!

I wonder if the test can be updated to also verify the optimal capacity 
of HashMap aliases;



On 12/11/18 10:33 AM, Naoto Sato wrote:

Hi Roger,

On 12/11/18 10:12 AM, Roger Riggs wrote:

Hi Naoto,

Looks ok,

I intended only the number of elements to be defined as a constant.
The other factors can be hard coded.


OK, I revised it again:

http://cr.openjdk.java.net/~naoto/8215194/webrev.02/



In the test, you will still have to edit the test when the number 
changes.
I meant to avoid that edit.  Though then may be there is not need for 
the test at all.


Yes, if we just reflectively use the constant in 
Character.UnicodeBlock in the test, it is guaranteed to succeed no 
matter what.
OptimalCapacity.ofHashMap uses the initialCapacity to verify that a 
HashMap created with that initialCapacity will not reallocate its 
internal array after inserting the factual number of elements.


So, if one day the number of entities is increased, but the constant 
NUM_ENTITIES is not updated, then the test will catch it.


With kind regards,
Ivan

Thus I added the assertion. Still, the test ofHashMap() succeeds till 
the block additions surpasses that 1024 capacity (thus this was 
overlooked on Unicode 11 upgrade).


Naoto



Roger


On 12/11/2018 12:59 PM, Naoto Sato wrote:

Hi Roger,

Thanks. I updated it as suggested (incl. test using reflection):

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

Naoto

On 12/11/18 7:57 AM, Roger Riggs wrote:

Hi Naoto,

Since the value changes from time to time, it would give it some 
visibility

if it were defined using a private final int  (or float)
  private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using 
reflection.

But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size 
of Character.UnicodeBlock.


Naoto








--
With kind regards,
Ivan Gerasimov

Re: RFR: 8215159: Improve initial setup of system Properties


Hi,

On 2018-12-11 19:08, Roger Riggs wrote:

Hi Claes,

SystemProps.java:
   - the import of Collections isn't used.


will fix before push.



VM.java:
  - line 180:  The savedProps doesn't need to be (and was not 
previously) unmodifiable.
    Seems safer this way though since the map at the bottom is not 
ConcurrentHashMap.

    Its not entirely clear who calls getSavedProperties().
    Would it be more efficient for savedProps to be the unmodifiable map 
and not create

    one on every call to getSavedProperties.



VM.getSavedProperties should be removed, but that requires some 
coordination that is out of scope for this improvement.


There's only one caller, and they cache it in a static field. Not
wrapping VM.savedProps marginally improves speed of VM.savedProperty 
which is more used during startup.




VersionProps.java.template:
  - The version properties don't need to be in the savedProps map and 
could be put only in the Properties.

    (save a few cycles and entries).


Good point. I have played with code that goes even further (no need to
put any of the Raw properties in the savedProps map). We can save maybe 
another 6k executed bytecode in total, but as it gets unclear what can

be found in VM.savedProperties and what can be found in the System
properties I decided against it in this patch.

Thanks!

/Claes



System.java:
  - swap lines 805/806 if you decide not to put version props in the map.
  - and call VerionProps.init() after the call to createProperties().

Regards, Roger



On 12/11/2018 12:41 PM, Mandy Chung wrote:



On 12/10/18 1:17 PM, Claes Redestad wrote:

Hi,

by inverting the order in which the internal property maps are created,
we avoid some classloading and get a slightly more efficient code
execution profile in System.initPhase1.

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



This change looks okay to me.

ModuleBootstrap also removes some `jdk.module.*` private system 
properties during
module system initialization but they are only set if module-related 
command-line
options are set.   These properties are not present in the common 
cases.   These
private system properties are the interface between VM and 
libraries.   There is
other mechanism that we can look into to replace using system 
properties in the

future.

Mandy

Re: RFR: 8215159: Improve initial setup of system Properties

2018-12-11 Thread Mandy Chung





On 12/11/18 10:08 AM, Roger Riggs wrote:


VM.java:
 - line 180:  The savedProps doesn't need to be (and was not 
previously) unmodifiable.
   Seems safer this way though since the map at the bottom is not 
ConcurrentHashMap.

   Its not entirely clear who calls getSavedProperties().
   Would it be more efficient for savedProps to be the unmodifiable 
map and not create

   one on every call to getSavedProperties.



JVMCI and Graal depends on VM::getSavedProperties to determine if JVMCI 
is enabled

and also to get other Graal-specific system properties set at startup.

VM::getSavedProperties is not called in the common case and creating a 
unmodifiable map

when getSavedProperties is called is reasonable.


Mandy

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Roger,

On 12/11/18 10:12 AM, Roger Riggs wrote:

Hi Naoto,

Looks ok,

I intended only the number of elements to be defined as a constant.
The other factors can be hard coded.


OK, I revised it again:

http://cr.openjdk.java.net/~naoto/8215194/webrev.02/



In the test, you will still have to edit the test when the number changes.
I meant to avoid that edit.  Though then may be there is not need for 
the test at all.


Yes, if we just reflectively use the constant in Character.UnicodeBlock 
in the test, it is guaranteed to succeed no matter what. Thus I added 
the assertion. Still, the test ofHashMap() succeeds till the block 
additions surpasses that 1024 capacity (thus this was overlooked on 
Unicode 11 upgrade).


Naoto



Roger


On 12/11/2018 12:59 PM, Naoto Sato wrote:

Hi Roger,

Thanks. I updated it as suggested (incl. test using reflection):

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

Naoto

On 12/11/18 7:57 AM, Roger Riggs wrote:

Hi Naoto,

Since the value changes from time to time, it would give it some 
visibility

if it were defined using a private final int  (or float)
  private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using reflection.
But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size of 
Character.UnicodeBlock.


Naoto

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Naoto,

Looks ok,

I intended only the number of elements to be defined as a constant.
The other factors can be hard coded.

In the test, you will still have to edit the test when the number changes.
I meant to avoid that edit.  Though then may be there is not need for 
the test at all.


Roger


On 12/11/2018 12:59 PM, Naoto Sato wrote:

Hi Roger,

Thanks. I updated it as suggested (incl. test using reflection):

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

Naoto

On 12/11/18 7:57 AM, Roger Riggs wrote:

Hi Naoto,

Since the value changes from time to time, it would give it some 
visibility

if it were defined using a private final int  (or float)
  private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using reflection.
But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size of 
Character.UnicodeBlock.


Naoto

Re: RFR: 8215159: Improve initial setup of system Properties


Hi Claes,

SystemProps.java:
  - the import of Collections isn't used.

VM.java:
 - line 180:  The savedProps doesn't need to be (and was not 
previously) unmodifiable.
   Seems safer this way though since the map at the bottom is not 
ConcurrentHashMap.

   Its not entirely clear who calls getSavedProperties().
   Would it be more efficient for savedProps to be the unmodifiable map 
and not create

   one on every call to getSavedProperties.

VersionProps.java.template:
 - The version properties don't need to be in the savedProps map and 
could be put only in the Properties.

   (save a few cycles and entries).

System.java:
 - swap lines 805/806 if you decide not to put version props in the map.
 - and call VerionProps.init() after the call to createProperties().

Regards, Roger



On 12/11/2018 12:41 PM, Mandy Chung wrote:



On 12/10/18 1:17 PM, Claes Redestad wrote:

Hi,

by inverting the order in which the internal property maps are created,
we avoid some classloading and get a slightly more efficient code
execution profile in System.initPhase1.

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



This change looks okay to me.

ModuleBootstrap also removes some `jdk.module.*` private system 
properties during
module system initialization but they are only set if module-related 
command-line
options are set.   These properties are not present in the common 
cases.   These
private system properties are the interface between VM and 
libraries.   There is
other mechanism that we can look into to replace using system 
properties in the

future.

Mandy

Re: RFR: 8215159: Improve initial setup of system Properties





On 2018-12-11 18:41, Mandy Chung wrote:



On 12/10/18 1:17 PM, Claes Redestad wrote:

Hi,

by inverting the order in which the internal property maps are created,
we avoid some classloading and get a slightly more efficient code
execution profile in System.initPhase1.

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



This change looks okay to me.


Thanks!



ModuleBootstrap also removes some `jdk.module.*` private system 
properties during
module system initialization but they are only set if module-related 
command-line
options are set.   These properties are not present in the common 
cases.   These
private system properties are the interface between VM and libraries.   
There is
other mechanism that we can look into to replace using system properties 
in the

future.


Right, as we've discussed a bit off-list there are further improvements
that could be made.  For one we'd probably be better off without the
internal VM.savedProps map altogether, but usage has snuck into some
weird places.  Something for another day. :-)

/Claes



Mandy

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Roger,

Thanks. I updated it as suggested (incl. test using reflection):

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

Naoto

On 12/11/18 7:57 AM, Roger Riggs wrote:

Hi Naoto,

Since the value changes from time to time, it would give it some visibility
if it were defined using a private final int  (or float)
  private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using reflection.
But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size of 
Character.UnicodeBlock.


Naoto

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Phil Race

1) Thanks for uploading the webrev. much better for one person to do 
this than make
everyone who wants to look at it go through a tedious and off-putting 
set of steps.


2) I've added some client lists since you are touching UI client files, 
not just core-libs.
   To me the client ones look OK, one looks more like it was a typo 
than anything intentional,

   and the other was pretty mild.

3) Regarding the comment in the bug report about hb-private.hh and the 
use of

/* CRAP pool: Common Region for Access Protection. */
since it not only is in an upstream library, but also used 14 times in 
variable names,
then I can't possibly agree with your comment that an argument for 
leaving them
would be "shaky". Take this up with the upstream library ... I have no 
interest in

renaming these every time we upgrade this library.

-phil.

On 12/11/18 8:45 AM, Adam Farley8 wrote:

Sure thing:

http://cr.openjdk.java.net/~afarley/8215217/webrev/

Best Regards

Adam Farley
IBM Runtimes


Volker Simonis  wrote on 11/12/2018 15:46:44:


From: Volker Simonis 
To: adam.far...@uk.ibm.com
Cc: Java Core Libs 
Date: 11/12/2018 15:47
Subject: Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

Hi Adam,

in order to prevent me from using swear words, could you please upload
your webrev to cr.openjdk.java.net :)

As you may have realized webrevs are a collection of HTML files and it
makes no big sense to provide them as a zip file.

Thank you and best regards,
Volker
On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8 

wrote:

Hey All,

I've spotted 12 instances of swear words in OpenJDK source comments,

and

it seems appropriate to remove them.

Bug: INVALID URI REMOVED

u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8215217=DwIBaQ=jf_iaSHvJObTbx-

siA1ZOg=P5m8KWUXJf-


CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=GfAb5QlDParO6DVrhdvPZTSafShnFACNF3JgqF-

_RkM=Qscaf2tTpPcZKpIelJ6SrP0uRYSFoKaCNATns0FX7_Y=

I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in

more

excusable locations. It would be good to get the community's take on
those.

Reviews and opinions welcome. :)

Best Regards

Adam Farley
IBM Runtimes

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
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: 8215159: Improve initial setup of system Properties

2018-12-11 Thread Mandy Chung





On 12/10/18 1:17 PM, Claes Redestad wrote:

Hi,

by inverting the order in which the internal property maps are created,
we avoid some classloading and get a slightly more efficient code
execution profile in System.initPhase1.

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



This change looks okay to me.

ModuleBootstrap also removes some `jdk.module.*` private system 
properties during
module system initialization but they are only set if module-related 
command-line
options are set.   These properties are not present in the common 
cases.   These
private system properties are the interface between VM and libraries.   
There is
other mechanism that we can look into to replace using system properties 
in the

future.

Mandy

Re: [PATCH] Windows 32-bit DLL name decoration

2018-12-11 Thread Bob Vandette

Hotspot has had support for decorated and non-decorated names for the JNI_OnLoad
functions.  Perhaps you should just follow the same procedure for the debug 
library.

#define JNI_ONLOAD_SYMBOLS   {"_JNI_OnLoad@8", "JNI_OnLoad"}
#define JNI_ONUNLOAD_SYMBOLS {"_JNI_OnUnload@8", "JNI_OnUnload"}
#define JVM_ONLOAD_SYMBOLS  {"_JVM_OnLoad@12", "JVM_OnLoad"}
#define AGENT_ONLOAD_SYMBOLS{"_Agent_OnLoad@12", "Agent_OnLoad"}
#define AGENT_ONUNLOAD_SYMBOLS  {"_Agent_OnUnload@4", "Agent_OnUnload"}
#define AGENT_ONATTACH_SYMBOLS  {"_Agent_OnAttach@12", “Agent_OnAttach”}

Bob.


> On Dec 11, 2018, at 11:43 AM, Simon Tooke  wrote:
> 
> On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:
>> Hi everyone,
>> 
>> I came up with the following patch:
>> http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/
>> 
>> It specifically addresses the problem in JDK-8214122 where on 32 bit
>> Windows jdwpTransport_OnLoad can exported with its plain and
>> __stdcall-mangled name. I used conditional compilation so that for
>> other platforms the code remains as it is now.
>> 
>> jshell starts successfully with this fix; an IDE debugger works as well.
>> 
> I am not a reviewer, but this patch only works for the specific case
> under discussion; the '@16' refers to the reserved stack size in the
> Pascal calling convention.  So, the patch only works for 16 bytes of
> parameters.  To be generic, the routine needs to have the stack size
> passed in by the caller.  If a generic fix isn't desired (and I hope it
> is), I'd prefer to see the caller simply pass the decorated or
> undecorated name depending on the Win32/64 defines.
> 
>#if defined(_WIN32) && !defined(_WIN64) onLoad =
>(jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
>"_jdwpTransport_OnLoad@16"); #else onLoad = (jdwpTransport_OnLoad_t)
>dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif
> 
> 
> Thanks,
> -Simon
> 
>> 
>> Regards,
>> Alexey
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8214122
>> 
>> On 10/12/2018 15:11, Magnus Ihse Bursie wrote:
 Since removing JNICALL is not an option, there are only two options:
 
 1. Add |/export| option to the Makefile or pragma-comment to the
 source file;
 2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
 with fallback to undecorated one.
>>> Yes.
>>> 
>>> I think the correct solution here is 2.
>> 
>

Re: [PATCH] Windows 32-bit DLL name decoration

2018-12-11 Thread Alexey Ivanov


Hi Simon,

Thank you for your comments.

The updated webrev:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Indeed, it looks much cleaner.

Regards,
Alexey

On 11/12/2018 16:43, Simon Tooke wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as well.


I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

 #if defined(_WIN32) && !defined(_WIN64) onLoad =
 (jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
 "_jdwpTransport_OnLoad@16"); #else onLoad = (jdwpTransport_OnLoad_t)
 dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

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

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:

Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: RFR: 8170769 Provide a simple hexdump facility for binary data


Ho Vincent,

On 12/11/2018 11:34 AM, Vincent Ryan wrote:

Responses in-line.




Its really nice/necessary that examples can be copy/pasted and compile.


 - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be 
confusing because it
   is using the relative methods of ByteBuffer, but the from and to 
offsets are within

   the position .. limit buffer.  That should be explicit.
   (Or consider switching to the absolute accesses in the 
ByteBuffer and not affect the position)


Is the additional javadoc line added earlier sufficient?
I would bear some reinforcing that the entire remainder of the buffer 
is read

and the from and to are indexes within that remainder.
And I'm not sure that's the desirable semantics.

It would make more sense if the from bytes from the buffer are skipped
and only the (to - from) bytes are dumped.  That leaves the ByteBuffer
reading only the requested bytes.  A natural usage such as:
 dumpAsStream(ByteBuffer buffer, 0, 256, 16, formatter)
would dump the next 256bytes of the ByteBuffer and position would be
moved by 256.


[As suggested by Alan]
How about dropping the fromIndex and toIndex parameters and requiring 
the caller

to provide a suitably sized buffer?

public static Stream dumpAsStream(ByteBuffer 
buffer, int chunkSize, Formatter formatter) {



I'd go the other direction and make dump use absolute offset and limit.
The current values for position and limit are readily available from the 
buffer.


If the dumping code is being added to perform some diagnostics then it 
would not
modify the position and not disturb the existing code that is using the 
buffer.


Absolute is simpler to explain and implement and has fewer side effects.










- dump(byte[], OutputStream) - I don't think the example is 
correct, there is no reference
  to a stream, only the PrintStream::println method, which is not 
static.


The code is just illustrative. Real values would have to be supplied 
for the input bytes and the
chosen print method on the output stream. Typically, that print 
method will be System.out::println

Examples that don't compile are really confusing and annoying.


Updated the ‘as if’ code to:

     *     byte[] bytes = ...
     *     PrintStream out = ...
     *     HexFormat.dumpAsStream(bytes, 16,
     *         (offset, chunk, from, to) ->
     *             String.format("%08x  %s  |%s|",
     *                 offset,
     *                 HexFormat.toFormattedString(chunk, from, to),
     *                 HexFormat.toPrintableString(chunk, from, to)))
     *         .forEachOrdered(out::println);



Looks fine, thanks

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect

2018-12-11 Thread Rachna Goel


Hi Naoto,

Thanks for fixing this.

Your fix looks good to me.

Thanks,

Rachna


On 12/11/18 8:21 PM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size of 
Character.UnicodeBlock.


Naoto


--
Thanks,
Rachna

Re: RFR - JDK-8203442 String::transform (Code Review)

2018-12-11 Thread Tomasz Linkowski

Alan,  Rémi,

If you're still interested in providing transform-like behavior for
CharSequence, there's a fairly neat (I hope) way to do that.

The idea is to use the following helper functional interface for forwarding
to the already added String.transform:


@FunctionalInterface
interface Transformer {
   R by(Function f);
}


public interface CharSequence extends Transformable {
  // ...
  Transformer transformed();
  // ...
}


public final class String implements /*...*/ CharSequence {
  // ...
  @Override
  public Transformer transformed() {
return this::transform;
  }
  // ...
}


Usage:

CharSequence transformed = charSeq
.transformed().by(s -> StringUtils.defaultIfBlank('0')) // sample
Function
.transformed().by(...)
.transformed().by(...);


If you find it interesting, it's described in detail in my blog post:
https://blog.tlinkowski.pl/2018/transformer-pattern/

Regards,
Tomasz Linkowski


On Fri, Sep 21, 2018 at 12:42 PM Remi Forax  wrote:

> - Mail original -
> > De: "Alan Bateman" 
> > À: "Jim Laskey" , "core-libs-dev" <
> core-libs-dev@openjdk.java.net>
> > Envoyé: Vendredi 21 Septembre 2018 12:22:42
> > Objet: Re: RFR - JDK-8203442 String::transform (Code Review)
>
> > On 18/09/2018 18:52, Jim Laskey wrote:
> >> Please review the code for String::transform. The goal is to provide a
> String
> >> instance method to allow function application of custom transformations
> applied
> >> to an instance of String.
> >>
> >> webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html
> >> jbs: https://bugs.openjdk.java.net/browse/JDK-8203442
> >> csr: https://bugs.openjdk.java.net/browse/JDK-8203703
> > I hate to bring up naming but if I have a Stream or
> > Optional then I'll use the "map" method to apply the mapping
> > function. Has this already been through the naming bike shed and it
> > settled on "transform"? Maybe a different name was chosen after looking
> > at code that would use it in stream operations? Maybe "transform" was
> > used as the argument to the function is always text? I'm not interested
> > in re-opening any discussions, just trying to see if options are
> > captured in a mail somewhere.
> >
> > I'm also wondering if this is general enough to be defined by
> > CharSequence. Retrofitting CharSequence is always problematic and never
> > done lightly but I'm just wondering if it has been discussed and
> > dismissed. I don't think it could be done at a later time, at least not
> > without changing the input type parameter so that the mapping function
> > is Function rather than Function.
>
> the main issue is that a lot of utility methods take a String as
> parameter, not a CharSequence :(
> and Function is a supertype not a subtype of Function super CharSequence, R>.
>
> >
> > -Alan
>
> Rémi
>


-- 
Pozdrawiam,
Tomasz Linkowski

Re: RFR: 8170769 Provide a simple hexdump facility for binary data


Hi Stuart,

The APIs for streams of characters bifurcated a bit between PrintStream 
and Writers.
Many common use cases would like to direct the output to System.out/err 
which are

PrintStreams.  Hence, I lean toward PrintStream that can be used directly.

$.02, Roger



On 12/10/2018 09:11 PM, Stuart Marks wrote:

On 12/7/18 10:22 AM, Vincent Ryan wrote:
I'm not convinced that the overloads that send output to an 
OutputStream pull their weight. They basically wrap the OutputStream 
in a PrintStream, which conveniently doesn't declare IOException, 
making it easy to use from a lambda passed to forEachOrdered(). If 
an error writing the output occurs, this is recorded by the 
PrintStream wrapper; however, the wrapper is then thrown away, 
making it impossible for the caller to check its error status.
The intent is to support a trivial convenience method call that 
generates the well-known hexdump format.
Especially for users that are interested in the hexdump data rather 
than the low-level details of how to terminate a stream.
The dumpAsStream methods are available to support cases that differ 
from that format.


Have you a suggestion to improve the dump() methods, or you’d like to 
see them omitted?


The PrintStream wrapper also uses the platform default charset, and 
doesn't provide any way for the caller to override the charset.
Is there a need for that? Originally the requirement was driven by 
the hexdump format which is ASCII-only.
Recently the class has been enhanced to also support the printable 
characters from ISO 8859-1.
A custom formatter be supplied to dumpAsStream() to cater for all 
other cases?


OK, let's step back from this a bit. I see this hexdump as a little 
subsystem that has the following facets:


1) a source of bytes
2) a converter to hex
3) a destination

The converter is HexDump.Formatter, which converts and formats a 
subrange of byte[] to a String. Since the user can supply the 
Formatter function, the result String can contain any unicode 
character. Thus, the destination needs to handle any unicode 
character. It can be a Writer, which accepts String data. Or if you 
want it to write bytes, it can be an OutputStream, which raises the 
issue of encoding (charset). I would recommend against relying on the 
platform default charset, as this has been a source of subtle bugs. 
The preferred approach these days is to default to UTF-8 and provide 
an overload that takes an explicit charset.


An alternative is PrintStream. (This overlaps somewhat with your 
recent exchange with Roger on this topic.) PrintStream also does 
charset encoding, and the charset it uses depends on how it's created. 
I think the same approach should be applied as I described above with 
OutputStream, namely avoid the platform default charset; default to 
UTF-8; and provide an overload that takes an explicit charset.


I'm not sure which of these is the right thing. You should decide 
which is the most convenient for the use cases you expect to see. 
However, the solution needs to handle charset encoding. (And it should 
also properly deal with I/O exceptions, per my previous message.)


**

ISO 8859-1 comes up in a different place. The toPrintableString() 
method (used by the default formatter) considers a byte "printable" if 
it encodes a valid ISO 8859-1 character. The byte is properly decoded 
to a String, so this is ok. Note this is a distinct issue from the 
encoding of the OutputStream or PrintStream as described above.


(As an aside I think that the encoding of ISO 8859-1 matches the 
corresponding code units of UTF-16, so you don't have to do the new 
String(..., ISO_8859_1) jazz. You can just cast the byte to a char and 
append it to the StringBuilder.)


s'marks

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Adam Farley8

Sure thing:

http://cr.openjdk.java.net/~afarley/8215217/webrev/

Best Regards

Adam Farley 
IBM Runtimes


Volker Simonis  wrote on 11/12/2018 15:46:44:

> From: Volker Simonis 
> To: adam.far...@uk.ibm.com
> Cc: Java Core Libs 
> Date: 11/12/2018 15:47
> Subject: Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words
> 
> Hi Adam,
> 
> in order to prevent me from using swear words, could you please upload
> your webrev to cr.openjdk.java.net :)
> 
> As you may have realized webrevs are a collection of HTML files and it
> makes no big sense to provide them as a zip file.
> 
> Thank you and best regards,
> Volker
> On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8  
wrote:
> >
> > Hey All,
> >
> > I've spotted 12 instances of swear words in OpenJDK source comments, 
and
> > it seems appropriate to remove them.
> >
> > Bug: INVALID URI REMOVED
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8215217=DwIBaQ=jf_iaSHvJObTbx-
> siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=GfAb5QlDParO6DVrhdvPZTSafShnFACNF3JgqF-
> _RkM=Qscaf2tTpPcZKpIelJ6SrP0uRYSFoKaCNATns0FX7_Y=
> >
> > I've created a webrev and attached to the bug.
> >
> > Also, I've mentioned in the bug that there are additional swears in 
more
> > excusable locations. It would be good to get the community's take on
> > those.
> >
> > Reviews and opinions welcome. :)
> >
> > Best Regards
> >
> > Adam Farley
> > IBM Runtimes
> >
> > 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
> 

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: [PATCH] Windows 32-bit DLL name decoration

2018-12-11 Thread Simon Tooke

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:
> Hi everyone,
>
> I came up with the following patch:
> http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/
>
> It specifically addresses the problem in JDK-8214122 where on 32 bit
> Windows jdwpTransport_OnLoad can exported with its plain and
> __stdcall-mangled name. I used conditional compilation so that for
> other platforms the code remains as it is now.
>
> jshell starts successfully with this fix; an IDE debugger works as well.
>
I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

#if defined(_WIN32) && !defined(_WIN64) onLoad =
(jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
"_jdwpTransport_OnLoad@16"); #else onLoad = (jdwpTransport_OnLoad_t)
dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon

>
> Regards,
> Alexey
>
> https://bugs.openjdk.java.net/browse/JDK-8214122
>
> On 10/12/2018 15:11, Magnus Ihse Bursie wrote:
>>> Since removing JNICALL is not an option, there are only two options:
>>>
>>> 1. Add |/export| option to the Makefile or pragma-comment to the
>>> source file;
>>> 2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
>>> with fallback to undecorated one.
>> Yes.
>>
>> I think the correct solution here is 2.
>

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

Responses in-line.

> On 10 Dec 2018, at 21:38, Roger Riggs  wrote:
> 
> Hi Vincent,
> 
> On 12/10/2018 03:59 PM, Vincent Ryan wrote:
>> Comments in-line.
>> Thanks.
>> 
>>> On 10 Dec 2018, at 16:59, Roger Riggs >> > wrote:
>>> 
>>> Hi,
>>> 
>>> Looks good, though some points to clarify.
>>> 
>>> - The methods that use ByteBuffers should be clear that accesses to the 
>>> ByteBuffers
>>>are relative and use and modify the position and ByteBuffer exceptions 
>>> may be thrown.
>> 
>> Added the line:
>> 'Access to the ByteBuffer is relative and its position gets set to the 
>> buffer's limit.'
> ok
>> 
>>> 
>>> - The methods that write output (Strings) to an output stream might be more 
>>> general
>>>   if the argument was a PrintStream, allowing the hex formatted output to 
>>> be 
>>>   assembled with other localized output.
>>>   I do see that as long as HexFormat is only writing hex digits, the effect 
>>> would be almost the same.
>>>   (It would also eliminate, the inefficient code in getPrintStream that 
>>> creates a PrintStream on demand).
>> 
>> I had wanted to provide flexibility by favouring OutputStream over 
>> PrintStream.
>> It is convenient for the common case of dumping to a file using 'new 
>> FileOutputStream’.
>> As you note it complicates assembly with other output.
>> 
>> I guess it’s fine to change the OutputStream args to PrintStream.
> ok, if the caller has a direct OutputStream, a PrintStream could be created.
> But I think the PrintStream is more common.
>> 
>> 
>>> 
>>>  - toString(byte[], from, to) and other methods there's no need for parens 
>>> around the note about fromIndex==toIndex.
>> 
>> OK
>> 
>>> 
>>>  - fromString methods: The optional prefix of "0x": add the phrase  "is 
>>> ignored".
>>> The IAE, does not mention the optional prefix, I'm not sure if that 
>>> allows some confusion.
>> 
>> Added the line:
>> 'The optional prefix of {@code "0x"} is ignored.'
> ok
>> 
>>> 
>>> - dumpAsStream(InputStream) does not have a throws NPE for in == null.
>>>   A catch all in the class javadoc could cover that. 
>>>   " Unless otherwise noted, passing a null argument to a constructor or 
>>> method in any class or 
>>>interface in this package will cause a NullPointerException to be 
>>> thrown. “
>> 
>> Added an @throws to the method.
> ok
>> 
>>> 
>>>  - dumpAsStream methods, the formatter argument is described as being used 
>>> "if present";  
>>>it might be clearer as "if not null”.
>> 
>> OK
>> Its really nice/necessary that examples can be copy/pasted and compile.
>>> 
>>> 
>>>  - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be confusing 
>>> because it
>>>is using the relative methods of ByteBuffer, but the from and to offsets 
>>> are within
>>>the position .. limit buffer.  That should be explicit.
>>>(Or consider switching to the absolute accesses in the ByteBuffer and 
>>> not affect the position)
>> 
>> Is the additional javadoc line added earlier sufficient?
> I would bear some reinforcing that the entire remainder of the buffer is read
> and the from and to are indexes within that remainder.
> And I'm not sure that's the desirable semantics.
> 
> It would make more sense if the from bytes from the buffer are skipped
> and only the (to - from) bytes are dumped.  That leaves the ByteBuffer 
> reading only the requested bytes.  A natural usage such as:
>  dumpAsStream(ByteBuffer buffer, 0, 256, 16, formatter)
> would dump the next 256bytes of the ByteBuffer and position would be
> moved by 256.

[As suggested by Alan]
How about dropping the fromIndex and toIndex parameters and requiring the caller
to provide a suitably sized buffer? 

public static Stream dumpAsStream(ByteBuffer buffer, int chunkSize, 
Formatter formatter) {


> 
>> 
>> 
>>> 
>>> - dump(byte[], OutputStream) - I don't think the example is correct, there 
>>> is no reference
>>>   to a stream, only the PrintStream::println method, which is not static.
>> 
>> The code is just illustrative. Real values would have to be supplied for the 
>> input bytes and the
>> chosen print method on the output stream. Typically, that print method will 
>> be System.out::println
> Examples that don't compile are really confusing and annoying.

Updated the ‘as if’ code to:

 * byte[] bytes = ...
 * PrintStream out = ...
 * HexFormat.dumpAsStream(bytes, 16,
 * (offset, chunk, from, to) ->
 * String.format("%08x  %s  |%s|",
 * offset,
 * HexFormat.toFormattedString(chunk, from, to),
 * HexFormat.toPrintableString(chunk, from, to)))
 * .forEachOrdered(out::println);


> 
> Thanks, Roger
> 
>> 
>> 
>>> 
>>> Regards, Roger
>>> 
>>> 
>>> 
>>> On 12/07/2018 08:18 PM, Vincent Ryan wrote:
 Here’s the latest version that addresses all the current open issues:
 
 webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/

Re: RFR: 8215159: Improve initial setup of system Properties

HashMap sizing was a terrible design mistake, but too much software depends
on it, e.g.
https://google.github.io/guava/releases/23.0/api/docs/com/google/common/collect/Maps.html#newHashMapWithExpectedSize-int-

On Tue, Dec 11, 2018 at 1:26 AM Claes Redestad 
wrote:

> Hi Peter,
>
> On 2018-12-11 10:02, Peter Levart wrote:
> > Hi Claes,
> >
> > Haven't checked all changes yet, although it looks like a
> > straightforward swap of Properties for HashMap for intermediary result,
> > but I noticed the following in SystemProps:
> >
> >   265 var cmdProps = new HashMap > String>((vmProps.length / 2) + Raw.FIXED_LENGTH);
> >
> > The HashMap(int) constructor is different from Properties(int) in that
> > for the former, the argument represents the lower bound on the initial
> > size of the table (which is just a rounding of this parameter up to the
> > nearest power of 2). The threshold where the table is resized is
> > calculated as (initialCapacity rounded up to nearest power of 2) *
> > loadFactor. The default load factor is 0.75 which means that the table
> > will be resized in worst case after 3/4 * initialCapacity of elements
> > are inserted into it. In order to guarantee that the table is not
> > resized you have to pass (size * 4 + 2) / 3 to the HashMap constructor,
> > where size is the number of elements added...
> >
> > I hope I'm not misleading you, I just think this is how HashMap has been
> > from the beginning. Peeking at HashMap code (line 693) it seems that it
> > is doing that:
> >
> >  float ft = (float)newCap * loadFactor;
> >  newThr = (newCap < MAXIMUM_CAPACITY && ft <
> > (float)MAXIMUM_CAPACITY ?
> >(int)ft : Integer.MAX_VALUE);
> >
> > newCap above is holding the initialCapacity constructor parameter
> > rounded up to the nearest power of 2. newThr is the threshold at which
> > the resize occurs.
> >
> > The Properties(int) constuctor behaves differently as it passes the
> > parameter directly to the underlying ConcurrentHashMap, which says:
> >
> >   * @param initialCapacity The implementation performs internal
> >   * sizing to accommodate this many elements.
>
> Fun story, but I noticed this unfortunate difference when I was working
> on this very patch and brought it up in the team. I think most agrees
> the CHM behavior is the more intuitive and would have loved to
> consolidate to that behavior, but the message I've gotten is that it's
> probably too late to fix: Whichever way you go you get lots of little
> subtle changes to sizes that may lead to incompabilities in
> applications/tests that inadvertedly depend on the iteration order or
> HashMap etc..
>
> That said: Raw typically contains quite a few empty/null values that'll
> never be put in the map. Enough so that for the applications I've looked
> at the initialCapacity is already a fair bit higher than it needs to be
> to avoid resizing. Thus it made little sense to take the maximum
> possible size and adjust it up even further by factoring in the default
> load factor.
>
> (Unless you have a *lot* of properties coming in via command line, but I
> think we should optimize for the common cases...)
>
> Thanks!
>
> /Claes
>
> >
> > Regards, Peter
> >
> > On 12/10/18 10:17 PM, Claes Redestad wrote:
> >> Hi,
> >>
> >> by inverting the order in which the internal property maps are created,
> >> we avoid some classloading and get a slightly more efficient code
> >> execution profile in System.initPhase1.
> >>
> >> Webrev: http://cr.openjdk.java.net/~redestad/8215159/jdk.00/
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8215159
> >>
> >> This reduces bytecode executed in initPhase1 from ~48k to ~36k. Not
> >> much by any measure, but minimizing System.initPhase1 is important since
> >> certain parts of the VM (JIT etc) are blocked from initializing until
> >> it's done.
> >>
> >> Thanks!
> >>
> >> /Claes
> >
>

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words


Hi,

And the patch by itself is sufficient, inline or attached to the issue 
or the email.

No need to carry around the whole webrev.
And it is very handy to have an quick link to cr.openjdk.java.net

$.02, Roger

On 12/11/2018 10:46 AM, Volker Simonis wrote:

Hi Adam,

in order to prevent me from using swear words, could you please upload
your webrev to cr.openjdk.java.net :)

As you may have realized webrevs are a collection of HTML files and it
makes no big sense to provide them as a zip file.

Thank you and best regards,
Volker
On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8  wrote:

Hey All,

I've spotted 12 instances of swear words in OpenJDK source comments, and
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in more
excusable locations. It would be good to get the community's take on
those.

Reviews and opinions welcome. :)

Best Regards

Adam Farley
IBM Runtimes

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: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Jeff Dinkins

! // these icons are pretty crappy to use in Mac OS X since
  // they really are interactive but we have to return a static
  // icon for now.

->

! // these icons are difficult to use in Mac OS X since
  // they really are interactive but we have to return a static
  // icon for now.

Crappy != Difficult, it changes the semantics of the comment. 

“unfortunate” or maybe “a poor substitute” would be a better replacement.

-jeff

> On Dec 11, 2018, at 9:03 AM, Adam Farley8  wrote:
> 
> Hey All,
> 
> I've spotted 12 instances of swear words in OpenJDK source comments, and 
> it seems appropriate to remove them.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215217
> 
> I've created a webrev and attached to the bug.
> 
> Also, I've mentioned in the bug that there are additional swears in more 
> excusable locations. It would be good to get the community's take on 
> those.
> 
> Reviews and opinions welcome. :)
> 
> Best Regards
> 
> Adam Farley 
> IBM Runtimes
> 
> 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: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace

2018-12-11 Thread Gustavo Romero


Hi Martin,

On 12/11/2018 01:30 PM, Doerr, Martin wrote:

thanks for updating. I think it can get shipped when Gustavo is fine with it 
and jdk/submit tests have passed.


Yep, I'm working on that right now. I'll update soon.
So I have to push to jdk/submit, wait the test results and if it's fine I'll 
push to jdk/jdk after, right?



Note that changes pushed before Thursday 16:00 UTC will go into jdk12.


Got it.


Thank you.

Best regards,
Gustavo


Best regards,

Martin

*From:*Michihiro Horie 
*Sent:* Dienstag, 11. Dezember 2018 14:08
*To:* Doerr, Martin 
*Cc:* core-libs-dev@openjdk.java.net; Gustavo Romero ; 
hotspot-compiler-...@openjdk.java.net; Roger Riggs ; Vladimir Kozlov 
; Simonis, Volker 
*Subject:* RE: RFR: 8213754: PPC64: Add Intrinsics for 
isDigit/isLowerCase/isUpperCase/isWhitespace

Hi Martin,

Thank you so much for your review and pointing out the issue on flag enablement 
on PPC64.

Latest webrev enables the flag on PPC64 using has_darn in vm_version_ppc and it 
is used in match_rule_supported in the ad file.
http://cr.openjdk.java.net/~mhorie/8213754/webrev.06/ 



Best regards,
--
Michihiro,
IBM Research - Tokyo

Inactive hide details for "Doerr, Martin" ---2018/12/11 20:31:40---Hi Michihiro, the 
shared code looks good to me, now."Doerr, Martin" ---2018/12/11 20:31:40---Hi Michihiro, 
the shared code looks good to me, now.

From: "Doerr, Martin" mailto:martin.do...@sap.com>>
To: Vladimir Kozlov mailto:vladimir.koz...@oracle.com>>, Michihiro Horie 
mailto:ho...@jp.ibm.com>>, Gustavo Romero mailto:grom...@linux.vnet.ibm.com>>
Cc: "core-libs-dev@openjdk.java.net " mailto:core-libs-dev@openjdk.java.net>>, "hotspot-compiler-...@openjdk.java.net " 
mailto:hotspot-compiler-...@openjdk.java.net>>, Roger Riggs mailto:roger.ri...@oracle.com>>, "Simonis, Volker" mailto:volker.simo...@sap.com>>
Date: 2018/12/11 20:31
Subject: RE: RFR: 8213754: PPC64: Add Intrinsics for 
isDigit/isLowerCase/isUpperCase/isWhitespace

--




Hi Michihiro,

the shared code looks good to me, now.

Looks like the PPC64 is not consistent any more.
Where do you enable UseCharacterCompareIntrinsics on PPC64?
Why aren't you using it for match_rule_supported in the ad file?
I think has_darn could be used to enable it in vm_version_ppc.

Best regards,
Martin


-Original Message-
From: Vladimir Kozlov mailto:vladimir.koz...@oracle.com>>
Sent: Montag, 10. Dezember 2018 20:33
To: Michihiro Horie mailto:ho...@jp.ibm.com>>; Gustavo Romero 
mailto:grom...@linux.vnet.ibm.com>>
Cc: core-libs-dev@openjdk.java.net ; hotspot-compiler-...@openjdk.java.net 
; Doerr, Martin mailto:martin.do...@sap.com>>; Roger Riggs mailto:roger.ri...@oracle.com>>; 
Simonis, Volker mailto:volker.simo...@sap.com>>
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for 
isDigit/isLowerCase/isUpperCase/isWhitespace

On 12/9/18 5:28 PM, Michihiro Horie wrote:

Hi Vladimir,

Thanks a lot for your review. I also fixed the copyright in intrinsicnode.hpp.

 >Did you look on code generated by C2 with your latest changes?
Yes,I usually check generated code with Oprofile and they were as expected like:
:
90 0.0905 : 3fff64c27654: rlwinm r12,r9,24,8,31
12 0.0121 : 3fff64c27658: li r11,14640
15 0.0151 : 3fff64c2765c: cmprb cr5,0,r31,r11
5527 5.5587 : 3fff64c27660: setb r11,cr5
36010 36.2164 : 3fff64c27664: stb r11,16(r15)


Good.


:

I found a CSR FAQ that mentions adding a diagnostic flag do not need CSR 
process. This time I do not need CSR.
https://wiki.openjdk.java.net/display/csr/CSR+FAQs


Thank is correct.



Hi Gustavo,
Could I ask you to sponsor the latest change webrev.05? I would like you to 
test it on your P9 node too.


Thanks,
Vladimir




Best regards,
--
Michihiro,
IBM Research - Tokyo

Inactive hide details for Vladimir Kozlov ---2018/12/08

Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi Naoto,

Since the value changes from time to time, it would give it some visibility
if it were defined using a private final int  (or float)
 private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using reflection.
But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:

Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size of 
Character.UnicodeBlock.


Naoto

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Pavel Rappo

Sweet! Learning English with the OpenJDK community.

You sir, probably, have a degree in public relations and/or marketing. This was
*the best* way to draw attention to those swear words. Had these words stayed
unexposed, no one would have been bothered by them. I guess...

On a serious note, I wouldn't change cave art just because we might find it
inappropriate, it's history now. 

> On 11 Dec 2018, at 15:32, Alan Bateman  wrote:
> 
> On 11/12/2018 15:03, Adam Farley8 wrote:
>> Hey All,
>> 
>> I've spotted 12 instances of swear words in OpenJDK source comments, and
>> it seems appropriate to remove them.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8215217
>> 
>> I've created a webrev and attached to the bug.
>> 
>> Also, I've mentioned in the bug that there are additional swears in more
>> excusable locations. It would be good to get the community's take on
>> those.
>> 
>> Reviews and opinions welcome. :)
> "Where's that damn torpedo?" might be from Star Trek.

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Volker Simonis

Hi Adam,

in order to prevent me from using swear words, could you please upload
your webrev to cr.openjdk.java.net :)

As you may have realized webrevs are a collection of HTML files and it
makes no big sense to provide them as a zip file.

Thank you and best regards,
Volker
On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8  wrote:
>
> Hey All,
>
> I've spotted 12 instances of swear words in OpenJDK source comments, and
> it seems appropriate to remove them.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215217
>
> I've created a webrev and attached to the bug.
>
> Also, I've mentioned in the bug that there are additional swears in more
> excusable locations. It would be good to get the community's take on
> those.
>
> Reviews and opinions welcome. :)
>
> Best Regards
>
> Adam Farley
> IBM Runtimes
>
> 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: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Adam Farley8

Alan Bateman  wrote on 11/12/2018 15:32:31:

> From: Alan Bateman 
> To: Adam Farley8 , core-libs-dev  d...@openjdk.java.net>
> Date: 11/12/2018 15:33
> Subject: Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words
> 
> On 11/12/2018 15:03, Adam Farley8 wrote:
> > Hey All,
> >
> > I've spotted 12 instances of swear words in OpenJDK source comments, 
and
> > it seems appropriate to remove them.
> >
> > Bug: INVALID URI REMOVED
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8215217=DwICaQ=jf_iaSHvJObTbx-
> siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=qU1GS7jvxOg5rsMfdgT3_qee4ZirvY2cajsOhIza2y8=R4gzHBoqzCQdmJLCSmPyLfRrHH-5xtvUArpwaaXk2Ck=
> >
> > I've created a webrev and attached to the bug.
> >
> > Also, I've mentioned in the bug that there are additional swears in 
more
> > excusable locations. It would be good to get the community's take on
> > those.
> >
> > Reviews and opinions welcome. :)
> "Where's that damn torpedo?" might be from Star Trek.
>

Yep, Kirk said it.

Perhaps "d**n" or "darn" would better preserve the quote?

- Adam
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: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

2018-12-11 Thread Baesken, Matthias

> File paths are, in general, always something that demands extra scrutiny
> as it can be the source of security issues (privacy leaks, traversal
> attacks, etc). It's not just me that thinks that way, you can do a
> search on the Internet and find lots of references.
...
> 
> It might be perfectly fine and your usage might be ok too. But I'll need
> some time to evaluate it further. I am not familiar with the code in
> this part of the JDK.
> 
> I would also see if you can think about the security issues as well.
> Where do these paths come from? What are the application use cases that
> invoke these lower methods? From application code or elsewhere? Are
> relative paths used? Are paths containing ".." canonicalized? Are they
> arbitrary paths or a fixed set of paths? Do they ever contain sensitive
> information like home directory user names, etc?
> 
> Once we understand if there are any security issues, then we can decide
> if allowing that via a system property is acceptable or not, and if so
> the security risks that we would have to document for that property.
> 

Hello, the file paths potentially printed in the enhanced exceptions  in 
*canonicalize0, *createFileExclusively,
Java_java_io_WinNTFileSystem_canonicalizeWithPrefix0 could potentially come 
from  more or less arbitrary paths.

This means, in some cases, there is a chance that information (like 
user-ids/user-names often found in paths below  HOME dirs )
might be in the printed paths that people do not want to have in log files or 
other output containing the exception messages.

For such potentially sensitive info in exception messages, we have already the  
jdk.includeInExceptions  
security property, see the java.security file :

1064 # Enhanced exception message information
1065 #
1066 # By default, exception messages should not include potentially sensitive
1067 # information such as file names, host names, or port numbers. This 
property
1068 # accepts one or more comma separated values, each of which represents a
1069 # category of enhanced exception message information to enable. Values are
1070 # case-insensitive. Leading and trailing whitespaces, surrounding each 
value,
1071 # are ignored. Unknown values are ignored.

I think this approach fits well for 8211752 (and is used already for some other 
categories).

I created an updated webrev,  it uses the jdk.includeInExceptions security 
property 
(had to do it via JNI because it did not work from the static class 
initializers of UnixFileSystem/WinNTFileSystem) :

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


Best regards, Matthias




> -Original Message-
> From: Sean Mullan 
> Sent: Donnerstag, 8. November 2018 20:36
> To: Baesken, Matthias ; Langer, Christoph
> ; Alan Bateman ;
> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> On 11/7/18 3:52 AM, Baesken, Matthias wrote:
> >> Sorry, I haven't had time to look at this in more detail yet. But, let's
> >> take a step back first. Can you or Matthias explain in more detail why
> >> this fix is necessary? What are the use cases and motivation?
> >
> > Hello,
> >  adding paths  (or maybe more details)  to exception messages just
> makes analyzing errors easier.
> > You do not get just "Bad path",  but also the real bad path which gives you 
> > a
> hint where to look and analyze further .
> 
> File paths are, in general, always something that demands extra scrutiny
> as it can be the source of security issues (privacy leaks, traversal
> attacks, etc). It's not just me that thinks that way, you can do a
> search on the Internet and find lots of references.
> 
> > That's why we introduced it in our JVM ages ago.
> > I have to agree that additionally  printing cwd / user.dir is a bit 
> > special,  so I
> omit that from this revision of the patch.
> > This makes the patch more simple.
> > New revision :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.1/
> >
> >
> > Unfortunately the usage of sun.security.util.SecurityProperties  (which was
> considered)  in the  static { ... }
> > class initializers (e.g. UnixFileSystem.java) just does not work.
> > It fails with already in the build (!) with :
> >
> > Error occurred during initialization of boot layer
> > java.lang.ExceptionInInitializerError
> > Caused by: java.lang.NullPointerException
> >
> > (seems it is too early in the game for SecurityProperties).
> > (btw. this is another  NOT very helpful exception error message)
> >
> >
> > So I unfortunately had to go back to using system properties.
> >
> >
> > Btw. another question regarding path output in exceptions  :
> > you seem to consider it a bad thing  to (unconditionally) print paths in the
> exception messages,
> > but then on the other hand we have  it  already in OpenJDK
> UnixFileSystem_md.c :
> >
> >   269 JNIEXPORT jboolean JNICALL
> >   270

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Alan Bateman


On 11/12/2018 15:03, Adam Farley8 wrote:

Hey All,

I've spotted 12 instances of swear words in OpenJDK source comments, and
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in more
excusable locations. It would be good to get the community's take on
those.

Reviews and opinions welcome. :)

"Where's that damn torpedo?" might be from Star Trek.

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

2018-12-11 Thread Volker Simonis

On Tue, Dec 11, 2018 at 3:35 PM Magnus Ihse Bursie
 wrote:
>
>
>
> On 2018-12-11 00:23, David Holmes wrote:
> > Hi Magnus,
> >
> > On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
> >> I propose that we introduce a new define, available to all JDK native
> >> files (Hotspot included), called JDK_EXPORT. The behavior of this
> >> symbol will be very similar (as of now, in fact identical) to
> >> JNIEXPORT; however, the semantics will not.
> >>
> >> Currently, we "mis-use" the JNIEXPORT define to mark a function for
> >> exporting from the library. The problem with this is that JNIEXPORT
> >> is part of the JNI interface, and is supposed to be used when C
> >> programs interact with Java. And, when doing this, the function
> >> should be fully decorated like this: "JNIEXPORT foo JNICALL".
> >
> > I've seen a lot of the emails on this issue and I don't fully
> > understand what has been going wrong. But the intent is obviously the
> > JNIEXPORT represents what is needed to export this function for use by
> > JNI, while JNICALL defines the calling convention. I agree there may
> > be some mistmatch when functions are actually not intended for general
> > export outside the JDK but are only for internal JDK use.
> >
> >> We do have many such JNI exports in our native libraries, but we also
> >> have a lot of other, non-JNI exports, where one native library just
> >> provides an interface to other libraries. In these cases, we have
> >> still used JNIEXPORT for the functionality of getting the function
> >> exported, but we have not been consistent in our use of JNICALL. This
> >> has caused us way too much trouble for something that should Just
> >> Work.
> >
> > Are you suggesting that the interface between different libraries in
> > the JDK should not be a JNI interface? Is this because you think the
> > functions in these libraries are only for JDK internal use or ... ??
> >
> >> I therefore propose that we define "JDK_EXPORT", with the same
> >> behavior as JNIEXPORT (that is, flagging the function for external
> >> visibility in the resulting native library), but which is *not*
> >> supposed to be exported to Java code using JNI, nor supposed to be
> >> decorated with
> >
> > Just a clarification there. JNI functions are not exported to Java
> > code, they are exported to native code. Java code can declare native
> > methods and those native methods must be written as JNI functions, but
> > that's not what we are discussing. Libraries expose a JNI interface (a
> > set of functions in the library) that can be called by application
> > native code, using JNI.
> We're apparently looking at "JNI" and "exporting" from two opposite
> sides here. :-) Just to make everything clear: If I have a Java class
> class MyClass {
>public static void native myNativeFunc();
> }
>
> then I have one half of the JNI function, the Java half. This must be
> matched by a corresponding implementation in native, like this:
> JNIEXPORT void JNICALL
> Java_MyClass_myNativeFunc(void) {
> // ... do stuff
> }
>
> And this is the native half of the JNI function. Right? Let's leave
> aside which side is "exporting" to the other for now. :-)
>
> This way of setting up native functions that can be called from Java is
> what I refer to as JNI. And when you declare a native JNI function, you
> *must* use both JNIEXPORT and JNICALL. Alright?
>
> We do have a lot of those functions in our native libraries. And they
> are correct just the way they are.
>
> However, we also have *other* native functions, that are exported, not
> as JNI functions that should be called from Java, but as normal native
> library functions that should be called by other native code. Okay so
> far? And *those* functions have been problematic in how we decorate
> them. My proposal is that we *refrain* from using JNIEXPORT for those
> functions, and instead use JDK_EXPORT as name for the macro that
> decorates them as exported. That way, we can clearly see that a function
> like this:
>
> JDK_EXPORT void
> JLI_ReadEnv(char* env);
>
> is correctly declared, and will be exported to other native libraries,
> but not to Java.
>

Hi Magnus,

I agree with your explanation and I think your proposal is sound.

Have you checked how many of the occurrences of "#include "jni.h"" can
really be replaced by "#include "jdk.h""?

I think native code also "misuses" jni.h in order to agree on common
types like jint, jlong, etc.. across different native libraries. We
could of course also define such common types in "jdk.h", but I'm not
sure if it's worth the effort?

Regards,
Volker

> Just to clarify, this has nothing to do with if this is a officially
> supported API or not. In general though, I assume that most (if not
> all?) of our exported functions (apart from the JNI_* stuff) is supposed
> to be consumed by other libraries in the JDK, and is not a public API.
>
> /Magnus
>
>
>
> >
> >> JNICALL. All current instances of JNIEXPORT which is not pure JNI
> >> native functions

Re: [PATCH] Windows 32-bit DLL name decoration

2018-12-11 Thread Alexey Ivanov


Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit 
Windows jdwpTransport_OnLoad can exported with its plain and 
__stdcall-mangled name. I used conditional compilation so that for other 
platforms the code remains as it is now.


jshell starts successfully with this fix; an IDE debugger works as well.


Regards,
Alexey

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

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:

Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the 
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32 
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Adam Farley8

Hey All,

I've spotted 12 instances of swear words in OpenJDK source comments, and 
it seems appropriate to remove them.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215217

I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in more 
excusable locations. It would be good to get the community's take on 
those.

Reviews and opinions welcome. :)

Best Regards

Adam Farley 
IBM Runtimes

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

[12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect


Hi,

Please review the fix for the following issue:

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

The proposed fix is located at:

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

This one line fix is for the correctness of the initial map size of 
Character.UnicodeBlock.


Naoto

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

2018-12-11 Thread Magnus Ihse Bursie





On 2018-12-11 00:23, David Holmes wrote:

Hi Magnus,

On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK native 
files (Hotspot included), called JDK_EXPORT. The behavior of this 
symbol will be very similar (as of now, in fact identical) to 
JNIEXPORT; however, the semantics will not.


Currently, we "mis-use" the JNIEXPORT define to mark a function for 
exporting from the library. The problem with this is that JNIEXPORT 
is part of the JNI interface, and is supposed to be used when C 
programs interact with Java. And, when doing this, the function 
should be fully decorated like this: "JNIEXPORT foo JNICALL".


I've seen a lot of the emails on this issue and I don't fully 
understand what has been going wrong. But the intent is obviously the 
JNIEXPORT represents what is needed to export this function for use by 
JNI, while JNICALL defines the calling convention. I agree there may 
be some mistmatch when functions are actually not intended for general 
export outside the JDK but are only for internal JDK use.


We do have many such JNI exports in our native libraries, but we also 
have a lot of other, non-JNI exports, where one native library just 
provides an interface to other libraries. In these cases, we have 
still used JNIEXPORT for the functionality of getting the function 
exported, but we have not been consistent in our use of JNICALL. This 
has caused us way too much trouble for something that should Just 
Work.


Are you suggesting that the interface between different libraries in 
the JDK should not be a JNI interface? Is this because you think the 
functions in these libraries are only for JDK internal use or ... ??


I therefore propose that we define "JDK_EXPORT", with the same 
behavior as JNIEXPORT (that is, flagging the function for external 
visibility in the resulting native library), but which is *not* 
supposed to be exported to Java code using JNI, nor supposed to be 
decorated with 


Just a clarification there. JNI functions are not exported to Java 
code, they are exported to native code. Java code can declare native 
methods and those native methods must be written as JNI functions, but 
that's not what we are discussing. Libraries expose a JNI interface (a 
set of functions in the library) that can be called by application 
native code, using JNI.
We're apparently looking at "JNI" and "exporting" from two opposite 
sides here. :-) Just to make everything clear: If I have a Java class

class MyClass {
  public static void native myNativeFunc();
}

then I have one half of the JNI function, the Java half. This must be 
matched by a corresponding implementation in native, like this:

JNIEXPORT void JNICALL
Java_MyClass_myNativeFunc(void) {
// ... do stuff
}

And this is the native half of the JNI function. Right? Let's leave 
aside which side is "exporting" to the other for now. :-)


This way of setting up native functions that can be called from Java is 
what I refer to as JNI. And when you declare a native JNI function, you 
*must* use both JNIEXPORT and JNICALL. Alright?


We do have a lot of those functions in our native libraries. And they 
are correct just the way they are.


However, we also have *other* native functions, that are exported, not 
as JNI functions that should be called from Java, but as normal native 
library functions that should be called by other native code. Okay so 
far? And *those* functions have been problematic in how we decorate 
them. My proposal is that we *refrain* from using JNIEXPORT for those 
functions, and instead use JDK_EXPORT as name for the macro that 
decorates them as exported. That way, we can clearly see that a function 
like this:


JDK_EXPORT void
JLI_ReadEnv(char* env);

is correctly declared, and will be exported to other native libraries, 
but not to Java.


Just to clarify, this has nothing to do with if this is a officially 
supported API or not. In general though, I assume that most (if not 
all?) of our exported functions (apart from the JNI_* stuff) is supposed 
to be consumed by other libraries in the JDK, and is not a public API.


/Magnus





JNICALL. All current instances of JNIEXPORT which is not pure JNI 
native functions should be changed to use JDK_EXPORT instead.


I further propose that this macro should reside in a new file 
"jdk.h", placed in the new directory 
src/java.base/share/native/include/internal. This header file path 
will automatically be provided to all native libraries, but not 
copied to the JDK being built. (The existence of a "include/internal" 
directory with this behavior has been discussed before. There are 
more files that ought to be moved there, if/when it is created.) I 
believe in many cases the #include "jni.h" can be just modified to 
#include "#jdk.h", since most native code will not require "jni.h" 
unless actually doing JNI calls -- most have included this file to 
get the JNIEXPORT macro, which would explain the

Re: Add convenience collect methods to the Stream interface

2018-12-11 Thread Remi Forax

And i've forgotten to add that currently calling stream.iterator() is slw,
i don't know if it's just because the implementation of this method did not get 
some love or it's a more fundamental issue because a spliterator is push while 
an iterator is pull so the implementation needs an intermediary memory to store 
the element in between.

Rémi

- Mail original -
> De: "Remi Forax" 
> À: "Peter Levart" 
> Cc: "Rob Griffin, rgriffin" , "core-libs-dev" 
> 
> Envoyé: Mardi 11 Décembre 2018 15:18:24
> Objet: Re: Add convenience collect methods to the Stream interface

> Hi Rob, Hi Petter,
> we (the lambda EG) have decided against implementing Iterable (see the answer 
> of
> Brian), so for consistency we have also not enable the right hand side of a
> enhanced for loop to be a poly-expression (that enables the lambda 
> conversion),
> hence the mandatory cast.
> 
> Rémi
> 
> - Mail original -
>> De: "Peter Levart" 
>> À: "Rob Griffin, rgriffin" , "Remi Forax"
>> 
>> Cc: "core-libs-dev" 
>> Envoyé: Mardi 11 Décembre 2018 14:42:51
>> Objet: Re: Add convenience collect methods to the Stream interface
> 
>> Hi Rob,
>> 
>> On 12/10/18 11:11 PM, Rob Griffin (rgriffin) wrote:
>>> Hi Remi,
>>>
>>> There are certainly places where we could do this when we are simply 
>>> iterating
>>> over the results but that is not always the case. However I was 
>>> disappointed to
>>> find that the enhanced for loop can't iterate over a stream so if callers of
>>> your example methods where doing something like this
>>>
>>> for (Employee emp : getAllEmployee()) {
>>>...
>>> }
>>>
>>> then it would have to change to a forEach call if getAllEmployee returned a
>>> Stream.
>> 
>> You can also get an Iterator from a Stream, so if you need external
>> iteration over elements of a Stream you don't have to collect it 1st to
>> some Collection:
>> 
>>     Stream names() {
>>     return Stream.of("John", "Jil", "Jack");
>>     }
>> 
>> ...and then...
>> 
>>     for (String name : (Iterable) names()::iterator) {
>>     System.out.println(name);
>>     }
>> 
>> This is hack-ish as it relies on the fact that enhanced for loop calls
>> Iterable.iterator() method only once, but is the only way to do it if
>> you already have a reference to Stream at hand. This would be more
>> correct way of doing it if you can call a factory for Stream:
>> 
>>     for (String name : (Iterable) () -> names().iterator()) {
>>     System.out.println(name);
>>     }
>> 
>> Regards, Peter
>> 
>> P.S. I wonder why the enhanced for loop doesn't establish a context
>> where the type of expression after the colon could be inferred, so no
>> cast would be necessary. Perhaps because that type could either be an
> > Iterable or a T[] ?

Re: JDK-8211844 [aix] ProcessBuilder: Piping between created processes does not work.


Yes please, with the Solaris update.

Thanks, Roger

On 12/11/2018 02:52 AM, Volker Simonis wrote:

Hi Roger,

just to clarify - do you want us to push the last version [1] which
adds "|| forceNullOutputStream)" to the Solaris version as well?

Thank you and best regards,
Volker

[1] http://cr.openjdk.java.net/~sgroeger/jtreg/8211844/webrev.01/
On Mon, Dec 10, 2018 at 6:30 PM Roger Riggs  wrote:

Hi,

The change looks fine and passes the current tests.
So ok to push.

I think the test isn't correct, but I have not yet worked up a revised test.

Thanks, Roger

On 12/10/2018 09:00 AM, Lindenmaier, Goetz wrote:

Hi Steve,

I will push it once Roger gives his ok.

Best regards,
Goetz.


-Original Message-
From: Steve Groeger 
Sent: Montag, 10. Dezember 2018 14:42
To: Lindenmaier, Goetz 
Cc: core-libs-dev@openjdk.java.net; Roger Riggs 
Subject: RE: JDK-8211844 [aix] ProcessBuilder: Piping between created
processes does not work.

Hi Goetz,

It is good that the tests you ran passed. What needs to be done now to get
this change pushed into the main code?

Thanks
Steve Groeger
IBM Runtime Technologies
Hursley, Winchester
Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
Fax (44) 1962 816800
Lotus Notes: Steve Groeger/UK/IBM
Internet: groe...@uk.ibm.com

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



From:"Lindenmaier, Goetz" 
To:Steve Groeger , Roger Riggs

Cc:"core-libs-dev@openjdk.java.net" 
Date:10/12/2018 10:06
Subject:RE: JDK-8211844 [aix] ProcessBuilder: Piping between created
processes does not work.






Hi,

I ran the fix through our tests. There are no new regressions, and the
addressed test works.

So reviewed from my side.

I increased the bug to P3 so we can push it to jdk12 in case we
miss Thursday.

Best regards,
   Goetz.


-Original Message-
From: core-libs-dev  On Behalf

Of

Steve Groeger
Sent: Freitag, 7. Dezember 2018 19:08
To: Roger Riggs 
Cc: core-libs-dev@openjdk.java.net
Subject: Re: JDK-8211844 [aix] ProcessBuilder: Piping between created
processes does not work.

Hi Roger,

I have made the same change to the Solaris code and also removed the test
from the ProblemList.txt
I have created a webrev here:
http://cr.openjdk.java.net/~sgroeger/jtreg/8211844/webrev.01/



Hope you can now test t

Thanks
Steve Groeger
IBM Runtime Technologies
Hursley, Winchester
Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
Fax (44) 1962 816800
Lotus Notes: Steve Groeger/UK/IBM
Internet: groe...@uk.ibm.com

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



From:   Roger Riggs 
To: core-libs-dev@openjdk.java.net
Date:   07/12/2018 14:55
Subject:Re: JDK-8211844 [aix] ProcessBuilder: Piping between
created processes does not work.
Sent by:"core-libs-dev" 



Hi,

I notice that the Solaris case also does not include "||
forceNullOutputStream".
I'll have to investigate why the Pipeline test didn't fail on Solaris.

Please add that to the patch and I'll run it through our tests.

Thanks, Roger

On 12/07/2018 03:05 AM, Volker Simonis wrote:

Hi Steve,

thanks a lot for this fix. I'm forwarding this to core-libs-dev as
well, because that's where the review has to take place. The
"ppc-aix-port-dev" list is mostly a marker for the port maintainers to
get their attention on relevant changes (so cross-posting is fine in
this case :)

On Thu, Dec 6, 2018 at 4:26 PM Steve Groeger 

wrote:

Hi all,

I have been investigating the issue

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



raised by Goetz Lindenmaier which is related to the

jdk/java/lang/ProcessBuilder/PipelineTest.java JTReg test failing on

AIX. Having done some investigation I have a potential fix fore the issue:

diff -r 9501a7b59111

src/java.base/unix/classes/java/lang/ProcessImpl.java

--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java Mon Dec

03 14:28:19 2018 +0300

+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java Thu Dec

06 15:01:03 2018 +

@@ -446,7 +446,7 @@
ProcessBuilder.NullOutputStream.INSTANCE :
new ProcessPipeOutputStream(fds[0]);

-stdout = (fds[1] == -1) ?
+stdout = (fds[1] == -1 || forceNullOutputStream) ?
 ProcessBuilder.NullInputStream.INSTANCE :
 new

DeferredCloseProcessPipeInputStream(fds[1]);

Your change looks good and I can sponsor it. Just as a hint for other
reviewers I'd like to mention that this change, albeit in a shared
Java

Re: Add convenience collect methods to the Stream interface

2018-12-11 Thread forax

Hi Rob, Hi Petter, 
we (the lambda EG) have decided against implementing Iterable (see the answer 
of Brian), so for consistency we have also not enable the right hand side of a 
enhanced for loop to be a poly-expression (that enables the lambda conversion), 
hence the mandatory cast.

Rémi

- Mail original -
> De: "Peter Levart" 
> À: "Rob Griffin, rgriffin" , "Remi Forax" 
> 
> Cc: "core-libs-dev" 
> Envoyé: Mardi 11 Décembre 2018 14:42:51
> Objet: Re: Add convenience collect methods to the Stream interface

> Hi Rob,
> 
> On 12/10/18 11:11 PM, Rob Griffin (rgriffin) wrote:
>> Hi Remi,
>>
>> There are certainly places where we could do this when we are simply 
>> iterating
>> over the results but that is not always the case. However I was disappointed 
>> to
>> find that the enhanced for loop can't iterate over a stream so if callers of
>> your example methods where doing something like this
>>
>> for (Employee emp : getAllEmployee()) {
>>...
>> }
>>
>> then it would have to change to a forEach call if getAllEmployee returned a
>> Stream.
> 
> You can also get an Iterator from a Stream, so if you need external
> iteration over elements of a Stream you don't have to collect it 1st to
> some Collection:
> 
>     Stream names() {
>     return Stream.of("John", "Jil", "Jack");
>     }
> 
> ...and then...
> 
>     for (String name : (Iterable) names()::iterator) {
>     System.out.println(name);
>     }
> 
> This is hack-ish as it relies on the fact that enhanced for loop calls
> Iterable.iterator() method only once, but is the only way to do it if
> you already have a reference to Stream at hand. This would be more
> correct way of doing it if you can call a factory for Stream:
> 
>     for (String name : (Iterable) () -> names().iterator()) {
>     System.out.println(name);
>     }
> 
> Regards, Peter
> 
> P.S. I wonder why the enhanced for loop doesn't establish a context
> where the type of expression after the colon could be inferred, so no
> cast would be necessary. Perhaps because that type could either be an
> Iterable or a T[] ?

Re: Add convenience collect methods to the Stream interface

2018-12-11 Thread Brian Goetz

You say it jokingly, but we've explored this.  (The exact way you phrase 
it (the supertype specified to throw if called more than once) means 
that Iterable can't extend IterableOnce, because then Iterable does not 
conform to the superclass contract, but there are other ways to stack 
it.)  It's not completely unworkable, but as you've observed, it's not 
so clean.


In the JSR-335 EG, we also explored (and rejected, because it felt like 
the library tail wagging the language dog) the route of adding language 
support for more kinds of things on the RHS of the for-each loop.


(Zooming out: the main reason this is irritating is the limitations of 
what you can do in a method like .forEach() vs the foreach loop -- 
exception transparency, nonlocal return, up-scope local mutation.  
Though it is unlikely that we'll have great solutions for these all that 
soon, so its reasonable to consider library-based solutions in the 
meantime.)


On 12/11/2018 7:55 AM, Rachel Greenham wrote:
Although I do understand the reasoning (non-repeatability of streams), 
Stream not implementing Iterable is a minor but frequent annoyance for 
this precise reason. Using forEach() instead isn't always an option.


Maybe Iterable can have a superinterface IterableOnce with the methods 
moved up to there, and iterator() specified to throw 
IllegalStateException if called twice, and which enhanced-for 
recognises, then Stream can implement that? (Ducks and runs...)


Or enhanced-for can also take Supplier, so we can do 'for 
(Thing e : stream::iterator)' instead of 'for (Thing e : 
(Iterable)stream::iterator)'


(runs faster...)

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

2018-12-11 Thread Sverre Moe

The help output of jpackager should mention how to set the classpath for
the various bundle resource files.
I have not found a working solution how to set the classpath.
  jpackager -classpath build/deploy/
  jpackager -cp build/deploy/

DEB
Using default package resource [menu icon]  (add
package/linux/application.png to the class path to customize)
Using default package resource [Menu shortcut descriptor]  (add
package/linux/application.desktop to the class path to customize)
Using default package resource [DEB control file]  (add
package/linux/control to the class path to customize)

RPM
Using default package resource [menu icon]  (add
package/linux/application.png to the class path to customize)
Using default package resource [Menu shortcut descriptor]  (add
package/linux/application.desktop to the class path to customize)
Using default package resource [RPM spec file]  (add
package/linux/application.spec to the class path to customize)

The application image can be set with the --icon argument.
--icon build/deploy/package/linux/application.png
Using custom package resource [menu icon]  (loaded from file
/home/user/workspace/application/build/deploy/package/linux/application.png)


Perhaps jpackager should have an package directory argument
--package-dir build/deploy


/Sverre


Den tor. 15. nov. 2018 kl. 15:05 skrev Andy Herrick :

>
> On 11/10/2018 8:12 AM, Sverre Moe wrote:
>
> I have been using the jpackager that Johan Vos backported for OpenJDK 11.
> For this I have some points of improvement I would like to mention.
>
> 1)
> The control file for debian package does not set correct description
>
> --name test
> --description This is a Test Application
>
> /tmp/jdk.packager607148779833718376/linux/control
> Package: test
> Description: test
>
> The RPM gets it correctly
> Summary : test
> Description :
> This is a Test Application
>
>
> 2)
> Category is not set on either DEB or RPM
>   --category
>   Category or group of the application.
> --category "Some/Category/Application"
> Group: Unspecified
> Section: unknown
>
> 3)
> The jpackager command line should have the flag --release in addition to
> --version. The only way to set a different release than "1" is to supply a
> custom spec file for RPM and control file for DEB.
> package/linux/test.spec
> Version: 1.0.0
> Release: RC1
> package/linux/control
> Version: 1.0.0-RC1
>
> I have filed issue JDK-8213941
>  and we will be looking
> into it.
>
> Thanks.
>
> /Andy
>
>
> /Sverre
>
>

Re: Add convenience collect methods to the Stream interface

2018-12-11 Thread Peter Levart


Hi Rob,

On 12/10/18 11:11 PM, Rob Griffin (rgriffin) wrote:

Hi Remi,

There are certainly places where we could do this when we are simply iterating 
over the results but that is not always the case. However I was disappointed to 
find that the enhanced for loop can't iterate over a stream so if callers of 
your example methods where doing something like this

for (Employee emp : getAllEmployee()) {
   ...
}

then it would have to change to a forEach call if getAllEmployee returned a 
Stream.


You can also get an Iterator from a Stream, so if you need external 
iteration over elements of a Stream you don't have to collect it 1st to 
some Collection:


    Stream names() {
    return Stream.of("John", "Jil", "Jack");
    }

...and then...

    for (String name : (Iterable) names()::iterator) {
    System.out.println(name);
    }

This is hack-ish as it relies on the fact that enhanced for loop calls 
Iterable.iterator() method only once, but is the only way to do it if 
you already have a reference to Stream at hand. This would be more 
correct way of doing it if you can call a factory for Stream:


    for (String name : (Iterable) () -> names().iterator()) {
    System.out.println(name);
    }

Regards, Peter

P.S. I wonder why the enhanced for loop doesn't establish a context 
where the type of expression after the colon could be inferred, so no 
cast would be necessary. Perhaps because that type could either be an 
Iterable or a T[] ?

Re: RFR: 8170769 Provide a simple hexdump facility for binary data

2018-12-11 Thread Alan Bateman

On 08/12/2018 01:18, Vincent Ryan wrote:

Here’s the latest version that addresses all the current open issues:

webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/

javadoc:
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html

CSR: https://bugs.openjdk.java.net/browse/CCC-8170769

I read through the latest javadoc corresponding to webrev.07.

Overall I think it looks very good except for dumpAsStream(ByteBuffer,
fromIndex, toIndex, chunkSize, Formatter) as it's not clear if fromIndex
is from the buffer position or an absolute index. If the former then
there are a couple of other issues. I see Roger has touched on the
advancement of the buffer position to its limit which isn't right unless
all remaining bytes in the buffer are consumer. There is also an issue
with the exception as attempting to consume beyond the limit is a
BufferUnderflowException. It might be simpler to replace this method
with a dumpAsStream(ByteBuffer, chunkSize, Formatter) that can lazily
(rather than eagerly) consume the remaining bytes. Additional overloads
could be added in the future if needed.

dumpAsStream(InputStream) specifies "On return, the input stream will be
at end-of-stream" but I assume this isn't right as the method is lazy.

The 3-arg dumpAsStream doesn't specify the exception/behavior for when
chunkSize is <= 0.

fromString(CharSequence) may need clarification on how it behaves when
the CS is a CharBuffer. Does it consume all the remaining chars in the
buffer so its position be advanced to the limit? The 3-arg version is
also not clear on this point.

In Formatter interface it may be useful to expand the description of the
"offset" parameter as readers may not immediately understand that it's
just an input for the formatted string rather than a real offset, an
example might help. It also doesn't say if the offset can be specified
as <= 0L or not.

A minor comment is that several places refer to a byte array as a "byte
buffers. Is this deliberate or can these be replaced with "byte array"
to avoid confusion. Another minor comment is that one of the
dumpAsStream methods is missing @throws NPE - maybe they should all be
removed and replaced with a statement in the class description.

-Alan

Re: Add convenience collect methods to the Stream interface

2018-12-11 Thread Rachel Greenham

Although I do understand the reasoning (non-repeatability of streams), 
Stream not implementing Iterable is a minor but frequent annoyance for 
this precise reason. Using forEach() instead isn't always an option.


Maybe Iterable can have a superinterface IterableOnce with the methods 
moved up to there, and iterator() specified to throw 
IllegalStateException if called twice, and which enhanced-for 
recognises, then Stream can implement that? (Ducks and runs...)


Or enhanced-for can also take Supplier, so we can do 'for 
(Thing e : stream::iterator)' instead of 'for (Thing e : 
(Iterable)stream::iterator)'


(runs faster...)

--
Rachel

On 10/12/2018 22:11, Rob Griffin (rgriffin) wrote:

Hi Remi,

There are certainly places where we could do this when we are simply iterating 
over the results but that is not always the case. However I was disappointed to 
find that the enhanced for loop can't iterate over a stream so if callers of 
your example methods where doing something like this

for (Employee emp : getAllEmployee()) {
   ...
}

then it would have to change to a forEach call if getAllEmployee returned a 
Stream.

Regards,

Rob Griffin
Software Analyst, Spotlight on SQL Server
Quest | R
rob.grif...@quest.com
Office +613-9811-8021

-Original Message-
From: Remi Forax 
Sent: Tuesday, 11 December 2018 4:26 AM
To: Rob Griffin (rgriffin) 
Cc: core-libs-dev ; Brian Goetz 

Subject: Re: Add convenience collect methods to the Stream interface

CAUTION: This email originated from outside of the organization. Do not click 
links or open attachments unless you recognize the sender and know the content 
is safe.


Hi Rob,
i will add to the answer of Brian that if you have too many .collect(toList()), 
it's perhaps your application perhaps suffers of the equivalent of the n + 1 
select query you have with SQL but with the Stream API.

You should try to see if returning a Stream instead of a List for some of 
methods is not a better option:
   public List getAllEmployee() {
 return employees.stream().filter(Employee::isActive).collect(toList());
   }
   public List getManager(List list) {
 return list.stream().filter(Employee::isManager).collect(toList());
   }
   ...
   getManager(getAllEmployee());

should be:
   public Stream getAllEmployee() {
 return employees.stream().filter(Employee::isActive);
   }
   public Stream getManager(Stream stream) {
 return stream.filter(Employee::isManager);
   }
   ...
   getManager(getAllEmployee()).collect(toList());


regards,
Rémi

- Mail original -

De: "Brian Goetz" 
À: "Rob Griffin (rgriffin)" , "core-libs-dev"

Envoyé: Lundi 10 Décembre 2018 17:14:41
Objet: Re: Add convenience collect methods to the Stream interface
As will surprise no one, this was extensively discussed during the
development of the Streams API.  (Our default position on "convenience
methods" is hostile.  While everyone sees the benefit of convenience
methods (it's convenient!), most people don't see the cost, which
includes the complexity for users to understand the model by looking
at the API; having lots of ad-hoc convenience method obscures the
underlying model, making it harder for everyone to learn or reason
about.  That default position seems to stand up pretty well here, as
the stream API is pretty well factored.)

Let's be honest: the "convenience" or concision of being able to say
.toList() instead of .collect(toList()) is really small.  I don't
think you'll be able to justify it by saying "but we do it a lot."
(Digression: to whoever is about to say "then why `toArray()`?  Arrays
are different; for better or worse, they're part of the language, and
they lend themselves particularly poorly to the Collector API, and
there are particular parallelization optimizations that are possible
for arrays that can't be accessed through Collector.  End digression.)

It is possible, however, that one could justify `toList()` on the
basis of _discoverability_.  (Though I'm having a hard time seeing any
world where `toSet()` makes the cut.)  New users who approach streams
will not easily be able to figure out how to materialize a list from a
stream, even though this is an entirely reasonable and quite common
thing to want to do.  Having to learn about `collect()` first is
asking a lot of users who are still wrapping their heads around
streams.  Not only would `toList()` be more discoverable, it would
provide a path to discovery of the rest of the `collect()` API.  This is a 
point in its favor.

A significant downside of adding `toList()` is that by diluting the
orthogonality of the existing API, it provides both incentive and
justification for further dilution, leading to someplace we don't want
to be.  (And, the cost of that falls heavily on the stewards, which in
turn takes time away from far more valuable activities.)

Put it this way: imagine we have a budget of one convenience method in
Stream for every five years.  Is this the one we want to spend the
next five year's

RE: RFR: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace

2018-12-11 Thread Doerr, Martin

Hi Michihiro,

the shared code looks good to me, now.

Looks like the PPC64 is not consistent any more.
Where do you enable UseCharacterCompareIntrinsics on PPC64?
Why aren't you using it for match_rule_supported in the ad file?
I think has_darn could be used to enable it in vm_version_ppc.

Best regards,
Martin


-Original Message-
From: Vladimir Kozlov  
Sent: Montag, 10. Dezember 2018 20:33
To: Michihiro Horie ; Gustavo Romero 

Cc: core-libs-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; 
Doerr, Martin ; Roger Riggs ; 
Simonis, Volker 
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for 
isDigit/isLowerCase/isUpperCase/isWhitespace

On 12/9/18 5:28 PM, Michihiro Horie wrote:
> Hi Vladimir,
> 
> Thanks a lot for your review. I also fixed the copyright in intrinsicnode.hpp.
> 
>  >Did you look on code generated by C2 with your latest changes?
> Yes,I usually check generated code with Oprofile and they were as expected 
> like:
> :
> 90 0.0905 : 3fff64c27654: rlwinm r12,r9,24,8,31
> 12 0.0121 : 3fff64c27658: li r11,14640
> 15 0.0151 : 3fff64c2765c: cmprb cr5,0,r31,r11
> 5527 5.5587 : 3fff64c27660: setb r11,cr5
> 36010 36.2164 : 3fff64c27664: stb r11,16(r15)

Good.

> :
> 
> I found a CSR FAQ that mentions adding a diagnostic flag do not need CSR 
> process. This time I do not need CSR.
> https://wiki.openjdk.java.net/display/csr/CSR+FAQs

Thank is correct.

> 
> Hi Gustavo,
> Could I ask you to sponsor the latest change webrev.05? I would like you to 
> test it on your P9 node too.

Thanks,
Vladimir

> 
> 
> Best regards,
> --
> Michihiro,
> IBM Research - Tokyo
> 
> Inactive hide details for Vladimir Kozlov ---2018/12/08 03:48:04---From: 
> Vladimir Kozlov  
> To: RogerVladimir Kozlov ---2018/12/08 03:48:04---From: Vladimir Kozlov 
>  To: Roger Riggs 
> , Michihiro Horie 
> 
> From: Vladimir Kozlov 
> To: Roger Riggs , Michihiro Horie 
> Cc: core-libs-dev@openjdk.java.net, hotspot-compiler-...@openjdk.java.net, 
> martin.do...@sap.com, volker.simo...@sap.com
> Date: 2018/12/08 03:48
> Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for 
> isDigit/isLowerCase/isUpperCase/isWhitespace
> 
> 
> 
> 
> 
> Hi Michihiro,
> 
> Hotspot changes looks fine.
> Did you look on code generated by C2 with your latest changes?
> 
> And Copyright year change in intrinsicnode.hpp is incorrect:
> 
> - * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 
> Should be
> 
> * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.
> 
> 
> Thanks,
> Vladimir
> 
> On 12/7/18 10:08 AM, Roger Riggs wrote:
>  > Hi Michihiro,
>  >
>  > Looks fine with the updates.
>  >
>  > Its great that the change to isDigit is beneficial on other platforms too.
>  >
>  > A very small editorial:
>  >    There should be a comma  "," after the 2018 in the copyright of:
>  >    test/micro/org/openjdk/bench/java/lang/Characters.java
>  >
>  > Thanks, Roger
>  >
>  >
>  > On 12/07/2018 03:13 AM, Michihiro Horie wrote:
>  >>
>  >> Hi Roger,
>  >>
>  >> I updated my webrev.
>  >> http://cr.openjdk.java.net/~mhorie/8213754/webrev.04/ 
> 
>  >> 
>  >>
>  >> >0x80 might be a good choice or 0xff.
>  >> I agree,thanks.
>  >>
>  >> >I was wondering about the performance without the PPC specific intrinsic 
> but with the
>  >> >CharacterDataLatin1.java.template code for isDigit being:
>  >> >        return '0' <= ch && ch <= '9';
>  >> I now understand your point,thanks for your explanation. Both on Power9 
> and Skylake, the
>  >> isDigit(ch)using ‘0’…’9’explicitly in CharacterDataLatin1.java.template 
> without PPC specific
>  >> intrinsicwas around 15% faster than the original code that does not 
> include my changes. I fixed my
>  >> change using ‘0’…’9’for this isDigit(ch).
>  >>
>  >>
>  >> Best regards,
>  >> --
>  >> Michihiro,
>  >> IBM Research - Tokyo
>  >>
>  >> Inactive hide details for Roger Riggs ---2018/12/07 01:23:39---Hi 
> Michihiro, On 12/06/2018 02:38
>  >> AM, Michihiro Horie wrote:Roger Riggs ---2018/12/07 01:23:39---Hi 
> Michihiro, On 12/06/2018 02:38
>  >> AM, Michihiro Horie wrote:
>  >>
>  >> From: Roger Riggs 
>  >> To: Michihiro Horie 
>  >> Cc: core-libs-dev@openjdk.java.net, 
> hotspot-compiler-...@openjdk.java.net, martin.do...@sap.com,
>  >> vladimir.koz...@oracle.com, volker.simo...@sap.com
>  >> Date: 2018/12/07 01:23
>  >> Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for 
> isDigit/isLowerCase/isUpperCase/isWhitespace
>  >>
>  >> 
> 
>  >>
>  >>
>  >>
>  >> Hi Michihiro,
>  >>
>  >> On 12/06/2018 02:38 AM, Michihiro Horie wrote:
>  >>
>  >>         Hi

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

2018-12-11 Thread Dmitry Chuyko


On 12/11/18 4:03 AM, David Holmes wrote:

Hi Dmitry,

On 11/12/2018 12:16 am, Dmitry Chuyko wrote:

Hello,

Please review a small fix in java_md_solinux.c: continuation is not 
truly compatible with pthread_create start_routine's signature but we 
control what actually happens. So it makes sense to add intermediate 
void* cast to silence the error.


I'd be tempted to fix the signature and get rid of all the casts.


David, the signature is a signature of

int JNICALL JavaMain(void * _args)

It would be fun to change it. But still on Windows it is correctly 
passed to _beginthreadex() and then return code is extracted with 
GetExitCodeThread(). In case we want it to return void* the cast will 
move there.


-Dmitry



Cheers,
David


bug: https://bugs.openjdk.java.net/browse/JDK-8215009
webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/
testing: submit repo 
(mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)


-Dmitry

Re: RFR: 8215159: Improve initial setup of system Properties