From 04612c2b5b2ad02cefce7d96e625355298273088 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 7 Nov 2018 16:30:30 -0600 Subject: Bug 541048 - Limit the number of threads resolving/unresolving to 10 Massive number of resolving threads causes high levels of lock/retry contention, so we limit the number of concurrent resolves to 10 Change-Id: I58e4a704f64e824573dfd9e9f5b32f6802d8982c Signed-off-by: Thomas Watson --- .../osgi/tests/bundles/SystemBundleTests.java | 98 ++++++++ .../eclipse/osgi/container/ModuleContainer.java | 278 +++++++++++++-------- 2 files changed, 269 insertions(+), 107 deletions(-) diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java index 7009ee5af..67607a729 100755 --- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java +++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java @@ -2855,6 +2855,20 @@ public class SystemBundleTests extends AbstractBundleTests { return bundles; } + private static File[] createBundles(File outputDir, int bundleCount, Map extraHeaders) throws IOException { + outputDir.mkdirs(); + + File[] bundles = new File[bundleCount]; + + for (int i = 0; i < bundleCount; i++) { + Map headers = new HashMap<>(extraHeaders); + headers.put(Constants.BUNDLE_SYMBOLICNAME, "-b" + i); + bundles[i] = createBundle(outputDir, "-b" + i, headers); + } + + return bundles; + } + public static File createBundle(File outputDir, String id, boolean emptyManifest, boolean dirBundle) throws IOException { File file = new File(outputDir, "bundle" + id + (dirBundle ? "" : ".jar")); //$NON-NLS-1$ //$NON-NLS-2$ if (!dirBundle) { @@ -3856,4 +3870,88 @@ public class SystemBundleTests extends AbstractBundleTests { } } } + + // Note this is more of a performance test. It has a timeout that will cause it to + // fail if it takes too long. + public void testMassiveParallelInstallStart() { + File config = OSGiTestsActivator.getContext().getDataFile(getName()); //$NON-NLS-1$ + Map configuration = new HashMap(); + configuration.put(Constants.FRAMEWORK_STORAGE, config.getAbsolutePath()); + + final Equinox equinox = new Equinox(configuration); + try { + equinox.init(); + } catch (BundleException e) { + fail("Unexpected exception in init()", e); //$NON-NLS-1$ + } + // should be in the STARTING state + assertEquals("Wrong state for SystemBundle", Bundle.STARTING, equinox.getState()); //$NON-NLS-1$ + final BundleContext systemContext = equinox.getBundleContext(); + assertNotNull("System context is null", systemContext); //$NON-NLS-1$ + try { + equinox.start(); + } catch (BundleException e) { + fail("Failed to start the framework", e); //$NON-NLS-1$ + } + assertEquals("Wrong state for SystemBundle", Bundle.ACTIVE, equinox.getState()); //$NON-NLS-1$ + + long startCreateBundle = System.nanoTime(); + final int numBundles = 2000; + final File[] testBundles; + try { + testBundles = createBundles(new File(config, "bundles"), numBundles, Collections.singletonMap(Constants.DYNAMICIMPORT_PACKAGE, "*")); //$NON-NLS-1$ + } catch (IOException e) { + fail("Unexpected error creating budnles", e); //$NON-NLS-1$ + throw new RuntimeException(); + } + System.out.println("Time to create: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startCreateBundle)); + + long startReslveTime = System.nanoTime(); + ExecutorService executor = Executors.newFixedThreadPool(50); + final List errors = new CopyOnWriteArrayList<>(); + try { + for (final File f : testBundles) { + executor.execute(new Runnable() { + @Override + public void run() { + try { + String location = f.toURI().toURL().toExternalForm(); + System.out.println("Installing: " + f.getName()); + Bundle b = systemContext.installBundle(location); + b.start(); + BundleWiring wiring = b.adapt(BundleWiring.class); + wiring.getClassLoader().loadClass(BundleContext.class.getName()); + } catch (Exception e) { + errors.add(e); + } + } + }); + } + } finally { + executor.shutdown(); + try { + assertTrue("Operation took too long", executor.awaitTermination(5, TimeUnit.MINUTES)); + } catch (InterruptedException e) { + fail("Interrupted.", e); + } + } + System.out.println("Time to resolve: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startReslveTime)); + + Assert.assertEquals("Errors found.", Collections.emptyList(), errors); + Assert.assertEquals("Wrong number of bundles.", numBundles + 1, systemContext.getBundles().length); + + List providedWires = equinox.adapt(BundleWiring.class).getProvidedWires(PackageNamespace.PACKAGE_NAMESPACE); + Assert.assertEquals("Wrong number of provided wires.", numBundles, providedWires.size()); + + try { + equinox.stop(); + } catch (BundleException e) { + fail("Unexpected erorr stopping framework", e); //$NON-NLS-1$ + } + try { + equinox.waitForStop(10000); + } catch (InterruptedException e) { + fail("Unexpected interrupted exception", e); //$NON-NLS-1$ + } + } } diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java index 8bf37cd79..73bfac5bb 100644 --- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java +++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java @@ -13,6 +13,7 @@ *******************************************************************************/ package org.eclipse.osgi.container; +import java.io.Closeable; import java.security.AccessController; import java.security.PrivilegedAction; import java.util.ArrayList; @@ -25,9 +26,11 @@ import java.util.List; import java.util.ListIterator; import java.util.Map; import java.util.Set; +import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.ReentrantLock; import org.eclipse.osgi.container.Module.StartOptions; import org.eclipse.osgi.container.Module.State; import org.eclipse.osgi.container.Module.StopOptions; @@ -482,19 +485,23 @@ public final class ModuleContainer implements DebugOptionsListener { return new ModuleResolutionReport(null, Collections.> emptyMap(), new ResolutionException("Unable to resolve while shutting down the framework.")); //$NON-NLS-1$ } ResolutionReport report = null; - do { - try { - report = resolveAndApply(triggers, triggersMandatory, restartTriggers); - } catch (RuntimeException e) { - if (e.getCause() instanceof BundleException) { - BundleException be = (BundleException) e.getCause(); - if (be.getType() == BundleException.REJECTED_BY_HOOK || be.getType() == BundleException.STATECHANGE_ERROR) { - return new ModuleResolutionReport(null, Collections.> emptyMap(), new ResolutionException(be)); + try (ResolutionLock resolutionLock = new ResolutionLock(1)) { + do { + try { + report = resolveAndApply(triggers, triggersMandatory, restartTriggers); + } catch (RuntimeException e) { + if (e.getCause() instanceof BundleException) { + BundleException be = (BundleException) e.getCause(); + if (be.getType() == BundleException.REJECTED_BY_HOOK || be.getType() == BundleException.STATECHANGE_ERROR) { + return new ModuleResolutionReport(null, Collections.> emptyMap(), new ResolutionException(be)); + } } + throw e; } - throw e; - } - } while (report == null); + } while (report == null); + } catch (ResolutionLockException e) { + return new ModuleResolutionReport(null, Collections.> emptyMap(), new ResolutionException("Timeout acquiring lock for resolution", e, Collections. emptyList())); //$NON-NLS-1$ + } return report; } @@ -553,90 +560,94 @@ public final class ModuleContainer implements DebugOptionsListener { Map deltaWiring; Collection modulesResolved; long timestamp; - do { - result = null; - Map wiringClone = null; - List dynamicReqs = null; - Collection unresolved = new ArrayList<>(); - moduleDatabase.readLock(); - try { - ModuleWiring wiring = revision.getWiring(); - if (wiring == null) { - // not resolved!! - return null; - } - if (wiring.isDynamicPackageMiss(dynamicPkgName)) { - // cached a miss for this package - return null; - } - // need to check that another thread has not done the work already - result = findExistingDynamicWire(revision.getWiring(), dynamicPkgName); - if (result != null) { - return result; - } - dynamicReqs = getDynamicRequirements(dynamicPkgName, revision); - if (dynamicReqs.isEmpty()) { - // save the miss for the package name - wiring.addDynamicPackageMiss(dynamicPkgName); - return null; - } - timestamp = moduleDatabase.getRevisionsTimestamp(); - wiringClone = moduleDatabase.getWiringsClone(); - Collection allModules = moduleDatabase.getModules(); - for (Module module : allModules) { - ModuleRevision current = module.getCurrentRevision(); - if (current != null && !wiringClone.containsKey(current)) - unresolved.add(current); + try (ResolutionLock resolutionLock = new ResolutionLock(MAX_RESOLUTION_PERMITS)) { + do { + result = null; + Map wiringClone = null; + List dynamicReqs = null; + Collection unresolved = new ArrayList<>(); + moduleDatabase.readLock(); + try { + ModuleWiring wiring = revision.getWiring(); + if (wiring == null) { + // not resolved!! + return null; + } + if (wiring.isDynamicPackageMiss(dynamicPkgName)) { + // cached a miss for this package + return null; + } + // need to check that another thread has not done the work already + result = findExistingDynamicWire(revision.getWiring(), dynamicPkgName); + if (result != null) { + return result; + } + dynamicReqs = getDynamicRequirements(dynamicPkgName, revision); + if (dynamicReqs.isEmpty()) { + // save the miss for the package name + wiring.addDynamicPackageMiss(dynamicPkgName); + return null; + } + timestamp = moduleDatabase.getRevisionsTimestamp(); + wiringClone = moduleDatabase.getWiringsClone(); + Collection allModules = moduleDatabase.getModules(); + for (Module module : allModules) { + ModuleRevision current = module.getCurrentRevision(); + if (current != null && !wiringClone.containsKey(current)) + unresolved.add(current); + } + } finally { + moduleDatabase.readUnlock(); } - } finally { - moduleDatabase.readUnlock(); - } - deltaWiring = null; - boolean foundCandidates = false; - for (DynamicModuleRequirement dynamicReq : dynamicReqs) { - ModuleResolutionReport report = moduleResolver.resolveDynamicDelta(dynamicReq, unresolved, wiringClone, moduleDatabase); - Map> resolutionResult = report.getResolutionResult(); - deltaWiring = resolutionResult == null ? Collections. emptyMap() : moduleResolver.generateDelta(resolutionResult, wiringClone); - if (deltaWiring.get(revision) != null) { - break; - } - // Did not establish a valid wire. - // Check to see if any candidates were available. - // this is used for caching purposes below - List revisionEntries = report.getEntries().get(revision); - if (revisionEntries == null || revisionEntries.isEmpty()) { - foundCandidates = true; - } else { - // must make sure there is no MISSING_CAPABILITY type entry - boolean isMissingCapability = false; - for (Entry entry : revisionEntries) { - isMissingCapability |= Entry.Type.MISSING_CAPABILITY.equals(entry.getType()); + deltaWiring = null; + boolean foundCandidates = false; + for (DynamicModuleRequirement dynamicReq : dynamicReqs) { + ModuleResolutionReport report = moduleResolver.resolveDynamicDelta(dynamicReq, unresolved, wiringClone, moduleDatabase); + Map> resolutionResult = report.getResolutionResult(); + deltaWiring = resolutionResult == null ? Collections. emptyMap() : moduleResolver.generateDelta(resolutionResult, wiringClone); + if (deltaWiring.get(revision) != null) { + break; + } + // Did not establish a valid wire. + // Check to see if any candidates were available. + // this is used for caching purposes below + List revisionEntries = report.getEntries().get(revision); + if (revisionEntries == null || revisionEntries.isEmpty()) { + foundCandidates = true; + } else { + // must make sure there is no MISSING_CAPABILITY type entry + boolean isMissingCapability = false; + for (Entry entry : revisionEntries) { + isMissingCapability |= Entry.Type.MISSING_CAPABILITY.equals(entry.getType()); + } + foundCandidates |= !isMissingCapability; } - foundCandidates |= !isMissingCapability; } - } - if (deltaWiring == null || deltaWiring.get(revision) == null) { - if (!foundCandidates) { - ModuleWiring wiring = revision.getWiring(); - if (wiring != null) { - // save the miss for the package name - wiring.addDynamicPackageMiss(dynamicPkgName); + if (deltaWiring == null || deltaWiring.get(revision) == null) { + if (!foundCandidates) { + ModuleWiring wiring = revision.getWiring(); + if (wiring != null) { + // save the miss for the package name + wiring.addDynamicPackageMiss(dynamicPkgName); + } } + return null; // nothing to do } - return null; // nothing to do - } - modulesResolved = new ArrayList<>(); - for (ModuleRevision deltaRevision : deltaWiring.keySet()) { - if (!wiringClone.containsKey(deltaRevision)) - modulesResolved.add(deltaRevision.getRevisions().getModule()); - } + modulesResolved = new ArrayList<>(); + for (ModuleRevision deltaRevision : deltaWiring.keySet()) { + if (!wiringClone.containsKey(deltaRevision)) + modulesResolved.add(deltaRevision.getRevisions().getModule()); + } - // Save the result - ModuleWiring wiring = deltaWiring.get(revision); - result = findExistingDynamicWire(wiring, dynamicPkgName); - } while (!applyDelta(deltaWiring, modulesResolved, Collections. emptyList(), timestamp, false)); + // Save the result + ModuleWiring wiring = deltaWiring.get(revision); + result = findExistingDynamicWire(wiring, dynamicPkgName); + } while (!applyDelta(deltaWiring, modulesResolved, Collections. emptyList(), timestamp, false)); + } catch (ResolutionLockException e) { + return null; + } return result; } @@ -660,19 +671,69 @@ public final class ModuleContainer implements DebugOptionsListener { return null; } - private final Object stateLockMonitor = new Object(); + // The resolution algorithm uses optimistic locking approach; + // This involves taking a snapshot of the state and performing an + // operation on the snapshot while holding no locks and then + // obtaining the write lock to apply the results. If we + // detect the state has changed since the snapshot taken then + // the process is started over. If we allow too many threads + // to try to do this at the same time it causes thrashing + // between taking the snapshot and successfully applying the + // results. Instead of resorting to single threaded operations + // we choose to limit the number of concurrent resolves + final static int MAX_RESOLUTION_PERMITS = 10; + final Semaphore _resolutionLock = new Semaphore(MAX_RESOLUTION_PERMITS); + final ReentrantLock _bundleStateLock = new ReentrantLock(); + + static class ResolutionLockException extends Exception { + private static final long serialVersionUID = 1L; + + public ResolutionLockException() { + super(); + } + + public ResolutionLockException(Throwable cause) { + super(cause); + } + } + + class ResolutionLock implements Closeable { + private final int permits; + + ResolutionLock(int permits) throws ResolutionLockException { + this.permits = permits; + boolean previousInterruption = Thread.interrupted(); + try { + if (!_resolutionLock.tryAcquire(permits, 30, TimeUnit.SECONDS)) { + throw new ResolutionLockException(); + } + } catch (InterruptedException e) { + throw new ResolutionLockException(e); + } finally { + if (previousInterruption) { + Thread.currentThread().interrupt(); + } + } + } + + @Override + public void close() { + _resolutionLock.release(permits); + } + } private boolean applyDelta(Map deltaWiring, Collection modulesResolved, Collection triggers, long timestamp, boolean restartTriggers) { List modulesLocked = new ArrayList<>(modulesResolved.size()); // now attempt to apply the delta try { - synchronized (stateLockMonitor) { - // Acquire the necessary RESOLVED state change lock. - // Note this is done while holding a global lock to avoid multiple threads trying to compete over - // locking multiple modules; otherwise out of order locks between modules can happen - // NOTE this MUST be done outside of holding the moduleDatabase lock also to avoid - // introducing out of order locks between the bundle state change lock and the moduleDatabase - // lock. + // Acquire the necessary RESOLVED state change lock. + // Note this is done while holding a global lock to avoid multiple threads trying to compete over + // locking multiple modules; otherwise out of order locks between modules can happen + // NOTE this MUST be done outside of holding the moduleDatabase lock also to avoid + // introducing out of order locks between the bundle state change lock and the moduleDatabase + // lock. + _bundleStateLock.lock(); + try { for (Module module : modulesResolved) { try { // avoid grabbing the lock if the timestamp has changed @@ -690,7 +751,10 @@ public final class ModuleContainer implements DebugOptionsListener { throw new IllegalStateException(Msg.ModuleContainer_StateLockError, e); } } + } finally { + _bundleStateLock.unlock(); } + Map> hostsWithDynamicFrags = new HashMap<>(0); moduleDatabase.writeLock(); try { @@ -895,18 +959,19 @@ public final class ModuleContainer implements DebugOptionsListener { // NOTE this MUST be done outside of holding the moduleDatabase lock also to avoid // introducing out of order locks between the bundle state change lock and the moduleDatabase // lock. - synchronized (stateLockMonitor) { - try { - // go in reverse order - for (ListIterator iTriggers = refreshTriggers.listIterator(refreshTriggers.size()); iTriggers.hasPrevious();) { - Module refreshModule = iTriggers.previous(); - refreshModule.lockStateChange(ModuleEvent.UNRESOLVED); - modulesLocked.add(refreshModule); - } - } catch (BundleException e) { - // TODO throw some appropriate exception - throw new IllegalStateException(Msg.ModuleContainer_StateLockError, e); + _bundleStateLock.lock(); + try { + // go in reverse order + for (ListIterator iTriggers = refreshTriggers.listIterator(refreshTriggers.size()); iTriggers.hasPrevious();) { + Module refreshModule = iTriggers.previous(); + refreshModule.lockStateChange(ModuleEvent.UNRESOLVED); + modulesLocked.add(refreshModule); } + } catch (BundleException e) { + // TODO throw some appropriate exception + throw new IllegalStateException(Msg.ModuleContainer_StateLockError, e); + } finally { + _bundleStateLock.unlock(); } // Must not hold the module database lock while stopping bundles // Stop any active bundles and remove non-active modules from the refreshTriggers @@ -974,7 +1039,6 @@ public final class ModuleContainer implements DebugOptionsListener { module.unlockStateChange(ModuleEvent.UNRESOLVED); } } - // publish unresolved events after giving up all locks for (Module module : modulesUnresolved) { adaptor.publishModuleEvent(ModuleEvent.UNRESOLVED, module, module); -- cgit v1.2.3