Re: 8226863: [aix] jdk/java/lang/reflect/exeCallerAccessTest cannot launch on primordial thread

[Thomas Stüfe]

Hi Mandy,

On Wed, Jul 3, 2019, 00:20 Mandy Chung  wrote:

> Hi Thomas,
>
> This is AIX-only.It would be cleaner to move AIX-specific launcher
> to a new file like main_aix.c.  Have you considered that?
>

I rather not since this would unnecessarily increase complexity even
further.

However, if you really dislike the platform specific code, I would withdraw
the patch and just disable the test on AIX. I was on the fence about that
anyway.

Cheers, Thomas


>
> I don't know how to specify additional .c file in
> make/test/JtregNativeJdk.gmk  though.
>
> Mandy
>
>
>
> On 7/1/19 10:23 PM, Thomas Stüfe wrote:
> > Second, corrected Webrev:
> >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/execalleraccesstest-cannot-launch-on-primordial-thread/webrev.01/webrev/
> >
> > Run through SAP nightlies on all platforms.
> >
> > Cheers, Thomas
> >
> >
> > On Thu, Jun 27, 2019 at 9:02 AM Thomas Stüfe 
> > wrote:
> >
> >> Hi all,
> >>
> >> Issue:
> >> https://bugs.openjdk.java.net/browse/JDK-8226863
> >> webrev:
> >>
> >>
> http://cr.openjdk.java.net/~stuefe/webrevs/8226863--jdk-java-lang-reflect-execalleraccesstest-cannot-launch-on-primordial-thread/webrev.00/webrev/
> >>
> >> we have this annoying issue on AIX that the libjvm cannot be invoked on
> a
> >> primordial thread. Therefore we need to change launchers which create
> the
> >> VM to spawn own threads (we only do this where it is worth the effort -
> >> this, admittedly, is a corner case, we could alternatively just disable
> the
> >> test on AIX).
> >>
> >> This is annoying and vaguely embarrassing :-/ Maybe IBM will step in
> some
> >> time and help us solve the underlying issues which have to do with the
> >> inability to create guard pages on the primordial thread stack (among
> other
> >> things IIRC).
> >>
> >> Thanks for reviewing,
> >>
> >> Thomas
> >>
> >>
> >>
>
>

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Kasper Nielsen]

On Tue, 2 Jul 2019 at 22:49, Ralph Goers  wrote:
>
> The timing of this question is perfect. I have been doing some testing over 
> the last week to address https://issues.apache.org/jira/browse/LOG4J2-2644 
> and found some interesting things - although they are related to the walk() 
> method, not getCallerClass().

Getting a single stack frame with the calling class + line number for logging
is probably the most common use case for using a StackWalker.

I think optimized versions of these two methods would be really useful and
cover most of these usecases:

Optional findFirstWithClassName(Predicate p) {...}
Optional findFirstWithDeclaringClass(Predicate> p) {...}

There is really no reason to create a StackFrame object for every
frame. Only the final frame.

Perhaps even skip returning a StackFrame but just returning a string with
ClassName:Linenumber if there is significant overhead in creating the
StackFrame. I use StackWalker as well for logging, and find that the
2-3 microseconds it typically takes to get the calling class + line
number a bit steep in my performance budget.

/Kasper

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Kasper Nielsen]

On Tue, 2 Jul 2019 at 18:50, Mandy Chung  wrote:
>
> I'm not getting how getCallerClass is used and related to access check.
> Can you elaborate?

Caller sensitive methods are viral, in the sense that if you invoke a caller
sensitive method in the JDK, as a library on behalf of a client (without
having a lookup object). You need to perform the same access checks as the JDK
does. That is, checking that the client that calls you - the library - has the
right access rights. The caller sensitive methods in the JDK cannot do this,
because there is no way for them to see that the library is merely a proxy
acting on behalf of a client.

Consider a very simple serialization library with one method
  String serializeToString(Object o) // prints out the value of every field
with an implementation in a module called SER. And two modules M1, M2 that uses
SER (For example, via a ServiceLoader). Both M1 and M2 are open to SER in order
for the serializer to serialize the objects in each of the two modules.
However, M1 and M2 are not open towards each other. So it is not the intention
that, for example, some code from M1 can call it with objects from M2 and have
them serialized or vice versa. However, this is entire possible unless
serializeToString() performs access checks on the caller. All M1 has to do is
get a hold of an object from M2 and then call serializeToString() with it.
There is no way the jdk can check this, it just sees an object from M2 which
is open to SER. It has no idea it is actually M1 trying to serialize it. So
the only way for this to work as intended is for serializeToString to check
that the caller matches the object. And unless you pass around Lookup objects
the only way you can do it, is similar to how the jdk does it; by looking at the
calling class. Reflection::getCallerClass is not available outside of the JDK,
so StackWalker is the only way to do this.

I've put up an example at https://github.com/kaspernielsen/modulestest.
Calling M2Use.main will serialize an object from M1 even though it M1 is
not open to M2.

As noted in another thread this gets further complicated because all the access
control code is buried in internal jdk classes. In this example you more or less
have to reimplement AccessibleObject.checkCanSetAccessible yourself.
In the end I don't think it is realistic to expect library developers to get
this right.

/Kasper

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Stuart Marks]

Hi Jaikiran,

OK, good analysis. Rather a lot of issues for what seems like a simple patch, 
eh?

 - Idempotency

Yes, it looks to me like Inflater.end() and Deflater.end() implementations are 
idempotent. As you point out, overrides by subclasses might not be. We should be 
clear when we're talking about the specific implementatations of the end() 
methods in the Deflater and Inflater classes, or whether we're talking about the 
contracts defined by these method specifications on these classes and subtypes.


The behavior of an implementation in a class can be specified with @implSpec 
without imposing this on the contract of subclasses. This is useful for 
subclasses that need to know the exact behavior of calling super.end(), and also 
for callers who know they have an instance of a particular class and not a subclass.


The upshot is that while we might not have the end() method's contract specify 
idempotency, we can certainly say so in an @implSpec, if doing this will help. 
I'm not sure we'll actually need it in this case, but let's keep it in mind.


 - Subclasses

I don't think there are any subclasses in the JDK, and I did some searching and 
I didn't find any subclasses "in the wild" either. I did find a few uses of 
these classes though. If these classes are rarely subclassed, we might be able 
to get away with changing the contract, though this does entail some risk.


If we need to modify the subclasses used in tests, that's fair game though.

 - Relationship between end() and close()

I think close() should have more-or-less the same semantics as end(), namely, 
pretty much the way end() is specified now regarding releasing resources. But it 
should be allowable to call both in either order.


try (var inf = new Inflater()) {
...
# explicitly call end()
inf.end();
}

I think this should allowed, but it shouldn't be required. The application can 
call either close() or end() and this will have the effect of releasing the 
native resources.


A key question is whether close() should be specified to call end() -- which is 
subject to being overridden by suclasses -- or whether close() is defined to do 
the same thing as the end() implementation in this class does. This can be 
implemented by taking the body of the current end() method and refactoring it 
into an internal method and then having both close() and end() call that 
internal method.


If close() calls end() then AC+TWR might call close() twice, and therefore call 
end() twice, which might be a problem for subclasses. So to be really 
conservative we might want to do the internal-method refactoring to avoid this 
problem.


On the other hand, suppose a subclass has extra resources it frees in its end() 
method, which then calls super.end(). Even though this class would inherit AC, 
using it in TWR would be a *bug* because close() would call the internal method 
to free the superclass resources, but this would leak the subclass resources. 
That sounds like a mistake to perpetuate in the API.


It's possible for a subclass to override end() in such a way that's not 
idempotent, but I think this is unlikely. I'm leaning toward risking the small 
possibility of incompatibility in declaring end() idempotent, allowing close() 
simply to call end(). This makes TWR more useful in the long run, which seems 
like a better position to be in. Of course, if somebody turns up evidence that 
this would break a bunch of stuff, we should reconsider.


