Proposed refactor with ~90% performance improvement on Export-TargetResource (for affected resources) #5615
Comments
|
I love it. I've been thinking about improvements in the speed as well because I wasn't very satisfied with it in my environment too, and your changes look great to me. I'll take a look at the telemetry function and why it could take so long. |
|
The unnecessary calls to |
|
@niwamo Quick question: What's the script you ran to measure the command execution time? Did you execute it in an elevated PowerShell session? I might have found something in the telemetry function which delays the export if you don't have admin privileges. |
|
I measured execution time using the built-in 'Export took {x seconds}" message, as well as by using System.Diagnostics.Stopwatch, i.e.: >> $sw = [System.Diagnostics.Stopwatch]::StartNew()
>> # export command
>> $sw.Elapsed.ToString()The final output looked like:
I believe I executed in a non-elevated session, though I would need to re-run tests to be 100% sure. |
|
Sorry for the delay in response, have been a little busy this week. I love these improvements, really great work! You are correct that some resources cache the objects in the Export method and then are passed as a script variable to the Get method. This is implemented in resource for which the export usually is very big and therefore time consuming, like AADUser, AADGroup, AADApplication, EXOMailbox, etc. Those resources can have lots and lots of objects and retrieving them individually doesn't make sense, so that is why we implemented this caching method. Your changes make total sense, just one question: In some resources we still have to call individual cmdlet, like in AADUser for example we have to call Get-MgUserLicenseDetail for each user: |
|
If If The only possible issue is if an additional command is called from within |
|
@niwamo If you executed it in a standard shell, there might be a performance hit by |
|
PR #5629 fixes the following 61 resources:
|
Export Performance Improvement - Addresses #5615
|
PR #5641 fixes the following 114 Intune resources:
|
Description of the issue
The interaction between
Export-TargetResourceandGet-TargetResourceis highly inefficient for multiple-instance resources (e.g. AADUser or other resources where multiple instances are expected). In pseudo-code:There are three significant performance impacts:
New-M365DSCConnectionfor every iteration ofGet-TargetResource. While it avoids reconnecting unnecessarily, it still results in thousands of extraneous + identical API calls forGet-AcceptedDomainwhen it is invoked for the EXO workload (and I have verified this with several Fiddler traces). Even if that could be fixed, there's no need to call the function, and invoking it thousands of times unnecessarily should be avoided.Get-TargetResourceis enormously impactful to the performance of this module. Without any other changes, I determined that the same export took 8,925 seconds with telemetry enabled and 1,400 seconds without. In other words, telemetry - just telemetry - accounted for ~85% of my export's runtime. I would by no means suggest removing telemetry altogether, but it does seem highly redundant to log the invocation ofExport-TargetResourceand each invocation ofGet-TargetResourcefrom within the export. If there is a desire to track the number ofGet-TargetResourcecalls or the average function time, I would suggest tracking these items within the export function and sending that data as a single telemetry call. There's just no good reason for anyone to keep telemetry enabled when it causes an export to run (literally) 8x slower; reducing the telemetry without reducing the useful information captured seems like a win for everyone.Get-TargetResourcefunction actually re-fetches the same instance information from the same API asExport-TargetResource. This should clearly be avoided, and has been for many modules:Export-TargetResourcedeclares$Script:exportedInstancesandGet-TargetResourceschecks if this variable exists, then filters the array to find the right instance. This is better but can be further improved. Why not declare$Script:exportedInstanceinside of the export'sforeachloop and avoid the filtering inside ofGet-TargetResource? For resources with large numbers of instances, this makes a meaningful impact.Altogether, my code changes the relevant section of a typical
Get-TargetResourcefrom this:to this:
I have made these changes locally for 61 resources. In my regression testing I found no "breakage" and actually found that this model fixed two bugs I was previously unaware of. (In both cases, the relevant API - strangely - returned more properties for bulk-retrieved items than individually retrieved items, so leveraging the
exportedInstanceresulted in keeping previously missing properties.My performance test results with identical configurations (runtime, measured in seconds):
Since this would be a large PR, I am hoping to get initial feedback from the maintainers before proceeding.
@NikCharlebois @ykuijs
Microsoft 365 DSC Version
DEV / 1.24.1218.1
Which workloads are affected
Azure Active Directory (Entra ID), Exchange Online, Security & Compliance Center, SharePoint Online, Office 365 Admin
The DSC configuration
Verbose logs showing the problem
Environment Information + PowerShell Version
The text was updated successfully, but these errors were encountered: