RFR JDK-8148624: Test failure of ConstructInflaterOutput.java

[Xueming Shen]

Hi,

Please help review the change for JDK-8148624

Issue: https://bugs.openjdk.java.net/browse/JDK-8148624
Webrev: http://cr.openjdk.java.net/~sherman/8148624/webrev/

The tests are intended to test/verify that the Deflater/Inflater.end() 
method will
not be invoked when they are passed in as the parameter to the 
constructor of

the DefaultInputStream and InflateOutputStream class, when the corresponding
close&/finish() method are invoked.  The stacktrace suggests that the end()
method is not actually being called (failure) by the close/finish(), but 
by the
finalize(). It appears the deflater/inflater is somehow being finalized 
by gc

between A and B showed below

public static void main(String[] args) throws Throwable {
try {realMain(args);} catch (Throwable t) {unexpected(t);}
[A]-->
System.out.println("\nPassed = " + passed + " failed = " + failed);
[B]-->
if (failed > 0) throw new AssertionError("Some tests failed");}


the problem can be easily reproduced by inserting a System.gc() in between.

The easy fix is just move the inflater/deflater out of realMain() to be 
the static variable

to prevent them from being gc-ed.

Thanks,
Sherman

Re: [9] RFR (S): 8148994: Replacing MH::invokeBasic with a direct call breaks LF customization

[John Rose]

Better!  You moved some inlining policy out of the VM and into the JDK runtime.
Reviewed.

Some loose brainstorming here:

We are creating MH nodes that provide hints to the JIT inlining policy.
The hint must vary over time as the profile evolves.  The CMH does this
indirectly via generating a @DontInline LF, with the LF patchable.
Another way to do it would be to have a @Stable MH pointer
initially null, but which would go non-null when an inlineable
MH is determined.  Another way would be to use a mutable CS.
The handshake might need to include a "push" notification to
the JIT that something has changed and recompilation should
be attempted.  The MCS can do this.  Or perhaps the push should
happen after some sort of further countdown.

— John

> On Feb 12, 2016, at 9:15 AM, Vladimir Ivanov  
> wrote:
> 
> Any reviews, please?
> 
> Best regards,
> Vladimir Ivanov
> 
> On 2/5/16 7:29 PM, Vladimir Ivanov wrote:
>> Thanks, John.
>> 
>> On 2/4/16 9:52 PM, John Rose wrote:
>>> Looks good. Except I don't recall how is_inline interacts, if at all,
>>> with customization. It's not clear what is being gated, and which
>>> optimizations get let through.
>> Inlining through MH::invokeBasic doesn't happen when there's @DontInline
>> on the target. It is the case for GWT which has CountingWrapper on both
>> paths. It starts with a reinvoker.dontInline LF and then switches to a
>> reinvoker LF (after 30 invocations). If a compilation happens when
>> reinvoker.dontInline is used, a direct call is generated and even if the
>> branch becomes hot, all the calls goes through reinvoker.dontInline.
>> Update to reinvoker LF isn't visible unless the dispatch goes through
>> MH::invokeBasic linker.
>> 
>> I had an impression that I introduced LF customization logic to the
>> non-inlined case in GWT, but I can't find it in the code. So, it seems
>> the regression is caused by repeated checks performed by
>> reinvoker.dontInline LF and not by LF customization missed in generated
>> code.
>> 
>> Also, I stumbled upon a quirk in inlining policy: inlining through
>> linkers can't be delayed. It happens when JIT hits some limit on IR
>> size. So, right now inlining can stop in the middle of MH chain and a
>> linker call is generated.
>> 
>> But after JDK-8072008 there's no problem with delaying inlining. C2 can
>> decide whether to keep the direct call or inline through it. So, I
>> enabled late inlining for all linkers. (Surprisingly, no significant
>> performance difference on nashorn.)
>> 
>> So, I decided to keep invokeBasic linker call replacement for now, but
>> piggypack LF customization on the stuck counting LF.
>> 
>> Updated webrev:
>>   http://cr.openjdk.java.net/~vlivanov/8148994/webrev.01/hotspot
>>   http://cr.openjdk.java.net/~vlivanov/8148994/webrev.01/jdk
>> 
>> * enabled late inlining for linkers;
>> * add LF customization check in CountingWrapper::countDown, so when
>> counting is stuck, the target will be customized.
>> 
>> Testing: octane, JPRT.
>> 
>> I'm working on JDK-8071793 [1] and will reconsider how LF customization
>> on non-inlined branch in GWTs works given there is a more JVM-friendly
>> mechanism to control inlining. With the proposed fix
>> reinvoker.dontInline is never customized (only the target it points to)
>> and serves as a trampoline between GWT & target LF. It would be better
>> to jump right to the customized LF, if non-inlined branch becomes hot.
>> 
>> Best regards,
>> Vladimir Ivanov
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8071793
>> "Prune rarely called invokedynamic call sites during inlining"
>>> 
>>> Maybe a comment about that would be useful
>>> Reviewed.
>>> 
>>> – John
>>> 
 On Feb 4, 2016, at 7:51 AM, Vladimir Ivanov
  wrote:
 
 http://cr.openjdk.java.net/~vlivanov/8148994/webrev.00
 https://bugs.openjdk.java.net/browse/JDK-8148994
 
 JDK-8072008 [1] introduced an optimization to bypass linker calls in
 generated code when target method can't be inlined.
 
 Unfortunately, it doesn't work well with MH::invokeBasic(), because
 it clashes with LambdaForm customization: if a call is not inlined,
 it is important for performance to update (customize) corresponding
 LambdaForm on-the-fly. MH::invokeBasic() linker does MH -> LF ->
 Method* -> _from_compiled_entry traversal on every invocation, while
 direct call points right to the _from_compiled_entry. So, any
 LambdaForm updates aren't visible until recompilation.
 
 The fix is to keep MH::invokeBasic linker, so up-to-date LambdaForm
 instance is used on every invocation.
 
 Also, cleaned up relevant tests a bit.
 
 Testing: octane, JPRT.
 
 Best regards,
 Vladimir Ivanov
 
 [1] https://bugs.openjdk.java.net/browse/JDK-8072008
