Re: RFR (XS): 8162815: unnecessary object creation in reflection

[Aleksey Shipilev]

On 08/09/2016 12:09 AM, Paul Sandoz wrote:
>> On 4 Aug 2016, at 12:02, Claes Redestad 
>> wrote:
>> Hi,
>> 
>> I'd like to sponsor this bug fix provided by Tomáš Hůrka to remove
>> a new and dup left behind in generated accessor methods after
>> moving these to use appropriate valueOf methods for boxing
>> primitive returns.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8162815 Webrev:
>> http://cr.openjdk.java.net/~redestad/8162815/webrev.01/
>> 
>> This appears to have a very limited effect to peak performance in
>> HotSpot (interpreter and C1 level code seem to hurt a bit), but
>> should be fixed for correctness.
>> 
> 
> +1

Looks good to me too.

Thanks,
-Aleksey

Collectors.toSet() small performance improvement

[Tagir F. Valeev]

Hello!

I'd like to propose a simple performance patch for Collectors.toSet():

--- a/src/java.base/share/classes/java/util/stream/Collectors.java  Tue Aug 
02 07:19:06 2016 +0530
+++ b/src/java.base/share/classes/java/util/stream/Collectors.java  Tue Aug 
09 11:47:20 2016 +0700
@@ -295,7 +295,12 @@
 public static 
 Collector> toSet() {
 return new CollectorImpl<>((Supplier>) HashSet::new, Set::add,
-   (left, right) -> { left.addAll(right); 
return left; },
+   (left, right) -> {
+   if (left.size() < right.size()) {
+   right.addAll(left); return right;
+   }
+   left.addAll(right); return left;
+   },
CH_UNORDERED_ID);
 }

This will add the smaller set to the larger which may improve parallel
performance when subtasks produced uneven results. In normal case the
additional overhead is insignificant (additional HashSet size
comparison per each combine operation which usually performed
~4*parallelism times).

Here's simple JDK benchmark with results (Core-i7-4702MQ 4 HT cores,
Win7, JDK 9-ea+129 64bit) which confirms the speedup:
http://cr.openjdk.java.net/~tvaleev/patches/toSet/

When data is unevenly distributed, it's significantly faster:

Original:
ToSet.toSetParallel1  true  avgt   50 88,580 ±   0,874  us/op
ToSet.toSetParallel   10  true  avgt   50676,243 ±  41,188  us/op
ToSet.toSetParallel  100  true  avgt   50   9126,399 ±  83,260  us/op
Patched:
ToSet.toSetParallel1  true  avgt   50 62,121 ±   1,207  us/op
ToSet.toSetParallel   10  true  avgt   50556,968 ±  37,404  us/op
ToSet.toSetParallel  100  true  avgt   50   6572,372 ± 183,466  us/op

When data is evenly distributed, no statistically significant changes
observed:

Original:
ToSet.toSetParallel1 false  avgt   50177,137 ±5,941  us/op
ToSet.toSetParallel   10 false  avgt   50   2056,332 ±   87,433  us/op
ToSet.toSetParallel  100 false  avgt   50  51864,198 ±  560,883  us/op
Patched:
ToSet.toSetParallel1 false  avgt   50172,606 ±   7,321  us/op
ToSet.toSetParallel   10 false  avgt   50   1922,478 ±  79,593  us/op
ToSet.toSetParallel  100 false  avgt   50  52648,100 ± 621,526  us/op

What do you think? Should I open an issue?

With best regards,
Tagir Valeev.

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Kim Barrett]

> On Aug 8, 2016, at 8:16 AM, Per Liden  wrote:
> 
> Hi Kim,
> 
> On 2016-08-01 20:47, Kim Barrett wrote:
>> This updated webrev addresses concerns about the performance of the VM
>> API used by the reference processor thread in the original webrev.
>> 
>> New webrevs:
>> full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03/
>>  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03/
>> incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03.incr/
>>  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03.incr/
> 
> Overall I think you managed to hit a good balance here, keeping the VM API 
> straight forward without loosing flexibility. Having the ReferenceHandler 
> doing the actual work seems sound and avoids some complexity, which I like.

