Review request for JDK-8163266 : Doc for ValidationEvent#getSeverity()

[Abhijit Roy]

Hi all,

Please review a fix for Bug  - 
https://bugs.openjdk.java.net/browse/JDK-8163266
Bug - Doc for ValidationEvent#getSeverity() should say it returns a 
constant from ValidationEvent instead of ValidationError
Webrev - http://cr.openjdk.java.net/~ntv/ababroy/8163266/webrev.00/ 

I have just rectified and modified those errors. And moving forward it 
for review.


Regards,
Abhijit

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

[Frank Yuan]

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/sax/xmlfiles/out/NSTableFTGF.out.frames.h
> > >>
> > >> tml
> > >>>
> > >>
> > >> We have to let Frank explain why namespace was turned off for these
> > >> tests :-)  In this case, XMLReaderNSTableTest. In general, I would
> say
> > >> enabling security shouldn't change tests or gold files in this case.
> > >>
> > >>>
> > >>>
> http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
> nctional/org/xml/sax/xmlfiles/publish.xml.frames.html
> > >>>
> > >>>
> > >>> This looks like a desirable change - but what does it have to do
> > >>> with enabling security manager?
> > >>
> > >> Probably to remove the reference to that particular server?  Seems to
> be
> > >> fine as a cleanup. Again, it (the original test) only v

Re: Review request for JDK-8163266 : Doc for ValidationEvent#getSeverity()

[Roger Riggs]

looks fine.

Roger


On 8/5/2016 6:48 AM, Abhijit Roy wrote:

Hi all,

Please review a fix for Bug  - 
https://bugs.openjdk.java.net/browse/JDK-8163266
Bug - Doc for ValidationEvent#getSeverity() should say it returns a 
constant from ValidationEvent instead of ValidationError
Webrev - http://cr.openjdk.java.net/~ntv/ababroy/8163266/webrev.00/ 

I have just rectified and modified those errors. And moving forward it 
for review.


Regards,
Abhijit

RFR : 8139965:Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Seán Coffey]

Hoping to get a review on this issue that's been sitting on my plate for 
a long while. Pavel drew up the bulk of the edits for this one (Thanks)


The fix basically delegates polling and timeout management to the 
BlockingQueue.poll(timeout.. ) method. As a result it makes Connection 
readReply logic much easier to handle.


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8139965.9/webrev/
bug report : https://bugs.openjdk.java.net/browse/JDK-8139965

--
Regards,
Sean.

Re: RFR : 8139965:Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Roger Riggs]

Hi Sean,

Looks like a cleaner solution to synchronize writer and readers.

I don't quite understand the 80% capacity value.  It is related to the 
obsolete highWatermark

but that does not seem relevant with the update.

If the caller is going to specify a replyQueueCapacity then why should 
it be downgraded to 80%?


Roger



On 8/5/2016 10:05 AM, Seán Coffey wrote:
Hoping to get a review on this issue that's been sitting on my plate 
for a long while. Pavel drew up the bulk of the edits for this one 
(Thanks)


The fix basically delegates polling and timeout management to the 
BlockingQueue.poll(timeout.. ) method. As a result it makes Connection 
readReply logic much easier to handle.


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8139965.9/webrev/
bug report : https://bugs.openjdk.java.net/browse/JDK-8139965

Re: RFR : 8139965:Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Seán Coffey]

Sorry - cc'ed the wrong Pavel earlier. Corrected.

I thought the consumer/producer model was best served by delegating to 
the BlockingQueue. Is there a need to synchronize as a result ?


The 80% logic is only implemented in the rare case where the 
com.sun.jndi.ldap.search.replyQueueSize ldap context property is set. It 
seems to be legacy behaviour from the old code where the reader thread 
was paused at 80% capacity. Setting the BlockingQueue to the same '80%' 
size should have the same net effect.


Regards,
Sean.

On 05/08/16 15:50, Roger Riggs wrote:

Hi Sean,

Looks like a cleaner solution to synchronize writer and readers.

I don't quite understand the 80% capacity value.  It is related to the 
obsolete highWatermark

but that does not seem relevant with the update.

If the caller is going to specify a replyQueueCapacity then why should 
it be downgraded to 80%?


