Re: RFR (s): 4354680: Runtime.runFinalization() silently clears interrupted flag in the calling thread
Aside, I noticed in this code:
146 forkSecondaryFinalizer(new Runnable() {
147 private volatile boolean running;
148 public void run() {
149 if (running)
150 return;
151 final JavaLangAccess jla = SharedSecrets.
getJavaLangAccess();
152 running = true;
that as we create a new Runnable on every call, the running field serves
absolutely no purpose.
Cheers,
David
It could be defense vs Thread.currentThread().run() in finalizer that would
cause calling run() of Thread.target. I am not sure if that would cause
invocation, though.
But I have done similar non-senses...
Stanimir
Re: RFR (s): 4354680: Runtime.runFinalization() silently clears interrupted flag in the calling thread
On 30/10/2014 4:00 PM, Stanimir Simeonoff wrote:
Aside, I noticed in this code:
146 forkSecondaryFinalizer(new Runnable() {
147 private volatile boolean running;
148 public void run() {
149 if (running)
150 return;
151 final JavaLangAccess jla =
SharedSecrets.__getJavaLangAccess();
152 running = true;
that as we create a new Runnable on every call, the running field
serves absolutely no purpose.
Cheers,
David
It could be defense vs Thread.currentThread().run() in finalizer that
would cause calling run() of Thread.target. I am not sure if that would
cause invocation, though.
That is an excellent point! But it deserves a comment in the code.
Thanks,
David
But I have done similar non-senses...
Stanimir
Re: Losing features of JVM_Open, e.g. CLOEXEC
On 29/10/2014 21:15, Mario Torre wrote: +1 We should have spotted it in the review though. We should but the ultimate rat catcher should be the tests, it's possible that we have a hole there.
Re: RFR (s): 4354680: Runtime.runFinalization() silently clears interrupted flag in the calling thread
On 30 Oct 2014, at 07:31, David Holmes david.hol...@oracle.com wrote:
On 30/10/2014 4:00 PM, Stanimir Simeonoff wrote:
Aside, I noticed in this code:
146 forkSecondaryFinalizer(new Runnable() {
147 private volatile boolean running;
148 public void run() {
149 if (running)
150 return;
151 final JavaLangAccess jla =
SharedSecrets.__getJavaLangAccess();
152 running = true;
that as we create a new Runnable on every call, the running field
serves absolutely no purpose.
Cheers,
David
It could be defense vs Thread.currentThread().run() in finalizer that
would cause calling run() of Thread.target. I am not sure if that would
cause invocation, though.
That is an excellent point! But it deserves a comment in the code.
Adding a comment is fine, but please to not remove this field. ‘hg log’ may
reveal more about it’s origins.
-Chris.
Thanks,
David
But I have done similar non-senses...
Stanimir
Re: Loading classes with many methods is very expensive
Hi Stanimir,
On 10/30/2014 12:00 AM, Stanimir Simeonoff wrote:
Hi Peter,
The removal of value wrapper is a clever approach to reduce the new
instances created although it feels very unnatural (at least to me).
Number of objects created is practically the same. Previous approach
used same MethodList instances for keys and values. New approach uses
just Key instances, values are Methods themselves. It's allways 1
additional object per Method, but new approach might use a couple of
more Map.Entry objects (when multiple methods with same signature are
present). New approach is different in that it was designed to keep the
insertion order of Method(s) (LinkedHashMap), but you found out this is
not true (below)...
Small
optimization; eagerly calculate the hash in the c'tor,
hash =
149 method.getReturnType().hashCode() ^
150 System.identityHashCode(method.getName()) ^
151 Arrays.hashCode(method.getParameterTypes()
and so the real int hashCode(){return seq+hash;}
Right, that would be an time/space trade-off. It might not be on 64bit
arch - just on 32bit. I should check. Perhaps hashCode should not depend
on seq (see below).
Also I am not sure if method.getName().hashCode() would be better or not,
if previously identityHashCode is not invoked on that string and depending
on the configured hashCode (-XX:hashCode) generator System.identityHashCode
might be slow or worse call C random() which doesn't scale. Setting the
hashCode requires a CAS too. CAS is of course one-time off but still
Thanks for pointing that out. I'll use String.hashCode() then.
About the order of the returned methods: if you remove and then put from/to
the LHM the order of iteration is going to be greatly altered. Is that ok?
Excelent point!
I might get away with excluding seq from hashCode computation and only
use it in equals(). This way Key(s) could be modified in-place (hashCode
would not change, entry would stay in same bucket), it would just become
equal to some other Key. Modification operations would keep the
invariant that there are no duplicate Key(s) at any time in the Map. All
entries sharing same method signature would end-up in the same bucket
(and maybe share it with entries that just happen to have the same
hashCode mod capacity) and we would get a similar linked list of entries
as with previous approach - only without another level of indirection...
Regards, Peter
Stanimir
On Wed, Oct 29, 2014 at 7:16 PM, Peter Levart peter.lev...@gmail.com
wrote:
On 10/29/2014 02:08 PM, Joel Borggrén-Franck wrote:
Hi Peter,
I'm not entirely convinced this is a bug.
The lookup order for getMethod has for a long time been walk up
superclasses and return what you find there first without even looking at
interfaces. It might be desirable to change that but I'm not sure.
Hi Joel,
It has been for a long time like that as you say, but for a long time Java
did not have default methods. It's unexpected for getMethod() to return a
method that is not contained in getMethods() result.
Anyway, I have created a bug to track this:
https://bugs.openjdk.java.net/browse/JDK-8062389
For next iteration of the getMethods() O(n^2) fix, I used a slightly
different approach, which you might like more or not. Instead of using
linked-lists of Method objects as values of a LinkedHashMap I created a
special Key object, holding a reference to Method object and an additional
'seq' int field, which discriminates among methods with same signature.
Values of LinkedHashMap are Method objects themselves:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.03/
I have encapsulated this functionality into a package-private
java.lang.MethodTable. The implementation of this API can be easily changed
to using linked-lists as values of LinkedHashMap if desired. The
performance characteristics are similar, with hardly measurable advantage
of this latest approach as can be seen from http://cr.openjdk.java.net/~
plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java benchmark:
Original code:
19658 classes loaded in 2.071013902 seconds.
494392 methods obtained in 1.089983418 seconds.
494392 methods obtained in 0.952488497 seconds.
494392 methods obtained in 0.912878317 seconds.
494392 methods obtained in 0.940293784 seconds.
494392 methods obtained in 0.987640733 seconds.
494392 methods obtained in 0.925393355 seconds.
494392 methods obtained in 0.89397002 seconds.
494392 methods obtained in 0.915042463 seconds.
494392 methods obtained in 0.897669082 seconds.
494392 methods obtained in 0.878140502 seconds.
Patched code:
19658 classes loaded in 2.153024197 seconds.
494392 methods obtained in 0.875651469 seconds.
494392 methods obtained in 0.791937742 seconds.
494392 methods obtained in 0.780995693 seconds.
494392 methods obtained in 0.759593461 seconds.
494392 methods obtained in 0.766528355 seconds.
494392 methods obtained in 0.756567663 seconds.
494392
Re: Loading classes with many methods is very expensive
On 10/30/2014 10:29 AM, Peter Levart wrote: I might get away with excluding seq from hashCode computation and only use it in equals(). This way Key(s) could be modified in-place (hashCode would not change, entry would stay in same bucket), it would just become equal to some other Key. Modification operations would keep the invariant that there are no duplicate Key(s) at any time in the Map. All entries sharing same method signature would end-up in the same bucket (and maybe share it with entries that just happen to have the same hashCode mod capacity) and we would get a similar linked list of entries as with previous approach - only without another level of indirection... That would not work. I would have to have the Key object accessible. Since Map does not have a getKey(key) operation, I would have to store the Key as a value too. And we're back to MethodList approach... Peter
Re: Losing features of JVM_Open, e.g. CLOEXEC
On 30/10/2014 6:46 PM, Alan Bateman wrote: On 29/10/2014 21:15, Mario Torre wrote: +1 We should have spotted it in the review though. We should but the ultimate rat catcher should be the tests, it's possible that we have a hole there. Not sure how the presence or absence of CLOEXEC would be visible to any test - especially given all the other uses of raw open in the JDK sources. David
Re: Loading classes with many methods is very expensive
Guys, can you please start a new RFR thread for Peter's change? You have a bug ID now, and the discussion should be searchable by bug ID. Also, you are spamming two aliases (corelibs + hotspot-runtime) instead of one ;) -Aleksey. On 10/30/2014 12:43 PM, Peter Levart wrote: On 10/30/2014 10:29 AM, Peter Levart wrote: I might get away with excluding seq from hashCode computation and only use it in equals(). This way Key(s) could be modified in-place (hashCode would not change, entry would stay in same bucket), it would just become equal to some other Key. Modification operations would keep the invariant that there are no duplicate Key(s) at any time in the Map. All entries sharing same method signature would end-up in the same bucket (and maybe share it with entries that just happen to have the same hashCode mod capacity) and we would get a similar linked list of entries as with previous approach - only without another level of indirection... That would not work. I would have to have the Key object accessible. Since Map does not have a getKey(key) operation, I would have to store the Key as a value too. And we're back to MethodList approach... Peter
Re: Loading classes with many methods is very expensive
Sorry, I apologize. I'll start a new thread on core-libs only. Peter On 10/30/2014 10:47 AM, Aleksey Shipilev wrote: Guys, can you please start a new RFR thread for Peter's change? You have a bug ID now, and the discussion should be searchable by bug ID. Also, you are spamming two aliases (corelibs + hotspot-runtime) instead of one ;) -Aleksey. On 10/30/2014 12:43 PM, Peter Levart wrote: On 10/30/2014 10:29 AM, Peter Levart wrote: I might get away with excluding seq from hashCode computation and only use it in equals(). This way Key(s) could be modified in-place (hashCode would not change, entry would stay in same bucket), it would just become equal to some other Key. Modification operations would keep the invariant that there are no duplicate Key(s) at any time in the Map. All entries sharing same method signature would end-up in the same bucket (and maybe share it with entries that just happen to have the same hashCode mod capacity) and we would get a similar linked list of entries as with previous approach - only without another level of indirection... That would not work. I would have to have the Key object accessible. Since Map does not have a getKey(key) operation, I would have to store the Key as a value too. And we're back to MethodList approach... Peter
Re: Losing features of JVM_Open, e.g. CLOEXEC
This is exactly what I was about to reply. One can only spot those things if there is a standardised API, and even in this case it's not at all easy. That doesn't stop from trying :) but removing the API was likely a mistake, which would not be a terrible one because of what David points, except the fact that it's invalidating a fix (at least this is what I understood from Martin's original mail). On a related note, some time ago there was a process of standardising and documenting the VM abstraction (Andrew Hughes was doing that), does anybody knows what's the state of that? Afaik there's a lot of infrastructure in place, but it seems more like we have it more or less when it makes sense rather than a carefully planned long term project. But I think it may be worth expanding over this rather than reducing it. This would benefit external VM implementation, which is a good thing, but the main goal for me would be to have a clearly documented and logical API with the overall benefit that if we abstract those things out, we have one single place where bugs can possible be [fixed]. What do you think? Cheers, Mario 2014-10-30 10:45 GMT+01:00 David Holmes david.hol...@oracle.com: On 30/10/2014 6:46 PM, Alan Bateman wrote: On 29/10/2014 21:15, Mario Torre wrote: +1 We should have spotted it in the review though. We should but the ultimate rat catcher should be the tests, it's possible that we have a hole there. Not sure how the presence or absence of CLOEXEC would be visible to any test - especially given all the other uses of raw open in the JDK sources. David -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens Proud GNU Classpath developer: http://www.classpath.org/ OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/
Re: Losing features of JVM_Open, e.g. CLOEXEC
I've been thinking perhaps one can use fcntl to check what flags are passed given we can retrieve all the file descriptors that have been opened? Using raw open complicates the matter though. Cheers, Mario 2014-10-30 11:16 GMT+01:00 Mario Torre neugens.limasoftw...@gmail.com: This is exactly what I was about to reply. One can only spot those things if there is a standardised API, and even in this case it's not at all easy. That doesn't stop from trying :) but removing the API was likely a mistake, which would not be a terrible one because of what David points, except the fact that it's invalidating a fix (at least this is what I understood from Martin's original mail). On a related note, some time ago there was a process of standardising and documenting the VM abstraction (Andrew Hughes was doing that), does anybody knows what's the state of that? Afaik there's a lot of infrastructure in place, but it seems more like we have it more or less when it makes sense rather than a carefully planned long term project. But I think it may be worth expanding over this rather than reducing it. This would benefit external VM implementation, which is a good thing, but the main goal for me would be to have a clearly documented and logical API with the overall benefit that if we abstract those things out, we have one single place where bugs can possible be [fixed]. What do you think? Cheers, Mario 2014-10-30 10:45 GMT+01:00 David Holmes david.hol...@oracle.com: On 30/10/2014 6:46 PM, Alan Bateman wrote: On 29/10/2014 21:15, Mario Torre wrote: +1 We should have spotted it in the review though. We should but the ultimate rat catcher should be the tests, it's possible that we have a hole there. Not sure how the presence or absence of CLOEXEC would be visible to any test - especially given all the other uses of raw open in the JDK sources. David -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens Proud GNU Classpath developer: http://www.classpath.org/ OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/ -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens Proud GNU Classpath developer: http://www.classpath.org/ OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/
Re: Losing features of JVM_Open, e.g. CLOEXEC
On 29/10/2014 20:12, Martin Buchholz wrote: Hi guys, In your change 805: Cleanup of old and unused VM interfaces you have made changes like this: -zfd = JVM_Open(path, flag, 0); +zfd = open(path, flag, 0); throwing away use of old shared infrastructure and replacing with naked calls to the OS. Although understandable, this abandons benefits of using shared infrastructure. Here I'm thinking of the close-on-exec flag. I just added use of O_CLOEXEC to linux hotspot, but e.g. zip file opening no longer uses JVM_Open, which is a code hygiene regression. What we want is to have almost all file descriptors have the close-on-exec flag automatically set at fd creation time, using O_CLOEXEC where available, and FD_CLOEXEC where not. How do we get there? I think Frederic's clean-up was generally good, it gets rid of a lot of crud from early JDK versions. In early versions of the JDK then it was important for the library code to go through the VM because the JDK could be run on green threads so every potentially-blocking syscall had to wrapped in the VM. There was also an attempt at a porting interface at the time but this bit rotted a long time ago (much of it was orphaned when we moved from the the classic VM to HotSpot). The remaining bits of that porting interface were finally removed in JDK 7 but that still left the JVM interface with a lot of functions, networking and some file I/O, that aren't interesting for the JVM interface. So I think it's good to have these removed from the JVM interface. Also good to see the no-op functions to support java.lang.Compiler and Runtime.traceXXX go away too. The change in behavior from JVM_Open ito open was missed but there is a long way to go in JDK 9 so lots of time to fix the issue (if there is an issue). As was noted in one of the replies, the only usages of JVM_Open were in the zip code, it hasn't been used consistently used in the libraries for at least 10+ years. When doing significant changes then thorough code reviews are important but good tests are equally, and sometimes more, important. I understand that having a test for the O_CLOEXEC might be too hard so I'm interested to know how you ran into it. Maybe it was inspection? Maybe an app that opens lots of zip files and executes fork/exec itself? For ProcessBuilder and Runtime.exec usages then the I thought the childproc code handled this by looping over the open file descriptors and closing them. So even if O_CLOEXEC or fcntl(FD_CLOEXEC) isn't used then it should be closed in the child. As to whether we need a JVM_Open replacement in libjava then maybe we do but I think it needs a discussion as to whether the JDK really needs a porting interface or just useful infrastructure. Also I think going forward then I assume that the amount of native code will reduce, particularly when FFI comes along and we start to get rid of some of the native code (at least I hope that happens). In the mean-time then I think I'd like to understand more as to whether the O_CLOEXEC is an issue or not. Aside from the zip code then I don't think we've been using it consistently so I'm wondering if the issue is some native code calling fork+exec or something else? -Alan
Re: RFR 9 8062513: doclint warnings in HijrahChronology
On 30/10/2014 02:28, roger riggs wrote: Please review a correction to address a doclint warning in HijrahChronology. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-doclint-8062513/ Issue: 8062513: doclint warnings in HijrahChronology This looks to me, you might to just split the line as it otherwise sticks out compared to the rest of the paragraph. -Alan.
Re: RFR 9 8062513: doclint warnings in HijrahChronology
Thanks Joe, Alan, On 10/30/2014 9:10 AM, Alan Bateman wrote: On 30/10/2014 02:28, roger riggs wrote: Please review a correction to address a doclint warning in HijrahChronology. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-doclint-8062513/ Issue: 8062513: doclint warnings in HijrahChronology This looks to me, you might to just split the line as it otherwise sticks out compared to the rest of the paragraph. done -Alan.
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Thanks Ioi - looks good.
thanks,
Karen
On Oct 30, 2014, at 1:26 AM, Ioi Lam wrote:
OK, here's the latest version. I removed the synchronization but kept the
resolveClass just for completeness, even if it currently does nothing.
class Launcher$AppClassLoader {
public Class? loadClass(String name, boolean resolve)
throws ClassNotFoundException
{
int i = name.lastIndexOf('.');
if (i != -1) {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPackageAccess(name.substring(0, i));
}
}
if (ucp.knownToNotExist(name)) {
// The class of the given name is not found in the parent
// class loader as well as its local URLClassPath.
// Check if this class has already been defined dynamically;
// if so, return the loaded class; otherwise, skip the parent
// delegation and findClass.
Class? c = findLoadedClass(name);
if (c != null) {
if (resolve) {
resolveClass(c);
}
return c;
}
throw new ClassNotFoundException(name);
}
return (super.loadClass(name, resolve));
}
Thanks
- Ioi
On 10/29/14, 12:10 PM, Mandy Chung wrote:
On 10/29/2014 7:16 AM, Karen Kinnear wrote:
Sorry, I was confused about who wrote what.
Sounds like David and I are in agreement that you can remove the
synchronization - I believe that would be much cleaner.
I agree that the class loader lock is not really needed in
the knownToNotExist case as it's checking if the class is
loaded or not. Good catch, Karen.
Mandy
And resolveClass does nothing and is final so no worries there.
thanks,
Karen
On Oct 29, 2014, at 2:37 AM, David Holmes wrote:
On 29/10/2014 4:04 PM, Ioi Lam wrote:
On 10/28/14, 7:34 PM, David Holmes wrote:
Hi Karen,
I haven't been tracking the details of this and am unclear on the
overall caching strategy however ...
On 29/10/2014 8:49 AM, Karen Kinnear wrote:
Ioi,
Looks good! Thanks to all who have contributed!
A couple of minor comments/questions:
1. jvm.h (hotspot and jdk)
All three APIs talk about loader_type, but the code uses loader.
2. Launcher.java
To the best of my understanding - the call to findLoadedClass does
not require synchronizing on the class loader lock,
that is needed to ensure find/define atomicity - so that we do not
call defineClass twice on the same class - i.e. in
loadClass - it is needed around the findLoadedClass /
findClass(defineClass) calls. This call is just a SystemDictionary
lookup
and does not require the lock to be held.
If the class can be defined dynamically - which it appears it can
(though I'm not sure what that means) - then you can have a race
between the thread doing the defining and the thread doing the
findLoadedClass. By doing findLoadedClass with the lock held you
enforce some serialization of the actions, but there is still a race.
So the only way the lock could matter is if user code could trigger
the second thread's lookup of the class after the lock has been taken
by the thread doing the dynamic definition - whether that is possible
depends on what this dynamic definition actually is.
I copied the code from ClassLoader.loadClass, which does it it a
synchronized block:
class ClassLoader {
protected Class? loadClass(String name, boolean resolve)
throws ClassNotFoundException
{
synchronized (getClassLoadingLock(name)) {
// First, check if the class has already been loaded
Class? c = findLoadedClass(name);
if (c == null) {
long t0 = System.nanoTime();
try {
if (parent != null) {
c = parent.loadClass(name, false);
} else {
c = findBootstrapClassOrNull(name);
}
} catch (ClassNotFoundException e) {
// ClassNotFoundException thrown if class not found
// from the non-null parent class loader
}
if (c == null) {
// If still not found, then invoke findClass in order
// to find the class.
long t1 = System.nanoTime();
c = findClass(name);
// this is the defining class loader; record the stats
sun.misc.PerfCounter.getParentDelegationTime().addTime(t1 - t0);
sun.misc.PerfCounter.getFindClassTime().addElapsedTimeFrom(t1);
sun.misc.PerfCounter.getFindClasses().increment();
}
}
if (resolve) {
resolveClass(c);
RFR: 8061950: Class.getMethods() exhibits quadratic time complexity
Hi, Here's a patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.04/ for the following issue: https://bugs.openjdk.java.net/browse/JDK-8061950 For those following the thread Loading classes with many methods is very expensive, this is a 4th iteration of this patch. I stepped back from the approach taken in 3rd webrev and rather refined approach taken in 2nd webrev. Instead of using a LinkedHashMap with values being linked-lists of nodes holding Method objects with same signature, which couldn't be made so that iteration would follow insertion order, I used plain HashMap this time. MethodNode(s) holding Method objects form a linked list of those sharing the same signature. In addition MethodNode(s) form a separate doubly-linked list of all nodes which is used to keep insertion order. The space use should be the same as I traded two object pointers in MethodNode for two less pointers in HashMap.Entry (vs. LinkedHashMap.Entry that was used before). So insertion order is not tracked by Map but instead by MethodTable now. I also track size of MethodTable now so there's no pre-traversing pass to allocate Method[] when requested. This version seems to be the fastest from all tried so far (measured by http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java using -Dsun.reflect.noCaches=true): Original: 19658 classes loaded in 2.027207954 seconds. 494392 methods obtained in 0.945519006 seconds. 494392 methods obtained in 0.95250834 seconds. 494392 methods obtained in 0.917841393 seconds. 494392 methods obtained in 0.971922977 seconds. 494392 methods obtained in 0.947145131 seconds. 494392 methods obtained in 0.937122672 seconds. 494392 methods obtained in 0.893975586 seconds. 494392 methods obtained in 0.90307736 seconds. 494392 methods obtained in 0.918782446 seconds. 494392 methods obtained in 0.881968668 seconds. Patched: 19658 classes loaded in 2.034081706 seconds. 494392 methods obtained in 0.8082686 seconds. 494392 methods obtained in 0.743307034 seconds. 494392 methods obtained in 0.751591979 seconds. 494392 methods obtained in 0.726954964 seconds. 494392 methods obtained in 0.761067189 seconds. 494392 methods obtained in 0.70825252 seconds. 494392 methods obtained in 0.696489363 seconds. 494392 methods obtained in 0.692555238 seconds. 494392 methods obtained in 0.695150116 seconds. 494392 methods obtained in 0.697665068 seconds. I also updated the test that checks multiple inheritance of abstract methods as I found that my 1st iteration of the algorithm was not getting the same results as original code although all jtreg tests were passing. I added 4 cases that include abstract classes as well as interfaces to the mix. Some of them would fail with my 1st version of algorithm. All 86 jtreg tests in java/lang/reflect/ and java/lang/Class/ pass with this patch applied. Regards, Peter
Re: Losing features of JVM_Open, e.g. CLOEXEC
Here's the state of the world: - the Unix designers made a design mistake that file descriptors are inherited by subprocesses by default. - all library code (almost all the code in the universe, including the JDK) needs to coexist with foreign code that might fork+exec at any time - to ensure that file descriptors don't leak into subprocesses, (almost) all library calls that create file descriptors must ensure that they have the close-on-exec bit set. (yes, this has been broken for a long time) - this is difficult enough that all creations of file descriptors must be wrapped using some kind of common infrastructure - JVM_Open (aka os::open) was terrible infrastructure (essentially undocumented) but at least it *was* infrastructure - we need openjdk-level or at least core-libraries-level native infrastructure, a place to put things like os::open and readFully. A project-wide commitment
Re: Losing features of JVM_Open, e.g. CLOEXEC
On Thu, Oct 30, 2014 at 6:08 AM, Alan Bateman alan.bate...@oracle.com wrote: The change in behavior from JVM_Open ito open was missed but there is a long way to go in JDK 9 so lots of time to fix the issue (if there is an issue). The changes to zip file file descriptors is just the one most visible to me. As was noted in one of the replies, the only usages of JVM_Open were in the zip code, it hasn't been used consistently used in the libraries for at least 10+ years. When doing significant changes then thorough code reviews are important but good tests are equally, and sometimes more, important. I understand that having a test for the O_CLOEXEC might be too hard so I'm interested to know how you ran into it. Maybe it was inspection? My hobby is running strace on the JDK and counting the fds with close-on-exec flag. I noticed that the calls to fcntl(...FD_CLOEXEC) had disappeared! You're unlikely to get bug reports because of the action-at-a-distance non-debuggability. Maybe an app that opens lots of zip files and executes fork/exec itself? For ProcessBuilder and Runtime.exec usages then the I thought the childproc code handled this by looping over the open file descriptors and closing them. So even if O_CLOEXEC or fcntl(FD_CLOEXEC) isn't used then it should be closed in the child. In general, Java is just a guest inside some Unix process, and should be well-behaved. As to whether we need a JVM_Open replacement in libjava then maybe we do but I think it needs a discussion as to whether the JDK really needs a porting interface or just useful infrastructure. Also I think going forward then I assume that the amount of native code will reduce, particularly when FFI comes along and we start to get rid of some of the native code (at least I hope that happens). I think it will just become more convenient to call the native code. Perhaps the common infrastructure can be written in Java, but it should still exist. In the mean-time then I think I'd like to understand more as to whether the O_CLOEXEC is an issue or not. Aside from the zip code then I don't think we've been using it consistently so I'm wondering if the issue is some native code calling fork+exec or something else? Yes. All broken since forever. I'm focused on close-on-exec, but there's also restartable system calls to consider. And partial reads ...
Re: Losing features of JVM_Open, e.g. CLOEXEC
On Thu, Oct 30, 2014 at 3:24 AM, Mario Torre neugens.limasoftw...@gmail.com wrote: I've been thinking perhaps one can use fcntl to check what flags are passed given we can retrieve all the file descriptors that have been opened? It's possible to find all the open file descriptors (e.g. using /proc/self/fd), but they may not belong to the JDK.
RFR: 8062558, Add javax/sql/rowset/spi tests
Hi all, Looking for a reviewer for the javax/sql/rowset/spi tests The webrev can be found at http://cr.openjdk.java.net/~lancea/8062558/webrev.00/ Best Lance 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: Losing features of JVM_Open, e.g. CLOEXEC
Indeed, but /proc/$PID/fd can perhaps be useful here? Not sure if this can make a reasonable test though. Cheers, Mario 2014-10-30 18:30 GMT+01:00 Martin Buchholz marti...@google.com: On Thu, Oct 30, 2014 at 3:24 AM, Mario Torre neugens.limasoftw...@gmail.com wrote: I've been thinking perhaps one can use fcntl to check what flags are passed given we can retrieve all the file descriptors that have been opened? It's possible to find all the open file descriptors (e.g. using /proc/self/fd), but they may not belong to the JDK. -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens Proud GNU Classpath developer: http://www.classpath.org/ OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/
Re: Losing features of JVM_Open, e.g. CLOEXEC
My bad. I missed the FD_CLOEXEC flag added by os::open() when I replaced JVM_Open with open in zip native code. However, I still think that removing those interfaces was a good move. But I understand that the change it introduces with zip file descriptors is an issue. Reverting the changeset to re-introduce JVM_Open is not a solution. Maintaining a VM interface only adding a flag for only 2 call sites doesn't make sense to me. Either the file descriptor leak issue is specific to the zip package and can be fixed locally. Or the issue impacts more native code and adding an infrastructure to libjava should be discussed. Quickly grepping the code, I found many naked calls to open, but only one use of FD_CLOEXEC. Further investigations would be required to track potential file descriptor leaks in all libraries. Did you create a bug to track this issue yet? Regards, Fred On 30/10/2014 18:15, Martin Buchholz wrote: Here's the state of the world: - the Unix designers made a design mistake that file descriptors are inherited by subprocesses by default. - all library code (almost all the code in the universe, including the JDK) needs to coexist with foreign code that might fork+exec at any time - to ensure that file descriptors don't leak into subprocesses, (almost) all library calls that create file descriptors must ensure that they have the close-on-exec bit set. (yes, this has been broken for a long time) - this is difficult enough that all creations of file descriptors must be wrapped using some kind of common infrastructure - JVM_Open (aka os::open) was terrible infrastructure (essentially undocumented) but at least it *was* infrastructure - we need openjdk-level or at least core-libraries-level native infrastructure, a place to put things like os::open and readFully. A project-wide commitment -- Frederic Parain - Oracle Grenoble Engineering Center - France Phone: +33 4 76 18 81 17 Email: frederic.par...@oracle.com
Re: RFR: 8061950: Class.getMethods() exhibits quadratic time complexity
Hi Peter! Thanks for all your hard work. Your patch is hard to digest, but some initial comments anyways: --- I'm surprised you got this much performance gain loading rt.jar methods. It looks like you are creating a more sophisticated data structure with more garbage, which won't show up as much on our small-memory benchmarks. Which is why I was expecting to have to write an adaptive data structure that switches from linear search to hash-based when some threshold is exceeded. --- (The introduction of default methods into openjdk makes method lookup even more confusing, and scares me, especially since I am uncertain we have enough tests...) --- If your changes to StarInheritance.java are general improvements, we could/should just check them in now (TDD?). --- Use javadoc comments /** even for private methods +/* This method returns 'root' Constructor object. It MUST be copied with ReflectionFactory + * before handed to any code outside java.lang.Class or modified. + */ private ConstructorT getConstructor0(Class?[] parameterTypes, --- The java way is to spell intfcsMethods interfacesMethods On Thu, Oct 30, 2014 at 9:53 AM, Peter Levart peter.lev...@gmail.com wrote: Hi, Here's a patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.04/ for the following issue: https://bugs.openjdk.java.net/browse/JDK-8061950 For those following the thread Loading classes with many methods is very expensive, this is a 4th iteration of this patch. I stepped back from the approach taken in 3rd webrev and rather refined approach taken in 2nd webrev. Instead of using a LinkedHashMap with values being linked-lists of nodes holding Method objects with same signature, which couldn't be made so that iteration would follow insertion order, I used plain HashMap this time. MethodNode(s) holding Method objects form a linked list of those sharing the same signature. In addition MethodNode(s) form a separate doubly-linked list of all nodes which is used to keep insertion order. The space use should be the same as I traded two object pointers in MethodNode for two less pointers in HashMap.Entry (vs. LinkedHashMap.Entry that was used before). So insertion order is not tracked by Map but instead by MethodTable now. I also track size of MethodTable now so there's no pre-traversing pass to allocate Method[] when requested. This version seems to be the fastest from all tried so far (measured by http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java using -Dsun.reflect.noCaches=true): Original: 19658 classes loaded in 2.027207954 seconds. 494392 methods obtained in 0.945519006 seconds. 494392 methods obtained in 0.95250834 seconds. 494392 methods obtained in 0.917841393 seconds. 494392 methods obtained in 0.971922977 seconds. 494392 methods obtained in 0.947145131 seconds. 494392 methods obtained in 0.937122672 seconds. 494392 methods obtained in 0.893975586 seconds. 494392 methods obtained in 0.90307736 seconds. 494392 methods obtained in 0.918782446 seconds. 494392 methods obtained in 0.881968668 seconds. Patched: 19658 classes loaded in 2.034081706 seconds. 494392 methods obtained in 0.8082686 seconds. 494392 methods obtained in 0.743307034 seconds. 494392 methods obtained in 0.751591979 seconds. 494392 methods obtained in 0.726954964 seconds. 494392 methods obtained in 0.761067189 seconds. 494392 methods obtained in 0.70825252 seconds. 494392 methods obtained in 0.696489363 seconds. 494392 methods obtained in 0.692555238 seconds. 494392 methods obtained in 0.695150116 seconds. 494392 methods obtained in 0.697665068 seconds. I also updated the test that checks multiple inheritance of abstract methods as I found that my 1st iteration of the algorithm was not getting the same results as original code although all jtreg tests were passing. I added 4 cases that include abstract classes as well as interfaces to the mix. Some of them would fail with my 1st version of algorithm. All 86 jtreg tests in java/lang/reflect/ and java/lang/Class/ pass with this patch applied. Regards, Peter
Re: Losing features of JVM_Open, e.g. CLOEXEC
On Thu, Oct 30, 2014 at 10:52 AM, Mario Torre neugens.limasoftw...@gmail.com wrote: Indeed, but /proc/$PID/fd can perhaps be useful here? Not sure if this can make a reasonable test though. Mario, I'm not sure what you mean. Even if we can find and inspect each fd, and even if we knew the stacktrace of where every fd was created, we still wouldn't know which close-on-exec flag should be cleared.
Re: Losing features of JVM_Open, e.g. CLOEXEC
On Oct 30, 11:26am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: Losing features of JVM_Open, e.g. CLOEXEC | On Thu, Oct 30, 2014 at 10:52 AM, Mario Torre | neugens.limasoftw...@gmail.com wrote: | Indeed, but /proc/$PID/fd can perhaps be useful here? Not sure if this | can make a reasonable test though. | | Mario, I'm not sure what you mean. | | Even if we can find and inspect each fd, and even if we knew the | stacktrace of where every fd was created, we still wouldn't know which | close-on-exec flag should be cleared. Let's not forget O_NOSIGPIPE, (and the equivalent SOCK_CLOEXEC and SOCK_NOSIGPIPE, for socket(2)). Also all the dup*(), pipe*(), kqueue*(), and fcntl() calls -- (in general all the system calls that can create file descriptors). christos
Re: Losing features of JVM_Open, e.g. CLOEXEC
Hi Frederic, On Thu, Oct 30, 2014 at 10:54 AM, frederic parain frederic.par...@oracle.com wrote: My bad. I missed the FD_CLOEXEC flag added by os::open() when I replaced JVM_Open with open in zip native code. However, I still think that removing those interfaces was a good move. But I understand that the change it introduces with zip file descriptors is an issue. By itself, it is not so bad - I I've been saying, the overall situation has been broken since forever. Reverting the changeset to re-introduce JVM_Open is not a solution. Maintaining a VM interface only adding a flag for only 2 call sites doesn't make sense to me. I agree this should not be a VM interface, so in that sense your change is an improvement! But we still need a common infrastructure/porting layer... and instead we're working on dismantling any that we have... Either the file descriptor leak issue is specific to the zip package and can be fixed locally. Or the issue impacts more native code and adding an infrastructure to libjava should be discussed. Quickly grepping the code, I found many naked calls to open, but only one use of FD_CLOEXEC. Further investigations would be required to track potential file descriptor leaks in all libraries. Did you create a bug to track this issue yet? Nope. But if I did, what component/sub-component would it go into.? We don't even seem to have a place to report cross-library bugs! We have no infrastructure to work on the no infrastructure problem!
Re: RFR (s): 4354680: Runtime.runFinalization() silently clears interrupted flag in the calling thread
On 10/30/14 2:21 AM, Chris Hegarty wrote: On 30 Oct 2014, at 07:31, David Holmes david.hol...@oracle.com wrote: On 30/10/2014 4:00 PM, Stanimir Simeonoff wrote: It could be defense vs Thread.currentThread().run() in finalizer that would cause calling run() of Thread.target. I am not sure if that would cause invocation, though. That is an excellent point! But it deserves a comment in the code. Adding a comment is fine, but please to not remove this field. ‘hg log’ may reveal more about it’s origins. Oh yes, quite right. I'll leave the field as-is, and add a comment here and to the other locations in this file to inform future maintainers. s'marks
Review request: JDK-8062556: Add jdk tests for JDK-8058322 and JDK-8058313
Hello, Please review this patch which adds tests to the JDK test suite for two reflection bugs that require hotspot changes (JDK-8058322 and JDK-8058313) The webrev is here: http://cr.openjdk.java.net/~emc/8062556/
Re: Review request: JDK-8062556: Add jdk tests for JDK-8058322 and JDK-8058313
I realize you are just adding new bad class files to an existing test, so you're just copying the idiom in the test, but it would be 1000 times more helpful if the comments included a javap or similar listing or even a description of what (bad) class this byte[] array purports to be. Otherwise, how can anyone review that the test does what it claims, other than by hand-disassembling this byte array? On 10/30/2014 7:41 PM, Eric McCorkle wrote: Hello, Please review this patch which adds tests to the JDK test suite for two reflection bugs that require hotspot changes (JDK-8058322 and JDK-8058313) The webrev is here: http://cr.openjdk.java.net/~emc/8062556/
Re: RFR: 8062558, Add javax/sql/rowset/spi tests
+1. Nice to improve test coverage. -Joe On 10/30/2014 10:38 AM, Lance Andersen wrote: Hi all, Looking for a reviewer for the javax/sql/rowset/spi tests The webrev can be found at http://cr.openjdk.java.net/~lancea/8062558/webrev.00/ Best Lance 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-8062556: Add jdk tests for JDK-8058322 and JDK-8058313
Hi Erik, On 31/10/2014 9:41 AM, Eric McCorkle wrote: Hello, Please review this patch which adds tests to the JDK test suite for two reflection bugs that require hotspot changes (JDK-8058322 and JDK-8058313) The webrev is here: http://cr.openjdk.java.net/~emc/8062556/ I second Brian's comment re the source of the bad classes. Your webrev is broken btw - no top-level html files. The new test needs a copyright year of 2014 not 2013. Thanks, David
