Bug 991153

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.