RFR: JDK-8150496,(zipfs) Fix performance issues in zip-fs

[Xueming Shen]

Hi,

Please help review the performance cleanup for zipfs

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

This is a cleanup update to improve the performance of most frequent
used access methods and the internal memory usage.

While there is definitely more places to investigate, it seems it is worth 
putting
the current version in with the performance improvement achieved.

Here are the result of the sample benchmark that measures the improvement.
http://cr.openjdk.java.net/~sherman/8150496/MyBenchmark.java

---new---
Benchmark  Mode  Cnt Score Error  Units
MyBenchmark.ZFS_Exists avgt   25 4.380 ±   0.745  ms/op
MyBenchmark.ZFS_GetPathavgt   2511.722 ±   0.606  ms/op
MyBenchmark.ZFS_GetPathExists  avgt   2521.031 ±   2.554  ms/op
MyBenchmark.ZFS_GetPathIsDirectory avgt   2526.823 ±   4.522  ms/op
MyBenchmark.ZFS_GetPathIsRegularFile   avgt   2526.049 ±   3.441  ms/op
MyBenchmark.ZFS_GetPathNewInputStream  avgt   2531.945 ±   3.330  ms/op
MyBenchmark.ZFS_GetPathToRealPath  avgt   2521.375 ±   2.875  ms/op
MyBenchmark.ZFS_IsDirectoryavgt   25 9.309 ±   0.633  ms/op
MyBenchmark.ZFS_IsRegularFile  avgt   25 9.454 ±   0.928  ms/op
MyBenchmark.ZFS_Itravgt   2518.583 ±   0.841  ms/op
MyBenchmark.ZFS_Open   avgt   25  1527.769 ± 221.997  ms/op
MyBenchmark.ZFS_ToRealPath avgt   25 4.666 ±   0.584  ms/op
MyBenchmark.ZFS_newInputStream avgt   2514.864 ±   0.618  ms/op
old---
Benchmark  Mode  Cnt Score Error  Units
MyBenchmark.ZFS_Exists avgt   2510.229 ±   1.236  ms/op
MyBenchmark.ZFS_GetPathavgt   2512.964 ±   2.739  ms/op
MyBenchmark.ZFS_GetPathExists  avgt   2528.095 ±   3.595  ms/op
MyBenchmark.ZFS_GetPathIsDirectory avgt   2528.566 ±   1.430  ms/op
MyBenchmark.ZFS_GetPathIsRegularFile   avgt   2529.102 ±   3.308  ms/op
MyBenchmark.ZFS_GetPathNewInputStream  avgt   25   151.271 ±   3.311  ms/op
MyBenchmark.ZFS_GetPathToRealPath  avgt   2542.717 ±   6.112  ms/op
MyBenchmark.ZFS_IsDirectoryavgt   2510.884 ±   0.810  ms/op
MyBenchmark.ZFS_IsRegularFile  avgt   2511.801 ±   1.053  ms/op
MyBenchmark.ZFS_Itravgt   2521.498 ±   1.165  ms/op
MyBenchmark.ZFS_Open   avgt   25  1673.597 ± 190.083  ms/op
MyBenchmark.ZFS_ToRealPath avgt   2519.266 ±   0.886  ms/op
MyBenchmark.ZFS_newInputStream avgt   25   125.307 ±   2.818  ms/op


Thanks,
Sherman

RFR 8155794 Move Objects.checkIndex BiFunction accepting methods to an internal package

[Paul Sandoz]

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8155794-checkIndex-bifunc-internal-jdk/webrev/
  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8155794-checkIndex-bifunc-internal-hotspot/webrev/

This patch is based on that for 8155258 (VarHandle impl improvements) [1] sent 
previously.

The hotspot changes are really small. Likewise for the 8155258 changes is there 
is precedent in such cases to push through jdk9-dev rather than hs?

CCC reviewers strongly indicated for the advanced methods that can customise 
the exceptions: "You aren't gonna need it”.

For expediency I propose to move such methods to an internal class 
jdk.internal.util.Preconditions. I would still like to sweep through java.base 
and leverage these methods while preserving exception reporting where possible.

The hotspot changes are just for renaming of the intrinsic method signatures. 
Since the intrinsic method is now internal i have added an @ForceInline on the 
corresponding public method, given the potential for this to be used in 
performance sensitive code.


JPRT core and hotspot tests pass.

Paul.

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-May/040740.html 

RFR 8155258: VarHandle implementation improvements

[Paul Sandoz]

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8155258-vhs-impl-improvements-jdk/webrev/
 

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8155258-vhs-impl-improvements-hotspot/webrev/
 


The hotspot changes are really small. Is there is precedent in such cases to 
push through jdk9-dev rather than hs, thus speeding up the integration?

There are a bunch of improvements (mostly space/time):

1) By default a VarHandle does not hold its method types for access mode 
methods. Instead the erased types are held on VarForm (this shared across many 
VH instances) and are leveraged by the guard methods (see 4).
1.1) The non-erased method types are created on demand and cached (see 3).

2) Remove the use of ClassValue to hold VarForm instances. Each VarHandle 
implement holds its VarForm as a static instance. MemberNames to each access 
mode method implementation are still calculated on construction (this can be 
revisited to be lazy as a further issue).

3) A VarHandle caches invoker-based MethodHandles (access mode method type 
prefixed with VarHandle as the first parameter) to the corresponding 
MemberNames held on VarForm. Such handles are used by the 
VarHandle.toMethodHandle, by the guard methods (see 4), or general 
linkers/invokers.