"Emit direct call instead of linkTo* for recursive
 indy/MH.invoke* calls"

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Xueming Shen]

Steve,

I would assume the difference of the webrev.04 "old" iterator and the 
webrev.06 "new" iterator
that might trigger a performance is how you create the JarFileEntry. The 
one parameter constructor
invokes isMultiRelease(), which might be relatively expensive, when the 
"mr" is enabled. There

are couple "if" checks involved.

For the "new" iterator is slower than the current jdk9 one. It might be 
desired to have two JarEntryIterator
classes defined, one for "notVersioned", one for the "versioned". Of 
course keep the two parameter
constructor for the "notVersioned". This might bring performance back 
for the normal "notVersioned"

usage, which I would assume is the majority.

There is also a "bug" in the new iterator. The iterator/Enumeration 
returned from ZipFile.entries()
checks "ensureOpen() in both hasNext() and next(). So close() the 
ZipFile after itr.hasNext() will
fails the next next() invocation in the old implementation. The latest 
itr will returns the cached
"ze". Not a big issue for sure, but a kind of "regression". Defining two 
iterator classes as suggested
above will also workaround this issue, as the "notVersioned" one will 
work just as expected, no

regression.

-Sherman

On 2/9/16 5:04 PM, Steve Drach wrote:

Hi,

Yet another webrev, 
http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ 
, with a 
change to JarEntryIterator to fix a problem discovered by performance 
tests — calling hasNext() twice as often as needed.  I also removed 
the @since 9 tags from the methods entries() and stream(), and added 
an additional sentence to the spec for each of those methods that 
clarifies what a base entry is (actually is not).


I think having stream and entries do this is right although I would 
like to see some performance data if possible.


See 
http://cr.openjdk.java.net/~sdrach/8132734/JarFile%20Performance.pdf 



I used JMH to run the benchmark.  See 
http://cr.openjdk.java.net/~sdrach/8132734/MyBenchmark.java 
.  I 
used two jar files, the rt.jar file from JDK 8 that has 20653 entries 
and the multi-release.jar found in the test directory with 14 entries. 
 Obviously rt.jar is not a multi-release jar file.


The first two tables (1 and 2) are comparable and the second two 
tables are somewhat comparable (3 and 4).


Tables 1 and 2 have 4 sections that show the results of tests on the 
two jar files in 4 configurations of JarFile.  The tests were done 
with a JarFile object constructed without the Release object argument, 
essentially the legacy constructor.  The section labeled "JDK 8 
JarFile” was done with JDK 8u66.  The section labeled “JDK 9 JarFile” 
was done with the latest build of openjdk/jdk9/dev without any changes 
in my 8132734 changeset.  I chose this section as the reference, so 
the last column shows the values normalized to 1 micro/milli second 
per operation (rt.jar times are in milliseconds and multi-release.jar 
times are in microseconds).  It should be obvious that JDK 9 is much 
faster than JDK 8, somewhere on the order of 5-6 times faster.  I 
think that is because ZipFile doesn’t use JNI in JDK 9.


Of the two remaining sections in Tables 1 and 2, the section labeled 
“MultiRelease JarFile” differs from the section labeled “MultiRelease 
JarFile, new iterator” only in the JarEntryIterator class.  The first 
one uses the original iterator in JarFile.java that can be seen 
starting with line 551 of webrev.04 
, and the new 
iterator starts with line 553 of webrev.06 
.  The 
results are strange.  The new, more complex, iterator appears to be 
faster than the old, simpler, iterator.  I double, and triple, checked 
it, but it was always faster.  I used jitwatch to look at the hotspot 
logs generated during compilation and neither method was compiled.  I 
suppose I could dig into it further but decided not to.  Consider it 
good news.  The results do show that the multi-release enhancement 
slows JarFile entries/stream down by 2-18% depending on the size of 
the jar file.  But they are still much better than the JDK 8 values.


Also I would expect that if a JAR file is not mult-release but the 
library opens it with Release.RUNTIME to perform the same as opening 
the JAR file with the Release-less constructors.


