Repository navigation
perf(analysis): one walk of the syntax tree per script, indexed by node type and command name - #17
Conversation
fadwen
left a comment
fadwen
left a comment
There was a problem hiding this comment.
Notes on the lines whose reason the diff does not show.
| # name they look for, was most of the time an analysis took. The table is keyed on the AST object | ||
| # itself, so a sub-tree a caller passes gets an index of its own and a tree that goes out of scope | ||
| # takes its index with it. | ||
| $script:IslAstIndex = [System.Runtime.CompilerServices.ConditionalWeakTable[object, object]]::new() |
There was a problem hiding this comment.
A ConditionalWeakTable rather than a hashtable so the index does not keep a tree alive: Test-IntuneScript parses thousands of scripts in a CI run and each tree would otherwise stay in memory with its index for the life of the module.
| [System.Collections.Concurrent.BlockingCollection[System.Management.Automation.Language.Ast]] | ||
| $script:IslAstGetType = [System.Delegate]::CreateDelegate( | ||
| [System.Func[System.Management.Automation.Language.Ast, type]], [object].GetMethod('GetType')) | ||
| $script:IslAstToLookup = ([System.Linq.Enumerable].GetMethods() | |
There was a problem hiding this comment.
Reflection because Windows PowerShell 5.1 has no syntax for calling a generic method with explicit type arguments; the MethodInfo is resolved once at load and reused.
| if ($script:IslAstIndex.TryGetValue($Ast, [ref]$index)) { return $index } | ||
|
|
||
| # FindAll visits the tree in document order, the root first, and that order is what the groups keep | ||
| $collector = $script:IslAstCollectorType::new() |
There was a problem hiding this comment.
BlockingCollection is the first .NET collection whose add method returns bool and keeps insertion order (ConcurrentQueue underneath). HashSet.Add also returns true but its enumeration order is not guaranteed. The ordering was checked against FindAll on both hosts for a 767-node tree.
| if ($index.ByType.ContainsKey($name)) { $index.ByType[$name] } | ||
| } | ||
| # Each type's list is in document order; several types are merged back into it | ||
| if ($TypeName.Count -gt 1) { $nodes = $nodes | Sort-Object -Property { $_.Extent.StartOffset } } |
There was a problem hiding this comment.
Only the three callers that ask for several types pay for the sort; a single type comes straight from its group in document order, which is the common case.
…de type and command name Every rule walked the syntax tree on its own through Find-IslAstNode and Find-IslCommand, and the rules that look for several command names walked it once per name: about forty walks for a script, at about 11 ms each for a 95-line detection. That was most of what an analysis cost. Get-IslAstIndex walks a tree once, on first use, and keeps the nodes grouped by type name and the commands by name for the tree's lifetime, in a ConditionalWeakTable keyed on the AST object. Find-IslAstNode and Find-IslCommand read the index; their parameters, results and result order are unchanged, so no rule changes. The walk and the grouping stay in .NET: FindAll takes a BlockingCollection.TryAdd delegate as its always-true predicate, and Enumerable.ToLookup groups the nodes with Object.GetType as the key selector, because a PowerShell script block called once per node cost more than the walk itself. Measured on a 95-line detection, warm: 91 ms a script against 233 ms, 60 scripts in 5.4 s against 14.0 s, the same 360 findings. The index itself takes 5 ms.
1 participant
Summary
Every rule walked the syntax tree on its own through
Find-IslAstNodeandFind-IslCommand, and the rules that look for several command names walked it once per name: about forty walks for a script, at about 11 ms each for a 95-line detection. That was most of what an analysis cost. The tree is now walked once per script and indexed, and the rules read the index. Findings are unchanged.Test-IntuneScriptIslPowerShell7SyntaxStacked on #16 (base branch); the diff is this change only. Merge #16 first.
Changes
Private/Find-IslAstNode.ps1.Get-IslAstIndexwalks a tree once, on first use, and keeps the nodes grouped by type name and the commands by name for the tree's lifetime, in aConditionalWeakTablekeyed on the AST object.Find-IslAstNodeandFind-IslCommandread it; their parameters, results and result order are unchanged, so no rule changes. The walk and the grouping stay in .NET:FindAlltakesBlockingCollection.TryAddas its always-true predicate, andEnumerable.ToLookupgroups the nodes withObject.GetTypeas the key selector, because a script block called once per node cost more than the walk itself (8 ms for the walk, 21 ms for a grouping loop in PowerShell; 5 ms for both in .NET).FindAllfinds, inFindAll's order; several command names come back in document order whatever the order asked for.Verification
Test-IntuneScriptandRepair-IntuneScripttests pass on Windows PowerShell 5.1 (152 of 152). PSScriptAnalyzer (Error and Warning) is clean; no line over 115 characters.FindAllon both hosts for the benchmark script: identical, 767 nodes.