(This might be moot anyway. The condition where TWR closes a resource multiple 
times occurs in cases where closing a wrapper closes the wrapped resource, and 
both are TWR resource variables. But in my earlier example


try (var inf = new Inflater();
 var iis = new InflaterInputStream(inputStream, inf)) {
...
}

and in fact all of {Inflater,Deflater}{Input,Output}Stream don't close the 
passed-in Deflater/Inflater, so multiple close() calls won't occur.)


 - Deprecation of end()

I don't think deprecation of end() is useful. It'll just cause noise for people. 
Uses such as


var deflater = new Deflater();
try {
...
} finally {
deflater.end();
}

are valid and correct and needn't be changed (though using TWR is preferable, 
this is more of a style issue).


 - Undefined behavior after close()/end()

OK, I'll admit this is possibly out of scope, but the line in the specs about 
"once end() is called, the behavior is undefined" rubs me the wrong way. Right 
now, most operations seem to call ensureOpen(), which throws NPE if the object 
has been closed. But "undefined" allows anything to happen, including filling 
the output buffer with garbage. That seems wrong. We might not want to specify 
what exception is thrown, but we might want to specify that *AN* exception is 
thrown -- which must be a RuntimeException, since most methods don't declare any 
checked exceptions.


In any case, the clause would have to be 

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Ralph Goers]

Thanks Mandy, 

It seems I commented on the thread mentioned in the issue you linked to. 
Unfortunately, it doesn’t look like any work has been done on the issue.in the 
last 18 months. 

Yes, LogRecord doesn’t get the StackTraceElement. We only get it for the one 
stack entry we are interested in and we only do that because Log4j is still 
compatible with Java 7 and creating our own class would be too disruptive. 
Still, the cost seems to be in locating the correct frame, not creating the 
StackTraceElement. I don’t have an account in the bug tracking system or I 
would comment on the bug there instead of here. But it seems like adding 
toArray() and toArray(int frames) wouldn’t be very hard and has to perform 
better than Stream.toArray. While it was a nice idea to use Streams to deal 
with the stack trace, in practical terms almost everything that needs to do it 
is going to be extremely performance sensitive and is going to want to traverse 
it as fast as possible.  

Ralph

> On Jul 2, 2019, at 4:12 PM, Mandy Chung  wrote:
> 
> Hi Ralph,
> 
> Thanks for this info.  
> 
> Quick comments: LogRecord does not get the line number nor StackTraceElement. 
>  There is cost to construct the string-based StackTraceElement or get the 
> line number mapped from BCI.   And it is orthogonal to 
> StackWalker::getCallerClass that is only interested in the Class object. 
> 
> You may also be interesting in 
> https://bugs.openjdk.java.net/browse/JDK-8189752 
>  to take a snapshot of a 
> stack trace (possibly the top N frames).
> 
> Mandy
> 
> On 7/2/19 2:49 PM, Ralph Goers wrote:
>> The timing of this question is perfect. I have been doing some testing over 
>> the last week to address https://issues.apache.org/jira/browse/LOG4J2-2644 
>>  and found some 
>> interesting things - although they are related to the walk() method, not 
>> getCallerClass().
>> 1. Using skip(n) to find a frame a couple frames up is pretty fast but isn’t 
>> too much faster than finding the same element using new 
>> Throwable().getStackTrace() was in Java 8.
>> 2. The cost of walking the stack becomes much more costly as the number of 
>> elements needing to be walked increases.
>> 3. The most shocking to me was that the fastest way to traverse a stack 
>> trace is to use a Function that immediately converts the Stream to an array 
>> and then use an old style for loop to traverse it. However, doing this is 
>> incredibly awkward because StackWalker only supports streams so there is no 
>> good way to pass the value being searched for into the Function. I had to 
>> resort to storing it in a ThreadLocal. Having a toArray() method on 
>> StackWalker would be a lot nicer, especially if I could limit the number of 
>> frames retrieved. I should note that java.util.logging.LogRecord uses a 
>> Filter to walk the stack which is faster than the stream methods I was 
>> originally using, but is much slower than what I ended up with. 
>> 
>> As for the issue mentioned here, I believe I reported that getCallerClass 
>> was much slower than the Reflection class in Java 9 and opened a bug here. 
>> As I recall that was addressed and I believe I verified that fix but it 
>> probably wouldn’t hurt for me to do it again.
>> 
>> Ralph
>> 
>>> On Jul 2, 2019, at 10:48 AM, Mandy Chung >> > wrote:
>>> 
>>> MethodHandles::lookup is optimized (@ForceInline) and so it may not
>>> represent apple-to-apple comparison.StackWalker::getCallerClass
>>> does have overhead compared to Reflection::getCallerClass and
>>> need to get the microbenchmark in the jdk repo and rerun the numbers [1].
>>> 
>>> I'm not getting how getCallerClass is used and related to access check.
>>> Can you elaborate?
>>> 
>>> Mandy
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8221623 
>>> 
>>> 
>>> 
>>> On 7/2/19 6:07 AM, Kasper Nielsen wrote:
 Hi Remi,
 
 Yes, setting up a StackWalker is more or less free. It is just
 wrapping a set of options.
 
 public class StackWalkerPerf {
 
 static final StackWalker sw =
 StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
 
 @Benchmark
 public StackWalker stackWalkerSetup() {
 return StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
 }
 
 @Benchmark
 public Class stackWalkerCallerClass() {
 return sw.getCallerClass();
 }
 
 @Benchmark
 public Lookup reflectionCallerClass() {
 return MethodHandles.lookup();
 }
 }
 
 Benchmark   Mode  Cnt ScoreError  Units
 StackWalkerPerf.stackWalkerSetupavgt   1011.958 ±  0.353  ns/op
 StackWalkerPerf.reflectionCallerClass   avgt   10 8.511 ±  0.415  ns/op
 

Re: Mechanism to maintain backwards compatibility when moving classes to different packages

[Scott Palmer]

Not worth it. How will package level access be affected? How can this be 
exploited? How will it affect security and signing when things from the same 
package aren’t *really* from the same package?

Now that the recommendation is to bundle a JRE with your application, that kind 
of backwards compatibility is becoming less critical.  If necessary, deprecate 
java.util.Random. Make an interface for RNGs to implement and go with factories 
and a service provider pattern. Sometime down the line retire java.util.Random. 

Scott

> On Jul 2, 2019, at 7:55 PM, Jacob Glickman  wrote:
> 
> Friendly reminder :)
> -- Forwarded message -
> From: Jacob Glickman 
> Date: Sat, Jun 22, 2019 at 4:42 PM
> Subject: Mechanism to maintain backwards compatibility when moving classes
> to different packages
> To: 
> 
> 
> Yesterday, Mark Reinhold introduced the idea of a java.util.random
> subpackage[1]. Obviously, moving java.util.Random into that subpackage is
> not currently an option, as that would break backwards compatibility.
> Assuming the subpackage is created, it would make sense to ultimately move
> java.util.Random into it. For that reason, I propose a new language feature
> that could make this possible:
> 
> package java.util.random previously java.util;
> 
> The syntax isn't important at the moment, but this mechanism should allow
> for users to run previously-compiled code with newer versions of Java, even
> if their code points to a class that has since been moved to a different
> package.
> 
> To eliminate potential bugs, users compiling new code shouldn't be allowed
> to reference java.util.Random, even if java.util.random.Random states that
> it previously resided in java.util.
> 
> I'm curious what you all would think of this, as it is not just applicable
> to this single example. There have been plenty of times that I've realized
> that my packages were named badly, but I'm forced to stick with that naming
> (unless I want my users to have to modify their code).
> 
> Thanks,
> 
> Jacob Glickman
> 
> [1]:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-June/060995.html

Fwd: Mechanism to maintain backwards compatibility when moving classes to different packages

[Jacob Glickman]

Friendly reminder :)
-- Forwarded message -
From: Jacob Glickman 
Date: Sat, Jun 22, 2019 at 4:42 PM
Subject: Mechanism to maintain backwards compatibility when moving classes
to different packages
To: 