The results in Table 3 attempts to answer this question since rt.jar 
is not a multi-release jar file.  This tells me that if one opens the 
JarFile with Release.RUNTIME, that there is a performance penalty of 
2-6% on this very large jar file.


Finally, the results in Table 4 tell me that processing a true 
multi-release jar file takes about 80% more time per entries() or 
stream() operation.  I’ve looked at this in a profiler and there is no 
pa

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Steve Drach]

> What's the issue? The bug description only includes the fix. If the env is 
> null, shouldn't
> it trigger a NPE?
> 
> The java.nio.file.spi package does have the note that "NPE, unless otherwise 
> noted ..."
> The api for FilesystemProvider.newFileSystem(..., env) says "env" can be 
> empty, means
> NPE for "null".
> 
> Maybe I miss something here?

No, you are not missing anything, I was.  I see ZipFileSystem will throw the 
NPE instead of ZipFileSystemProvider if my “fix” goes in, so it’ll still 
happen.  I rescind my request and will close the bug report with appropriate 
comment.


> 
> -Sherman
> 
> On 2/12/16 1:11 PM, Steve Drach wrote:
>> Hi,
>> 
>> Please review this simple fix to ZipFileSystemProvider.  The issue is 
>> JDK-8149769 .  I didn’t do 
>> a webrev but instead provide the following patch.
>> 
>> Thanks
>> Steve
>> 
>> diff -r 2d6c2c75f338 
>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java
>> --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java   
>> Tue Feb 09 14:07:28 2016 -0800
>> +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java   
>> Fri Feb 12 12:59:46 2016 -0800
>> @@ -100,7 +100,7 @@
>>  }
>>  ZipFileSystem zipfs = null;
>>  try {
>> -if (env.containsKey("multi-release")) {
>> +if (env != null && env.containsKey("multi-release")) {
>>  zipfs = new JarFileSystem(this, path, env);
>>  } else {
>>  zipfs = new ZipFileSystem(this, path, env);
> 

Re: [9] RFR (S): 8148994: Replacing MH::invokeBasic with a direct call breaks LF customization

[Claes Redestad]

Hi,

this seems OK to me, and I've verified that this gets octane performance
back to 9-b102 levels across a number of platforms.

Thanks!

/Claes

On 2016-02-12 18:15, Vladimir Ivanov wrote:

Any reviews, please?

Best regards,
Vladimir Ivanov

On 2/5/16 7:29 PM, Vladimir Ivanov wrote:

Thanks, John.

On 2/4/16 9:52 PM, John Rose wrote:

Looks good. Except I don't recall how is_inline interacts, if at all,
with customization. It's not clear what is being gated, and which
optimizations get let through.

Inlining through MH::invokeBasic doesn't happen when there's @DontInline
on the target. It is the case for GWT which has CountingWrapper on both
paths. It starts with a reinvoker.dontInline LF and then switches to a
reinvoker LF (after 30 invocations). If a compilation happens when
reinvoker.dontInline is used, a direct call is generated and even if the
branch becomes hot, all the calls goes through reinvoker.dontInline.
Update to reinvoker LF isn't visible unless the dispatch goes through
MH::invokeBasic linker.

I had an impression that I introduced LF customization logic to the
non-inlined case in GWT, but I can't find it in the code. So, it seems
the regression is caused by repeated checks performed by
reinvoker.dontInline LF and not by LF customization missed in generated
code.

Also, I stumbled upon a quirk in inlining policy: inlining through
linkers can't be delayed. It happens when JIT hits some limit on IR
size. So, right now inlining can stop in the middle of MH chain and a
linker call is generated.

But after JDK-8072008 there's no problem with delaying inlining. C2 can
decide whether to keep the direct call or inline through it. So, I
enabled late inlining for all linkers. (Surprisingly, no significant
performance difference on nashorn.)

So, I decided to keep invokeBasic linker call replacement for now, but
piggypack LF customization on the stuck counting LF.

Updated webrev:
http://cr.openjdk.java.net/~vlivanov/8148994/webrev.01/hotspot
   http://cr.openjdk.java.net/~vlivanov/8148994/webrev.01/jdk

* enabled late inlining for linkers;
* add LF customization check in CountingWrapper::countDown, so when
counting is stuck, the target will be customized.

Testing: octane, JPRT.

I'm working on JDK-8071793 [1] and will reconsider how LF customization
on non-inlined branch in GWTs works given there is a more JVM-friendly
mechanism to control inlining. With the proposed fix
reinvoker.dontInline is never customized (only the target it points to)
and serves as a trampoline between GWT & target LF. It would be better
to jump right to the customized LF, if non-inlined branch becomes hot.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8071793
 "Prune rarely called invokedynamic call sites during inlining"


Maybe a comment about that would be useful
Reviewed.

– John


On Feb 4, 2016, at 7:51 AM, Vladimir Ivanov
 wrote:

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

JDK-8072008 [1] introduced an optimization to bypass linker calls in
generated code when target method can't be inlined.

Unfortunately, it doesn't work well with MH::invokeBasic(), because
it clashes with LambdaForm customization: if a call is not inlined,
it is important for performance to update (customize) corresponding
LambdaForm on-the-fly. MH::invokeBasic() linker does MH -> LF ->
Method* -> _from_compiled_entry traversal on every invocation, while
direct call points right to the _from_compiled_entry. So, any
LambdaForm updates aren't visible until recompilation.

The fix is to keep MH::invokeBasic linker, so up-to-date LambdaForm
instance is used on every invocation.

Also, cleaned up relevant tests a bit.

Testing: octane, JPRT.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8072008
"Emit direct call instead of linkTo* for recursive
indy/MH.invoke* calls"

Re: RFR 9:8148775 : Spec for j.l.ProcessBuilder.Redirect.DISCARD need to be improved

[Martin Buchholz]

Still looks good!

On Fri, Feb 12, 2016 at 3:08 PM, Roger Riggs  wrote:
> Thanks,  Martin
>
> Updated in place.
>
>
>
>
> On 2/12/2016 5:48 PM, Martin Buchholz wrote:
>
> looks good.
> The previous line
>
> Redirect.DISCARD.file() the filename appropriate for the operating system
>  557  * and may be null &&
>
> could use the word "is"
>
> On Fri, Feb 12, 2016 at 2:42 PM, Roger Riggs  wrote:
>
> Please review a minor javadoc cleanup to remove a reference to a method
> (append)
> of the implementation.
>
> Webrev:
>http://cr.openjdk.java.net/~rriggs/webrev-discard-8148775/
>
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8148775
>
> Thanks, Roger
>
>

Re: RFR 9:8148775 : Spec for j.l.ProcessBuilder.Redirect.DISCARD need to be improved

[Roger Riggs]

Thanks,  Martin

Updated in place.



On 2/12/2016 5:48 PM, Martin Buchholz wrote:

looks good.
The previous line

Redirect.DISCARD.file() the filename appropriate for the operating system
  557  * and may be null &&

could use the word "is"

On Fri, Feb 12, 2016 at 2:42 PM, Roger Riggs  wrote:

Please review a minor javadoc cleanup to remove a reference to a method
(append)
of the implementation.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-discard-8148775/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8148775

Thanks, Roger

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Xueming Shen]

Steve,

What's the issue? The bug description only includes the fix. If the env 
is null, shouldn't

it trigger a NPE?

The java.nio.file.spi package does have the note that "NPE, unless 
otherwise noted ..."
The api for FilesystemProvider.newFileSystem(..., env) says "env" can be 
empty, means

NPE for "null".

Maybe I miss something here?

-Sherman

On 2/12/16 1:11 PM, Steve Drach wrote:

Hi,

Please review this simple fix to ZipFileSystemProvider.  The issue is JDK-8149769 
.  I didn’t do a webrev but 
instead provide the following patch.

Thanks
Steve

diff -r 2d6c2c75f338 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Tue Feb 09 14:07:28 2016 -0800
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Fri Feb 12 12:59:46 2016 -0800
@@ -100,7 +100,7 @@
  }
  ZipFileSystem zipfs = null;
  try {
-if (env.containsKey("multi-release")) {
+if (env != null && env.containsKey("multi-release")) {
  zipfs = new JarFileSystem(this, path, env);
  } else {
  zipfs = new ZipFileSystem(this, path, env);

Re: RFR 9:8148775 : Spec for j.l.ProcessBuilder.Redirect.DISCARD need to be improved

[Martin Buchholz]

looks good.
The previous line

Redirect.DISCARD.file() the filename appropriate for the operating system
 557  * and may be null &&

could use the word "is"

On Fri, Feb 12, 2016 at 2:42 PM, Roger Riggs  wrote:
> Please review a minor javadoc cleanup to remove a reference to a method
> (append)
> of the implementation.
>
> Webrev:
>http://cr.openjdk.java.net/~rriggs/webrev-discard-8148775/
>
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8148775
>
> Thanks, Roger
>

RFR 9:8148775 : Spec for j.l.ProcessBuilder.Redirect.DISCARD need to be improved

[Roger Riggs]

Please review a minor javadoc cleanup to remove a reference to a method  
(append)

of the implementation.

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-discard-8148775/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8148775

Thanks, Roger

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Steve Drach]

>> Please review this simple fix to ZipFileSystemProvider.  The issue is 
>> JDK-8149769 .  I didn’t do 
>> a webrev but instead provide the following patch.
>> 
>> 
> This looks okay. Can one of the existing tests be updated to cover this case?

Yes, almost any of them can be updated.  There is one, ZipFSTester.java that 
seems to be associated with a lot of bugs, so maybe that one.  Or I can put it 
in MultiReleaseJarTest, which makes sense because the code it tests was the 
code that caused the bug.  Any preferences?

RE: RFR: 8149601 Update references from "1.9" to "9"

[Iris Clark]

Hi, Kumar.

Thank you so much for pushing changesets to fix this bug.

Regards,
iris

-Original Message-
From: Iris Clark 
Sent: Thursday, February 11, 2016 9:13 AM
To: core-libs-dev@openjdk.java.net; verona-...@openjdk.java.net
Cc: Iris Clark; Kumar Srinivasan
Subject: RFR: 8149601 Update references from "1.9" to "9"

Hi.

Please review the following changes to the jaxp and jdk repositories to fix a 
few additional references to "1.9".

Bug

  8149601 Update references from "1.9" to "9"
  https://bugs.openjdk.java.net/browse/JDK-8149601

Webrev

  http://cr.openjdk.java.net/~iris/verona/8149601/webrev/

This is a follow-up to JDK-8136494 which changed "@since 1.9" 
to "@since 9" matching java.specification.version.

Thanks,
iris

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Alan Bateman]

On 12/02/2016 21:11, Steve Drach wrote:

Hi,

Please review this simple fix to ZipFileSystemProvider.  The issue is JDK-8149769 
.  I didn’t do a webrev but 
instead provide the following patch.


This looks okay. Can one of the existing tests be updated to cover this 
case?


-Alan

Re: RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Roger Riggs]

Look fine.

Roger


On 2/12/16 4:11 PM, Steve Drach wrote:

Hi,

Please review this simple fix to ZipFileSystemProvider.  The issue is JDK-8149769 
.  I didn’t do a webrev but 
instead provide the following patch.

Thanks
Steve

diff -r 2d6c2c75f338 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Tue Feb 09 14:07:28 2016 -0800
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Fri Feb 12 12:59:46 2016 -0800
@@ -100,7 +100,7 @@
  }
  ZipFileSystem zipfs = null;
  try {
-if (env.containsKey("multi-release")) {
+if (env != null && env.containsKey("multi-release")) {
  zipfs = new JarFileSystem(this, path, env);
  } else {
  zipfs = new ZipFileSystem(this, path, env);

RFR JDK-8149769: Null pointer exception in ZipFileSystemProvider

[Steve Drach]

Hi,

Please review this simple fix to ZipFileSystemProvider.  The issue is 
JDK-8149769 .  I didn’t do a 
webrev but instead provide the following patch.

Thanks
Steve

diff -r 2d6c2c75f338 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Tue Feb 09 14:07:28 2016 -0800
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java  
Fri Feb 12 12:59:46 2016 -0800
@@ -100,7 +100,7 @@
 }
 ZipFileSystem zipfs = null;
 try {
-if (env.containsKey("multi-release")) {
+if (env != null && env.containsKey("multi-release")) {
 zipfs = new JarFileSystem(this, path, env);
 } else {
 zipfs = new ZipFileSystem(this, path, env);

RE: RFR: 8149601 Update references from "1.9" to "9"

[Iris Clark]

Hi, Joe.

Thanks for the review.

Regards,
iris

-Original Message-
From: huizhe wang 
Sent: Thursday, February 11, 2016 11:17 AM
To: Iris Clark
Cc: core-libs-dev@openjdk.java.net; verona-...@openjdk.java.net
Subject: Re: RFR: 8149601 Update references from "1.9" to "9"

Hi Iris,

The change looks good.

Two of the previously included classes appeared twice in the webrev index page 
:-)

Thanks,
Joe

On 2/11/2016 11:06 AM, Iris Clark wrote:
> Hi.
>
> A private review comment as noted that I missed a few files in the jaxp 
> repository.
>
> The webrev has been updated in place.
>
> Thanks,
> iris
>
> -Original Message-
> From: Iris Clark
> Sent: Thursday, February 11, 2016 9:13 AM
> To: core-libs-dev@openjdk.java.net; verona-...@openjdk.java.net
> Cc: Iris Clark; Kumar Srinivasan
> Subject: RFR: 8149601 Update references from "1.9" to "9"
>
> Hi.
>
> Please review the following changes to the jaxp and jdk repositories to fix a 
> few additional references to "1.9".
>
> Bug
>
>8149601 Update references from "1.9" to "9"
>https://bugs.openjdk.java.net/browse/JDK-8149601
>
> Webrev
>
>http://cr.openjdk.java.net/~iris/verona/8149601/webrev/
>
> This is a follow-up to JDK-8136494 which changed "@since 1.9"
> to "@since 9" matching java.specification.version.
>
> Thanks,
> iris

RFR 9: 8149750 Decouple sun.misc.Signal from the base module

[Roger Riggs]

Please review moving the functionality of sun.misc.Signal to 
jdk.internal.misc and

creating a wrapper sun.misc.Signal for existing external uses.

+++ This time including the hotspot changes to update the target of the 
upcalls.


A new replacement API will be considered separately.

The update includes a test that passes with or without the changes.
(Except for an NPE instead of SEGV if null is passed).

Webrev:
  jdk: http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-9149750/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8149750

Thanks, Roger

JEP 260: https://bugs.openjdk.java.net/browse/JDK-8132928

Re: [9] RFR (S): 8148994: Replacing MH::invokeBasic with a direct call breaks LF customization

[Vladimir Ivanov]

Any reviews, please?

Best regards,
Vladimir Ivanov

On 2/5/16 7:29 PM, Vladimir Ivanov wrote:

Thanks, John.

On 2/4/16 9:52 PM, John Rose wrote:

Looks good. Except I don't recall how is_inline interacts, if at all,
with customization. It's not clear what is being gated, and which
optimizations get let through.

Inlining through MH::invokeBasic doesn't happen when there's @DontInline
on the target. It is the case for GWT which has CountingWrapper on both
paths. It starts with a reinvoker.dontInline LF and then switches to a
reinvoker LF (after 30 invocations). If a compilation happens when
reinvoker.dontInline is used, a direct call is generated and even if the
branch becomes hot, all the calls goes through reinvoker.dontInline.
Update to reinvoker LF isn't visible unless the dispatch goes through
MH::invokeBasic linker.

I had an impression that I introduced LF customization logic to the
non-inlined case in GWT, but I can't find it in the code. So, it seems
the regression is caused by repeated checks performed by
reinvoker.dontInline LF and not by LF customization missed in generated
code.

Also, I stumbled upon a quirk in inlining policy: inlining through
linkers can't be delayed. It happens when JIT hits some limit on IR
size. So, right now inlining can stop in the middle of MH chain and a
linker call is generated.

But after JDK-8072008 there's no problem with delaying inlining. C2 can
decide whether to keep the direct call or inline through it. So, I
enabled late inlining for all linkers. (Surprisingly, no significant
performance difference on nashorn.)

So, I decided to keep invokeBasic linker call replacement for now, but
piggypack LF customization on the stuck counting LF.

Updated webrev:
   http://cr.openjdk.java.net/~vlivanov/8148994/webrev.01/hotspot
   http://cr.openjdk.java.net/~vlivanov/8148994/webrev.01/jdk

* enabled late inlining for linkers;
* add LF customization check in CountingWrapper::countDown, so when
counting is stuck, the target will be customized.

Testing: octane, JPRT.

I'm working on JDK-8071793 [1] and will reconsider how LF customization
on non-inlined branch in GWTs works given there is a more JVM-friendly
mechanism to control inlining. With the proposed fix
reinvoker.dontInline is never customized (only the target it points to)
and serves as a trampoline between GWT & target LF. It would be better
to jump right to the customized LF, if non-inlined branch becomes hot.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8071793
 "Prune rarely called invokedynamic call sites during inlining"


Maybe a comment about that would be useful
Reviewed.

– John


On Feb 4, 2016, at 7:51 AM, Vladimir Ivanov
 wrote:

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

JDK-8072008 [1] introduced an optimization to bypass linker calls in
generated code when target method can't be inlined.

Unfortunately, it doesn't work well with MH::invokeBasic(), because
it clashes with LambdaForm customization: if a call is not inlined,
it is important for performance to update (customize) corresponding
LambdaForm on-the-fly. MH::invokeBasic() linker does MH -> LF ->
Method* -> _from_compiled_entry traversal on every invocation, while
direct call points right to the _from_compiled_entry. So, any
LambdaForm updates aren't visible until recompilation.

The fix is to keep MH::invokeBasic linker, so up-to-date LambdaForm
instance is used on every invocation.

Also, cleaned up relevant tests a bit.

Testing: octane, JPRT.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8072008
"Emit direct call instead of linkTo* for recursive
indy/MH.invoke* calls"

RFR 9: 8149750 Decouple sun.misc.Signal from the base module

[Roger Riggs]

Please review moving the functionality of sun.misc.Signal to 
jdk.internal.misc and

creating a wrapper sun.misc.Signal for existing external uses.
A new replacement API will be considered separately.

The update includes a test that passes with or without the changes.
(Except for an NPE instead of SEGV if null is passed).

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8149750

Thanks, Roger

JEP 260: https://bugs.openjdk.java.net/browse/JDK-8132928

Re: RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

[Roger Riggs]

+1

On 2/12/2016 11:30 AM, Claes Redestad wrote:

On 2016-02-12 17:26, Chris Hegarty wrote:


http://cr.openjdk.java.net/~chegar/8149656.01/


This looks great, thanks!

/Claes

Re: RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

[Claes Redestad]

On 2016-02-12 17:26, Chris Hegarty wrote:


http://cr.openjdk.java.net/~chegar/8149656.01/


This looks great, thanks!

/Claes

Re: RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

[Chris Hegarty]

Thanks for the feedback.

On 12/02/16 16:05, e...@zusammenkunft.net wrote:

Since it is only implemented with one anonymous class you could also think 
about making it a concrete PatternLRUCache


On 12/02/16 15:59, Claes Redestad wrote:
> +1
>
> nits: I guess the class should be made static if nested here, or even
> made concrete to get rid of the anonymous class - that depends on if we
> think we'll ever reuse this utility for other things.


With all these changes, my opening position is to keep things
the same. Accepting the move, then there is of course much
refactoring/cleanup that can be done. Here is the next
iteration, with your suggestions.

http://cr.openjdk.java.net/~chegar/8149656.01/

-Chris.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Paul Sandoz]

Hi Steve,

This patch has been cooking and re-heated enough times that i think it’s as 
good as done in terms of being reviewed.

We can follow up with further issues for other aspects, such as performance if 
need be.

Paul.

> On 10 Feb 2016, at 02:04, Steve Drach  wrote:
> 
> Hi,
> 
> Yet another webrev, http://cr.openjdk.java.net/~sdrach/8132734/webrev.06/ 
> , with a change to 
> JarEntryIterator to fix a problem discovered by performance tests — calling 
> hasNext() twice as often as needed.  I also removed the @since 9 tags from 
> the methods entries() and stream(), and added an additional sentence to the 
> spec for each of those methods that clarifies what a base entry is (actually 
> is not).
> 
>> I think having stream and entries do this is right although I would like to 
>> see some performance data if possible.
> 
> See http://cr.openjdk.java.net/~sdrach/8132734/JarFile%20Performance.pdf 
> 
> 
> I used JMH to run the benchmark.  See 
> http://cr.openjdk.java.net/~sdrach/8132734/MyBenchmark.java 
> .  I used two 
> jar files, the rt.jar file from JDK 8 that has 20653 entries and the 
> multi-release.jar found in the test directory with 14 entries.  Obviously 
> rt.jar is not a multi-release jar file.
> 
> The first two tables (1 and 2) are comparable and the second two tables are 
> somewhat comparable (3 and 4).
> 
> Tables 1 and 2 have 4 sections that show the results of tests on the two jar 
> files in 4 configurations of JarFile.  The tests were done with a JarFile 
> object constructed without the Release object argument, essentially the 
> legacy constructor.  The section labeled "JDK 8 JarFile” was done with JDK 
> 8u66.  The section labeled “JDK 9 JarFile” was done with the latest build of 
> openjdk/jdk9/dev without any changes in my 8132734 changeset.  I chose this 
> section as the reference, so the last column shows the values normalized to 1 
> micro/milli second per operation (rt.jar times are in milliseconds and 
> multi-release.jar times are in microseconds).  It should be obvious that JDK 
> 9 is much faster than JDK 8, somewhere on the order of 5-6 times faster.  I 
> think that is because ZipFile doesn’t use JNI in JDK 9.
> 
> Of the two remaining sections in Tables 1 and 2, the section labeled 
> “MultiRelease JarFile” differs from the section labeled “MultiRelease 
> JarFile, new iterator” only in the JarEntryIterator class.  The first one 
> uses the original iterator in JarFile.java that can be seen starting with 
> line 551 of webrev.04 
> , and the new iterator 
> starts with line 553 of webrev.06 
> .  The results are 
> strange.  The new, more complex, iterator appears to be faster than the old, 
> simpler, iterator.  I double, and triple, checked it, but it was always 
> faster.  I used jitwatch to look at the hotspot logs generated during 
> compilation and neither method was compiled.  I suppose I could dig into it 
> further but decided not to.  Consider it good news.  The results do show that 
> the multi-release enhancement slows JarFile entries/stream down by 2-18% 
> depending on the size of the jar file.  But they are still much better than 
> the JDK 8 values.
> 
>> Also I would expect that if a JAR file is not mult-release but the library 
>> opens it with Release.RUNTIME to perform the same as opening the JAR file 
>> with the Release-less constructors.
> 
> The results in Table 3 attempts to answer this question since rt.jar is not a 
> multi-release jar file.  This tells me that if one opens the JarFile with 
> Release.RUNTIME, that there is a performance penalty of 2-6% on this very 
> large jar file.
> 
> Finally, the results in Table 4 tell me that processing a true multi-release 
> jar file takes about 80% more time per entries() or stream() operation.  I’ve 
> looked at this in a profiler and there is no particular area that stands out 
> to me, it’s just more complicated to process a multi-release jar file, as 
> would be expected.
> 
>> I think the javadoc will need to also need to make it clear whether entries 
>> with names starting with META-INF/versions/ are returned.
> 
> Fixed.
> 
>> 
>> I see you've added @since 9 to the existing methods, I assume you didn't 
>> mean to do this.
> 
> Fixed.
> 
>> 
>> At some point then we need to discuss how RUNTIME_VERSION is computed. Iris 
>> (via Mandy) has pushed jdk.Version to jdk9/dev but having it exported by 
>> java.base conflicts with the design principles in JEP 200. Moving it to 
>> another module means that code in java.base cannot use it and thus the JAR 
>> file can't use it.
> 
> Left as is.
> 

Re: RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

[ecki]

Since it is only implemented with one anonymous class you could also think 
about making it a concrete PatternLRUCache 

-- 
http://bernd.eckenfels.net

-Original Message-
From: Chris Hegarty 
To: Core-Libs-Dev 
Sent: Fr., 12 Feb. 2016 16:55
Subject: RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

sun.misc.LRUCache is a utility class for implementing Least Recently
Used Caches. It is only used by j.u.Scanner in the JDK. It should be
relocated to a more suitable location, possibly an inner class of
Scanner.

http://cr.openjdk.java.net/~chegar/8149656/

-Chris.

Re: RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

[Claes Redestad]

+1

nits: I guess the class should be made static if nested here, or even 
made concrete to get rid of the anonymous class - that depends on if we 
think we'll ever reuse this utility for other things.


/Claes

On 2016-02-12 16:45, Chris Hegarty wrote:

sun.misc.LRUCache is a utility class for implementing Least Recently
Used Caches. It is only used by j.u.Scanner in the JDK. It should be
relocated to a more suitable location, possibly an inner class of
Scanner.

http://cr.openjdk.java.net/~chegar/8149656/

-Chris.

RFR [9 ] 8149656: Examine usages of sun.misc.LRUCache

[Chris Hegarty]

sun.misc.LRUCache is a utility class for implementing Least Recently
Used Caches. It is only used by j.u.Scanner in the JDK. It should be
relocated to a more suitable location, possibly an inner class of
Scanner.

http://cr.openjdk.java.net/~chegar/8149656/

-Chris.

Re: DeserializationPermission Proposal

[David M. Lloyd]

Things are getting confused (for me anyway) so I'm going to try and 
spell out each idea separately.


Given a class hierarchy H of:

C -> B -> A -> NonSerializableClass -> Object

where C, B, and A are serializable classes, and protection domain M 
which is initiating deserialization, I think the following requirements 
definitely apply:


* When M deserializes an instance of A, both M and the protection domain 
of A must be sufficient to allow the deserialization of A
* When M deserializes an instance of B, both M and the protection domain 
of B must be sufficient to allow the deserialization of B
* When M deserializes an instance of C, both M and the protection domain 
of C must be sufficient to allow the deserialization of C

* Extrapolate generally

The following requirements have definite pros and cons, which is what I 
think we're touching on in this thread:


* When M deserializes an instance of B (or any subtype thereof), the 
protection domain of A must also be sufficient to allow the 
deserialization of B
* When M deserializes an instance of C (or any subtype thereof), the 
protection domains of both A and B must also be sufficient to allow the 
deserialization of C

* Extrapolate generally

Based on the above, the following requirements might or might not make 
sense:


* When M deserializes an instance of A, the protection domains of 
NonSerializableClass and also Object must also be sufficient to allow 
the deserialization of A

* Likewise for B & C, extrapolate generally

Regardless of which is the best set of requirements, one of the major 
complicating questions is, what is the ideal representation of the 
permission(s) needed to allow the above?  I can see several use cases:


1. Grant the ability for a serializable class S to deserialize itself
2. Grant the ability for M to deserialize a specific class (by name?)
3. Grant the ability to any (serializable?) class X to allow 
deserialization of a specific subtype (by name?)
4. Grant the ability to any (serializable?) class X to allow 
deserialization of any subtype (think java.lang.Object, or general (but 
harmless) base classes like java.util.EventObject)


The big problem with permissions by name is that names are completely 
relative.  It is totally valid (and happens in the wild today) that a 
serialization graph contains classes from a variety of class loaders 
which are isolated from one another - in fact, it's even possible to 
have more than one class with the same class name and the same 
superclass in the same graph!  Given this fact, a flat class namespace 
*cannot* be assumed.


I think that the *only* possible remedy for this is that it should be up 
to each class' class loader (and/or module) to determine what permission 
is required in order to deserialize that class.  In this way, the JDK 
(9) can use a permission of the form class+module name for installed 
module classes, Java EE systems could use permissions in the form of 
application+module+jar+class name for deployment, OSGi systems could use 
permissions including bundle identification information, etc.


To be secure, the default class loader implementation of this method 
would have to produce a result which causes all checks to fail, 
preventing deserialization in a security manager environment of classes 
whose class loaders do not explicitly allow for it.


On 02/12/2016 12:30 AM, Peter Firmstone wrote:

Thanks David,

I'd originally considered something like that, but I later realised I'd
need to grant DeserializationPermission to other domains I might not
want allow objects to be deserialized with, simply because their domain
is included in the call stack.  It looked like it would widen the scope,
which was contrary to the intended goal of minimising the attack
surface.  In the end I decided to limit the context to only the domains
of classes involved in deserialization.

Cheers,

Peter.


On 02/11/2016 03:52 AM, Peter Firmstone wrote:
>/  An example might be more useful.
/>/
/>/  Traditionally, when the first non serializable superclass zero
argument
/>/  constructor is called, the domains of the subclasses aren't
present on
/>/  the call stack.  Any security checks performed in the constructor
of the
/>/  superclass will not include the subclasses domains.
/>/
/>/  In the proposed case, prior to construction, all domains in the
class
/>/  heirarchy of the to be deserialized object via the local
/>/  ObjectStreamClass, are added to an AccessControlContext, which is
then
/>/  passed to the SecurityManager two argument permission check.
/
Sure, that makes sense; in fact this could be a very good
standalone/incremental enhancement.

>/  Now an attacker will not be able to construct this object unless all
/>/  domains have DeserializationPermission.
/>/
/>/  If we use the stack context, it won't contain the yet to be
deserialized
/>/  classes.
/
True; perhaps the ideal solution would use the initial context plus a
per-deserializing-class context, so that both the