Roger



On 8/5/2016 10:05 AM, Seán Coffey wrote:
Hoping to get a review on this issue that's been sitting on my plate 
for a long while. Pavel drew up the bulk of the edits for this one 
(Thanks)


The fix basically delegates polling and timeout management to the 
BlockingQueue.poll(timeout.. ) method. As a result it makes 
Connection readReply logic much easier to handle.


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8139965.9/webrev/
bug report : https://bugs.openjdk.java.net/browse/JDK-8139965

RFR 9: JDK-8155102: Process.toString could include pid, isAlive, exitStatus

[Roger Riggs]

Hi Andrey,

I added a test to test/java/lang/ProcessBuilder/Basic.java.
Every change must have a test.

   http://cr.openjdk.java.net/~rriggs/webrev-process-8155102/

I can handle the extension request.

Addition review comments welcome.

Roger


On 8/2/2016 1:30 AM, Andrey Dyachkov wrote:

Roger,

I have changed impl according to your comments, thank you!
Regarding fc-extension, I am not able to add comments in the bug 
tracker I think you can do it, correct? Also I am already in OCA list.


diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java 
b/src/java.base/unix/classes/java/lang/ProcessImpl.java

--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -630,6 +630,19 @@
 return !hasExited;
 }

+/**
+ * The {@code toString} method returns a string consisting of
+ * the native process ID of the process and the exit value of the 
process.

+ *
+ * @return a string representation of the object.
+ */
+@Override
+public String toString() {
+return new StringBuilder("Process[pid=").append(pid)
+.append(", exitValue=").append(hasExited ? exitcode : "\"not 
exited\"")

+.append("]").toString();
+}
+
 private static native void init();

 static {
diff --git a/src/java.base/windows/classes/java/lang/ProcessImpl.java 
b/src/java.base/windows/classes/java/lang/ProcessImpl.java

--- a/src/java.base/windows/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/windows/classes/java/lang/ProcessImpl.java
@@ -564,6 +564,20 @@
 private static native boolean isProcessAlive(long handle);

 /**
+ * The {@code toString} method returns a string consisting of
+ * the native process ID of the process and the exit value of the 
process.

+ *
+ * @return a string representation of the object.
+ */
+@Override
+public String toString() {
+int exitCode = getExitCodeProcess(handle);
+return new StringBuilder("Process[pid=").append(getPid())
+.append(", exitValue=").append(exitCode == 
STILL_ACTIVE ? "\"not exited\"" : exitCode)

+.append("]").toString();
+}
+
+/**
  * Create a process using the win32 function CreateProcess.
  * The method is synchronized due to MS kb315939 problem.
  * All native handles should restore the inherit flag at the end 
of call.




On Mon, 1 Aug 2016 at 19:34 Roger Riggs > wrote:


Hi Andrey,

Sorry to be slow in getting back to this.

Thanks for the update;   did you notice that Windows has a
separate implementation of
ProcessImpl and would also need a similar, though not identical
toString() implementation?
[jdk/src/java.base/windows/classes/java/lang/ProcessImpl.java ]

Windows has a slightly different implementation of the internal
state with respect to knowing if it has exited.
See exitValue() and the native getExitCodeProcess() method.


On 7/24/2016 7:42 AM, Andrey Dyachkov wrote:

Hi Roger,

Thank you for reviewing it! I've prepared another version.

diff --git
a/src/java.base/unix/classes/java/lang/ProcessImpl.java
b/src/java.base/unix/classes/java/lang/ProcessImpl.java
--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -630,6 +630,21 @@
 return !hasExited;
 }

+/**
+ * The {@code toString} method for class {@code ProcessImpl}
+ * returns a string consisting of the native process ID of
the process,
+ * exit code of the process and the status of the process.

The javadoc for Process uses the terminology "exit value" or "exit
status"; the usage should be consistent.


+ *
+ * @return  a string representation of the object.
+ */
+@Override
+public String toString() {
+return new StringBuilder("ProcessImpl{pid=").append(pid)

The implementation class should be hidden, use "Process".
Other toString methods use '[]' to enclose the aggregate of the
values.


+.append(", exitcode=").append(exitcode)

'exitcode=' -> "exitValue" to be consistent with the exitValue method.

This will need an alternate value if the process has not exited,
perhaps "not exited"


+.append(", hasExited=").append(hasExited)

This isn't needed with the above 'not exited' string.

+.append("}").toString();

'}' -> ']'


Also, I think this would be viewed more as an enhancement than a
bug and we'll need
to request an extension[1][2] for it when the code is complete.

Thanks, Roger

[1]
http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004443.html
[2] http://openjdk.java.net/projects/jdk9/fc-extension-process



+}
+
 private static native void init();

  

Re: RFR : 8139965:Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Roger Riggs]