4) The guard methods (on VarHandleGuards) were revamped to:
4.1) leverage the erased types in 1)
4.2) optimize for the void calling case (dropping the return value) when 
linking to a non-void access mode method. This is an edge case but avoids 
surprises (e.g. dropping the return value of a CompareAndExchange). This 
leverages a little known fact of invokehandle linking where it is safe to drop 
the return value (John assures me this is safe!), and required a slight tweak 
to hotspot for verification under fastdebug execution.
4.3) leverage the cached invoker method handle in 3) for the case where 
boxing/conversion is required, with an asType transformation. This is the 
non-optimal path, but is much better than before since the invoker method 
handle is a constant.


JPRT core and hotspot tests pass.

Paul.

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Steve Drach]

Looks fine to me, although I am not an official reviewer.  Thanks for doing 
this.

> On May 2, 2016, at 1:03 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi,
> 
> Can you please take a look on:
> http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
> ?
> 
> Thank you
> 
> Shura
> 

Re: RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Mandy Chung]

> On May 2, 2016, at 1:03 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi,
> 
> Can you please take a look on:
> http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
> ?

Looks okay to me.

Mandy

Re: RFR:JDK-8079628:java.time: DateTimeFormatter containing "DD" fails on 3-digit day-of-year value

[Roger Riggs]

Hi Nadeesh,

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

line 1835: the appendValue(field) method has a sign-style of NORMAL; 
should that be NOT_NEGATIVE?


test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java:

What is being tested in test_dayOfYearFieldPattern?
It does not check that the value parsed matches the input.


test/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java

686, 688: should these cases show with sign-style NOT_NEGATIVE also?

Thanks, Roger



On 4/28/2016 2:04 PM, nadeesh tv wrote:

Hi all,

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8079628/webrev.02/


Thanks and Regards,
Nadeesh TV

On 4/28/2016 8:17 PM, Stephen Colebourne wrote:

My mistake on the spec for "DD". It should be SignStyle.NOT_NEGATIVE,
not NORMAL.

I'd like to see a test that checks adjacent value parsing works
correctly for "DDD". ie. DDDHHmm should be able to parse into a
LocalDateTime where the date-time value can be checked against the
input string.

I think this will be a good fix once complete.
thanks
Stephen

On 28 April 2016 at 14:10, nadeesh tv  wrote:

Hi all,

Thanks Stephen for the comments.

Please see the updated webrev
http://cr.openjdk.java.net/~ntv/8079628/webrev.01/

Regards,
Nadeesh TV

On 4/26/2016 5:42 PM, Stephen Colebourne wrote:

This change introduces inconsistencies and problems. For example, it
will parse up to 19 digits for "D" but only up to 2 digits for "d".
The following would be better:

   *D   1 appendValue(ChronoField.DAY_OF_YEAR)
   *DD  2 appendValue(ChronoField.DAY_OF_YEAR, 2, 3,
SignStyle.NORMAL)
   *DDD 3 appendValue(ChronoField.DAY_OF_YEAR, 3)

This limits the accepted input to 3 digits, which is quite sufficient
here. It is also clearer for the common "D" and "DDD" cases.

Note that a test case needs to cover adjacent value parsing for
day-of-year. This pattern "DDD" should be tested and work. With
the webrev changes, it will not work as adjacent value parsing mode
will not be triggered.

(The change will alter the meaning of "DD" but no-one should be
using that anyway as it is broken, only working for the first 99 days
of the year.)

The code in TCKDateTimeFormatterBuilder needs a blank line after the
new methods.

Stephen


On 26 April 2016 at 12:48, nadeesh tv  wrote:

Hi all,

Please  review a fix for

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

Issue -  Pattern 'D' is not implemented as intended by CLDR

Solution - Changed the definition of 'D' to D..D 1..3
appendValue(ChronoField.DAY_OF_YEAR, n, 19, SignStyle.NORMAL)

Webrev - http://cr.openjdk.java.net/~ntv/8079628/webrev.00/

--
Thanks and Regards,
Nadeesh TV


--
Thanks and Regards,
Nadeesh TV

Re: RFR: 8155795: Optimize Integer/Long.reverse by using reverseBytes

[Jaroslav Kameník]

Thanks for comments. I have changed tests as Aleksey suggested.

Btw. is not there some class used for utility methods as leftpad?

Ad performance of changed reverseBytes, I tried to compare
both reverseBytes alone, and it seems to be same, but differs
when inlined to reverse().

J.

2016-05-02 17:00 GMT+02:00 Claes Redestad :

