Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 4/2/2014 7:44 AM, Seán Coffey wrote: On 02/04/14 15:05, Alan Bateman wrote: On 02/04/2014 11:42, Seán Coffey wrote: I'm looking to port this fix from JDK 8 to 9. Other changes were made for JDK 9 given that we have access to the javaAwtAccess.getAppletContext() method when using JDK 8 as the boot strap JDK for CORBA. Direct access to the sun.awt.AppContect class has been removed. bug : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618.jdk9.v3/webrev/ This mostly looks okay to me except that pmContexts should be final. It looks like this code is also a candidate to use computeIfAbsent so you could replace L251-254 with one statement. I'll make pmContexts final before pushing. Thanks for the lambda tip. As per trial though, it looks like CORBA code is still compiled with -source 1.7, we can revisit after this changes. Looks good to me too.Corba's use of lambda would need to wait until the boot jdk for jdk9 will be updated to jdk8? Mandy
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 02/04/14 15:05, Alan Bateman wrote: On 02/04/2014 11:42, Seán Coffey wrote: I'm looking to port this fix from JDK 8 to 9. Other changes were made for JDK 9 given that we have access to the javaAwtAccess.getAppletContext() method when using JDK 8 as the boot strap JDK for CORBA. Direct access to the sun.awt.AppContect class has been removed. bug : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618.jdk9.v3/webrev/ This mostly looks okay to me except that pmContexts should be final. It looks like this code is also a candidate to use computeIfAbsent so you could replace L251-254 with one statement. I'll make pmContexts final before pushing. Thanks for the lambda tip. As per trial though, it looks like CORBA code is still compiled with -source 1.7, we can revisit after this changes. regards, Sean. -Alan.
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 02/04/2014 11:42, Seán Coffey wrote: I'm looking to port this fix from JDK 8 to 9. Other changes were made for JDK 9 given that we have access to the javaAwtAccess.getAppletContext() method when using JDK 8 as the boot strap JDK for CORBA. Direct access to the sun.awt.AppContect class has been removed. bug : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618.jdk9.v3/webrev/ This mostly looks okay to me except that pmContexts should be final. It looks like this code is also a candidate to use computeIfAbsent so you could replace L251-254 with one statement. -Alan.
RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
I'm looking to port this fix from JDK 8 to 9. Other changes were made for JDK 9 given that we have access to the javaAwtAccess.getAppletContext() method when using JDK 8 as the boot strap JDK for CORBA. Direct access to the sun.awt.AppContect class has been removed. bug : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618.jdk9.v3/webrev/ Regards, Sean.
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 27/02/2014 09:38, Seán Coffey wrote: On 27/02/2014 07:45, Alan Bateman wrote: On 26/02/2014 21:47, Seán Coffey wrote: Good points Alan. Changes uploaded here : http://cr.openjdk.java.net/~coffeys/webrev.8035618.v2/webrev/ This looks much better. Are you sure that pmLock is needed? Is there any reason not to synchronize on the AppContext when adding the PresentationManager key? I was conscious of possible contention coming on the AppContext object if syncing on it. I figured a static lock should work (given that we had zero synchronization in the old code) Assuming AppContext is mostly for applets and JNLP applications then I wouldn't expect it should be a problem. I also wouldn't expect there are many cases with multiple applets in the same VM all using CORBA at the same time. So what you have it okay but equally okay to drop pmLock and use ac instead. Yes - I'll get the direct AppContext reference removed for JDK 9 fix via JavaAWTAccess but will need to re-examine the code to see if we can avoid sun.awt code. That would be great. -Alan.
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 27/02/2014 07:45, Alan Bateman wrote: On 26/02/2014 21:47, Seán Coffey wrote: Good points Alan. Changes uploaded here : http://cr.openjdk.java.net/~coffeys/webrev.8035618.v2/webrev/ This looks much better. Are you sure that pmLock is needed? Is there any reason not to synchronize on the AppContext when adding the PresentationManager key? I was conscious of possible contention coming on the AppContext object if syncing on it. I figured a static lock should work (given that we had zero synchronization in the old code) - Apologies for not including your name in final push. It's been pointed out to me that I should include all reviewers. I thought I could only include reviewers once I had an 'ok'. Are you planning to propose a fix for jdk9/dev too? It would be good to eliminate the direct dependency on sun.awt.AppContext while doing that. Yes - I'll get the direct AppContext reference removed for JDK 9 fix via JavaAWTAccess but will need to re-examine the code to see if we can avoid sun.awt code. regards, Sean. -Alan
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 26/02/2014 21:47, Seán Coffey wrote: Good points Alan. Changes uploaded here : http://cr.openjdk.java.net/~coffeys/webrev.8035618.v2/webrev/ This looks much better. Are you sure that pmLock is needed? Is there any reason not to synchronize on the AppContext when adding the PresentationManager key? Are you planning to propose a fix for jdk9/dev too? It would be good to eliminate the direct dependency on sun.awt.AppContext while doing that. -Alan
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 26/02/2014 21:47, Seán Coffey wrote: Good points Alan. Changes uploaded here : http://cr.openjdk.java.net/~coffeys/webrev.8035618.v2/webrev/ Looks ok to me Sean. -Chris. regards, Sean. On 26/02/14 18:54, Alan Bateman wrote: On 26/02/2014 16:50, Seán Coffey wrote: Looking to push a fix to JDK 8. A CORBA issue was discovered during TCK-Plugin testing. The NPE seen should be handled and the defaultPresentationManager should be returned where necessary. Bug ID : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618/webrev/ The AppContext usage looks okay but I worry that changing this method to synchronized will lead to contention. At least for the non security manager then can this be changed so that the defaultPresentationManager is initialized via a holder class and changed to volatile and initialized via the usual double checked locking idiom. There are other options but I think this one is the important case and should not be synchronized. -Alan.
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
Looks good to me. Mandy On 2/26/2014 1:47 PM, Seán Coffey wrote: Good points Alan. Changes uploaded here : http://cr.openjdk.java.net/~coffeys/webrev.8035618.v2/webrev/ regards, Sean. On 26/02/14 18:54, Alan Bateman wrote: On 26/02/2014 16:50, Seán Coffey wrote: Looking to push a fix to JDK 8. A CORBA issue was discovered during TCK-Plugin testing. The NPE seen should be handled and the defaultPresentationManager should be returned where necessary. Bug ID : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618/webrev/ The AppContext usage looks okay but I worry that changing this method to synchronized will lead to contention. At least for the non security manager then can this be changed so that the defaultPresentationManager is initialized via a holder class and changed to volatile and initialized via the usual double checked locking idiom. There are other options but I think this one is the important case and should not be synchronized. -Alan.
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
Good points Alan. Changes uploaded here : http://cr.openjdk.java.net/~coffeys/webrev.8035618.v2/webrev/ regards, Sean. On 26/02/14 18:54, Alan Bateman wrote: On 26/02/2014 16:50, Seán Coffey wrote: Looking to push a fix to JDK 8. A CORBA issue was discovered during TCK-Plugin testing. The NPE seen should be handled and the defaultPresentationManager should be returned where necessary. Bug ID : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618/webrev/ The AppContext usage looks okay but I worry that changing this method to synchronized will lead to contention. At least for the non security manager then can this be changed so that the defaultPresentationManager is initialized via a holder class and changed to volatile and initialized via the usual double checked locking idiom. There are other options but I think this one is the important case and should not be synchronized. -Alan.
Re: RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
On 26/02/2014 16:50, Seán Coffey wrote: Looking to push a fix to JDK 8. A CORBA issue was discovered during TCK-Plugin testing. The NPE seen should be handled and the defaultPresentationManager should be returned where necessary. Bug ID : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618/webrev/ The AppContext usage looks okay but I worry that changing this method to synchronized will lead to contention. At least for the non security manager then can this be changed so that the defaultPresentationManager is initialized via a holder class and changed to volatile and initialized via the usual double checked locking idiom. There are other options but I think this one is the important case and should not be synchronized. -Alan.
RFR: 8035618 : Four api/org_omg/CORBA TCK tests fail under plugin only
Looking to push a fix to JDK 8. A CORBA issue was discovered during TCK-Plugin testing. The NPE seen should be handled and the defaultPresentationManager should be returned where necessary. Bug ID : https://bugs.openjdk.java.net/browse/JDK-8035618 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8035618/webrev/ regards, Sean.