Hi Sean,

I agree that the 80% reflects the previous coding and is ok.

I don't have enough background on this code to know whether the 80% is 
now spurious and confusing and could be removed.


Roger


On 8/5/2016 11:46 AM, Seán Coffey wrote:

Sorry - cc'ed the wrong Pavel earlier. Corrected.

I thought the consumer/producer model was best served by delegating to 
the BlockingQueue. Is there a need to synchronize as a result ?


The 80% logic is only implemented in the rare case where the 
com.sun.jndi.ldap.search.replyQueueSize ldap context property is set. 
It seems to be legacy behaviour from the old code where the reader 
thread was paused at 80% capacity. Setting the BlockingQueue to the 
same '80%' size should have the same net effect.


Regards,
Sean.

On 05/08/16 15:50, Roger Riggs wrote:

Hi Sean,

Looks like a cleaner solution to synchronize writer and readers.

I don't quite understand the 80% capacity value.  It is related to 
the obsolete highWatermark

but that does not seem relevant with the update.

If the caller is going to specify a replyQueueCapacity then why 
should it be downgraded to 80%?


Roger



On 8/5/2016 10:05 AM, Seán Coffey wrote:
Hoping to get a review on this issue that's been sitting on my plate 
for a long while. Pavel drew up the bulk of the edits for this one 
(Thanks)


The fix basically delegates polling and timeout management to the 
BlockingQueue.poll(timeout.. ) method. As a result it makes 
Connection readReply logic much easier to handle.


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8139965.9/webrev/
bug report : https://bugs.openjdk.java.net/browse/JDK-8139965

Re: RFR : 8139965:Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Vincent Ryan]

IIRC pausing the receiver was introduced to prevent an LDAP client (with 
limited memory) from being overwhelmed
by server results and having no opportunity to transmit an abandon request. The 
80% value was chosen to balance
maximising queue capacity against the risk of an OOME.

If that behaviour is maintained via a BlockingQueue then all is well.


> On 5 Aug 2016, at 16:56, Roger Riggs  wrote:
> 
> Hi Sean,
> 
> I agree that the 80% reflects the previous coding and is ok.
> 
> I don't have enough background on this code to know whether the 80% is now 
> spurious and confusing and could be removed.
> 
> Roger
> 
> 
> On 8/5/2016 11:46 AM, Seán Coffey wrote:
>> Sorry - cc'ed the wrong Pavel earlier. Corrected.
>> 
>> I thought the consumer/producer model was best served by delegating to the 
>> BlockingQueue. Is there a need to synchronize as a result ?
>> 
>> The 80% logic is only implemented in the rare case where the 
>> com.sun.jndi.ldap.search.replyQueueSize ldap context property is set. It 
>> seems to be legacy behaviour from the old code where the reader thread was 
>> paused at 80% capacity. Setting the BlockingQueue to the same '80%' size 
>> should have the same net effect.
>> 
>> Regards,
>> Sean.
>> 
>> On 05/08/16 15:50, Roger Riggs wrote:
>>> Hi Sean,
>>> 
>>> Looks like a cleaner solution to synchronize writer and readers.
>>> 
>>> I don't quite understand the 80% capacity value.  It is related to the 
>>> obsolete highWatermark
>>> but that does not seem relevant with the update.
>>> 
>>> If the caller is going to specify a replyQueueCapacity then why should it 
>>> be downgraded to 80%?
>>> 
>>> Roger
>>> 
>>> 
>>> 
>>> On 8/5/2016 10:05 AM, Seán Coffey wrote:
 Hoping to get a review on this issue that's been sitting on my plate for a 
 long while. Pavel drew up the bulk of the edits for this one (Thanks)
 
 The fix basically delegates polling and timeout management to the 
 BlockingQueue.poll(timeout.. ) method. As a result it makes Connection 
 readReply logic much easier to handle.
 
 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8139965.9/webrev/
 bug report : https://bugs.openjdk.java.net/browse/JDK-8139965
 