> On 2016-05-02 16:48, Aleksey Shipilev wrote:
>
>> On 05/02/2016 05:07 PM, Claes Redestad wrote:
>>
>>> I'd like to sponsor this patch proposed by Jaroslav Kameník here:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040660.html
>>>
>>> Bug https://bugs.openjdk.java.net/browse/JDK-8155795
>>> Webrev: http://cr.openjdk.java.net/~redestad/8155795/webrev.01/
>>>
>> *) I wonder if Integer.reverseBytes is better spelled as:
>>
>>   return (i >>> 24) |
>>  ((i & 0xFF) >> 8) |
>>  ((i & 0xFF00) << 8)   |
>>  (i << 24);
>>
>
> The reverseBytes changes were motivated by seeing slightly better
> performance on the micro with the suggested changes, but after
> discussing this a bit I think we should revert to the original code alone
> and investigate if there's some compiler quirk lurking here separately.
>
> I'll file a bug.
>
>
>> *) The test should probably follow the same randomized testing pattern
>> as other tests:
>>
>>   for (int i = 0; i < N; i++) {
>> int x = rnd.nextInt();
>>
>> String expected = new StringBuilder().
>> .append(leftpad(Integer.toBinaryString(x), 32))
>> .reverse().toString();
>>
>> String actual   =
>> leftpad(Integer.toBinaryString(Integer.reverse(x)), 32);
>>
>> if (!expected.equals(actual)) {
>> throw new RuntimeException("reverse: \n" +
>>expected + " \n" + actual);
>> }
>>
>>   // That's how development looks like in 2016.
>>   private static String leftpad(String s, int width) {
>> String r = s;
>> for (int c = 0; c < width - s.length(); c++) {
>>   r = "0" + r;
>> }
>> return r;
>>   }
>>
>> ...and should probably precede any other test that uses reverse().
>>
>
> Seems reasonable.
>
> Jaroslav, do you mind improving the test as per Aleksey's
> suggestions?
>
> Thanks!
>
> /Claes
>
diff -r 2bf84670f079 src/java.base/share/classes/java/lang/Integer.java
--- a/src/java.base/share/classes/java/lang/Integer.javaSat Apr 30 
16:08:48 2016 -0700
+++ b/src/java.base/share/classes/java/lang/Integer.javaMon May 02 
22:09:54 2016 +0200
@@ -1790,9 +1790,8 @@
 i = (i & 0x) << 1 | (i >>> 1) & 0x;
 i = (i & 0x) << 2 | (i >>> 2) & 0x;
 i = (i & 0x0f0f0f0f) << 4 | (i >>> 4) & 0x0f0f0f0f;
-i = (i << 24) | ((i & 0xff00) << 8) |
-((i >>> 8) & 0xff00) | (i >>> 24);
-return i;
+
+return reverseBytes(i);
 }
 
 /**
@@ -1820,10 +1819,10 @@
  */
 @HotSpotIntrinsicCandidate
 public static int reverseBytes(int i) {
-return ((i >>> 24)   ) |
-   ((i >>   8) &   0xFF00) |
-   ((i <<   8) & 0xFF) |
-   ((i << 24));
+return (i << 24)|
+   ((i & 0xff00) << 8)  |
+   ((i >>> 8) & 0xff00) |
+   (i >>> 24);
 }
 
 /**
diff -r 2bf84670f079 src/java.base/share/classes/java/lang/Long.java
--- a/src/java.base/share/classes/java/lang/Long.java   Sat Apr 30 16:08:48 
2016 -0700
+++ b/src/java.base/share/classes/java/lang/Long.java   Mon May 02 22:09:54 
2016 +0200
@@ -1952,10 +1952,8 @@
 i = (i & 0xL) << 1 | (i >>> 1) & 0xL;
 i = (i & 0xL) << 2 | (i >>> 2) & 0xL;
 i = (i & 0x0f0f0f0f0f0f0f0fL) << 4 | (i >>> 4) & 0x0f0f0f0f0f0f0f0fL;
-i = (i & 0x00ff00ff00ff00ffL) << 8 | (i >>> 8) & 0x00ff00ff00ff00ffL;
-i = (i << 48) | ((i & 0xL) << 16) |
-((i >>> 16) & 0xL) | (i >>> 48);
-return i;
+
+return reverseBytes(i);
 }
 
 /**
diff -r 2bf84670f079 test/java/lang/Integer/BitTwiddle.java
--- a/test/java/lang/Integer/BitTwiddle.javaSat Apr 30 16:08:48 2016 -0700
+++ b/test/java/lang/Integer/BitTwiddle.javaMon May 02 22:09:54 2016 +0200
@@ -58,6 +58,21 @@
 
 for (int i = 0; i < N; i++) {
 int x = rnd.nextInt();
+
+String expected = new StringBuilder()
+.append(leftpad(toBinaryString(x), 32))
+.reverse().toString();
+
+String actual = leftpad(toBinaryString(reverse(x)), 32);
+
+if (!expected.equals(actual)) {
+throw new RuntimeException("reverse: \n" +
+expected + " \n" + actual);
+}
+}
+
+for (int i = 0; i < N; i++) {
+int x = rnd.nextInt();
 if (highestOneBit(x) != reverse(lowestOneBit(reverse(x
 throw new RuntimeException("g: " + toHexString(x));
 }
@@

RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Alexandre (Shura) Iline]

Hi,

Can you please take a look on:
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
?

Thank you

Shura

Re: [9] RFR: 8155859: Problem list tools/pack200/Pack200Props.java

[Roger Riggs]

Looks fine.

(It looks like all the spaces got squeezed out; the bugids should line up.)

Roger



On 5/2/2016 3:40 PM, Artem Smotrakov wrote:

Hello,

tools/pack200/Pack200Props.java fails on multiple platforms. Please 
see details in https://bugs.openjdk.java.net/browse/JDK-8155857


Please review the following patch below which adds the test to 
ProblemList.txt


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

diff -r 2bf84670f079 test/ProblemList.txt
--- a/test/ProblemList.txtSat Apr 30 16:08:48 2016 -0700
+++ b/test/ProblemList.txtMon May 02 12:02:42 2016 -0700
@@ -319,6 +319,8 @@

 tools/launcher/FXLauncherTest.java 8068049 linux-all,macosx-all

+tools/pack200/Pack200Props.java 8155857 generic-all
+
  



 # jdk_jdi



Artem

[9] RFR: 8155859: Problem list tools/pack200/Pack200Props.java

[Artem Smotrakov]

Hello,

tools/pack200/Pack200Props.java fails on multiple platforms. Please see 
details in https://bugs.openjdk.java.net/browse/JDK-8155857


Please review the following patch below which adds the test to 
ProblemList.txt


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

diff -r 2bf84670f079 test/ProblemList.txt
--- a/test/ProblemList.txtSat Apr 30 16:08:48 2016 -0700
+++ b/test/ProblemList.txtMon May 02 12:02:42 2016 -0700
@@ -319,6 +319,8 @@

 tools/launcher/FXLauncherTest.java 8068049 linux-all,macosx-all

+tools/pack200/Pack200Props.java 8155857 generic-all
+
 

 # jdk_jdi



Artem

Re: RFR 8147984: WindowsTerminal should support function keys

[Stuart Marks]

Hi Jan,

Thanks for the update. Including the link is fine, but I'm a bit suspicious of 
the durability of that website -- it appears to be the personal website of the 
current maintainer. Who knows if it'll be around in a couple years.


I'd suggest including in the comment the official title of the document, "XTerm 
Control Sequences" along with a mention of the authors (Moy, Gildea, Dickey) so 
that if the link were to go bad, it would be possible to do a web search to find 
some version of the document.


No need for an updated webrev.

Thanks,

s'marks

On 5/1/16 11:55 PM, Jan Lahoda wrote:

Hi Stuart,

Thanks for the comments and the link!

A webrev which includes the link is here:
http://cr.openjdk.java.net/~jlahoda/8147984/webrev.01/

Delta webrev to the last iteration is here:
http://cr.openjdk.java.net/~jlahoda/8147984/webrev.01/delta/webrev

Thanks,
Jan

On 29.4.2016 23:49, Stuart Marks wrote:

Hi Jan,

I finally got a chance to take a look at this. The change looks fine.

It would be nice to have a reference to where the escape sequences are
documented. There are links to the Windows VK_ codes there, which is
great. But there's no reference for the escape sequences that each
keypress is mapped to, e.g. F4 is "ESC O S", and F5 is "ESC [ 1 5 ~"
(and what happened to "ESC [ 1 6 ~"??)

I did some searching, and it seems really hard to find a definitive
reference. Perhaps the best reference is "XTerm Control Sequences" [1]
which seems to document xterm pretty thoroughly, which is what everybody
seems to follow nowadays. It even looks like it's being kept up to date
(last modified 2016-02-21).

Anyway I'd suggest adding a comment with a reference to this document.

As a cross-check, these sequences match what my Mac's Terminal.app
emits, at least for unshifted F1-F12. (The Terminal app was probably
copied from xterm.)

Thanks,

s'marks


[1] http://invisible-island.net/xterm/ctlseqs/ctlseqs.html


On 1/22/16 3:41 AM, Jan Lahoda wrote:

Hello,

I'd like to enhance the WindowsTerminal in jdk.internal.le with
function keys
handling. The intent is so that jshell can bind actions for shortcuts
including
function keys.

The patch for adding the function keys support is here:
http://cr.openjdk.java.net/~jlahoda/8147984/webrev.00/

An example of a feature that uses/may use this support is here:
http://mail.openjdk.java.net/pipermail/kulla-dev/2016-January/001226.html

Any comments are welcome!

Thanks,
   Jan

Re: RFR (M) 8155739: [TESTBUG] VarHandles/Unsafe tests for weakCAS should allow spurious failures

[Paul Sandoz]

> On 2 May 2016, at 04:56, Aleksey Shipilev  wrote:
> 
> On 04/30/2016 02:42 AM, Paul Sandoz wrote:
>>> On 29 Apr 2016, at 15:12, Aleksey Shipilev  
>>> wrote:
>> Looks good.
>> 
>> Small tweak if you so wish to do so:
>> 
>> #if[JdkInternalMisc]
>>static final int WEAK_ATTEMPTS = Integer.getInteger("weakAttempts", 10);
>> #end[JdkInternalMisc]
>> 
>> which avoids changes to the SunMiscUnsafe* tests.
> 
> Alas, there are still whitespace changes in SunMiscUnsafe* tests, so
> changes are unavoidable, and I would like to keep the patch in its
> current form.
> 

No objections from me.

Paul.

RFR (JAXP) 8152912: SAX XMLReaderFactory needs to be ServiceLoader compliant

[huizhe wang]

Hi,

Please review a change that adds a step using the ServiceLoader to 
XMLReaderFactory's provider-lookup process. Meanwhile, XMLReaderFactory 
is deprecated in favor of javax.xml.parsers.SAXParserFactory.


JBS: https://bugs.openjdk.java.net/browse/JDK-8152912
Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev/

The change has been verified by SQE test that Frank will submit soon for 
review (that is, when he starts to work at his local time).


Thanks,
Joe

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

On 02/05/16 20:06, Mandy Chung wrote:



On May 2, 2016, at 10:59 AM, Daniel Fuchs  wrote:

Hi Mandy,

I applied the suggested changes.

http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.08/index.html



Looks very good and much cleaner.  Nits:

 166 // The LoggingMXBeanSupport class uses reflection to determine

s/LoggingMXBeanSupport/LoggingMXBeanAccess/

Also renaming the variable name “support” to “loggingAccess” (or something like 
that) might help.

You can fix it up before you push.  No need to generate a new webrev.


Thanks! Done.

-- done


Mandy

Re: RFR: 8155775: Re-examine naming of privileged methods to access System properties

[Claes Redestad]

On 2016-05-02 16:15, Sean Mullan wrote:
This looks good. 


Thanks!



* src/java.base/share/classes/jdk/Version.java

This is not an issue in your changes, but the current javadoc for 
Version.current() says:


 266  * @throws  SecurityException
 267  *  If a security manager exists and its {@link
 268  *  SecurityManager#checkPropertyAccess(String)
 269  *  checkPropertyAccess} method does not allow access 
to the

 270  *  system property "java.version"

but this can never occur since the code is wrapping the call to 
System.getProperty("java.version") in doPrivileged, so the caller's 
permissions are never checked.


I think that this is a bug in the javadoc of this method and that it 
should not be specified to throw SecurityException. All code already 
has permission to read "java.version" in the default java.policy file. 
Can you file a followon bug to have this fixed?


Filed: https://bugs.openjdk.java.net/browse/JDK-8155853

/Claes

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Mandy Chung]

> On May 2, 2016, at 10:59 AM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> I applied the suggested changes.
> 
> http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.08/index.html
> 

Looks very good and much cleaner.  Nits:

 166 // The LoggingMXBeanSupport class uses reflection to determine

s/LoggingMXBeanSupport/LoggingMXBeanAccess/

Also renaming the variable name “support” to “loggingAccess” (or something like 
that) might help.

You can fix it up before you push.  No need to generate a new webrev.

Mandy

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

Hi Mandy,

I applied the suggested changes.

http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.08/index.html

best regards,

-- daniel

On 02/05/16 19:00, Mandy Chung wrote:

hanks a lot for the feedback!
>
> New webrev here:
> http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.07/index.html

Nit: in the class spec for LoggingMXBean.java - wrap type names with {@code…} 
such as MBeanServer, PlatformLoggingMXBean.

ManagementFactoryHelper.java
line 333: extra line to be removed?

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Mandy Chung]

> On May 2, 2016, at 7:13 AM, Daniel Fuchs  wrote:
> 
> 
>> One question about: 
>> ManagementFactory::getPlatformMXBean(MBeanServerConnection, 
>> PlatformLoggingMXBean.class)
>> - what would happen if this method is called from an image with java.logging 
>> module present and connect to
>> a VM with no java.logging module?
>> Should the ManagementFactory::getPlatformMXBean spec or 
>> PlatformLoggingMXBean spec be clarified?
> 
> I don't think there's anything to clarify: it will throw
> an IllegalArgumentException as specified in the spec.
> (it will get an InstanceNotFoundException from the
> remote VM, which will be wrapped and thrown as the
> cause of an IllegalArgumentException)
> 

OK.  thanks for checking.

> 
>> 
>> 252 final Map methods = 
>> AccessController.doPrivileged(
>> 253 new PrivilegedAction<>() {
>> 254 @Override
>> 255 public Map run() {
>> 256 return initMethodMap(impl);
>> 257 }
>> 258 });
>> 
>> I believe this doPrivileged is not necessary and so do the other getMethod 
>> calls.
>> Probably you are thinking ahead what if java.management may one day be 
>> defined
>> by a child loader of the defining loader of java.logging.
>> Then you can move doPrivileged to initMethodMap.
> 
> The doPrivileged around initMethod is necessary because
> the implementation class from which the methods are looked
> up is package protected, so unfortunately Method.setAccessible
> needs to be called. Another possibility would be to lookup
> the method on java.util.logging.LoggingMXBean instead of
> looking them up on the concrete implementation class.
> In that case setAccessible should no longer be needed.
> 
> Do you think I should modify the code to do that?
> 

I think that’d be cleaner to inspect java.util.logging.LoggingMXBean instead as 
Logging class now implements it.
  
> However your remark made me review the doPrivs and I believe
> the one in getMXBeanImplementation() is not needed, since
> it invokes a public static method on public exported class.
> I removed that.
> 

Good.

> 
>> 217 // skip
>> - better to throw InternalError since it expects all methods are used?
> 
> No. getObjectName() will fall in this bucket - it's not
> defined on LoggingMXBean.

I missed it inspected implementation class rather than LoggingMXBean.  Once you 
change it to find methods on LoggingMXBean, it should expect all methods can be 
found.
 
> 
>> 273 throw new SecurityException(ex);
>> - should not reach here.  SecurityException may cause confusion since this 
>> will be thrown even without security manager.  could simply throw 
>> InternalException
> 
> OK - Wrapped in UnsupportedOperationException instead.
> 
> 
>> 296 private PlatformLoggingImpl(LoggingMXBeanSupport support) {
>> - perhaps renaming LoggingMXBeanSupport to LoggingProxy to better represent 
>> it.
> 
> It's not really a proxy - it only implemements "invoke". Maybe it should
> be called LoggingMXBeanAccess?
> 

Sounds good.

> 
>> 
>> Any impact to permission confiugred in security policy? Should also document 
>> that.
> 
> The permission are checked against the concrete implementation
> class name "sun.management.ManagementFactoryHelper$PlatformLoggingImpl"
> and we're not changing that - so there should be no incompatibility
> here.
> 

Good.

> Thanks a lot for the feedback!
> 
> New webrev here:
> http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.07/index.html

Nit: in the class spec for LoggingMXBean.java - wrap type names with {@code…} 
such as MBeanServer, PlatformLoggingMXBean.

ManagementFactoryHelper.java
line 333: extra line to be removed?

Mandy

Re: RFR: 8151542: URL resources for multi-release jar files have a #runtime fragment appended to them

[Steve Drach]

Another webrev:  
http://cr.openjdk.java.net/~sdrach/8151542/webrev.05/index.html 


Only URLClassPath has changed.  I put a comment on url.getProtocol indicating 
the URL assures it’s lower case and I changed the long lines into multiple 
shorter lines.

> Also, it may be worth considering an overloaded JarLoader constructor
> that accepts a “jar:” URL, rather than stripping and wrapping.

I tried that this morning.  There aren’t any “good” constructors for URL that 
have a jarHandler that I can use.  The ones I found do much more work than
what we are doing.

> 
> Also, some jar URLs will now go through JarLoader that previously didn’t.
> I’m not saying that this wrong, just trying to understand the behavioural 
> change. For example, some URLs that previously didn’t have their jar index
> checked now will.
> 
> I’ve been looking at this code, for another reason, sorry you have had to
> deal with it, it’s a bit of a mess.

It is indeed a bit of a mess


>  We may need to revisit some of this when
> 8155770 is resolved.
> 
> -Chris.
> 
> 
> 

Re: RFR: 8155795: Optimize Integer/Long.reverse by using reverseBytes

[Ulf Zibis]

Am 02.05.2016 um 17:00 schrieb Claes Redestad:

The reverseBytes changes were motivated by seeing slightly better
performance on the micro with the suggested changes, but after
discussing this a bit I think we should revert to the original code alone
and investigate if there's some compiler quirk lurking here separately.
Maybe (i & 0xFF00) is faster than (i & 0xFF), because the first can be executed by shorter 
16-bit CPU op code.

Looking at HotSpot disassembler output could solve the miracle.

-Ulf

Re: RFR: 8155795: Optimize Integer/Long.reverse by using reverseBytes

[Claes Redestad]

On 2016-05-02 16:48, Aleksey Shipilev wrote:

On 05/02/2016 05:07 PM, Claes Redestad wrote:

I'd like to sponsor this patch proposed by Jaroslav Kameník here:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040660.html

Bug https://bugs.openjdk.java.net/browse/JDK-8155795
Webrev: http://cr.openjdk.java.net/~redestad/8155795/webrev.01/

*) I wonder if Integer.reverseBytes is better spelled as:

  return (i >>> 24) |
 ((i & 0xFF) >> 8) |
 ((i & 0xFF00) << 8)   |
 (i << 24);


The reverseBytes changes were motivated by seeing slightly better
performance on the micro with the suggested changes, but after
discussing this a bit I think we should revert to the original code alone
and investigate if there's some compiler quirk lurking here separately.

I'll file a bug.



*) The test should probably follow the same randomized testing pattern
as other tests:

  for (int i = 0; i < N; i++) {
int x = rnd.nextInt();

String expected = new StringBuilder().
.append(leftpad(Integer.toBinaryString(x), 32))
.reverse().toString();

String actual   =
leftpad(Integer.toBinaryString(Integer.reverse(x)), 32);

if (!expected.equals(actual)) {
throw new RuntimeException("reverse: \n" +
   expected + " \n" + actual);
}

  // That's how development looks like in 2016.
  private static String leftpad(String s, int width) {
String r = s;
for (int c = 0; c < width - s.length(); c++) {
  r = "0" + r;
}
return r;
  }

...and should probably precede any other test that uses reverse().


Seems reasonable.

Jaroslav, do you mind improving the test as per Aleksey's
suggestions?

Thanks!

/Claes

Re: RFR: 8155775: Re-examine naming of privileged methods to access System properties

[Sean Mullan]

On 05/02/2016 10:15 AM, Sean Mullan wrote:

This looks good. Just a couple of comments:

* src/java.base/share/classes/java/util/TimeZone.java

698 props.setProperty("user.timezone", id);

This will only change the local copy of the property. I think you want
to keep the original code which does a System.setProperty.


Ignore this comment. I missed the fact that System.getProperties() 
returns the props field without any defensive cloning.


--Sean

Re: RFR: 8155795: Optimize Integer/Long.reverse by using reverseBytes

[Aleksey Shipilev]

On 05/02/2016 05:07 PM, Claes Redestad wrote:
> I'd like to sponsor this patch proposed by Jaroslav Kameník here:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040660.html
> 
> Bug https://bugs.openjdk.java.net/browse/JDK-8155795
> Webrev: http://cr.openjdk.java.net/~redestad/8155795/webrev.01/

*) I wonder if Integer.reverseBytes is better spelled as:

 return (i >>> 24) |
((i & 0xFF) >> 8) |
((i & 0xFF00) << 8)   |
(i << 24);

*) The test should probably follow the same randomized testing pattern
as other tests:

 for (int i = 0; i < N; i++) {
   int x = rnd.nextInt();

   String expected = new StringBuilder().
   .append(leftpad(Integer.toBinaryString(x), 32))
   .reverse().toString();

   String actual   =
   leftpad(Integer.toBinaryString(Integer.reverse(x)), 32);

   if (!expected.equals(actual)) {
   throw new RuntimeException("reverse: \n" +
  expected + " \n" + actual);
   }

 // That's how development looks like in 2016.
 private static String leftpad(String s, int width) {
   String r = s;
   for (int c = 0; c < width - s.length(); c++) {
 r = "0" + r;
   }
   return r;
 }

...and should probably precede any other test that uses reverse().

Thanks,
-Aleksey

Re: RFR (M) 8155739: [TESTBUG] VarHandles/Unsafe tests for weakCAS should allow spurious failures

[Volker Simonis]

Hi Aleksey,

thanks for this quick fix. The change looks good!

Adding to Paul, you could restrict the definition of WEAK_ATTEMPTS
even further if you want:

#if[CAS]
#if[JdkInternalMisc]
static final int WEAK_ATTEMPTS = Integer.getInteger("weakAttempts", 10);
#end[JdkInternalMisc]
#end[CAS]

We still have two other test failures with our new intrinsic
implementation on ppc64, but that's pretty sure our fault :)

Regards,
Volker


On Sat, Apr 30, 2016 at 1:42 AM, Paul Sandoz  wrote:
>
>> On 29 Apr 2016, at 15:12, Aleksey Shipilev  
>> wrote:
>>
>> Hi,
>>
>> I would like to fix a simple testbug in our weakCompareAndSet VarHandles
>> and Unsafe intrinsics tests. weakCompareAndSet is spec-ed to allow
>> spurious failures, but current tests do not allow that. This blocks
>> development and testing on non-x86 platforms.
>>
>> Bug:
>>  https://bugs.openjdk.java.net/browse/JDK-8155739
>>
>> Webrevs:
>>  http://cr.openjdk.java.net/~shade/8155739/webrev.hs.00/
>>  http://cr.openjdk.java.net/~shade/8155739/webrev.jdk.00/
>>
>
> Looks good.
>
> Small tweak if you so wish to do so:
>
> #if[JdkInternalMisc]
> static final int WEAK_ATTEMPTS = Integer.getInteger("weakAttempts", 10);
> #end[JdkInternalMisc]
>
> which avoids changes to the SunMiscUnsafe* tests.
>
> Paul.
>
>> The tests are auto-generated, and the substantiative changes are in
>> *.template files. I also removed obsolete generate-unsafe-tests.sh. I
>> would like to push through hs-comp to expose this to Power and AArch64
>> folks early.
>>
>> Testing: x86_64, jdk:java/lang/invoke/VarHandle, hotspot:compiler/unsafe
>>
>> Thanks,
>> -Aleksey
>>
>

RFR: 8155795: Optimize Integer/Long.reverse by using reverseBytes

[Claes Redestad]

Hi,

I'd like to sponsor this patch proposed by Jaroslav Kameník here: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040660.html


Bug https://bugs.openjdk.java.net/browse/JDK-8155795
Webrev: http://cr.openjdk.java.net/~redestad/8155795/webrev.01/

The idea is that reusing reverseBytes can bring a speedup since this 
method is intrinsified on many platforms.


Thanks!

/Claes

Re: RFR: 8155775: Re-examine naming of privileged methods to access System properties

[Sean Mullan]

This looks good. Just a couple of comments:

* src/java.base/share/classes/java/util/TimeZone.java

698 props.setProperty("user.timezone", id);

This will only change the local copy of the property. I think you want 
to keep the original code which does a System.setProperty.


* src/java.base/share/classes/jdk/Version.java

This is not an issue in your changes, but the current javadoc for 
Version.current() says:


 266  * @throws  SecurityException
 267  *  If a security manager exists and its {@link
 268  *  SecurityManager#checkPropertyAccess(String)
 269  *  checkPropertyAccess} method does not allow access 
to the

 270  *  system property "java.version"

but this can never occur since the code is wrapping the call to 
System.getProperty("java.version") in doPrivileged, so the caller's 
permissions are never checked.


I think that this is a bug in the javadoc of this method and that it 
should not be specified to throw SecurityException. All code already has 
permission to read "java.version" in the default java.policy file. Can 
you file a followon bug to have this fixed?


Thanks,
Sean

On 05/01/2016 07:35 PM, Claes Redestad wrote:

Hi,

JDK-8154231 added methods to simplify access to system properties from
internal code, but after some discussion it's been decided to rename
these methods and document them to make it abundantly clear that they
are performing a privileged action and that care needs to be taken to
tread the input and output accordingly:

Webrev: http://cr.openjdk.java.net/~redestad/8155775/webrev.01
Bug: https://bugs.openjdk.java.net/browse/JDK-8155775

Thanks!

/Claes

Re: RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

Hi Mandy,

Answers inline, and new webrev at the end.

On 29/04/16 21:55, Mandy Chung wrote:

Hi Daniel,


On Apr 29, 2016, at 8:08 AM, Daniel Fuchs  wrote:

Hi,

Please find below a patch [2] that eliminates a static
dependency of java.lang.management on java.util.logging.LoggingMXBean.

When JDK-6876135 was fixed, it introduced the PlatformLoggingMXBean
interface, and recommended using PlatformLoggingMXBean over
LoggingMXBean.


Adding to this, JDK-6876135 prepared the JDK for modularization and 
PlatformLoggingMXBean was introduced that can be replaced with existing usage 
of LoggingMXBean.

With this change, java.management dependency on java.logging will become 
implementation details for logging purpose and can be eliminated completely in 
the future.

About the deprecation, to be specific, LoggingMXBean will no longer be a 
platform MXBean and an instance of LoggingMXBean will not register in the 
platform MBeanServer.

This is a revised wording for the deprecation note for LoggingMXBean:

@deprecated {@code LoggingMXBean} is no longer a {@link 
java.lang.management.PlatformManagedObject platform MXBean} and replaced with 
{@link java.lang.management.PlatformLoggingMXBean}.
It will not register in the platform MBeanServer. Use
{@code ManagementFactory.getPlatformMXBean(PlatformLoggingMXBean.class)} 
instead.


Done.


One question about: ManagementFactory::getPlatformMXBean(MBeanServerConnection, 
PlatformLoggingMXBean.class)
- what would happen if this method is called from an image with java.logging 
module present and connect to
a VM with no java.logging module?
Should the ManagementFactory::getPlatformMXBean spec or PlatformLoggingMXBean 
spec be clarified?


I don't think there's anything to clarify: it will throw
an IllegalArgumentException as specified in the spec.
(it will get an InstanceNotFoundException from the
remote VM, which will be wrapped and thrown as the
cause of an IllegalArgumentException)



ManagementFactoryHelper.java

 191 if (result != null) {
 192 LoggingMXBeanSupport.class.getModule().addReads(m);
 193 }

Reflection access assumes readability now:
  
http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectionWithoutReadability

>
> So this addReads is not needed.

Oh - OK thanks for the pointer!



 252 final Map methods = AccessController.doPrivileged(
 253 new PrivilegedAction<>() {
 254 @Override
 255 public Map run() {
 256 return initMethodMap(impl);
 257 }
 258 });

I believe this doPrivileged is not necessary and so do the other getMethod 
calls.
Probably you are thinking ahead what if java.management may one day be defined
by a child loader of the defining loader of java.logging.
Then you can move doPrivileged to initMethodMap.


The doPrivileged around initMethod is necessary because
the implementation class from which the methods are looked
up is package protected, so unfortunately Method.setAccessible
needs to be called. Another possibility would be to lookup
the method on java.util.logging.LoggingMXBean instead of
looking them up on the concrete implementation class.
In that case setAccessible should no longer be needed.

Do you think I should modify the code to do that?

However your remark made me review the doPrivs and I believe
the one in getMXBeanImplementation() is not needed, since
it invokes a public static method on public exported class.
I removed that.



 217 // skip
- better to throw InternalError since it expects all methods are used?


No. getObjectName() will fall in this bucket - it's not
defined on LoggingMXBean.


 273 throw new SecurityException(ex);
- should not reach here.  SecurityException may cause confusion since this will 
be thrown even without security manager.  could simply throw InternalException


OK - Wrapped in UnsupportedOperationException instead.



 296 private PlatformLoggingImpl(LoggingMXBeanSupport support) {
- perhaps renaming LoggingMXBeanSupport to LoggingProxy to better represent it.


It's not really a proxy - it only implemements "invoke". Maybe it should
be called LoggingMXBeanAccess?



Backward Compatibility considerations:
--

1. Local clients which obtain an instance of the logging
MXBean by calling ManagementFactory.getPlatformMXBean(
   "java.util.logging:type=Logging",
PlatformLoggingMXBean.class)
will no longer be able to cast the result on
java.util.logging.LoggingMXBean.
[There should be few, given that PlatformLoggingMXBean
 already has all the methods defined in LoggingMXBean]



I expect this would be really rare too.


2. ManagementFactory.getPlatformMBeanServer().isInstanceOf(
ObjectName, "java.util.logging.LoggingMXBean")
will now return 'false' instead of 'true'.

3. The Logging MXBean MBeanInfo will now report that its
management interface 

Re: JDK 9 RFR of adding a few @jls tags to java.lang.String

[David Holmes]

On 1/05/2016 5:34 AM, joe darcy wrote:

Hello,

Please review the small patch below to add some JLS references to the
string class to supplement its textual discussion of a few sections of
the JLS.


Side question: what mechanism is there to detect stale references 
if/when the JLS section numbers change?


David


Thanks,

-Joe


--- a/src/java.base/share/classes/java/lang/String.javaFri Apr 29
16:58:00 2016 -0700
+++ b/src/java.base/share/classes/java/lang/String.javaSat Apr 30
12:32:19 2016 -0700
@@ -117,6 +117,7 @@
  * @see java.lang.StringBuilder
  * @see java.nio.charset.Charset
  * @since   1.0
+ * @jls 15.18.1 String Concatenation Operator +
  */

 public final class String
