0

I know this is pulling quite a bit of data, but at present it's capping my memory consumption when I run it on my local machine. The good news is, it's returning the output that I need. Can someone help me with performance optimization? So far, I haven't done much for fear of messing up a script that returns my desired output. Thanks in advance for any suggestions.

#// Start of script 
#// Get year and month for csv export file 
$DateTime = Get-Date -f "yyyy-MM" 

#// Set CSV file name 
$CSVFile = "C:\Temp\AD_Groups"+$DateTime+".csv" 

#// Create emy array for CSV data 
$CSVOutput = @() 

Measure-Command {

    #// Get all AD groups in the domain 
    $ADGroups = Get-ADGroup -Filter "GroupScope -ne 'DomainLocal' -AND GroupCategory -eq 'Security' -AND Member -like '*'" -SearchBase "OU=SHS, DC=shs, DC=net" -Properties Member #-ResultSetSize 1000 Name -like '*''s*' -AND 

    #// Set progress bar variables 
    $i=0 
    $tot = $ADGroups.count 

    foreach ($ADGroup in $ADGroups) {
        #// Set up progress bar 
        $i++ 
        $status = "{0:N0}" -f ($i / $tot * 100) 
        Write-Progress -Activity "Exporting AD Groups" -status "Processing Group $i of $tot : $status% Completed" -PercentComplete ($i / $tot * 100) 

        #// Ensure Members variable is empty 
        $Members = ""

        #// Get group members which are also groups and add to string 
        $MembersArr = Get-ADGroup $ADGroup.DistinguishedName -Properties Member | Select-Object -ExpandProperty Member

        if ($MembersArr) {

            foreach ($Member in $MembersArr) {

                $ADObj = Get-ADObject -filter {DistinguishedName -eq $Member}
                #// Initialize regex variable
                $matches = "" 

                if ($ADObj.ObjectClass -eq "user") {
                    $UserObj = Get-ADObject -filter {DistinguishedName -eq $Member}

                    $match = $UserObj -match '\([a-zA-Z0-9]+\)'
                    $empid=$matches[0] -replace ".*\(","" -replace "\)",""

                    if ($UserObj.Enabled -eq $False) { 
                        continue
                    }

                    $Members = $empid

                }

                # Check for null members to avoid error for empty groups 
                if ([string]::IsNullOrEmpty($Members)) { 
                    continue        
                }

                $HashTab = [ordered]@{ 
                        "GroupName" = $ADGroup.Name -replace "'s", "''s"
                        "GroupCategory" = $ADGroup.GroupCategory 
                        "GroupScope" = $ADGroup.GroupScope 
                        "MemberID" = if([string]::IsNullOrEmpty($empid)){""}
                                        else{$empid}
                }

                #// Add hash table to CSV data array 
                $CSVOutput += New-Object PSObject -Property $HashTab
            }
        }

        #// Export to CSV files 
        $CSVOutput | Sort-Object Name, Member | Export-Csv $CSVFile -NoTypeInformation
    }
}
Gabriel Luci
  • 38,328
  • 4
  • 55
  • 84
Ryan
  • 19
  • 9
  • [Try to avoid using the increase assignment operator (`+=`) to create a collection](https://stackoverflow.com/questions/60708578/why-should-i-avoid-using-the-increase-assignment-operator-to-create-a-colle), use the PowerShell pipeline instead: `$CSVOutput = foreach ($ADGroup in $ADGroups) { ... }` – iRon Apr 13 '20 at 08:54
  • Hi Ron, Thanks for the tip. I'm trying your suggestions buy not sure I have the syntax correct. What should go in the { } here? I think I'm still inside the foreach ($ADGroup...) loop, so do I need to rewrite this differently? – Ryan Apr 13 '20 at 19:03
  • Although you might make a few other improvements, I guess that the big win is in avoiding this assignment operator, the only thing that is required for that is removing `$CSVOutput +=` and leaving the **`New-Object PSObject -Property $HashTab`** this will drop the object on the pipeline where you eventually assign it to the variable as shown in my early comment. Note that is it will be even better (in terms of memory consumption) to pass it directly on to the next cmdlet: `... | Sort-Object ... | ...`. Unfortunately a cmdlet as `Sort-Object` requires the whole stream and therefore stalls it. – iRon Apr 14 '20 at 06:04
  • I have added some correction instructions to the [Try to avoid using the increase assignment operator (`+=`) to create a collection](https://stackoverflow.com/questions/60708578/why-should-i-avoid-using-the-increase-assignment-operator-to-create-a-colle) answer. – iRon Apr 17 '20 at 16:09

1 Answers1

2

I've experienced this too with code that loops through thousands of accounts. The problem is that the garbage collector doesn't have time during the loop to clean up, since your code is constantly doing something. In .NET, I'd call .Dispose() manually to make sure stuff is cleaned up, but here you can't.

You can try calling [System.GC]::Collect() after you assign each variable in the loop. For example, after $MembersArr = and after $ADObj = to (hopefully) make it deallocate the memory used for the previous value.

Also, I think that $UserObj = Get-ADObject... line should be calling Get-ADUser, not Get-ADObject. As it is, $UserObj.Enabled will never have a value and your continue will never be hit.

But you can save yourself the use of Get-ADUser entirely by asking for the userAccountControl value in Get-ADObject and using that to determine if the user is disabled. For example:

$ADObj = Get-ADObject -filter {DistinguishedName -eq $Member} -Properties userAccountControl

# Clean up the old $ADObj value
[System.GC]::Collect()

#// Initialize regex variable
$matches = "" 

if ($ADObj.ObjectClass -eq "user") {
    $match = $ADObj -match '\([a-zA-Z0-9]+\)'
    $empid=$matches[0] -replace ".*\(","" -replace "\)",""

    if ($ADObj.userAccountControl -band 2) { 
        continue
    }

    $Members = $empid
}

The $ADObj.userAccountControl -band 2 condition checks is a bitwise AND comparison to check if the second bit of the userAccountControl value is set, which means that the account is disabled.

Gabriel Luci
  • 38,328
  • 4
  • 55
  • 84
  • Unfortunately, no luck on performance improvement. However, I did make the change to the active user criteria per your recommendation above and that seems to be running without a hitch. Thank you. – Ryan Apr 13 '20 at 03:05