Thanks for looking at this.

> I have one suggestion though, regarding CheckReferencePendingList(). While 
> reviewing I found that I had to check several times what its return value 
> actually meant, the "check" part of the name doesn't quite reveal that.
> Further, it seems to me that the waiting path of this function has fairly 
> little in common with the non-waiting path, e.g. it always returns true. So, 
> to make both the naming and implementation more clear I'd like to suggest 
> that we split this into two separate functions, HasReferencePendingList() and 
> WaitForReferencePendingList(), like this:

I was thinking about splitting things way, and ended up not doing so
for no good reason I can think of. And it does seem clearer that way,
so...

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04.inc/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04.inc/

> Other than this I think the patch looks good.
> 
>> 
>> The originally offered approach was an attempt to minimize changes.  I
>> was trying to be as similar to the existing code as possible, while
>> moving part of it into the VM, where we have the necessary control
>> over suspension and the like.  We already know we want to make changes
>> in this area, in order to eliminate the use of
>> jdk.internal.ref.Cleaner (JDK-8149925).  But we've also agreed that
>> JDK-8149925 wasn't urgent and to defer it to JDK 10.  I don't think we
>> should revisit that.
> 
> Is JDK-8149925 the correct bug id? Looks like that bug has already been fixed 
> in 9.

That CR was originally for removing all the uses.  During review the
nio.Bits / direct buffer use was separated, and is the last one.
There was a bunch of discussion about removing that final one in that
review (especially later parts)
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/038663.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039315.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/039862.html

and also here:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039561.html.

However, I don't see a followup CR for removing the nio.Bits use.
Maybe that didn't get created when we split the issue, or maybe I'm
just failing to find it.  Peter?

Note that the consensus back in March/April was to defer it, along
with the dependent actual removal of the internal Cleaner, to JDK 10.

Re: RFE to re-purpose option --version:

[Kumar Srinivasan]

Hello Paul,

There is a light weight method to get the version from the launcher,
"-fullversion" noting that, this does not invoke the VM, and obtains
the version string set at build time in the launcher itself.

However you would have to exec java twice, once to get the version,
another to invoke it.

Thanks

Kumar


Dear Committers,

In Java 9, the --version: has been deprecated. The error message
returned to me is:

Error: Specifying an alternate JDK/JRE version is no longer supported.
   The use of the flag '-version:' is no longer valid.
   Please download and execute the appropriate version.
Unrecognized option: -version:9

I am happy with that. This is not a complain on removing that "alternate"
feature.

However, I would like to propose bringing back the option with a different
purpose. I would like to use --version: as a validation check. I
want Java to execute ONLY if the version specified matches the actual
platform version. This would be a wonderful help to scripts that require a
particular version of the Java platform, and should fail if the environment
has been accidentally setup with the wrong Java platform version.

Examples:
java --version:9
java --version:9.1

AFAICT, the only way to do this now is to execute Java twice. Once to pipe
--version to some find/grep command and check return code, and then execute
java again if the check pass. Loading the runtime twice is not optimal,
wouldn't you agree? Yet if you agree to this proposal, it would be a big
win for script writers, I believe.

Opinions please. Thank you.

Cheers,
Paul

Re: RFR (XS): 8162815: unnecessary object creation in reflection

[Paul Sandoz]

> On 4 Aug 2016, at 12:02, Claes Redestad  wrote:
> 
> Hi,
> 
> I'd like to sponsor this bug fix provided by Tomáš Hůrka to remove a new and 
> dup left behind in generated accessor methods after moving these to use 
> appropriate valueOf methods for boxing primitive returns.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8162815
> Webrev: http://cr.openjdk.java.net/~redestad/8162815/webrev.01/
> 
> This appears to have a very limited effect to peak performance in HotSpot 
> (interpreter and C1 level code seem to hurt a bit), but should be fixed for 
> correctness.
> 

+1

Paul.

Re: RFR: JDK-8161230 - Create java.util.stream.Stream from Iterator / Enumeration