>>> 
>> 
> 

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

[Gregg Wonderly]

Sent from my iPad

> On Aug 4, 2016, at 11:18 PM, Alan Bateman  wrote:
> 
>> On 04/08/2016 10:33, Patrick Reinhart wrote:
>> 
>> Hi Paul,
>> 
>> 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 ...".

I think that the use of the Stream should throw checked exceptions and these 
method invocations should only throw RuntimeException instances to make it 
clear that lazy implementations are expected.

Gregg

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

[Joe Wang]

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/sax/xmlfiles/out/NSTableFTGF.out.frames.h

tml
We have to let Frank explain why namespace was turned off for these
tests :-)  In this case, XMLReaderNSTableTest. In general, I would

say

enabling security shouldn't change tests or gold files in this case.




http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
nctional/org/xml/sax/xmlfiles/publish.xml.frames.html


This looks like a desirable change - but what does it have to do
with enabling security manager?

Probably to remove the reference to that particular server?  Seems to

be

fine as a cleanup. Again, it (the original test) only verifies a

resolve

is used, it didn't even need to write out a file to prove that.

Also:




http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/un
ittest/transform/XSLTFunctionsTest.java.frames.html



   70


tf.setFeature("http://www.oracle.com/xml/jaxp/properties/enableExtensionFu
nctions",

true);

Is this needed only when there is a security

Re: RFR: 8161588: MemberName::resolveOrNull cause and hide NoSuchMethodErrors

[Claes Redestad]

Withdrawing this as clearing exceptions actually doesn't seem to get rid of
overhead(!), making this change moot. The overhead for my use case
is small but annoying.

Sorry for the noise.

/Claes

On 07/19/2016 12:17 PM, Claes Redestad wrote:

Hi,

please review this bug fix to ensure MemberName::resolveOrNull doesn't
throw exceptions when speculatively looking up members that aren't
there.

HS: http://cr.openjdk.java.net/~redestad/8161588/hs.01
JDK: http://cr.openjdk.java.net/~redestad/8161588/jdk.01

This avoids throwing NoSuchMethodError etc just to be ignored, avoiding
a performance penalty when looking things up speculatively (which is key
to possible upcoming work to generate more JLI code with jlink).

There's a pre-existing issue not dealt with by this fix in that the
exceptions thrown in MHN_resolve_Mem are never observed, instead the
exceptions thrown from various LinkResolver methods are observed. We
could clear all pending exceptions in resolve_MemberName, but this
breaks tests that are very particular about which exception is thrown
when and where, thus I opted to add the clear_pending boolean to
allow clearing the exception conditionally instead, keeping behavior
identical for MemberName::resolveOrFail

Thanks!

/Claes

Re: RFR: 8161379: Force inline methods calling Reflection.getCallerClass

[Claes Redestad]

Anyone?

On 07/19/2016 08:16 AM, Claes Redestad wrote:

Hi,

most @CallerSensitive methodscall Reflection.getCallerClass(), which
turn out to have problematic performance characteristics when it fails
to inline.

Making @CallerSensitive imply @ForceInline actually works rather well
across benchmarks, but didn't meet with approval from hotspot-compiler
because it's a hack (unlike @ForceInline /s).

Instead, here is a patch to explicitly @ForceInline those methods where
it can plausibly be helping with performance:

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

Thanks!

/Claes

Re: RFR: 8161379: Force inline methods calling Reflection.getCallerClass

[Aleksey Shipilev]

This looks good to me, Claes.

I wouldn't mind to have a comment at @ForceInline line mentioning this
is for Reflection.getCallerClass() optimization. But, it might be
recoverable from the source control anyway.

Thanks,
-Aleksey

