Community
Participate
Working Groups
Build ID: I20070323-1616 I'd like to propose an optimization to MutableActivityManager#setEnabledActivityIds(). In the method MutableActivityManager#updateIdentifier(Identifier identifier) change the following: boolean matchesAtLeastOneEnabled = false; boolean matchesAtLeastOneDisabled = false; for (Iterator iterator = definedActivityIds.iterator(); iterator.hasNext();) { ... } To: boolean matchesAtLeastOneEnabled = false; boolean matchesAtLeastOneDisabled = false; for (Iterator iterator = definedActivityIds.iterator(); !matchesAtLeastOneEnabled && iterator.hasNext();) { ... } Since the largest chunk of time when setting activities is taken up within this for loop by checking the activity patterns, this could skip on average a large number of pattern matches.
The proposed change looks good to me.
This wont work. While IIdentifier.isEnabled() will continue working as expected this change causes IIdentifier.getActivityIds() to only contain up to the first enabled activity and nothing more. It might be possible to change Identifier such that the activities it contains are computed lazily as needed and this shortcut simply populates the enabled field but I dont have time to investigate that at the moment. If you have a patch that could help with this (that still satisfies all of the tests in ActivitiesTestSuite) I'll be happy to look at it.
Ah yes, my scenario only involved IIdentifier.isEnabled(). I'll look into a possible way of lazily computing the activity Ids, but at first glance it doesn't look feasible in this situation. Too bad, because there was a significant speed increase for setting enabled activities for my scenario. Too good to be true perhaps :)
It wouldn't be easy but all is not lost. We could calculate isEnabled as you suggest. If that calculation results in the full calculation of the activity set we just set it as is. If not, we hit a method on Identifier that signifies that the set should be calculated when we ask for it. The same pattern would also need to be added to the IdentifierEvent as well. A callback into the activityManager that calls updateIdentifier(Identifier) with a boolean attribute that specifies a full calculation, for instance, could be used.
In method MutableActivityManager#setEnabledActivityIds() there is a check if !this.enabledActivityIds.equals(enabledActivityIds) is true then the activity manager is said to have changed. Otherwise nothing else is done in this method. When this check does pass, it proceeds to update all the activities in the manager. Later on when updateIdentifier is called, a loop through all the defined activity Ids is made in order to update the set of activityIds to set back into the identifier. I can understand why a loop through all the activity Ids is made here because in setEnabledActivityIds() all the activities in the manager were updated. What I'm wondering is whether or not it is necessary to process all defined activity Ids, or whether processing the delta of the enabled activity Ids is sufficient? Back to where the if statement !this.enabledActivityIds.equals(enabledActivityIds) is. Here we're only comparing the Ids of the activities, and we're not interested in whether or not any of the enabled activities have had their requirement bindings or pattern bindings changed. So if the same set of activities is set to be enabled, but somehow the requirement bindings or pattern bindings had changed for any of the activities, still nothing will change. But if the new set of enabled activities is at all different from the previous set, then we are interested again in re-computing enablement of every activityId, and not just those that have changed. I did a quick change to the code, and by using a delta, all the junits passed. But I can clearly see how the flow has changed slightly. Can you tell me whether a delta approach here would work? Or is it really necessary to re-compute enablement for all activities? Also, am I right in saying that the reason all activities are processed is because their requiement or pattern bindings could have changed? (hope that this all makes sense :)
Created attachment 65777 [details] patch for MutableActivityManager This patch contains the changes I made which corresponds to my last comment above.
Tod, +1?
+1
Christian: I'm going to apply this. Could you please be on the lookout for any errant activity behavior in 3.3 that could be caused by this? Thanks for the patch!
Sure, will do!
Verified that the code has been tagged in I20070516-0010.