Yesterday, Mark Reinhold introduced the idea of a java.util.random
subpackage[1]. Obviously, moving java.util.Random into that subpackage is
not currently an option, as that would break backwards compatibility.
Assuming the subpackage is created, it would make sense to ultimately move
java.util.Random into it. For that reason, I propose a new language feature
that could make this possible:

package java.util.random previously java.util;

The syntax isn't important at the moment, but this mechanism should allow
for users to run previously-compiled code with newer versions of Java, even
if their code points to a class that has since been moved to a different
package.

To eliminate potential bugs, users compiling new code shouldn't be allowed
to reference java.util.Random, even if java.util.random.Random states that
it previously resided in java.util.

I'm curious what you all would think of this, as it is not just applicable
to this single example. There have been plenty of times that I've realized
that my packages were named badly, but I'm forced to stick with that naming
(unless I want my users to have to modify their code).

Thanks,

Jacob Glickman

[1]:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-June/060995.html

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Mandy Chung]

Hi Ralph,

Thanks for this info.

Quick comments: LogRecord does not get the line number nor 
StackTraceElement.  There is cost to construct the string-based 
StackTraceElement or get the line number mapped from BCI.   And it is 
orthogonal to StackWalker::getCallerClass that is only interested in the 
Class object.


You may also be interesting in 
https://bugs.openjdk.java.net/browse/JDK-8189752 to take a snapshot of a 
stack trace (possibly the top N frames).


Mandy

On 7/2/19 2:49 PM, Ralph Goers wrote:
The timing of this question is perfect. I have been doing some testing 
over the last week to address 
https://issues.apache.org/jira/browse/LOG4J2-2644 and found some 
interesting things - although they are related to the walk() method, 
not getCallerClass().
1. Using skip(n) to find a frame a couple frames up is pretty fast but 
isn’t too much faster than finding the same element using new 
Throwable().getStackTrace() was in Java 8.
2. The cost of walking the stack becomes much more costly as the 
number of elements needing to be walked increases.
3. The most shocking to me was that the fastest way to traverse a 
stack trace is to use a Function that immediately converts the Stream 
to an array and then use an old style for loop to traverse it. 
However, doing this is incredibly awkward because StackWalker only 
supports streams so there is no good way to pass the value being 
searched for into the Function. I had to resort to storing it in a 
ThreadLocal. Having a toArray() method on StackWalker would be a lot 
nicer, especially if I could limit the number of frames retrieved. I 
should note that java.util.logging.LogRecord uses a Filter to walk the 
stack which is faster than the stream methods I was originally using, 
but is much slower than what I ended up with.


As for the issue mentioned here, I believe I reported that 
getCallerClass was much slower than the Reflection class in Java 9 and 
opened a bug here. As I recall that was addressed and I believe I 
verified that fix but it probably wouldn’t hurt for me to do it again.


Ralph

On Jul 2, 2019, at 10:48 AM, Mandy Chung > wrote:


MethodHandles::lookup is optimized (@ForceInline) and so it may not
represent apple-to-apple comparison.StackWalker::getCallerClass
does have overhead compared to Reflection::getCallerClass and
need to get the microbenchmark in the jdk repo and rerun the numbers [1].

I'm not getting how getCallerClass is used and related to access check.
Can you elaborate?

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8221623


On 7/2/19 6:07 AM, Kasper Nielsen wrote:

Hi Remi,

Yes, setting up a StackWalker is more or less free. It is just
wrapping a set of options.

public class StackWalkerPerf {

static final StackWalker sw =
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);

@Benchmark
public StackWalker stackWalkerSetup() {
return StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
}

@Benchmark
public Class stackWalkerCallerClass() {
return sw.getCallerClass();
}

@Benchmark
public Lookup reflectionCallerClass() {
return MethodHandles.lookup();
}
}

Benchmark   Mode  Cnt Score    Error 
 Units
StackWalkerPerf.stackWalkerSetup    avgt   10    11.958 ±  0.353 
 ns/op
StackWalkerPerf.reflectionCallerClass   avgt   10 8.511 ±  0.415 
 ns/op
StackWalkerPerf.stackWalkerCallerClass  avgt   10  1269.825 ± 66.471 
 ns/op


I'm using MethodHandles.lookup() in this test because it is cheapest
way to invoke Reflection.getCallerClass() without any tricks.
So real performance is likely better.

/Kasper

On Tue, 2 Jul 2019 at 13:53, Remi Forax > wrote:

Hi Kasper,
did you store the StackWalker instance in a static final field ?

Rémi

- Mail original -

De: "Kasper Nielsen" mailto:kaspe...@gmail.com>>
À: "core-libs-dev" >

Envoyé: Mardi 2 Juillet 2019 11:09:11
Objet: Slow performance of StackWalker.getCallerClass() vs 
Reflection.getCallerClass()

Hi,

Are there any security reasons for why StackWalker.getCallerClass()
cannot be made as performant as Reflection.getCallerClass()?
StackWalker.getCallerClass() is at least 100 times slower then
Reflection.getCallerClass() (~1000 ns/op vs ~10 ns/op).

I'm trying to retrofit some existing APIs where I cannot take a Lookup
object to do some access control checks.
But the performance of StackWalker.getCallerClass() is making it 
impossible.


Best
 Kasper

Re: (13) RFR (S): 8227055: Minor edits to launcher help text

[Mandy Chung]

On 7/2/19 3:44 PM, David Holmes wrote:

webrev: http://cr.openjdk.java.net/~dholmes/8227055/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8227055

The launcher help text needs some minor updates/corrections
- -verbose needs more info
- -Xdebug needs to say it does nothing


Should this be removed instead?


- -Xloggc is deprecated and replaced by -Xlog:gc:filename


I think it can just list -Xlog:gc (the replacement) and no need
to mention the deprecated option in the launcher help message.


- -Xshare:on needs warning about use

Reference to Mac OS X should be macOS.

The --disable-@files is documented as both a standard and extra option.



I think this is intended to be an extra option.

Otherwise, looks okay.

Mandy


This aligns the help info with the Java manpage info.

Thanks,
David

(13) RFR (S): 8227055: Minor edits to launcher help text

[David Holmes]

webrev: http://cr.openjdk.java.net/~dholmes/8227055/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8227055

The launcher help text needs some minor updates/corrections
- -verbose needs more info
- -Xdebug needs to say it does nothing
- -Xloggc is deprecated and replaced by -Xlog:gc:filename
- -Xshare:on needs warning about use

Reference to Mac OS X should be macOS.

The --disable-@files is documented as both a standard and extra option.

This aligns the help info with the Java manpage info.

Thanks,
David

Re: 8226863: [aix] jdk/java/lang/reflect/exeCallerAccessTest cannot launch on primordial thread

[Mandy Chung]

Hi Thomas,

This is AIX-only.    It would be cleaner to move AIX-specific launcher 
to a new file like main_aix.c.  Have you considered that?


I don't know how to specify additional .c file in 
make/test/JtregNativeJdk.gmk  though.


Mandy

On 7/1/19 10:23 PM, Thomas Stüfe wrote:

Second, corrected Webrev:

http://cr.openjdk.java.net/~stuefe/webrevs/execalleraccesstest-cannot-launch-on-primordial-thread/webrev.01/webrev/

Run through SAP nightlies on all platforms.

Cheers, Thomas


On Thu, Jun 27, 2019 at 9:02 AM Thomas Stüfe 
wrote:


Hi all,

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

http://cr.openjdk.java.net/~stuefe/webrevs/8226863--jdk-java-lang-reflect-execalleraccesstest-cannot-launch-on-primordial-thread/webrev.00/webrev/

we have this annoying issue on AIX that the libjvm cannot be invoked on a
primordial thread. Therefore we need to change launchers which create the
VM to spawn own threads (we only do this where it is worth the effort -
this, admittedly, is a corner case, we could alternatively just disable the
test on AIX).

This is annoying and vaguely embarrassing :-/ Maybe IBM will step in some
time and help us solve the underlying issues which have to do with the
inability to create guard pages on the primordial thread stack (among other
things IIRC).

Thanks for reviewing,

Thomas

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Ralph Goers]