[Patrick Reinhart]

> Am 08.08.2016 um 18:55 schrieb Alan Bateman :
> 
> On 08/08/2016 17:29, Patrick Reinhart wrote:
> 
>> :
>> I tried to integrate your suggested changes here:
>> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.01
>> 
> I should have been clearer. A lazy implementation of 
> resources/systemResources methods won't throw any exceptions, instead any I/O 
> exceptions will be wrapped with an UncheckedIOException and then thrown from 
> the method that caused the access to take place. There are several examples 
> of this already. For the javadoc then this will be described in the method 
> description rather than a @throws.
> 
> -Alan

I hope that this version is more likely that what you meant…

http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.02
 


Patrick

Re: What's correct: "--modulepath" or "--module-path"?

[Alan Bateman]

On 08/08/2016 18:17, Volker Simonis wrote:


Hi,

currently (in jdk9/dev), tools like jlink/jmod support only
"--modulepath" but not "--module-path". But notice that they support
"--class-path" and "--module-version". As far as I can see, JEP 293:
Guidelines for JDK Command-Line Tool Options [1] recommends
"--module-path".

The changes to align with JEP 293 aren't in jdk9/dev yet but Mandy is in 
the process of bringing the changes in (discussion is on jigsaw-dev [1]).


-Alan

[1] 
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-August/009025.html

NIO Path enhancements?

[Stephen Colebourne]

I've been using the java.nio.file.Path API in the last few days and
have found it a little tricky to use. I'd like to understand if the
following ideas were ever considered (to consider whether an issue
should be raised).

Currently, developers create a Path instance and it contains various
methods for managing the path, plus a few that actually access the
filing system. However, for most filing system operations, developers
have to use the static methods on Files. This feels very non-intuitive
and hard to discover. Current code:

  Path path = Paths.get("/my/path/to/file.txt");
  if (Files.exists(path)) {
boolean dir = Files.isDirectory(path);
...
  }

Almost all methods on Files simply obtain the FileSystemProvider from
the FileSystem and invoke methods on that. Why not add a new class
PathOperations that binds the Path to the static methods?

  Path path = Paths.get("/my/path/to/file.txt");
  if (path.operations().exists()) {
boolean dir = path.operations().isDirectory();
...
  }

I can't see any reason why this wouldn't work, but perhaps I'm missing
something obvious. (All methods on Files would need to be replicated
on PathOperations, and maybe Files could be deprecated if the
deprecation tools improve.)


Separately, it is a little tricky grasping the ZIP file system, and
that it needs to be opened and closed separately. Could we see a new
method to indicate if a given path/file can be expanded to another
FileSystem?

if (path.operations().isExpandableToFileSystem()) {
  try (FileSystem fs = path.operations().expand()) {
// work with ZIP or similar
  }
}

This would be a lot more discoverable with appropriate docs.

BTW, this also relates to Jigsaw as the new JFS provider may make
expandable paths more widely used.

Stephen

What's correct: "--modulepath" or "--module-path"?

[Volker Simonis]

Hi,

currently (in jdk9/dev), tools like jlink/jmod support only
"--modulepath" but not "--module-path". But notice that they support
"--class-path" and "--module-version". As far as I can see, JEP 293:
Guidelines for JDK Command-Line Tool Options [1] recommends
"--module-path".

Also, the current help text for jmod in the jmod.properties resource
file, contains this message:

err.modulepath.must.be.specified=--module-path must be specified when
hashing modules

but jmod itself only understands "--modulepath"

Is somebody taking care to harmonize all these command line options?
Will this be done as part of JEP 293?

Regards,
Volker

[1] http://openjdk.java.net/jeps/293

Re: RFR: JDK-8161230 - Create java.util.stream.Stream from Iterator / Enumeration

[Alan Bateman]

On 08/08/2016 17:29, Patrick Reinhart wrote:


:
I tried to integrate your suggested changes here:
http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.01

