CPU Limit Exceeded because of nested for loops in apex [closed]

Question

Below code is not working when I add the 1000 active users as per the requirement and giving the CPU exceed limit / Collection size 1,189 exceeds maximum size of 1,000.

I want to change the code to avoid the error.

public void buildQueueInfo(List queues, Map<Id, List> queueIdToMembersMap, List allUsers, Map<Id, User> userMap) {

Map<Id, Group> queueMap = new Map<Id, Group>(queues);
// Remove queues that have no users.
queueMap.keySet().retainAll(queueIdToMembersMap.keySet());
Set<Id> usergroupIds = new Set<Id>();
for(List<GroupMember> queueMembers: queueIdToMembersMap.values()) {
    for(GroupMember gm: queueMembers) {
        usergroupIds.add(gm.UserOrGroupId);
    }
}
Map<Id, User> allUsersMap = new Map<Id, User>(allUsers);
allUsersMap.keySet().removeAll(userGroupIds);
List<User> nonUsers = allUsersMap.values();
for(Group queue : queueMap.values()) {
    populateQueueInfo(queue, queueIdToMembersMap.get(queue.Id), nonUsers, userMap);   
}

}

public void populateQueueInfo(Group theQueue, List currentMembers, List nonMembers, Map<Id, User> userMap) { queueList.put(theQueue.Id, new QueueInfoWrapper(theQueue, currentMembers, nonMembers, userMap)); }

Answer 1

First, something to clear up:

//To Reduce heap size
queues=null;

No, this has no impact on heap size, as you're only holding a reference to an object, not an entire copy of the object itself. See this answer I wrote on that topic that may be illuminating.

To get to the meat of the problem, we have a few problems in this code. Mostly that (a) you're doing more work than you need to by not filtering the list of queues that are not in the queueIdToMembersMap before processing, and (b) you're not utilizing some library functions that you can use to improve performance drastically.

Here, I can quickly create a Map, and filter out all Queues that won't need any processing, as they have no group members:

Map<Id, Group> queueMap = new Map<Id, Group>(queues);
// Remove queues that have no users.
queueMap.keySet().retainAll(queueIdToMembersMap.keySet());

This is a lot better than:

for(GroupMember queue: queues) {
  if(queueIdToMembersMap.containsKey(queue.Id)) {

Once we have this new collection, we can get the UserOrGroupId values more efficiently:

for(Group queue : queuesMap.values()) {
    for(GroupMember gm : queueIdToMembersMap.get(queue.Id)){ 
        usergroupIds.add(gm.UserOrGroupId);  
    }
}

Similarly, your entire second nested loop can be trivialized to:

Map<Id, User> allUsersMap = new Map<Id, User>(allUsers);
allUsersMap.keySet().removeAll(userGroupIds);
List<User> nonUsers = allUsersMap.values();

Or, we can even optimize the first loop further:

for(List<GroupMember> queueMembers: queueIdToMembersMap.values()) {
    for(GroupMember gm: queueMembers) {
        usergroupIds.add(gm.UserOrGroupId);  
    }
}

This results in final code that looks something like:

public void buildQueueInfo(List<Group> queues, Map<Id, List<GroupMember>> queueIdToMembersMap, List<User> allUsers, Map<Id, User> userMap) {
    Map<Id, Group> queueMap = new Map<Id, Group>(queues);
    // Remove queues that have no users.
    queueMap.keySet().retainAll(queueIdToMembersMap.keySet());
    Set<Id> usergroupIds = new Set<Id>();
    for(List<GroupMember> queueMembers: queueIdToMembersMap.values()) {
        for(GroupMember gm: queueMembers) {
            usergroupIds.add(gm.UserOrGroupId);
        }
    }
    Map<Id, User> allUsersMap = new Map<Id, User>(allUsers);
    allUsersMap.keySet().removeAll(userGroupIds);
    List<User> nonUsers = allUsersMap.values();
    for(Group queue : queueMap.values()) {
        populateQueueInfo(queue, queueIdToMembersMap.get(queue.Id), nonUsers, userMap);   
    }
}

---

I've seen your comment about the "1,428 exeeds maximum size of 1,000." This problem has nothing to do with heap limits or CPU limits, but rather that you can't display lists of larger than 1,000 items in Visualforce. If you don't need DML operations, set the page to read-only mode:

<apex:page controller="myController" readOnly="true" ...

Which relaxes this limit from 1,000 to 10,000, but restricts DML operations.

If you need DML operations, you need to create a nested list of results and iterate over chunks of data. I thought I'd written an answer of this type previously, but I couldn't find it, so I present this one to you.

The general idea is that you create a top-level collection, then nest it. I'm copy-pasting the original source from that answer for you:

public class MultiExample
{
  public List<MultiWrapper> wraps{get; set;}

  public MultiExample()
  {
    wraps = new List<MultiWrapper>();
    wraps.add(wrap);
    MultiWrapper wrap = new MultiWrapper();
    for (Account acc: [SELECT Id FROM Account LIMIT 3000])
    {
      if (wrap.size() == 1000)
      {
        wrap = new MultiWrapper();
        wraps.add(wrap);
      }
      wrap.addAcc(acc);
    }
  }

  public class MultiWrapper
  {
    public List<Account> accs{get; set;}

    public MultiWrapper()
    {
      accs = new List<Account>();
    }

    public void addAcc(Account acc)
    {
      accs.add(acc);
    }
  }
}

---

<apex:repeat value="{!wraps}" var="wrap">
  <apex:pageBlockTable value="{!wrap.accs}" var="{!acc}">
    <apex:column>
      <apex:outputField value="{!acc.Id}" />
    </apex:column>
  </apex:pageBlockTable>
</apex:repeat>

As you can see, a top-level wrapper breaks up the items into groups of 1,000, thus providing a nested loop that supports up to 1,000,000 rows, of which you'll reach other limits far before you break this one.

Answer 2

Finding all of the users who are not members of each group is inherently somewhat expensive, but you're doing it in a way that is very inefficient. You're also showing us only portions of your code, so the remainder may be heavily impacting this transaction-wide limit as well.

The core problem shown here is that you are iterating triply, for something like an O(N^3) complexity.

        for (Group queue : queues) {
            for (User u : allUsers) {
                    for (GroupMember gm : gmList) {
                    }
                }
            }

One clear optimization would be to preprocess each list of GroupMember records into a Set<Id>. Then, you can check whether each User is a member with a constant-time call to Set#contains() instead of a third nested for loop.

It's also possible that building a Set<Id> of Users (once, outside the loop) and using Set intersection to build the list of users that aren't members of each group could further increase performance, rather than iterating. You'd need to benchmark to determine if that were successful.

That said, I suspect that this whole design has a lot of built-in inefficiency that I don't see enough information to comment on. The optimization above might or might not be sufficient to make it run within the transaction limit.