Re: #ReflectiveAccessByInstrumentationAgents
On 20/04/16 12:40, Alan Bateman wrote: > > On 19/04/2016 18:00, Andrew Dinn wrote: >> : >> I have been building and testing on JDK9 Byteman using the EA program >> releases and have not seen anything break so far. However, that may just >> be because i) Byteman and the Byteman tests are only using runtime >> classes from the java base module and ii) app code used in tests is not >> modularized -- well, at least not using Jigsaw (see below). > It's good to hear that you are testing with the EA builds. > > I think the main thing for Byteman, and this will be true at least some > other agents too, is that there will likely need to be updated to > support instrumentation with modules. We have a reasonable compatibility > story for existing JVM TI and java agents but as soon as you get into > instrumenting code to statically or reflectively access code in other > modules then it may require the agent to arrange for this access to be > allowed. Having to explicitly manage module dependencies to retain compatibiity with previous behaviour is at the very least a /nuisance/ because it now means I -- and, no doubt, others in the agent business -- will have to implement a dual-source agent, one for JDK9(+) and another to continue to operate in JDK8/7/6 (yes, people can and do still use the latest Byteman release in all those JDK releases). A solution internal to the JVM which preserved existing behaviour and so did not require the insertion of JDK-specific jumps through JDK-specific hoops would be much preferred. Still, I suppose the releavant implementors are a relatively small crowd and ought to know what we are doing and how to fix it. That minor rant aside (sorry, I feel better for that but I don't suppose anyone else on this list does :-) I will happily investigate the available dynamic module-management APIs and see what options exist to remedy the changes to the status quo and retain as much backwards compatibility as I can. At that point I will come back and comment on the very useful advice you have provided below. Thanks very much for your help so far. regards, Andrew Dinn --- Senior Principal Software Engineer Red Hat UK Ltd Registered in UK and Wales under Company Registration No. 3798903 Directors: Michael Cunningham (US), Michael O'Neill (Ireland), Paul Argiry (US)
Re: [9] Review request: 8153872: Nashorn no longer needs access to com.sun.javafx.application
+1 On 4/21/2016 10:19 AM, Mandy Chung wrote: >> On Apr 19, 2016, at 8:25 AM, Kevin Rushforth >> wrote: >> >> Jim, >> >> Please review the following fix: >> >> https://bugs.openjdk.java.net/browse/JDK-8153872 >> http://cr.openjdk.java.net/~kcr/8153872/webrev.00/ >> >> This is a simple backout of the earlier fix for JDK-8153754. > +1 > > Glad that this dependency is eliminated. > > Mandy
Re: #ReflectiveAccessByInstrumentationAgents
On 21/04/2016 08:31, Andrew Dinn wrote: : Having to explicitly manage module dependencies to retain compatibiity with previous behaviour is at the very least a /nuisance/ because it now means I -- and, no doubt, others in the agent business -- will have to implement a dual-source agent, one for JDK9(+) and another to continue to operate in JDK8/7/6 (yes, people can and do still use the latest Byteman release in all those JDK releases). A solution internal to the JVM which preserved existing behaviour and so did not require the insertion of JDK-specific jumps through JDK-specific hoops would be much preferred. Still, I suppose the releavant implementors are a relatively small crowd and ought to know what we are doing and how to fix it. In the JVM TI and java.lang.instrument docs then you'll see that the VM is required to arrange for the module of instrumented code to read the unnamed module of both the application and boot class loaders. This is how we get many existing agents doing instrumentation, profilers and the like, to instrument code in modules without any knowledge of modules. We know of course that this isn't sufficient for all use-cases. In the discussion here then Byteman finds itself in a new world where it injects code into module A to access a type in module B that is not accessible to A. The only way that I can see this working is where Byteman adds supports for modules and in the example, it injects and executes code in module B to export the package to module A. -Alan
Re: #ReflectiveAccessByInstrumentationAgents
Hi Andrew,
On 04/21/2016 09:31 AM, Andrew Dinn wrote:
On 20/04/16 12:40, Alan Bateman wrote:
On 19/04/2016 18:00, Andrew Dinn wrote:
:
I have been building and testing on JDK9 Byteman using the EA program
releases and have not seen anything break so far. However, that may just
be because i) Byteman and the Byteman tests are only using runtime
classes from the java base module and ii) app code used in tests is not
modularized -- well, at least not using Jigsaw (see below).
It's good to hear that you are testing with the EA builds.
I think the main thing for Byteman, and this will be true at least some
other agents too, is that there will likely need to be updated to
support instrumentation with modules. We have a reasonable compatibility
story for existing JVM TI and java agents but as soon as you get into
instrumenting code to statically or reflectively access code in other
modules then it may require the agent to arrange for this access to be
allowed.
Having to explicitly manage module dependencies to retain compatibiity
with previous behaviour is at the very least a /nuisance/ because it now
means I -- and, no doubt, others in the agent business -- will have to
implement a dual-source agent, one for JDK9(+) and another to continue
to operate in JDK8/7/6 (yes, people can and do still use the latest
Byteman release in all those JDK releases). A solution internal to the
JVM which preserved existing behaviour and so did not require the
insertion of JDK-specific jumps through JDK-specific hoops would be much
preferred. Still, I suppose the releavant implementors are a relatively
small crowd and ought to know what we are doing and how to fix it.
That minor rant aside (sorry, I feel better for that but I don't suppose
anyone else on this list does :-) I will happily investigate the
available dynamic module-management APIs and see what options exist to
remedy the changes to the status quo and retain as much backwards
compatibility as I can. At that point I will come back and comment on
the very useful advice you have provided below. Thanks very much for
your help so far.
regards,
Andrew Dinn
The situation is not so complicated, I think. If instrumented code calls
some code in your Byteman module(s) then you should 1st make this code
public and reside in exported packages. All you have to do then is add
read edges from modules of instrumented classes to modules of Byteman
classes that are called by instrumented code as they are being
instrumented. For example, having the following utility class compiled
with JDK 8 (not using any JDK 9 API):
public class ClassFileTransformerWrapper {
public ClassFileTransformer wrap(Instrumentation instrumentation,
ClassFileTransformer transformerImpl,
Class... readableClasses) {
try {
Class.forName("java.lang.reflect.Module");
} catch (ClassNotFoundException e) {
// don't need to wrap if on JDK 8 or less
return transformerImpl;
}
// wrap it
return new ReadEdgeAddingClassFileTransformer(instrumentation,
transformerImpl,
readableClasses);
}
}
...which you call with your ClassFileTransformer implementation and the
Class(es) where your Byteman code resides that contains methods called
from instrumented code. The following delegating ClassFileTransformer,
compiled with -target 9, will add read edges then:
public class ReadEdgeAddingClassFileTransformer implements
ClassFileTransformer {
private final Instrumentation instrumentation;
private final ClassFileTransformer transformerImpl;
private final Set readableModules;
ReadEdgeAddingClassFileTransformer(Instrumentation instrumentation,
ClassFileTransformer
transformerImpl,
Class[] readableClasses) {
this.instrumentation = instrumentation;
this.transformerImpl = transformerImpl;
this.readableModules = new HashSet<>();
for (Class c : readableClasses) {
this.readableModules.add(c.getModule());
}
}
@Override
public byte[] transform(ClassLoader loader,
String className,
Class classBeingRedefined,
ProtectionDomain protectionDomain,
byte[] classfileBuffer) throws
IllegalClassFormatException {
return transformerImpl.transform(loader,
className,
classBeingRedefined,
protectionDomain,
classfileBuffer);
}
@Override
public byte[] transform(Module module,
String className,
Class classBeingRedefined,
ProtectionDomain protectionD
Re: #ReflectiveAccessByInstrumentationAgents
Hi Peter, On 21/04/16 12:09, Peter Levart wrote: > The situation is not so complicated, I think. . . . Thank you very much for your example code. I understand that it will be possible to localize the changes into a small part of the agent codebase. My problem was not with this factoring of the code but rather with the build and deployment process. I was assuming that I would have to locate any jdk9-specific code in a separate extension jar and ensure that clients consistently hoist both the jdk9-specific extension jar and the vanilla agent jar into either the system classpath or the bootstrap classpath when running on JDK9(+). The alternative was to use the new multi-release jar model to deploy the agent jar but, of course, I don't suppose that is ever going to work for JDK6 deployments. As you say, I can avoid this dilemma simply by bundling all the code into the one agent jar and instantiating the relevant management classes indirectly according to whether modules are present or not. That will ensure that the JDK9(+)-compiled classes are not loaded when running on JDK8(-). Thanks once again for your, as ever, very helpful insights. regards, Andrew Dinn ---
Re: #ReflectiveAccessByInstrumentationAgents
On 21/04/2016 12:09, Peter Levart wrote: : The situation is not so complicated, I think. If instrumented code calls some code in your Byteman module(s) then you should 1st make this code public and reside in exported packages. All you have to do then is add read edges from modules of instrumented classes to modules of Byteman classes that are called by instrumented code as they are being instrumented. If the instrumentation is just injecting code to call into Byteman then I assume it just works now. That is, it doesn't appear that Byteman has embraced modules yet and so I assume it must be an agent specified via -javaagent with perhaps supporting code in other JAR files. In that case, the Byteman code is the unnamed module of the app class loader and the VM will automatically add a read edge when the transformer touches code in any named module. This is the compatibility support that I mentioned in another mail. If you right this it should be simple as you say. On the other hand, some of the mails mention accessing non-public members, setAccessible, and even (it would appear) adjusting class loader delegation on the fly to facilitate the patches. -Alan
java.lang.reflect.Module.WeakSet is not thread-safe
Hi,
While browsing code in java.lang.reflect.Module (I sometimes do that
just to see how thinks work ;-) I stumbled on the following nested class:
private static class WeakSet {
private final ReadWriteLock lock = new ReentrantReadWriteLock();
private final Lock readLock = lock.readLock();
private final Lock writeLock = lock.writeLock();
private final WeakHashMap map = new WeakHashMap<>();
/**
* Adds the specified element to the set.
*/
void add(E e) {
writeLock.lock();
try {
map.put(e, Boolean.TRUE);
} finally {
writeLock.unlock();
}
}
/**
* Returns {@code true} if this set contains the specified element.
*/
boolean contains(E e) {
readLock.lock();
try {
return map.containsKey(e);
} finally {
readLock.unlock();
}
}
}
...while this seems OK from 1st look, it is not. WeakHashMap is not
thread-safe even for seemingly read-only operations. All its operations
can mutate internal state in a non-thread-safe way. The simplest way to
fix this is to use a writeLock for containsKey operation too. But such
structure does not scale well to multiple threads for frequent lookups.
WeakSet is used in Module to keep track of transient read edges, exports
and uses of services added dynamically to the module. Modification
operations are not so performance critical, but lookup operations are.
I propose to add a thread-safe WeakPairMap data structure which
associates a pair of weakly-reachable keys with a strongly-reachable
value based on ConcurrentHashMap. Such data structure is
footprint-friendly, since only a single instance exists for a particular
purpose, totaling 3 instances for the transient structures serving all
Modules in the system:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.01/
So, what do you think?
Regards, Peter
Re: java.lang.reflect.Module.WeakSet is not thread-safe
On 21/04/2016 17:07, Peter Levart wrote: : ...while this seems OK from 1st look, it is not. WeakHashMap is not thread-safe even for seemingly read-only operations. All its operations can mutate internal state in a non-thread-safe way. The simplest way to fix this is to use a writeLock for containsKey operation too. But such structure does not scale well to multiple threads for frequent lookups. Sigh, it's a left over from early prototyping and was meant to be replaced (but wasn't). Now seems the right time. -Alan
Re: java.lang.reflect.Module.WeakSet is not thread-safe
On 04/21/2016 06:07 PM, Peter Levart wrote: I propose to add a thread-safe WeakPairMap data structure which associates a pair of weakly-reachable keys with a strongly-reachable value based on ConcurrentHashMap. Such data structure is footprint-friendly, since only a single instance exists for a particular purpose, totaling 3 instances for the transient structures serving all Modules in the system: http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.01/ Oops... It looks like I replaced one thread-unsafe construction with another one: 389 // additional exports added at run-time 390 // source module (1st key), target module (2nd key), exported packages (value) 391 private static final WeakPairMap> transientExports = 392 new WeakPairMap<>(); ...that would've been OK if I hadn't used normal HashSet for holding the set of packages: 623 // add package name to transientExports if absent 624 transientExports 625 .computeIfAbsent(this, other, (_this, _other) -> new HashSet<>()) 626 .add(pn); Luckily this can be easily fixed by using a ConcurrentHashMap instead of HashSet which is even more space-friendly (HashSet is just a wrapper around HashMap and HashMap is basically the same structure as ConcurrentHashMap): http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.02/ Regards, Peter
Re: java.lang.reflect.Module.WeakSet is not thread-safe
I remember seeing this codd an thinking that synchronized should do the job. I don't believe this use case requires something more complex. Remi Le 21 avril 2016 18:21:55 CEST, Alan Bateman a écrit : > > >On 21/04/2016 17:07, Peter Levart wrote: >> : >> >> ...while this seems OK from 1st look, it is not. WeakHashMap is not >> thread-safe even for seemingly read-only operations. All its >> operations can mutate internal state in a non-thread-safe way. The >> simplest way to fix this is to use a writeLock for containsKey >> operation too. But such structure does not scale well to multiple >> threads for frequent lookups. >Sigh, it's a left over from early prototyping and was meant to be >replaced (but wasn't). Now seems the right time. > >-Alan
Re: java.lang.reflect.Module.WeakSet is not thread-safe
On 21/04/2016 20:52, Rémi Forax wrote: I remember seeing this codd an thinking that synchronized should do the job. I don't believe this use case requires something more complex. Remi I've taken a first pass over it and WeakPairMap seems straight-forward to use but its implementation, with Pair/Weak/Primary/Secondary/Lookup is complex. Prior to #ReflectionWithoutReadability then transientReads was important, less so now although I think we should continue to allow concurrent lookups. -Alan