I should have been clearer. A lazy implementation of 
resources/systemResources methods won't throw any exceptions, instead 
any I/O exceptions will be wrapped with an UncheckedIOException and then 
thrown from the method that caused the access to take place. There are 
several examples of this already. For the javadoc then this will be 
described in the method description rather than a @throws.


-Alan

Re: Executors enhancement

[Andrew Thompson]

Christian,

A number of years ago I put together two small libraries to provide access to 
Apple's Grand Central Dispatch from Java. My thought was precisely to play 
nicely with the host operating system, power management etc. I've often thought 
something similar to fork-join's common pool that each platform implements with 
the best native alternative would be a good platform addition, but I've not 
developed the idea any further. 

http://pixel.recoil.org/code/rococoa/

Andrew 

> On Aug 7, 2016, at 3:51 PM, Christian Schulte  wrote:
> 
> Hello,
> 
> whenever I need to update an existing codebase to add some kind of
> parallelization, I am in the need of an executor matching the number of
> processors available at runtime. Most of the time I end up using
> something like:
> 
> 'Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors())'
> 
> If every library used by an application needs to maintain its' own
> executors the application ends up with starting number of libraries used
> times number of available processors threads. That does not scale. I
> think there really is a need for some kind of default executor provided
> by the platform. I am suprised I cannot find anything like that
> anywhere. I would like to request an enhancement providing such a
> default platform executor everyone can use to add parallelization
> without ending up with tons of threads when all you want is making use
> of the system's parallelization capabilities.
> 
> Regards,
> -- 
> Christian

Re: RFR: JDK-8161230 - Create java.util.stream.Stream from Iterator / Enumeration

[Patrick Reinhart]

On 05.08.2016 06:18, Alan Bateman wrote:
> On 04/08/2016 10:33, Patrick Reinhart wrote:
>
>> I was quit busy lately and this comes a bit late, I guess you do not
>> have less work ;-)
>>
>> On 15.07.2016 17:10, Paul Sandoz wrote:
 When I understand you correctly here we should concentrate on the
 public
 methods naming firstly? I initially was not sure, what a proper naming
 for the steams method was. It seem to me reasonable the way Stuart
 pointed
 them out on his first comment to name them something like this:

 Stream resources(String name)
 Stream systemResources(String name)


 Yes.
>> I have a first proposal for the new methods and their documentation
>> to start with the discussion about the actual API without the
>> implementation jet:
> The method names look right but I don't think `throws IOException` is
> needed. If overridden then the implementations could be truely lazy
> and the method will need to specify how stream operations will wrap
> the errors in UncheckedIOExceptions.
>
> For the initial sentence then it might be better to say that it
> "Returns a stream that loads the resources ...".
>
> As I was mentioned previously, we will be replacing the javadoc for
> the existing methods and this will impact the wording for the new
> methods. It's okay to align the wording for the new methods with the
> old and we'll adjust once there is agreement on the proposal in JSR
> 376 and we bring the changes to JDK 9.
>
> -Alan

I tried to integrate your suggested changes here:
http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.01

Patrick

RFE to re-purpose option --version:

[Paul Benedict]

Dear Committers,

In Java 9, the --version: has been deprecated. The error message
returned to me is:

Error: Specifying an alternate JDK/JRE version is no longer supported.
  The use of the flag '-version:' is no longer valid.
  Please download and execute the appropriate version.
Unrecognized option: -version:9

I am happy with that. This is not a complain on removing that "alternate"
feature.

However, I would like to propose bringing back the option with a different
purpose. I would like to use --version: as a validation check. I
want Java to execute ONLY if the version specified matches the actual
platform version. This would be a wonderful help to scripts that require a
particular version of the Java platform, and should fail if the environment
has been accidentally setup with the wrong Java platform version.

Examples:
java --version:9
java --version:9.1

AFAICT, the only way to do this now is to execute Java twice. Once to pipe
--version to some find/grep command and check return code, and then execute
java again if the check pass. Loading the runtime twice is not optimal,
wouldn't you agree? Yet if you agree to this proposal, it would be a big
win for script writers, I believe.

Opinions please. Thank you.

Cheers,
Paul