On 08/05/2016 10:37 PM, Claes Redestad wrote:
> Anyone?
> 
> On 07/19/2016 08:16 AM, Claes Redestad wrote:
>> Hi,
>>
>> most @CallerSensitive methodscall Reflection.getCallerClass(), which
>> turn out to have problematic performance characteristics when it fails
>> to inline.
>>
>> Making @CallerSensitive imply @ForceInline actually works rather well
>> across benchmarks, but didn't meet with approval from hotspot-compiler
>> because it's a hack (unlike @ForceInline /s).
>>
>> Instead, here is a patch to explicitly @ForceInline those methods where
>> it can plausibly be helping with performance:
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8161379/jdk.01/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161379
>>
>> Thanks!
>>
>> /Claes
>>
> 

Re: RFR: 8161379: Force inline methods calling Reflection.getCallerClass

[Claes Redestad]

On 08/05/2016 12:56 PM, Aleksey Shipilev wrote:

This looks good to me, Claes.


Thanks!



I wouldn't mind to have a comment at @ForceInline line mentioning this
is for Reflection.getCallerClass() optimization. But, it might be
recoverable from the source control anyway.


Sure, how about:

@ForceInline // to ensure Reflection.getCallerClass optimization

http://cr.openjdk.java.net/~redestad/8161379/jdk.02/

While source control history helps, I think it's alright with some
additional verbiage here in case someone figures out a way to
implement any of these without Reflection.getCallerClass (since
we ideally don't want to @ForceInline in too many places)

/Claes


Thanks,
-Aleksey

On 08/05/2016 10:37 PM, Claes Redestad wrote:

Anyone?

On 07/19/2016 08:16 AM, Claes Redestad wrote:

Hi,

most @CallerSensitive methodscall Reflection.getCallerClass(), which
turn out to have problematic performance characteristics when it fails
to inline.

Making @CallerSensitive imply @ForceInline actually works rather well
across benchmarks, but didn't meet with approval from hotspot-compiler
because it's a hack (unlike @ForceInline /s).

Instead, here is a patch to explicitly @ForceInline those methods where
it can plausibly be helping with performance:

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

Thanks!

/Claes

Re: RFR: 8161379: Force inline methods calling Reflection.getCallerClass

[Aleksey Shipilev]

On 08/05/2016 11:22 PM, Claes Redestad wrote:
> On 08/05/2016 12:56 PM, Aleksey Shipilev wrote:
>> I wouldn't mind to have a comment at @ForceInline line mentioning this
>> is for Reflection.getCallerClass() optimization. But, it might be
>> recoverable from the source control anyway.
> 
> Sure, how about:
> 
> @ForceInline // to ensure Reflection.getCallerClass optimization
> 
> http://cr.openjdk.java.net/~redestad/8161379/jdk.02/

+1

-Aleksey

Re: RFR: 8161379: Force inline methods calling Reflection.getCallerClass

[Mandy Chung]

> On Aug 5, 2016, at 1:22 PM, Claes Redestad  wrote:
> 
> http://cr.openjdk.java.net/~redestad/8161379/jdk.02/
> 

Looks fine.

How much improvement do you see from the benchmark run with this patch?

Mandy

Re: RFR: 8161379: Force inline methods calling Reflection.getCallerClass

[Claes Redestad]

Thanks! 

Up to 12-15x on some reflection micros, but it varies wildly. This change does 
not improve the best case performance, but rather eliminates a cause for 
degradation and high run to run variance.

/Claes

Mandy Chung  skrev: (5 augusti 2016 14:55:59 GMT-07:00)
>
>> On Aug 5, 2016, at 1:22 PM, Claes Redestad
> wrote:
>> 
>> http://cr.openjdk.java.net/~redestad/8161379/jdk.02/
>> 
>
>Looks fine.
>
>How much improvement do you see from the benchmark run with this patch?
>
>Mandy

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

RFR: 8156499 Update jlink to support creating images with modules that are packaged as multi-release JARs

[Steve Drach]

Hi,

Please review this changset that makes jlink multi-release jar aware.

issue: https://bugs.openjdk.java.net/browse/JDK-8156499 

webrev: http://cr.openjdk.java.net/~sdrach/8156499/webrev.00/index.html 


Thanks
Steve