Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/76#discussion_r15054170
--- Diff:
core/src/main/java/brooklyn/management/internal/BrooklynGarbageCollector.java
---
@@ -199,9 +199,25 @@ public boolean shouldDeleteTask(Task<?> task) {
/**
* Deletes old tasks. The age/number of tasks to keep is controlled by
fields like
- * {@link #maxTasksPerTag} and {@link #maxTaskAge}.
+ * {@link #maxTasksPerTag} and {@link #maxTaskAge}.
*/
private void gcTasks() {
+ // TODO Must be careful with memory usage here : saw an OOME when
copying the tasks set to sort it.
+ // (This happened when there were a *lot* of tasks (e.g. 100,000s)
due to a crazy trigger for
+ // effector calls.)
+ //
+ // At that point we have potentially three copies of the list
in-memory:
+ // - original in ExecutionManager
+ // - copy returned by getTasksWithTag(tag)
+ // - copy for sortedTasks (but at least now that only contains
done tasks)
+ //
+ // It's hard to avoid fewer copies: getTasksWithTag(tag) should
continue to return copy rather than
+ // mutable underlying list; and if we're going to sort then we
need to take a copy.
+ //
+ // An option is for getTasksWithTag(tag) to return an ArrayList
rather than a LinkedHashSet. That
+ // is a far more memory efficient data structure (e.g. 4 bytes
overhead per object rather than
+ // 32 bytes overhead per object for HashSet).
--- End diff --
Saving a couple of dozen bytes per item in the data structure sounds like
unnecessary micro-optimisation! I expect the task objects themselves easily
dwarf the savings. The extra few meg of overhead *might* have tipped the
balance in your OOME case, but the real problem was the crazy-high number of
queued tasks to begin with.
Thoughts:
* Iterating over the result from `getTasksWithTag()` absolutely must be
possible without raising `ConcurrentModificationException`s; a defensive copy
is probably necessary here, assuming the live collection is frequently changing
(i.e. so `CopyOnWriteArrayList` would always degenerate to making the copy).
* If other callers similarly sort the returned tasks, or if they don't
care, the `ExecutionManager` could simply store them in the commonly-desired
order, avoiding the explicit sorting step (and associated copy). `TreeSet`'s
log(n) performance might kill that idea thoguh.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
