Noticed this during a code review... modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java: When copying a 'synchronized' map, you must lock it to copy it. But this was not done. It makes sense, instead, to use a ConcurrentHashMap, especially because the copy was only done for iteration purposes. Patch coming.
Created attachment 781697 [details] Patch for master (69656f42348a) Fixes concurrent modification issues; reduces object copying.
Noticed that 'inventoryEventListeners' could also be a CopyOnWriteArraySet(), although the synchronization as-is isn't a problem. diff --git a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java index 0a3859c..717544d 100644 --- a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java +++ b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java @@ -36,6 +36,7 @@ import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.Future; @@ -186,12 +187,12 @@ */ private boolean newPlatformWasDeletedRecently = false; // value only is valid/relevant if platform.getInventoryStatus == NEW - private ReentrantReadWriteLock inventoryLock = new ReentrantReadWriteLock(true); + private final ReentrantReadWriteLock inventoryLock = new ReentrantReadWriteLock(true); /** * Used only for the outside the agent model to # resources */ - private AtomicInteger temporaryKeyIndex = new AtomicInteger(-1); + private final AtomicInteger temporaryKeyIndex = new AtomicInteger(-1); /** * UUID to ResourceContainer map @@ -201,9 +202,9 @@ /** * Collection of event listeners to inform of changes to the inventory. */ - private Set<InventoryEventListener> inventoryEventListeners = new HashSet<InventoryEventListener>(); + private final Set<InventoryEventListener> inventoryEventListeners = new CopyOnWriteArraySet<InventoryEventListener>() - private PluginManager pluginManager = PluginContainer.getInstance().getPluginManager(); + private final PluginManager pluginManager = PluginContainer.getInstance().getPluginManager(); private DiscoveryComponentProxyFactory discoveryComponentProxyFactory; @@ -215,7 +216,7 @@ /** * Handles the resource upgrade during the initialization of the inventory manager. */ - private ResourceUpgradeDelegate resourceUpgradeDelegate = new ResourceUpgradeDelegate(this); + private final ResourceUpgradeDelegate resourceUpgradeDelegate = new ResourceUpgradeDelegate(this); public InventoryManager() { super(DiscoveryAgentService.class); @@ -2438,9 +2439,7 @@ public void requestFullAvailabilityReport() { * @param listener instance to notify of change events */ public void addInventoryEventListener(InventoryEventListener listener) { - synchronized (this.inventoryEventListeners) { - this.inventoryEventListeners.add(listener); - } + this.inventoryEventListeners.add(listener); } /** @@ -2449,9 +2448,7 @@ public void addInventoryEventListener(InventoryEventListener listener) { * @param listener instance to remove from event notification */ public void removeInventoryEventListener(InventoryEventListener listener) { - synchronized (this.inventoryEventListeners) { - this.inventoryEventListeners.remove(listener); - } + this.inventoryEventListeners.remove(listener); } /** @@ -2478,9 +2475,7 @@ public boolean isResourceUpgradeActive() { * @return all inventory event listeners */ private Set<InventoryEventListener> getInventoryEventListeners() { - synchronized (this.inventoryEventListeners) { - return new HashSet<InventoryEventListener>(this.inventoryEventListeners); - } + return this.inventoryEventListeners; } /**
When you have about 10,000 resources on one agent, I do see all sorts of performance issues, i.e. 100% CPU usage at times. Namely the hotspot is the iteration for finding a resource by ID: public ResourceContainer getResourceContainer(Integer resourceId) { - List<ResourceContainer> containers = new ArrayList<ResourceContainer>(this.resourceContainers.values()); // avoids concurrent mod exc - ResourceContainer retContainer = null; - for (ResourceContainer container : containers) { + for (ResourceContainer container : this.resourceContainers.values()) { should fix this, but it may make sense to create a Map<Integer, ResourceContainers> instead, as you avoid a linear search. It's sightly more complicated, though, as you have to maintain two maps.
Created attachment 786704 [details] Patch with some other fixed issues
Also, with two agents managing 10,000 resources, one running the patch one not, the difference is about 5 times: 2013-08-15 01:35:57,815 INFO [MeasurementManager.sender-1] (rhq.core.pc.measurement.MeasurementSenderRunner)- Measurement collection for [10574] metrics took 2917ms - sending report to Server... ... 2013-08-15 01:36:21,656 INFO [MeasurementManager.sender-1] (rhq.core.pc.measurement.MeasurementSenderRunner)- Measurement collection for [10591] metrics took 14842ms - sending report to Server... It probably would run a lot better if the getResourceContainer method was optimized a little more to avoid O(n).
i applied the patch that is attached to master branch: 3c216a95be3b3c042fadaf93aaaf95a7fc387c5a
Bulk closing now that 4.10 is out. If you think an issue is not resolved, please open a new BZ and link to the existing one.