Code Review Request - Bug #6948101 & 7142596: RMI JPRT tests are failing
Hello core-libs. Please review this webrev to fix Bug #6948101 and #7142596. This commit fixes concurrency issues with the RMI tests. - Added TestLibrary.getUnusedRandomPort method. This attempts to return a random port number, which is unused, between the port ranges 1024 and 64000 (inclusive). It will try up to 10 times before giving up. - Changed almost all tests from using hard port numbers to using random port numbers for running the RMI Registry and RMID. - Removed othervm from those tests which don't need it. - Added parameters for tests which spawn a separate VM to pass RMI Registry and RMID ports in cases where needed. - Added PropertyPermission to security policy files where needed. - Removed java/rmi and sun/rmi from tests which cannot be run concurrently. - Removed java/rmi and sun/rmi from tests which must run in othervm mode. - Added java/rmi/Naming to list of tests which cannot be run concurrently. Webrev, can be found here: http://cr.openjdk.java.net/~dmocek/7142596/webrev.00 Thanks, Darryl
On 14/04/2012 00:45, Darryl Mocek wrote:
Hello core-libs. Please review this webrev to fix Bug #6948101 and #7142596. This commit fixes concurrency issues with the RMI tests.
- Added TestLibrary.getUnusedRandomPort method. This attempts to return a random port number, which is unused, between the port ranges 1024 and 64000 (inclusive). It will try up to 10 times before giving up. - Changed almost all tests from using hard port numbers to using random port numbers for running the RMI Registry and RMID. - Removed othervm from those tests which don't need it. - Added parameters for tests which spawn a separate VM to pass RMI Registry and RMID ports in cases where needed. - Added PropertyPermission to security policy files where needed. - Removed java/rmi and sun/rmi from tests which cannot be run concurrently. - Removed java/rmi and sun/rmi from tests which must run in othervm mode. - Added java/rmi/Naming to list of tests which cannot be run concurrently.
Webrev, can be found here: http://cr.openjdk.java.net/~dmocek/7142596/webrev.00 It's great to see this patch as the RMI tests have been just too slow and the last area of the core libraries that can't take advantage of multi-core machines. Also this patch should eliminate the cascading failures that periodically arise when something goes wrong and a test leaves a registry on the default port, causing problems for subsequent tests.
I've looked briefly at the changes (not a full review, I assume Stuart will do that) and if I understand correctly, then this changes don't mean we can run the tests in agentvm mode, right? In that case I don't think the test directory can be removed from the othervms.dir lists in TEST.ROOT. I see the logic in TestLibrary to choose a random port and I'm just wondered if you considered doing LocaleRegistry.createRegistry(0) so that the registry binds to an ephemeral port. It would mean that TestLibrary would need to get to the transport endpoint and port but it would avoid having to try multiple ports. -Alan
See inline On Sat 14 Apr 2012 09:12:37 AM PDT, Alan Bateman wrote:
On 14/04/2012 00:45, Darryl Mocek wrote:
Hello core-libs. Please review this webrev to fix Bug #6948101 and #7142596. This commit fixes concurrency issues with the RMI tests.
- Added TestLibrary.getUnusedRandomPort method. This attempts to return a random port number, which is unused, between the port ranges 1024 and 64000 (inclusive). It will try up to 10 times before giving up. - Changed almost all tests from using hard port numbers to using random port numbers for running the RMI Registry and RMID. - Removed othervm from those tests which don't need it. - Added parameters for tests which spawn a separate VM to pass RMI Registry and RMID ports in cases where needed. - Added PropertyPermission to security policy files where needed. - Removed java/rmi and sun/rmi from tests which cannot be run concurrently. - Removed java/rmi and sun/rmi from tests which must run in othervm mode. - Added java/rmi/Naming to list of tests which cannot be run concurrently.
Webrev, can be found here: http://cr.openjdk.java.net/~dmocek/7142596/webrev.00
It's great to see this patch as the RMI tests have been just too slow and the last area of the core libraries that can't take advantage of multi-core machines. Also this patch should eliminate the cascading failures that periodically arise when something goes wrong and a test leaves a registry on the default port, causing problems for subsequent tests.
I've looked briefly at the changes (not a full review, I assume Stuart will do that) and if I understand correctly, then this changes don't mean we can run the tests in agentvm mode, right? In that case I don't think the test directory can be removed from the othervms.dir lists in TEST.ROOT.
Ah, yes, with -agentvm specified, many tests fail. I'll have to add java/rmi and sun/rmi back to the list of tests which must be run in othervm mode and I should probably add back othervm to the tests I removed it from. At least with the port conflict fix, we can run the tests concurrently and they will run much faster.
I see the logic in TestLibrary to choose a random port and I'm just wondered if you considered doing LocaleRegistry.createRegistry(0) so that the registry binds to an ephemeral port. It would mean that TestLibrary would need to get to the transport endpoint and port but it would avoid having to try multiple ports.
I would have like to use LocaleRegistry.createRegistry(0), but I don't see a way to get the port number once the registry is created, which is required. I originally tried creating a TestLocaleRegistry which extends LocateRegistry, with a getPort() method, but this didn't work either. Darryl
-Alan
On 16/04/2012 19:34, Darryl Mocek wrote:
Ah, yes, with -agentvm specified, many tests fail. I'll have to add java/rmi and sun/rmi back to the list of tests which must be run in othervm mode and I should probably add back othervm to the tests I removed it from. At least with the port conflict fix, we can run the tests concurrently and they will run much faster. Yes, it's a good first step and I look forward to getting much better through-put of these tests.
I see the logic in TestLibrary to choose a random port and I'm just wondered if you considered doing LocaleRegistry.createRegistry(0) so that the registry binds to an ephemeral port. It would mean that TestLibrary would need to get to the transport endpoint and port but it would avoid having to try multiple ports.
I would have like to use LocaleRegistry.createRegistry(0), but I don't see a way to get the port number once the registry is created, which is required. I originally tried creating a TestLocaleRegistry which extends LocateRegistry, with a getPort() method, but this didn't work either. I don't think there is an API to get the endpoint (Peter Jones, any ideas?) so it may require TestLibrary to use reflection or sun.* code to get it. Another sleazy suggestion is that it's in toString output and TestLibrary would provide method to get the port that wouldn't require all the tests to depend on it.
-Alan
On Tue 17 Apr 2012 02:00:23 AM PDT, Alan Bateman wrote:
On 16/04/2012 19:34, Darryl Mocek wrote:
Ah, yes, with -agentvm specified, many tests fail. I'll have to add java/rmi and sun/rmi back to the list of tests which must be run in othervm mode and I should probably add back othervm to the tests I removed it from. At least with the port conflict fix, we can run the tests concurrently and they will run much faster.
Yes, it's a good first step and I look forward to getting much better through-put of these tests.
I see the logic in TestLibrary to choose a random port and I'm just wondered if you considered doing LocaleRegistry.createRegistry(0) so that the registry binds to an ephemeral port. It would mean that TestLibrary would need to get to the transport endpoint and port but it would avoid having to try multiple ports.
I would have like to use LocaleRegistry.createRegistry(0), but I don't see a way to get the port number once the registry is created, which is required. I originally tried creating a TestLocaleRegistry which extends LocateRegistry, with a getPort() method, but this didn't work either.
I don't think there is an API to get the endpoint (Peter Jones, any ideas?) so it may require TestLibrary to use reflection or sun.* code to get it. Another sleazy suggestion is that it's in toString output and TestLibrary would provide method to get the port that wouldn't require all the tests to depend on it.
Brilliant! I didn't realize Registry.toString included the port number. However, I did in fact find a way to get the port through the API's (see below), although I think toString is more efficient. I'm pretty sure I can get rid of the instanceof checks. Darryl public static int getRegistryPort(Registry registry) { int port = -1; if (registry instanceof RegistryImpl) { System.out.println("registry instanceof RegistryImpl"); RemoteRef remoteRef = ((RegistryImpl)registry).getRef(); if (remoteRef instanceof UnicastServerRef) { System.out.println("remoteRef instanceof UnicastServerRef"); LiveRef liveRef = ((UnicastServerRef)remoteRef).getLiveRef(); try { Endpoint endpoint = liveRef.getChannel().getEndpoint(); if (endpoint instanceof TCPEndpoint) { TCPEndpoint tcpEndpoint = (TCPEndpoint) endpoint; port = tcpEndpoint.getPort(); } System.out.println("registry = " + registry); } catch (RemoteException ex) { Logger.getLogger(TestLibrary.class.getName()).log(Level.SEVERE, null, ex); } } } return port; }
-Alan
On 18/04/2012 21:39, Darryl Mocek wrote:
:
Brilliant! I didn't realize Registry.toString included the port number. However, I did in fact find a way to get the port through the API's (see below), although I think toString is more efficient. I'm pretty sure I can get rid of the instanceof checks. I don't have a strong opinion as to whether it uses toString or depends on sun.rmi.server.RegistryImpl. The main thing is that I think it will be a lot more reliable to bind to an ephemeral port when compared to find an unused port.
Just so you know, I grabbed the patch from your webrev and tried it out, it was nice to see close to x4 improvement when running these tests with -concurrency:auto on an 4-core system. -Alan
I've modified the implementation: - Use LocateRegistry.createRegistry(0) where possible (it's not possible in all places), rather then creating a ServerSocket, getting the port, then closing it and returning the port number. - Added a TestLibrary.getRegistryPort(Registry) method to get the port number of the registry. - Added java/rmi and sun/rmi back to the list of tests which must be run in othervm mode. - Added back othervm to the tests I removed it from. - Fixed the AltSecurityManager test and removed it from the ProblemList. Revised webrev can be found here: http://cr.openjdk.java.net/~dmocek/7142596/webrev.01 Darryl On 04/19/2012 03:50 AM, Alan Bateman wrote:
On 18/04/2012 21:39, Darryl Mocek wrote:
:
Brilliant! I didn't realize Registry.toString included the port number. However, I did in fact find a way to get the port through the API's (see below), although I think toString is more efficient. I'm pretty sure I can get rid of the instanceof checks. I don't have a strong opinion as to whether it uses toString or depends on sun.rmi.server.RegistryImpl. The main thing is that I think it will be a lot more reliable to bind to an ephemeral port when compared to find an unused port.
Just so you know, I grabbed the patch from your webrev and tried it out, it was nice to see close to x4 improvement when running these tests with -concurrency:auto on an 4-core system.
-Alan
On 20/04/2012 23:40, Darryl Mocek wrote:
I've modified the implementation:
- Use LocateRegistry.createRegistry(0) where possible (it's not possible in all places), rather then creating a ServerSocket, getting the port, then closing it and returning the port number. - Added a TestLibrary.getRegistryPort(Registry) method to get the port number of the registry. - Added java/rmi and sun/rmi back to the list of tests which must be run in othervm mode. - Added back othervm to the tests I removed it from. - Fixed the AltSecurityManager test and removed it from the ProblemList.
Revised webrev can be found here: http://cr.openjdk.java.net/~dmocek/7142596/webrev.01 As a sanity check I ran the java/rmi and sun/rmi tests on an 8-core machine, specifying -concurrency:auto to jtreg.
Without your patch then these tests are forced to run sequentially (by the exclusiveAccess.dirs setting) and completed in 28m 11s for me. I applied your patch and they completed (successfully) in 4m 8s. So this is good stuff. I think Stuart is going to review the patch in detail, I don't think I have time to go through all of it but will try to at least scan it. One thing that I want to look for is to check that the VM options are passed through to any rmid or rmiregistry processes that are created in the tests. This is important when running with concurrency>1 to ensure that we don't over-commit memory - for example I might run with jtreg -vmoption:-Xmx512m so that agent VMs or processes launched by the test harness don't consume 1/4 of the memory and we need to ensure these options get propagated through to rmi* processes that these tests might launch. This issue wasn't a problem when trying your patch because the system I ran on had 24GB (but it could be an issue on a lesser machine). -Alan
participants (2)
-
Alan Bateman
-
Darryl Mocek