Re: RFR: 8163369: Enable generating DMH classes at link time

[Aleksey Shipilev]

On 08/08/2016 04:18 PM, Claes Redestad wrote:
> http://cr.openjdk.java.net/~redestad/8163369/webrev.02/

Looks good.

-Aleksey

Re: RFR: 8163369: Enable generating DMH classes at link time

[Claes Redestad]

On 2016-08-08 14:28, Aleksey Shipilev wrote:

On 08/08/2016 02:46 PM, Claes Redestad wrote:

Hi,

please review this change to add the ability to generate
DirectMethodHandles to the --generate-jli-classes jlink plugin.

The implementation generates all the specified DMHs as methods into a
single class, java.lang.invoke.DirectMethodHandle$DMH. At runtime when a
DMH's LF is set up, we speculatively resolve the member from this class
instead of generating and loading the bytecode as a distinct anonymous
class. This avoids loading a potentially large number of anonymous
classes at runtime, and also enables other startup optimizations such as
allowing CDS to see and dump this class to the shared archive.

webrev: http://cr.openjdk.java.net/~redestad/8163369/webrev.01/

DirectMethodHandle:

  * DMH placeholder class might better be called
DirectMethodHandle.Holder, or something else that clearly spells out the
intent?


Sure.



InvokerBytecodeGenerator:

  * Both generateNamedFunctionInvokerImpl() and
generateLambdaFormInterpreterEntryPointBytes() have methodEpilog() call,
but no methodPrologue()? classFilePrologue() used to do the prologue.


Nice catch, there was even 2 failed tests I had missed due to this.



GenerateJLIClassesPlugin:

  * Do we actually need this block in this form?

// DirectMethodHandles
// Enable by default
boolean dmhEnabled = true;
if (mainArgument != null) {
Set args = Arrays.stream(mainArgument.split(","))
.collect(Collectors.toSet());
if (!args.contains(DMH_PARAM)) {
dmhEnabled = false;
}
}

  seems equivalent to a simpler:

boolean dmhEnabled = true;
if (mainArgument != null) {
List args = Arrays.asList(mainArgument.split(","));
if (!args.contains(DMH_PARAM)) {
dmhEnabled = false;
}
 }

  You can even split the mainArgument once and reuse throughout configure()?


Fixed.



  * This block does not check the first argument as the comment suggests,
only checks the second group:

 String[] typeParts = type.split("_");
 if (typeParts.length != 2 || typeParts[1].length() != 1
 || "LJIFDV".indexOf(typeParts[1].charAt(0)) == -1) {
throw new PluginException(
  "Method type signature must be of form [LJIFD]*_[LJIFDV]");
 }


I'm checking the return type here (the char after _ in "LL_L") and
then checking the first part inside expandSignature. Clarified the
intent with comments.



  * primitiveType('Z') would throw a misleading "Not a primitive: Z"


Yes, added special casing for sub-int primitives (B, S, Z, C)
to indicate that 'I' should be used instead.



  * This can iterate over values only:

for (Map.Entry> entry : dmhMethods.entrySet()) {
   count += entry.getValue().size();
}


Sure.



  * I don't know the module visibility rules between jlink and java.base,
but can we reference DirectMethodHandle.DMH class as literal from the
plugin?


No, while java.lang.invoke is exported, DirectMethodHandle is 
package-private

which prohibits static visibility from the plugin.



  * requireBasicType(last) is loop invariant here:

 for (int j = 1; j < count; j++) {
 requireBasicType(last);
 sb.append(last);
 }


Sure.

http://cr.openjdk.java.net/~redestad/8163369/webrev.02/

/Claes



Thanks,
-Aleksey

Re: RFR: 8163369: Enable generating DMH classes at link time

[Aleksey Shipilev]