The timing of this question is perfect. I have been doing some testing over the 
last week to address https://issues.apache.org/jira/browse/LOG4J2-2644 
 and found some interesting 
things - although they are related to the walk() method, not getCallerClass().
1. Using skip(n) to find a frame a couple frames up is pretty fast but isn’t 
too much faster than finding the same element using new 
Throwable().getStackTrace() was in Java 8.
2. The cost of walking the stack becomes much more costly as the number of 
elements needing to be walked increases.
3. The most shocking to me was that the fastest way to traverse a stack trace 
is to use a Function that immediately converts the Stream to an array and then 
use an old style for loop to traverse it. However, doing this is incredibly 
awkward because StackWalker only supports streams so there is no good way to 
pass the value being searched for into the Function. I had to resort to storing 
it in a ThreadLocal. Having a toArray() method on StackWalker would be a lot 
nicer, especially if I could limit the number of frames retrieved. I should 
note that java.util.logging.LogRecord uses a Filter to walk the stack which is 
faster than the stream methods I was originally using, but is much slower than 
what I ended up with. 

As for the issue mentioned here, I believe I reported that getCallerClass was 
much slower than the Reflection class in Java 9 and opened a bug here. As I 
recall that was addressed and I believe I verified that fix but it probably 
wouldn’t hurt for me to do it again.

Ralph

> On Jul 2, 2019, at 10:48 AM, Mandy Chung  wrote:
> 
> MethodHandles::lookup is optimized (@ForceInline) and so it may not
> represent apple-to-apple comparison.StackWalker::getCallerClass
> does have overhead compared to Reflection::getCallerClass and
> need to get the microbenchmark in the jdk repo and rerun the numbers [1].
> 
> I'm not getting how getCallerClass is used and related to access check.
> Can you elaborate?
> 
> Mandy
> [1] https://bugs.openjdk.java.net/browse/JDK-8221623
> 
> 
> On 7/2/19 6:07 AM, Kasper Nielsen wrote:
>> Hi Remi,
>> 
>> Yes, setting up a StackWalker is more or less free. It is just
>> wrapping a set of options.
>> 
>> public class StackWalkerPerf {
>> 
>> static final StackWalker sw =
>> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
>> 
>> @Benchmark
>> public StackWalker stackWalkerSetup() {
>> return StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
>> }
>> 
>> @Benchmark
>> public Class stackWalkerCallerClass() {
>> return sw.getCallerClass();
>> }
>> 
>> @Benchmark
>> public Lookup reflectionCallerClass() {
>> return MethodHandles.lookup();
>> }
>> }
>> 
>> Benchmark   Mode  Cnt ScoreError  Units
>> StackWalkerPerf.stackWalkerSetupavgt   1011.958 ±  0.353  ns/op
>> StackWalkerPerf.reflectionCallerClass   avgt   10 8.511 ±  0.415  ns/op
>> StackWalkerPerf.stackWalkerCallerClass  avgt   10  1269.825 ± 66.471  ns/op
>> 
>> I'm using MethodHandles.lookup() in this test because it is cheapest
>> way to invoke Reflection.getCallerClass() without any tricks.
>> So real performance is likely better.
>> 
>> /Kasper
>> 
>> On Tue, 2 Jul 2019 at 13:53, Remi Forax  wrote:
>>> Hi Kasper,
>>> did you store the StackWalker instance in a static final field ?
>>> 
>>> Rémi
>>> 
>>> - Mail original -
 De: "Kasper Nielsen" 
 À: "core-libs-dev" 
 Envoyé: Mardi 2 Juillet 2019 11:09:11
 Objet: Slow performance of StackWalker.getCallerClass() vs 
 Reflection.getCallerClass()
 Hi,
 
 Are there any security reasons for why StackWalker.getCallerClass()
 cannot be made as performant as Reflection.getCallerClass()?
 StackWalker.getCallerClass() is at least 100 times slower then
 Reflection.getCallerClass() (~1000 ns/op vs ~10 ns/op).
 
 I'm trying to retrofit some existing APIs where I cannot take a Lookup
 object to do some access control checks.
 But the performance of StackWalker.getCallerClass() is making it 
 impossible.
 
 Best
  Kasper
> 
> 

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Mandy Chung]

MethodHandles::lookup is optimized (@ForceInline) and so it may not
represent apple-to-apple comparison.StackWalker::getCallerClass
does have overhead compared to Reflection::getCallerClass and
need to get the microbenchmark in the jdk repo and rerun the numbers [1].

I'm not getting how getCallerClass is used and related to access check.
Can you elaborate?

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8221623


On 7/2/19 6:07 AM, Kasper Nielsen wrote:

Hi Remi,

Yes, setting up a StackWalker is more or less free. It is just
wrapping a set of options.

public class StackWalkerPerf {

 static final StackWalker sw =
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);

 @Benchmark
 public StackWalker stackWalkerSetup() {
 return StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
 }

 @Benchmark
 public Class stackWalkerCallerClass() {
 return sw.getCallerClass();
 }

 @Benchmark
 public Lookup reflectionCallerClass() {
 return MethodHandles.lookup();
 }
}

Benchmark   Mode  Cnt ScoreError  Units
StackWalkerPerf.stackWalkerSetupavgt   1011.958 ±  0.353  ns/op
StackWalkerPerf.reflectionCallerClass   avgt   10 8.511 ±  0.415  ns/op
StackWalkerPerf.stackWalkerCallerClass  avgt   10  1269.825 ± 66.471  ns/op

I'm using MethodHandles.lookup() in this test because it is cheapest
way to invoke Reflection.getCallerClass() without any tricks.
So real performance is likely better.

/Kasper

On Tue, 2 Jul 2019 at 13:53, Remi Forax  wrote:

Hi Kasper,
did you store the StackWalker instance in a static final field ?

Rémi

- Mail original -

De: "Kasper Nielsen" 
À: "core-libs-dev" 
Envoyé: Mardi 2 Juillet 2019 11:09:11
Objet: Slow performance of StackWalker.getCallerClass() vs 
Reflection.getCallerClass()
Hi,

Are there any security reasons for why StackWalker.getCallerClass()
cannot be made as performant as Reflection.getCallerClass()?
StackWalker.getCallerClass() is at least 100 times slower then
Reflection.getCallerClass() (~1000 ns/op vs ~10 ns/op).

I'm trying to retrofit some existing APIs where I cannot take a Lookup
object to do some access control checks.
But the performance of StackWalker.getCallerClass() is making it impossible.

Best
  Kasper

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Alan Bateman]

On 02/07/2019 18:20, Mandy Chung wrote:

:

I've taken several passes over the changes and I don't see anything 
obviously wrong. One minor nit is that the @throws 
IllegalAccessException in accessClass(Class) needs to be updated to 
allow for it to thrown when not accessible from the previous lookup 
class. Also VerifyAccess::isModuleAccessible shouldn't need to if the 
package is exported unconditionally as it is covered by 
isExported(pn, m1) && isExported(pn, m2).


Good point.  Fixed.

Webrev updated:
   http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/

The updated @throws IAE in accessClass and the updated 
VerifyAccess::isModuleAccessible looks good.


-Alan.

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Joe Wang]

Thanks Lance!

-Joe

On 7/2/19, 10:22 AM, Lance Andersen wrote:

Looks OK to me Joe
On Jul 2, 2019, at 11:51 AM, Joe Wang > wrote:


Thanks Daniel. That test case is added.

Best,
Joe

On 7/2/19, 1:21 AM, Daniel Fuchs wrote:

Hi Joe,

I haven't spotted anything obviously wrong.

The content of "abcxyz" should be "abc & xyz", not " 
   abc\n & \nxyz\n" as that before this fix.


Maybe the test could have a test case for that specific example too?
Just to make sure whitespaces in CDATA aren't eaten away?

best regards,

-- daniel


On 01/07/2019 20:46, Joe Wang wrote:
Please review a fix to xml pretty print. This is a regression 
introduced during the JDK 9 development. CDATA is marked up to be 
interpreted literally as textual data, in other words, it's still 
character data. The processor therefore shall treat it as text 
data. The content of "abcxyz" should be "abc & xyz", 
not "abc\n & \n xyz\n" as that before this fix.


JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/ 



Thanks,
Joe







Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Lance Andersen]

Looks OK to me Joe
> On Jul 2, 2019, at 11:51 AM, Joe Wang  wrote:
> 
> Thanks Daniel. That test case is added.
> 
> Best,
> Joe
> 
> On 7/2/19, 1:21 AM, Daniel Fuchs wrote:
>> Hi Joe,
>> 
>> I haven't spotted anything obviously wrong.
>> 
>>> The content of "abcxyz" should be "abc & xyz", not "
>>> abc\n & \nxyz\n" as that before this fix. 
>> 
>> Maybe the test could have a test case for that specific example too?
>> Just to make sure whitespaces in CDATA aren't eaten away?
>> 
>> best regards,
>> 
>> -- daniel
>> 
>> 
>> On 01/07/2019 20:46, Joe Wang wrote:
>>> Please review a fix to xml pretty print. This is a regression introduced 
>>> during the JDK 9 development. CDATA is marked up to be interpreted 
>>> literally as textual data, in other words, it's still character data. The 
>>> processor therefore shall treat it as text data. The content of 
>>> "abcxyz" should be "abc & xyz", not "abc\n & \n xyz\n" 
>>> as that before this fix.
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
>>> webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/
>>> 
>>> Thanks,
>>> Joe
>>> 
>> 

 
  

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

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Mandy Chung]

On 7/2/19 6:59 AM, Alan Bateman wrote:
This is really good work and fixes several issues left over from JDK 
9. The compatibility issues (mostly small/advanced) are unfortunate 
but necessary and I hope it won't impact too many things. It will need 
a detail release note once the CSR is done and the changes are in.




The existing usage of `privateLookupIn` I am aware of is using a private
lookup object and do deep reflection in one module.   I expect that the
compatibility risk should be low while the library developers/maintainers
are encouraged to test with EA early.

I've taken several passes over the changes and I don't see anything 
obviously wrong. One minor nit is that the @throws 
IllegalAccessException in accessClass(Class) needs to be updated to 
allow for it to thrown when not accessible from the previous lookup 
class. Also VerifyAccess::isModuleAccessible shouldn't need to if the 
package is exported unconditionally as it is covered by isExported(pn, 
m1) && isExported(pn, m2).


Good point.  Fixed.

Webrev updated:
   http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/

Mandy

Re: RFR: 8224974: Implement JEP 352

[Andrew Dinn]

Hi Alan,

On 02/07/2019 16:59, Alan Bateman wrote:
> On 18/06/2019 12:43, Andrew Dinn wrote:
> One nit is that the update to the Goals section says "rework" in two
> places. I think it might more accurate to say "upgrade" or "update" as
> it doesn't significantly re-working the existing implementation.
Thanks for checking this rewording. I have updated the JEP text modulo
replacing reworked with upgraded.

regards,


Andrew Dinn
---

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Joe Wang]

Thanks Daniel!

On 7/2/19, 9:16 AM, Daniel Fuchs wrote:

Hi Joe,

On 02/07/2019 17:51, Joe Wang wrote:

Thanks Daniel. That test case is added.


LGTM!

best regards,

-- daniel



Best,
Joe

On 7/2/19, 1:21 AM, Daniel Fuchs wrote:

Hi Joe,

I haven't spotted anything obviously wrong.

The content of "abcxyz" should be "abc & xyz", not 
"abc\n & \nxyz\n" as that before this fix. 


Maybe the test could have a test case for that specific example too?
Just to make sure whitespaces in CDATA aren't eaten away?

best regards,

-- daniel


On 01/07/2019 20:46, Joe Wang wrote:
Please review a fix to xml pretty print. This is a regression 
introduced during the JDK 9 development. CDATA is marked up to be 
interpreted literally as textual data, in other words, it's still 
character data. The processor therefore shall treat it as text 
data. The content of "abcxyz" should be "abc & xyz", 
not "abc\n & \n xyz\n" as that before this fix.


JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/

Thanks,
Joe

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Kasper Nielsen]

On Tue, 2 Jul 2019 at 16:51, Alan Bateman  wrote:
>
> Are you using privateLookupIn and dropping PRIVATE access? Are you
> teleporting in Lookup.in? It might be that you don't have to change
> anything.

I don't really have any control over how the Lookup object is created.
It is provide by users of the library. And I just use the "raw" version
(this might change at some point).

/Kasper


/Kasper

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Mandy Chung]

Hi Daniel,

Thanks for catching these.

On 7/2/19 8:03 AM, Daniel Fuchs wrote:

Hi Mandy,

Don't count me as reviewer; this is only editorial:

In MethodHandles.java:



 165  * A caller, specified as a {@code Lookup} object, is in 
module {@code M1} is
 166  * allowed to do deep reflection on module {@code M2} and 
package of the target class
 167  * if and only if all of the following conditions are {@code 
true}:


There are two 'is' in this sentence, which makes it a bit difficult
to parse. Should the first be removed?



Yes the first "is" is removed.
* A caller, specified as a {@code Lookup} object, in module {@code M1} 
is ...




 169  * If there is a security manager, its {@code 
checkPermission} method is
 170  * called to check {@code 
ReflectPermission("suppressAccessChecks")} and

 171  * that must return normally.
 172  * 
 173  * If there is a security manager, its {@code 
checkPermission} method is
 174  * called to check {@code 
ReflectPermission("suppressAccessChecks")}.

 175  * This method must return normally.

Probably one of these two bullets should be removed: they look
the same to me.



Removed. Thanks for catching it.


=

 585  * A {@code Lookup} with {@link #PUBLIC} mode and a lookup 
class in {@code M1}
 586  * can access public types in a module {@code M2} when {@code 
M2} is readable to
 587  * {@code M1} when the type is in a package of {@code M2} 
that is exported to at least {@code M1}.

 
I believe there is "and" missing here:

  <<... when M2 is readable to M1 *and* when the type >>



Fixed.

===

Is the specdiff report incomplete? I was hoping to see the table
added to the Lookup class level API under

 582  * Cross-module lookups

but it seems to be missing from the specdiff?



The specdiff tool is broken.  Sigh... I workaround to generate the 
method/field summary but hit another bug that it does not generate the 
class summary.

BTW: is  the right header? I thought  was reserved for the
 class name. John Gibbon will know more ;-)



It should be .   Updated.   This was written prior to that change.

thanks
Mandy

best regards,

-- daniel

On 02/07/2019 03:47, Mandy Chung wrote:



On 7/1/19 12:51 PM, Mandy Chung wrote:
This is an enhancement to |`Lookup::in`| and 
|`MethodHandles::privateLookupIn`| API
for cross module teleporting.  A `Lookup` object will record the 
previous

lookup class from which this |Lookup| object was teleported such that
the access check will use both the previous lookup class and the 
current
|lookup| context (current lookup class and allowed modes) to 
determine if

a type is accessible to this `Lookup` object or not.

In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
(lookup class in M1) and `PLC` (previous lookup class in M0) if and 
only if

1. both M0 and M1 can read M2
2. T is in a package that is exported from M2 at least to both M0 
and M1


Detailed specification is in Lookup class spec and `accessClass` 
javadoc.

The relevant spec about cross-module teleporting is in the Lookup class
spec and `Lookup::in` and `MethodHandles::privateLookupIn`.

CSR: https://bugs.openjdk.java.net/browse/JDK-8226916

webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00

javadoc:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html 



http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) 



I have yet to generate the spec diff. The tool is currently broken
due to javadoc change.  I'll try to workaround it and post the
spec diff soon.


specdiff:

http://cr.openjdk.java.net/~mchung/jdk14/8173978/specdiff/overview-summary.html 



Mandy

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Daniel Fuchs]

Hi Joe,

On 02/07/2019 17:51, Joe Wang wrote:

Thanks Daniel. That test case is added.


LGTM!

best regards,

-- daniel



Best,
Joe

On 7/2/19, 1:21 AM, Daniel Fuchs wrote:

Hi Joe,

I haven't spotted anything obviously wrong.

