Bug 991153
Summary: | Agent uses incorrect synchronization for 'UUID to ResourceContainer map' | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Other] RHQ Project | Reporter: | Elias Ross <genman> | ||||||
Component: | Agent | Assignee: | John Mazzitelli <mazz> | ||||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Mike Foley <mfoley> | ||||||
Severity: | unspecified | Docs Contact: | |||||||
Priority: | unspecified | ||||||||
Version: | 4.8 | CC: | hrupp, mazz | ||||||
Target Milestone: | --- | ||||||||
Target Release: | RHQ 4.9 | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2014-03-26 08:32:10 UTC | Type: | Bug | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Attachments: |
|
Description
Elias Ross
2013-08-01 18:15:56 UTC
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. |