On 08/08/2016 02:46 PM, Claes Redestad wrote:
> Hi,
> 
> please review this change to add the ability to generate
> DirectMethodHandles to the --generate-jli-classes jlink plugin.
> 
> The implementation generates all the specified DMHs as methods into a
> single class, java.lang.invoke.DirectMethodHandle$DMH. At runtime when a
> DMH's LF is set up, we speculatively resolve the member from this class
> instead of generating and loading the bytecode as a distinct anonymous
> class. This avoids loading a potentially large number of anonymous
> classes at runtime, and also enables other startup optimizations such as
> allowing CDS to see and dump this class to the shared archive.
> 
> webrev: http://cr.openjdk.java.net/~redestad/8163369/webrev.01/

DirectMethodHandle:

 * DMH placeholder class might better be called
DirectMethodHandle.Holder, or something else that clearly spells out the
intent?


InvokerBytecodeGenerator:

 * Both generateNamedFunctionInvokerImpl() and
generateLambdaFormInterpreterEntryPointBytes() have methodEpilog() call,
but no methodPrologue()? classFilePrologue() used to do the prologue.

GenerateJLIClassesPlugin:

 * Do we actually need this block in this form?

   // DirectMethodHandles
   // Enable by default
   boolean dmhEnabled = true;
   if (mainArgument != null) {
   Set args = Arrays.stream(mainArgument.split(","))
   .collect(Collectors.toSet());
   if (!args.contains(DMH_PARAM)) {
   dmhEnabled = false;
   }
   }

 seems equivalent to a simpler:

   boolean dmhEnabled = true;
   if (mainArgument != null) {
   List args = Arrays.asList(mainArgument.split(","));
   if (!args.contains(DMH_PARAM)) {
   dmhEnabled = false;
   }
}

 You can even split the mainArgument once and reuse throughout configure()?

 * This block does not check the first argument as the comment suggests,
only checks the second group:

String[] typeParts = type.split("_");
if (typeParts.length != 2 || typeParts[1].length() != 1
|| "LJIFDV".indexOf(typeParts[1].charAt(0)) == -1) {
   throw new PluginException(
 "Method type signature must be of form [LJIFD]*_[LJIFDV]");
}

 * primitiveType('Z') would throw a misleading "Not a primitive: Z"

 * This can iterate over values only:

   for (Map.Entry> entry : dmhMethods.entrySet()) {
  count += entry.getValue().size();
   }

 * I don't know the module visibility rules between jlink and java.base,
but can we reference DirectMethodHandle.DMH class as literal from the
plugin?

 * requireBasicType(last) is loop invariant here:

for (int j = 1; j < count; j++) {
requireBasicType(last);
sb.append(last);
}

Thanks,
-Aleksey

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi Kim,

On 2016-08-01 20:47, Kim Barrett wrote:

This updated webrev addresses concerns about the performance of the VM
API used by the reference processor thread in the original webrev.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03.incr/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03.incr/


Overall I think you managed to hit a good balance here, keeping the VM 
API straight forward without loosing flexibility. Having the 
ReferenceHandler doing the actual work seems sound and avoids some 
complexity, which I like.


I have one suggestion though, regarding CheckReferencePendingList(). 
While reviewing I found that I had to check several times what its 
return value actually meant, the "check" part of the name doesn't quite 
reveal that. Further, it seems to me that the waiting path of this 
function has fairly little in common with the non-waiting path, e.g. it 
always returns true. So, to make both the naming and implementation more 
clear I'd like to suggest that we split this into two separate 
functions, HasReferencePendingList() and WaitForReferencePendingList(), 
like this:



JVM_ENTRY(jboolean, JVM_HasReferencePendingList(JNIEnv* env))
  JVMWrapper("JVM_HasReferencePendingList");
  MonitorLockerEx ml(Heap_lock);
  return Universe::has_reference_pending_list();
JVM_END

JVM_ENTRY(void, JVM_WaitForReferencePendingList(JNIEnv* env))
  JVMWrapper("JVM_WaitForReferencePendingList");
  MonitorLockerEx ml(Heap_lock);
  while (!Universe::has_reference_pending_list()) {
ml.wait();
  }
JVM_END


And of course, this would then also be reflected in j.l.r.Reference, 
which would have:


private static native boolean hasReferencePendingList();
private static native void waitForReferencePendingList();