The content of "abcxyz" should be "abc & xyz", not 
"    abc\n & \n    xyz\n" as that before this fix. 


Maybe the test could have a test case for that specific example too?
Just to make sure whitespaces in CDATA aren't eaten away?

best regards,

-- daniel


On 01/07/2019 20:46, Joe Wang wrote:
Please review a fix to xml pretty print. This is a regression 
introduced during the JDK 9 development. CDATA is marked up to be 
interpreted literally as textual data, in other words, it's still 
character data. The processor therefore shall treat it as text data. 
The content of "abcxyz" should be "abc & xyz", not 
"    abc\n & \n xyz\n" as that before this fix.


JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/

Thanks,
Joe

Re: RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string

[naoto . sato]

Hi Thejasvi,

Here are my comments to the webrev:

TCKOffsetPrinterParser.java

- No need to define Locale_US as a static field, nor have it in the test 
data (data_print_localized) then pass it as an argument to the test. 
Specifying Locale.US in line 572, 578, and 586 should suffice.


TestOffsetPrinterParser.java

- Copyright year is 2019.

- It would be nice if some comments that reads something like "This test 
relies on the localized text of "GMT" in the CLDR."


- Test class (and the description) should include "Localized", as it is 
testing the implementation of localized version of OffsetIdPrinterParser.


- Line 64-76: I prefer just instantiating them in the test data, not 
defining a static field for each, unless there will be duplicate in the 
test data.


- Line 111, 118, 124, 132: I believe the locale parameter is required. 
Make sure that with Objects.requireNonNull(), or fail if it's null.


Naoto

On 7/2/19 7:32 AM, Thejasvi Voniadka wrote:

Hi Naoto,

Thank you for the review. I have performed the modifications, and here is the 
updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.2/


I have moved the new tests from TCK area. I have also updated the current TCK 
test to explicitly pass Locale.US while calling format.




-Original Message-
From: Naoto Sato
Sent: Monday, July 01, 2019 9:02 PM
To: Thejasvi Voniadka ; 
core-libs-dev@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8154520: java.time: appendLocalizedOffset() should return 
the localized "GMT" string

Hi Thejasvi,

Thanks for fixing this.

Since those new test cases depend on the CLDR localization, which might change in other 
implementations, those test cases should be in jdk/java/time/test directory, as 
"tck" tests should only test the spec.
Please create a new test case for this in the "test" directory (with @modules 
jdk.localedata directive) similar to the existing TCK one. Also the current test in the TCK should 
enforce that it runs in Locale.US so that the result should match "GMT."

Naoto

On 6/28/19 5:59 AM, Thejasvi Voniadka wrote:

Hi,

Request you to please review this change.


JBS:https://bugs.openjdk.java.net/browse/JDK-8154520


Description:At present, the "DateTimeFormatterBuilder.appendLocalizedOffset()" method formulates the base string as "GMT", without accounting for 
locale-specific transformations. This change is to return the localized version of "GMT" instead. So for example, instead of returning "GMT +5.30", 
it may now return " +5.30" where "" is the localized string for "GMT" for the locale associated with the formatter. I have used 
DateTimeTextProvider.getLocalizedResource() method to return the "gmtZeroFormat" value from CLDR/LDML corresponding to the given locale. The code defaults to 
"GMT" in the absence of such a localized value.


Webrev:http://cr.openjdk.java.net/~vagarwal/8154520/webrev.1/


Additional notes:I preferred to update and reuse an existing test instead 
of creating a new one. It already has the niceties in place, and creating 
another method would mean some amount of code redundancy. However, if that's 
the recommended norm, then I can change it.

Re: RFR: 8224974: Implement JEP 352

[Alan Bateman]

On 18/06/2019 12:43, Andrew Dinn wrote:

Hi Alan,

Thanks for reviewing the JEP one more time.

The proposed updates look good to me.

One nit is that the update to the Goals section says "rework" in two 
places. I think it might more accurate to say "upgrade" or "update" as 
it doesn't significantly re-working the existing implementation.


-Alan

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Joe Wang]

On 7/2/19, 1:23 AM, Daniel Fuchs wrote:

Oh, and BTW is the change to PrettyPrintTest.java intended?
(the removal of the line:
 385 dbf.setValidating(true);
seems to be the only change)


Yes. The only effect of setting validating without an ErrorHandler was 
producing lots of warnings in the test result. It was therefore removed.


Best,
Joe



best regards,

-- daniel

On 02/07/2019 10:21, Daniel Fuchs wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Alan Bateman]

On 02/07/2019 16:35, Kasper Nielsen wrote:

Right now,
I'm using a cache of ClassValue>
with lookupClass and allowedModes as parameters [3]. This will
obviously break with
these changes.

Are you using privateLookupIn and dropping PRIVATE access? Are you 
teleporting in Lookup.in? It might be that you don't have to change 
anything.


-Alan

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Joe Wang]

Thanks Daniel. That test case is added.

Best,
Joe

On 7/2/19, 1:21 AM, Daniel Fuchs wrote:

Hi Joe,

I haven't spotted anything obviously wrong.

The content of "abcxyz" should be "abc & xyz", not 
"abc\n & \nxyz\n" as that before this fix. 


Maybe the test could have a test case for that specific example too?
Just to make sure whitespaces in CDATA aren't eaten away?

best regards,

-- daniel


On 01/07/2019 20:46, Joe Wang wrote:
Please review a fix to xml pretty print. This is a regression 
introduced during the JDK 9 development. CDATA is marked up to be 
interpreted literally as textual data, in other words, it's still 
character data. The processor therefore shall treat it as text data. 
The content of "abcxyz" should be "abc & xyz", not 
"abc\n & \n xyz\n" as that before this fix.


JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/

Thanks,
Joe

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Kasper Nielsen]

Hi Mandy,

Are there any plans to make functionality from VerifyAccess public in any way?
Changes to the access model like these are extremely frustrating because
the access check mechanisms in VerifyAccess are not available outside of
the JDK.

I've posted about this before [1][2], But if you are a library
developer there are basically
no way to cache a MethodHandle and allow it to be used against
multiple different
Lookup objects because the access checks are so complicated.

Another really useful addition would be a LookupValue class (similar
to ClassValue) which
would allow you to cache access check information for a given lookup
object. Right now,
I'm using a cache of ClassValue>
with lookupClass and allowedModes as parameters [3]. This will
obviously break with
these changes.

A simple use case is implement a simple serialization library with the
following signature
  String serializeToString(Class clazz, MethodHandles.Lookup lookup).
Trying to implement something like this that is both performant,
secure and classloader
friendly is really difficult. Creating varhandles/methodhandles for
every call to
'serializeToString ' is not performant for obvious reasons. So you
need to cache it in
some way, which is currently non-trivial.

/Kasper

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2018-October/014012.html
[2] https://bugs.openjdk.java.net/browse/JDK-8213251
[3] https://gist.github.com/kaspernielsen/540aacaf0581e7be80b0ebe3348f4a24







On Mon, 1 Jul 2019 at 20:51, Mandy Chung  wrote:
>
> This is an enhancement to |`Lookup::in`| and
> |`MethodHandles::privateLookupIn`| API
> for cross module teleporting.  A `Lookup` object will record the previous
> lookup class from which this |Lookup| object was teleported such that
> the access check will use both the previous lookup class and the current
> |lookup| context (current lookup class and allowed modes) to determine if
> a type is accessible to this `Lookup` object or not.
>
> In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
> (lookup class in M1) and `PLC` (previous lookup class in M0) if and only if
> 1. both M0 and M1 can read M2
> 2. T is in a package that is exported from M2 at least to both M0 and M1
>
> Detailed specification is in Lookup class spec and `accessClass` javadoc.
> The relevant spec about cross-module teleporting is in the Lookup class
> spec and `Lookup::in` and `MethodHandles::privateLookupIn`.
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8226916
>
> webrev:
> http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00
>
> javadoc:
> http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html
>
> http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup)
>
> I have yet to generate the spec diff. The tool is currently broken
> due to javadoc change.  I'll try to workaround it and post the
> spec diff soon.
>
> thanks
> Mandy

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Daniel Fuchs]