@@ -2979,6 +2980,7 @@
  *
  * @return  a string that has the same contents as this string, but is
  *  guaranteed to be from a pool of unique strings.
+ * @jls 3.10.5 String Literals
  */
 public native String intern();

Re: JDK 9 RFR of adding a few @jls tags to java.lang.String

[Aleksey Shipilev]

Hi,

Looks good to me! Thanks for taking care of this.

-Aleksey

On 04/30/2016 10:34 PM, joe darcy wrote:
> Hello,
> 
> Please review the small patch below to add some JLS references to the
> string class to supplement its textual discussion of a few sections of
> the JLS.
> 
> Thanks,
> 
> -Joe
> 
> 
> --- a/src/java.base/share/classes/java/lang/String.javaFri Apr 29
> 16:58:00 2016 -0700
> +++ b/src/java.base/share/classes/java/lang/String.javaSat Apr 30
> 12:32:19 2016 -0700
> @@ -117,6 +117,7 @@
>   * @see java.lang.StringBuilder
>   * @see java.nio.charset.Charset
>   * @since   1.0
> + * @jls 15.18.1 String Concatenation Operator +
>   */
> 
>  public final class String
> @@ -2979,6 +2980,7 @@
>   *
>   * @return  a string that has the same contents as this string, but is
>   *  guaranteed to be from a pool of unique strings.
> + * @jls 3.10.5 String Literals
>   */
>  public native String intern();
> 
> 

Re: RFR (M) 8155739: [TESTBUG] VarHandles/Unsafe tests for weakCAS should allow spurious failures

[Aleksey Shipilev]

On 04/30/2016 02:42 AM, Paul Sandoz wrote:
>> On 29 Apr 2016, at 15:12, Aleksey Shipilev  
>> wrote:
> Looks good.
> 
> Small tweak if you so wish to do so:
> 
> #if[JdkInternalMisc]
> static final int WEAK_ATTEMPTS = Integer.getInteger("weakAttempts", 10);
> #end[JdkInternalMisc]
> 
> which avoids changes to the SunMiscUnsafe* tests.

Alas, there are still whitespace changes in SunMiscUnsafe* tests, so
changes are unavoidable, and I would like to keep the patch in its
current form.

Thanks,
-Aleksey