Other than this I think the patch looks good.



The originally offered approach was an attempt to minimize changes.  I
was trying to be as similar to the existing code as possible, while
moving part of it into the VM, where we have the necessary control
over suspension and the like.  We already know we want to make changes
in this area, in order to eliminate the use of
jdk.internal.ref.Cleaner (JDK-8149925).  But we've also agreed that
JDK-8149925 wasn't urgent and to defer it to JDK 10.  I don't think we
should revisit that.


Is JDK-8149925 the correct bug id? Looks like that bug has already been 
fixed in 9.


cheers,
Per



As Peter pointed out, my original change introduces a small
performance regression on a microbenchmark for reference processing
throughput.  It also showed a small performance benefit on a different
microbenchmark (allocation rate for a stress test of direct byte
buffers), as noted in the original RFR.  I can reproduce both of
those.

I think reference processing thread throughput is the right measure to
use, so long as others don't become horrible.  Focusing on that, it's
better to just let the reference processing thread do the processing,
rather than slowing it down to allow for the rare case when there's
another thread that could possibly help.  This is especially true now
that Cleaner has such limited usage.

This leads to a different API for other threads; rather than
tryHandlePending that other threads can call to help and to examine
progress, we now have waitForReferenceProcessing.  The VM API is also
different; rather than popReferencePendingList to get or wait, we now
have getAndClearReferencePendingList and checkReferencePendingList,
the latter with an optional wait.

The splitting of the VM API allows us to avoid a narrow race condition
discussed by Peter in his prototypes.  Peter suggested this race was
ok because java.nio.Bits makes several retries.  However, those
retries are only done before throwing OOME.  There are no retries
before invoking GC, so this race could lead to unnecessary successive
GCs.

Doing all the processing on the reference processing thread eliminates
execution of Cleaners on application threads, though that's not nearly
as much an issue now that the use of Cleaner is so restricted.

We've also eliminated a pre-existing issue where a helper could report
no progress even though the reference processing thread (and other
helpers) had removed a pending reference, but not yet processed it.
This could result in the non-progressing helper invoking GC or
reporting failure, even though it might have succeeded if it had
waited for the other thread(s) to complete processing the reference(s)
being worked on.

I think the new waitForReferenceProcessing could be used to fix
JDK-6306116, though that isn't part of this change, and was not really
a motivating factor.

I don't know if the new waitForReferenceProcessing will be used by
whatever we eventually do for JDK-8149925, but I think the new VM API
will suffice for that.  That is, I think JDK-8149925 might require
changes to the core-libs API used 

RFR: 8163369: Enable generating DMH classes at link time

[Claes Redestad]

Hi,

please review this change to add the ability to generate 
DirectMethodHandles to the --generate-jli-classes jlink plugin.


The implementation generates all the specified DMHs as methods into a 
single class, java.lang.invoke.DirectMethodHandle$DMH. At runtime when a 
DMH's LF is set up, we speculatively resolve the member from this class 
instead of generating and loading the bytecode as a distinct anonymous 
class. This avoids loading a potentially large number of anonymous 
classes at runtime, and also enables other startup optimizations such as 
allowing CDS to see and dump this class to the shared archive.


webrev: http://cr.openjdk.java.net/~redestad/8163369/webrev.01/
JBS: https://bugs.openjdk.java.net/browse/JDK-8163369

This is a sub-task of https://bugs.openjdk.java.net/browse/JDK-8086045, 
which captures more information about the ongoing effort to reduce 
j.l.invoke initialization overheads.


Thanks!

/Claes

RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

[Frank Yuan]

Thank you! I pushed the change into jaxp repo.

Frank