Hi Mandy,

Don't count me as reviewer; this is only editorial:

In MethodHandles.java:



 165  * A caller, specified as a {@code Lookup} object, is in 
module {@code M1} is
 166  * allowed to do deep reflection on module {@code M2} and 
package of the target class
 167  * if and only if all of the following conditions are {@code 
true}:


There are two 'is' in this sentence, which makes it a bit difficult
to parse. Should the first be removed?

* A caller, specified as a {@code Lookup} object, in module {@code M1} 
is ...




 169  * If there is a security manager, its {@code 
checkPermission} method is
 170  * called to check {@code 
ReflectPermission("suppressAccessChecks")} and

 171  * that must return normally.
 172  * 
 173  * If there is a security manager, its {@code checkPermission} 
method is
 174  * called to check {@code 
ReflectPermission("suppressAccessChecks")}.

 175  * This method must return normally.

Probably one of these two bullets should be removed: they look
the same to me.

=

 585  * A {@code Lookup} with {@link #PUBLIC} mode and a lookup 
class in {@code M1}
 586  * can access public types in a module {@code M2} when {@code 
M2} is readable to
 587  * {@code M1} when the type is in a package of {@code M2} that 
is exported to at least {@code M1}.

 
I believe there is "and" missing here:

  <<... when M2 is readable to M1 *and* when the type >>

===

Is the specdiff report incomplete? I was hoping to see the table
added to the Lookup class level API under

 582  * Cross-module lookups

but it seems to be missing from the specdiff?

BTW: is  the right header? I thought  was reserved for the
 class name. John Gibbon will know more ;-)

best regards,

-- daniel

On 02/07/2019 03:47, Mandy Chung wrote:



On 7/1/19 12:51 PM, Mandy Chung wrote:
This is an enhancement to |`Lookup::in`| and 
|`MethodHandles::privateLookupIn`| API

for cross module teleporting.  A `Lookup` object will record the previous
lookup class from which this |Lookup| object was teleported such that
the access check will use both the previous lookup class and the current
|lookup| context (current lookup class and allowed modes) to determine if
a type is accessible to this `Lookup` object or not.

In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
(lookup class in M1) and `PLC` (previous lookup class in M0) if and 
only if

1. both M0 and M1 can read M2
2. T is in a package that is exported from M2 at least to both M0 and M1

Detailed specification is in Lookup class spec and `accessClass` javadoc.
The relevant spec about cross-module teleporting is in the Lookup class
spec and `Lookup::in` and `MethodHandles::privateLookupIn`.

CSR: https://bugs.openjdk.java.net/browse/JDK-8226916

webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00

javadoc:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html 



http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) 



I have yet to generate the spec diff. The tool is currently broken
due to javadoc change.  I'll try to workaround it and post the
spec diff soon.


specdiff:

http://cr.openjdk.java.net/~mchung/jdk14/8173978/specdiff/overview-summary.html 



Mandy

RE: RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string

[Thejasvi Voniadka]

Hi Naoto,

Thank you for the review. I have performed the modifications, and here is the 
updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.2/


I have moved the new tests from TCK area. I have also updated the current TCK 
test to explicitly pass Locale.US while calling format.




-Original Message-
From: Naoto Sato 
Sent: Monday, July 01, 2019 9:02 PM
To: Thejasvi Voniadka ; 
core-libs-dev@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8154520: java.time: appendLocalizedOffset() should 
return the localized "GMT" string

Hi Thejasvi,

Thanks for fixing this.

Since those new test cases depend on the CLDR localization, which might change 
in other implementations, those test cases should be in jdk/java/time/test 
directory, as "tck" tests should only test the spec. 
Please create a new test case for this in the "test" directory (with @modules 
jdk.localedata directive) similar to the existing TCK one. Also the current 
test in the TCK should enforce that it runs in Locale.US so that the result 
should match "GMT."

Naoto

On 6/28/19 5:59 AM, Thejasvi Voniadka wrote:
> Hi,
> 
> Request you to please review this change.
> 
> 
> JBS:https://bugs.openjdk.java.net/browse/JDK-8154520
> 
> 
> Description:At present, the 
> "DateTimeFormatterBuilder.appendLocalizedOffset()" method formulates the base 
> string as "GMT", without accounting for locale-specific transformations. This 
> change is to return the localized version of "GMT" instead. So for example, 
> instead of returning "GMT +5.30", it may now return " +5.30" where "" 
> is the localized string for "GMT" for the locale associated with the 
> formatter. I have used DateTimeTextProvider.getLocalizedResource() method to 
> return the "gmtZeroFormat" value from CLDR/LDML corresponding to the given 
> locale. The code defaults to "GMT" in the absence of such a localized value.
> 
> 
> Webrev:http://cr.openjdk.java.net/~vagarwal/8154520/webrev.1/
> 
> 
> Additional notes:I preferred to update and reuse an existing test instead 
> of creating a new one. It already has the niceties in place, and creating 
> another method would mean some amount of code redundancy. However, if that's 
> the recommended norm, then I can change it.
> 

Re: Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Alan Bateman]

On 01/07/2019 20:51, Mandy Chung wrote:
This is an enhancement to |`Lookup::in`| and 
|`MethodHandles::privateLookupIn`| API

for cross module teleporting.  A `Lookup` object will record the previous
lookup class from which this |Lookup| object was teleported such that
the access check will use both the previous lookup class and the current
|lookup| context (current lookup class and allowed modes) to determine if
a type is accessible to this `Lookup` object or not.

In a nutshell, `T` in M2 is accessible to a `Lookup` object on `C`
(lookup class in M1) and `PLC` (previous lookup class in M0) if and 
only if

1. both M0 and M1 can read M2
2. T is in a package that is exported from M2 at least to both M0 and M1

Detailed specification is in Lookup class spec and `accessClass` javadoc.
The relevant spec about cross-module teleporting is in the Lookup class
spec and `Lookup::in` and `MethodHandles::privateLookupIn`.

CSR: https://bugs.openjdk.java.net/browse/JDK-8226916

webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.00

javadoc:
http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.Lookup.html 



http://cr.openjdk.java.net/~mchung/jdk14/8173978/javadoc/java.base/java/lang/invoke/MethodHandles.html#privateLookupIn(java.lang.Class,java.lang.invoke.MethodHandles.Lookup) 

This is really good work and fixes several issues left over from JDK 9. 
The compatibility issues (mostly small/advanced) are unfortunate but 
necessary and I hope it won't impact too many things. It will need a 
detail release note once the CSR is done and the changes are in.


I've taken several passes over the changes and I don't see anything 
obviously wrong. One minor nit is that the @throws 
IllegalAccessException in accessClass(Class) needs to be updated to 
allow for it to thrown when not accessible from the previous lookup 
class. Also VerifyAccess::isModuleAccessible shouldn't need to if the 
package is exported unconditionally as it is covered by isExported(pn, 
m1) && isExported(pn, m2).


-Alan

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Kasper Nielsen]

Hi Remi,

Yes, setting up a StackWalker is more or less free. It is just
wrapping a set of options.

public class StackWalkerPerf {

static final StackWalker sw =
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);

@Benchmark
public StackWalker stackWalkerSetup() {
return StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
}

@Benchmark
public Class stackWalkerCallerClass() {
return sw.getCallerClass();
}

@Benchmark
public Lookup reflectionCallerClass() {
return MethodHandles.lookup();
}
}

Benchmark   Mode  Cnt ScoreError  Units
StackWalkerPerf.stackWalkerSetupavgt   1011.958 ±  0.353  ns/op
StackWalkerPerf.reflectionCallerClass   avgt   10 8.511 ±  0.415  ns/op
StackWalkerPerf.stackWalkerCallerClass  avgt   10  1269.825 ± 66.471  ns/op

I'm using MethodHandles.lookup() in this test because it is cheapest
way to invoke Reflection.getCallerClass() without any tricks.
So real performance is likely better.

/Kasper

