Another strange stacktrace in agent log

Lukas Krejci lkrejci at redhat.com
Wed Feb 12 14:07:57 UTC 2014


On Tuesday, February 11, 2014 12:17:05 Jay Shaughnessy wrote:
> Hi Thomas,
> 
> OK, sorry, I wasn't aware that we were already in this situation given
> Ian's work to address the concurrency issue at an earlier time.
> Although I realize that the CopyOnWriteArraySet implementation may not
> be necessary on the server-side the benefit gained is a simpler
> implementation overall.   And, we have seen instances of
> ConcurrentModificationException server-side, although I'm not sure it
> has affected Resource children.   I'm not sure we'll give up too much
> server-side as I think most of the childResources setting is done once,
> and child cardinality tends to be small.  Even if we took a hit in some
> areas of the code, I doubt it would be noticeable, in the overall scheme
> of things.
> 
> The agent-side gets implicit thread-safety and would not need any
> changes/special case code.
> 
> One thing I see in the branched impl is that we don't allow a null for
> childResources, when the custom Set is in place, the remove() method
> ensures an empty Set.  This may be OK but I think Heiko tried hard to
> ensure we don't have empty collections lying around agent-side.
> 
> Anyway, I can go either way, but I'm attracted to the idea of a single
> Set impl for resource children.  Moreover, I think there are other Sets
> that could benefit.  A search of the server code shows several places
> where we copy child sets to avoid ConcurrentModExceptions (like
> childResourceTypes).
> 

IMHO, ResourceTypes are readonly structures on agent side, because any change 
to a resource type (i.e. a plugin) requires a restart of plugin container 
anyway.

Therefore I'd vote for going an extra mile and actually DROP all the results 
of plugin descriptor parsing once we're done with them during startup and have 
immutable impl. of resource types on the agent side, possibly with arrays 
instead of sets/lists/whatever (of course, that depends on how we use those - 
for example we would have to think how to sort the arrays so that we can 
efficiently search in them, etc).

Having immutable data structures gives us 2 things:
* implicit thread-safety
* chance to minimize memory usage by using minimalistic data-structures

Of course, this would not be a simple change because it would basically mean 
"forking" resource/resourceType data structures for the agent-side but I do 
believe it would give us both speed boost and memory footprint decrease.

> On 2/11/2014 3:57 AM, Thomas Segismont wrote:
> > Le 10/02/2014 22:13, Jay Shaughnessy a écrit :
> >> I guess I still don't quite get the details here.   But first one basic
> >> up-front question:  would we want to consider just making
> >> Resource.childResources always be a CopyOnWriteArraySet?  Or is the
> >> thinking that it would incur unnecessary overhead server-side?
> > 
> > On the server side it's not really useful: Resource objects are JPA
> > entities and should (almost) always not be updated concurrently.
> > 
> >> More questions inline:
> >> 
> >> On 2/10/2014 12:12 PM, Thomas Segismont wrote:
> >>> Le 10/02/2014 17:34, Jay Shaughnessy a écrit :
> >>>> A couple of notes/questions on the current branch changes:
> >>>> - I'm not sure I quite understand the changes to Resource, with the
> >>>> customChildResourcesCollection flag. is this really necessary?
> >>> 
> >>> It is necessary otherwise you can't guarantee that all threads are
> >>> working on the same, thread-safe collection.
> >> 
> >> I guess I'm not sure I like the idea of the Resource entity behaving
> >> differently based on the constructor being used.  What exactly makes
> >> this necessary?  Why can't we, agent-side, just ensure the
> >> childResources Set is a CopyOnWriteArraySet, converting when the
> >> information comes from the server, or initializing when created on the
> >> agent?
> > 
> > I wasn't in a very creative mood so I just put the code back in the
> > state it was before commit 5a63da5b :)
> > 
> > Jokes aside, the Resource class constructor was introduced by Ian in
> > commit 3e149ee on 02/21/2012. Have a look at the commit:
> > https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?id=3e149ee
> > The purpose was exactly what we're talking about: "Use a
> > ConcurrentHashMap-based Set for childResources to allow the field to
> > be concurrently accessed safely"
> > 
> > The only difference I added is the use of CopyOnWriteArraySet instead
> > of the default-initialized ConcurrentHashMap-based Set.
> > Default-initialized CHM are very bad from a memory consumption
> > perspective when you expect a few or no elements in the collection and
> > a low number of concurrent threads updating the collection:
> > http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHa
> > shMap.html#ConcurrentHashMap%28%29
> > 
> > "default initial capacity (16), load factor (0.75) and
> > concurrencyLevel (16)"
> > 
> >> Or, as asked above, maybe just settle on the thread-safe type and be
> >> done with it.
> > 
> > That would be a step backwards considering the job done in commit
> > 5a63da5b. But if everyone is happy with a simpler version and we still
> > meet the performance expectations, I'm fine.
> > 
> > Thomas
> > _______________________________________________
> > rhq-devel mailing list
> > rhq-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
> 
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel



More information about the rhq-devel mailing list