> -Original Message-
> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> Sent: Saturday, August 06, 2016 1:59 AM
> To: Frank Yuan
> Cc: 'Daniel Fuchs'; 'core-libs-dev'
> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit 
> tests
> 
> Hi Frank,
> 
> Looks good overall. Thanks for adding runs with/without SM, that's a lot
> of tedious work! I wish there's a way to do this in a general
> configuration. But it's fine since it does serve the purpose.
> 
> Thanks,
> Joe
> 
> On 8/5/16, 6:26 AM, Frank Yuan wrote:
> > Hi Joe and Daniel
> >
> > Please review the update: 
> > http://cr.openjdk.java.net/~fyuan/8067170/webrev.04/
> > In this version, I
> > 1. made all tests run twice, to pass in the different argument to the jtreg 
> > TestNG test, it has to run in othervm(jaxp test also
run
> > in othervm before this but it's possible to run in agentvm), however, I 
> > didn't delete the code supporting agentvm from
> > JAXPPolicyManager.java because jtreg may provide more support in agentvm 
> > mode someday:)
> > 2. enabled sm in unittest/catalog/CatalogTest.java as jdk-8161176's 
> > conclusion
> > 3. fixed the bug in runWithTmpPermission methods, Daniel mentioned it but I 
> > didn't understand at that time :P
> >
> > Thanks
> > Frank
> >
> >> -Original Message-
> >> From: Frank Yuan [mailto:frank.y...@oracle.com]
> >> Sent: Thursday, August 04, 2016 6:06 PM
> >> To: 'Joe Wang'; 'Daniel Fuchs'
> >> Cc: 'core-libs-dev'
> >> Subject: RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit 
> >> tests
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> >>> Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
> >> unit tests
> >>>
> >>>
> >>> On 8/3/16, 3:45 AM, Daniel Fuchs wrote:
>  Hi Frank,
> 
>  I looked at the diffs of the diffs between webrev.02 and webrev.03 and
>  it looks good to me.
> 
>  +1 - provided all tests pass :-)
> >>> +1, thanks for re-enabling the tests that had dependencies on 8162848. I
> >>> also closed 8162848.
> >>>
>  But I have the same question than Joe: aren't all the test supposed
>  to run twice: once with security manager and once without?
>  (except for those test that might explicitly require a security
> >> manager)
> >>> I agree, all of the tests need to run with and without security manager.
> >>> It would be good to combine the  runWithSecurityManager and
> >>> runWithoutSecurityManager methods into one with a condition that
> >>> determines whether a security manager is present.
> >>>
> >> Alright, I will make the tests run twice. To impl this, I have to add
> >> jtreg tags like the following:
> >> /*
> >>   * @test
> >>   * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest
> >>   * @run testng/othervm -DrunSecMngr=true common.Bug6350682
> >>   * @run testng/othervm common.Bug6350682
> >>   */
> >>
> >> And modify the Policy class accordingly. I am writing a small program to
> >> update the tests, will send the new version tomorrow...
> >>
> >> Frank
> >>
> >>
> >>> Best,
> >>> Joe
> >>>
>  best regards,
> 
>  -- daniel
> 
>  On 03/08/16 11:12, Frank Yuan wrote:
> > Hi Joe
> >
> > Thank you for your review!
> >
> > I updated the tests according to the latest comments as well as had
> > unittest/transform/TransformerTest.java up to date, please check
> > http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/
> >
> >
> > Thanks
> > Frank
> >
> > -Original Message-
> > From: Joe Wang [mailto:huizhe.w...@oracle.com]
> > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
> > unit tests
> >
> >
> >
> > On 8/2/16, 5:30 AM, Daniel Fuchs wrote:
> >> Hi Frank,
> >>
> >> I am intrigued by these change which do not seem
> >> to have anything to do with the rest. I mean, I'm not opposed
> >> as long as they are intended and that Joe validates them:
> >> 
> >>
> >>
> >> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
> >> nctional/org/xml/sax/ptests/XMLReaderNSTableTest.java.fram
> > es.html
> >>
> >> 118 spf.setNamespaceAware(false);
> > Not sure why this was changed.
> >>
> >> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
> >> nctional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra
> > mes.html
> > The test itself wasn't very clear on the content it tests. If it only
> > verifies whether a resolver is used as is said, then this and related
> > changes in ResolverTest and publish.xml are fine. To the extent it
> > verifies, it didn't have to output to a file.
> >>
> >> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
> >> nctional/org/xml/sa