On Tue, 2 Jul 2019 at 13:53, Remi Forax  wrote:
>
> Hi Kasper,
> did you store the StackWalker instance in a static final field ?
>
> Rémi
>
> - Mail original -
> > De: "Kasper Nielsen" 
> > À: "core-libs-dev" 
> > Envoyé: Mardi 2 Juillet 2019 11:09:11
> > Objet: Slow performance of StackWalker.getCallerClass() vs 
> > Reflection.getCallerClass()
>
> > Hi,
> >
> > Are there any security reasons for why StackWalker.getCallerClass()
> > cannot be made as performant as Reflection.getCallerClass()?
> > StackWalker.getCallerClass() is at least 100 times slower then
> > Reflection.getCallerClass() (~1000 ns/op vs ~10 ns/op).
> >
> > I'm trying to retrofit some existing APIs where I cannot take a Lookup
> > object to do some access control checks.
> > But the performance of StackWalker.getCallerClass() is making it impossible.
> >
> > Best
> >  Kasper

Re: Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Remi Forax]

Hi Kasper,
did you store the StackWalker instance in a static final field ?

Rémi

- Mail original -
> De: "Kasper Nielsen" 
> À: "core-libs-dev" 
> Envoyé: Mardi 2 Juillet 2019 11:09:11
> Objet: Slow performance of StackWalker.getCallerClass() vs 
> Reflection.getCallerClass()

> Hi,
> 
> Are there any security reasons for why StackWalker.getCallerClass()
> cannot be made as performant as Reflection.getCallerClass()?
> StackWalker.getCallerClass() is at least 100 times slower then
> Reflection.getCallerClass() (~1000 ns/op vs ~10 ns/op).
> 
> I'm trying to retrofit some existing APIs where I cannot take a Lookup
> object to do some access control checks.
> But the performance of StackWalker.getCallerClass() is making it impossible.
> 
> Best
>  Kasper

Slow performance of StackWalker.getCallerClass() vs Reflection.getCallerClass()

[Kasper Nielsen]

Hi,

Are there any security reasons for why StackWalker.getCallerClass()
cannot be made as performant as Reflection.getCallerClass()?
StackWalker.getCallerClass() is at least 100 times slower then
Reflection.getCallerClass() (~1000 ns/op vs ~10 ns/op).

I'm trying to retrofit some existing APIs where I cannot take a Lookup
object to do some access control checks.
But the performance of StackWalker.getCallerClass() is making it impossible.

Best
 Kasper

Re: [ BUG ? ] simple code throws on Windows but works fine on *nix

[Сергей Цыпанов]

Hi Daniel,

then I'll use URI in my code.

Thanks for explanation, I was sure something is wrong with JDK.

Regards,
Sergey Tsypanov


02.07.2019, 10:36, "Daniel Fuchs" :
> Hi Sergey,
>
> On 02/07/2019 10:25, Сергей Цыпанов wrote:
>>  Hello,
>>
>>  this is exactly how I've worked this around.
>>
>>  But shouldn't behaviour be the same on all platforms
>>  at least when accessing files on a local drive?
>
> Hierarchical URLs/URIs are specified to use '/' as file separator
> This is platform independent.
> On the other hand file systems are platform dependents:
> the file system file separator is '/' on UNIX and '\' on
> windows.
>
> Therefore converting a URL path to a String and then feeding
> it to the file system without any validation/conversion is a bug
> in your code. As Alan mentioned, an URL path is not a file path
> even if it looks like it.
>
> Using Path.of(URI) is the right API, and not a work around.
>
> best regards,
>
> -- daniel
>
>>  Regards,
>>  Sergey Tsypanov

Re: [ BUG ? ] simple code throws on Windows but works fine on *nix

[Daniel Fuchs]

Hi Sergey,

On 02/07/2019 10:25, Сергей Цыпанов wrote:

Hello,

this is exactly how I've worked this around.

But shouldn't behaviour be the same on all platforms
at least when accessing files on a local drive?


Hierarchical URLs/URIs are specified to use '/' as file separator
This is platform independent.
On the other hand file systems are platform dependents:
the file system file separator is '/' on UNIX and '\' on
windows.

Therefore converting a URL path to a String and then feeding
it to the file system without any validation/conversion is a bug
in your code. As Alan mentioned, an URL path is not a file path
even if it looks like it.

Using Path.of(URI) is the right API, and not a work around.

best regards,

-- daniel



Regards,
Sergey Tsypanov

Re: [ BUG ? ] simple code throws on Windows but works fine on *nix

[Сергей Цыпанов]

> For the example, change getPath to
> toURI so you get a URI rather than a String and it should work.

Hello,

this is exactly how I've worked this around.

But shouldn't behaviour be the same on all platforms 
at least when accessing files on a local drive?

Regards,
Sergey Tsypanov

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Daniel Fuchs]

Oh, and BTW is the change to PrettyPrintTest.java intended?
(the removal of the line:
 385 dbf.setValidating(true);
seems to be the only change)

best regards,

-- daniel

On 02/07/2019 10:21, Daniel Fuchs wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/

Re: RFR(JDK 14/java.xml) 8223291: Whitespace is added to CDATA tags when using OutputKeys.INDENT to format XML

[Daniel Fuchs]

Hi Joe,

I haven't spotted anything obviously wrong.

The content of "abcxyz" should be "abc & xyz", not "abc\n & \nxyz\n" as that before this fix. 


Maybe the test could have a test case for that specific example too?
Just to make sure whitespaces in CDATA aren't eaten away?

best regards,

-- daniel


On 01/07/2019 20:46, Joe Wang wrote:
Please review a fix to xml pretty print. This is a regression introduced 
during the JDK 9 development. CDATA is marked up to be interpreted 
literally as textual data, in other words, it's still character data. 
The processor therefore shall treat it as text data. The content of 
"abcxyz" should be "abc & xyz", not "    abc\n & \n 
xyz\n" as that before this fix.


JBS: https://bugs.openjdk.java.net/browse/JDK-8223291
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8223291/webrev/

Thanks,
Joe

Re: [ BUG ? ] simple code throws on Windows but works fine on *nix

[Alan Bateman]

On 02/07/2019 08:43, Сергей Цыпанов wrote:

Hello,

one of key Java principles is "write once - run everywhere".
It seems to me that this code breaks this rule


import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;

public class Main {
   public static void main(String[] args) throws IOException {
 String path = Main.class
 .getClassLoader()
 .getResource("tsypanov/example/war-and-peace.json")
 .getPath();
Resource name -> URL -> URL path component.  A URL path component is not 
a file path. For file URLs then it encodes a file path (at least file 
paths that are not located on the network). There's a warning in the URL 
javadoc on this but maybe it's time to deprecate URL::getPath (in time 
we need to deprecate all of URL constructors and several methods but 
that is a topic for another day). For the example, change getPath to 
toURI so you get a URI rather than a String and it should work.


-Alan

[ BUG ? ] simple code throws on Windows but works fine on *nix

[Сергей Цыпанов]

Hello,

one of key Java principles is "write once - run everywhere".
It seems to me that this code breaks this rule


import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;

public class Main {
  public static void main(String[] args) throws IOException {
String path = Main.class
.getClassLoader()
.getResource("tsypanov/example/war-and-peace.json")
.getPath();
List lines = Files.readAllLines(Path.of(path));
assert lines.size() == 1;
  }
}



This code work under *nix (tested on MacOS and Linux Mint) 
and fails on Windows 10 with exception:

Exception in thread "main" java.nio.file.InvalidPathException:
Illegal char <:> at index 2: 
/C:/Users/sergii_tsypanov/IdeaProjects/jdk-bug/out/production/jdk-bug/tsypanov/example/war-and-peace.json
at 
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at 
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at 
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at 
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
at java.base/java.nio.file.Path.of(Path.java:147)
at tsypanov.example.Main.main(Main.java:15)


As far as I use a relative path in my Java code 
I expect this to have same behaviour on all supported platforms.

This was reproduced on JDK 12.

I've prepared a test project on GitHub to reproduce this in IDEA:

 https://github.com/stsypanov/jdk-bug

Regards,
Sergey Tsypanov