Avoid creating instances of the generated delegate helper class every time ReplaceOperatorImpl is called
#14128
Conversation
|
|
||
| // Local helper function to avoid creating an instance of the generated delegate helper class | ||
| // every time 'ReplaceOperatorImpl' is invoked. | ||
| static MatchEvaluator GetMatchEvaluator(ExecutionContext context, ScriptBlock sb) |
There was a problem hiding this comment.
We could put the method in a helper static class and save the result in an private static. So we will create the delegate only once.
I found the pattern in https://www.stevejgordon.co.uk/high-performance-logging-in-net-core.
There was a problem hiding this comment.
Does the compiler not cache the delegate?
There was a problem hiding this comment.
@iSazonov The delegate body for MatchEvaluator requires 2 closed-over variables from its lexically-enclosing method sb and context -- they are not passed to the delegate as paramters, and thus a generated helper class is needed.
The article you referred to is a good read, but the key perf improvement there is avoid processing the message template for every call to each of the log methods.
The key improvement here, vs. Standard logging, is that the template for the log message now only needs to be parsed once when this message is defined. In standard logging, that occurs for each call.
There was a problem hiding this comment.
@xtqqczze The delegate here cannot be cached becuase it depends on the scirpt block sb and also the execution context context. These 2 closed-over variables make caching not useful in most cases.
There was a problem hiding this comment.
The delegate body for MatchEvaluator requires 2 closed-over variables from its lexically-enclosing method sb and context -- they are not passed to the delegate as paramters, and thus a generated helper class is needed.
Can't we pass them as parameters?
I guess the pattern I mentioned excludes delegate allocations at all.
There was a problem hiding this comment.
We seem to be talking about different things. :-)
One code path where the ReplaceOperatorImpl() is called is a while cycle. There the same context and the same scriptblock but we create the delegate again and again - what if the lval is a large list?
There was a problem hiding this comment.
I see what you were talking about now :)
I'm not trying to optimize for all scenarios, but the most common scenario.
To optimize for the scenario you are talking about, you will need a delegate cache with both the scirpt block and the context together as the key. But as I said previously, the cache will not be useful for most cases. You need to think about how often would $ABigList -replace "pattern", $AScriptBlock be used. Holding the cache indefinitely would be a waste of memory; adding logic to clear the cache smartly would be extra complexity. Either way, I don't think the value would be worth the effort.
There was a problem hiding this comment.
Either way, I don't think the value would be worth the effort.
After bad experience, I avoid such scripts. I suppose others do the same. But this does not mean that it cannot be improved. For example, the original script from the question could be rewritten using this pattern and if optimized, it could possibly run even faster than the original script.
I will open a tracking issue for the optimization.
There was a problem hiding this comment.
For example, the original script from the question could be rewritten using this pattern and if optimized could possibly run even faster than the original script.
No, the original repro scirpt cannot be rewritten with $ABigList -replace "pattern", $AScriptBlock, because
- the script uses many different patterns
- the substitute strings are known, not requiring calculation based on
$match. (be noted on the$AScriptBlockpart)
The repro script uses -replace in a way that doesn't allocate delegate at all.
There was a problem hiding this comment.
I am definitely in another dimension :-)
I implemented my thoughts in #14221.
|
🎉 Handy links: |
PR Summary
Unnecessary Allocation
In
ReplaceOperatorImpl, a lambda expression is used in one of the switch case. But in the generated IL code, an instance of the generated delegate helper class is created at the beginning of the function, so even if the that specific switch case is never hit during a script execution, an instance of the helper class will be created for every-replacecall.For the particular repro in #14087, 76 MB of the delegate helper class instances got allocated in @iSazonov's profiling, which should all be avoided.
Here is the IL code generated for
ReplaceOperatorImpl:The Fix
The change in this PR moves the delegate to a local static method, so the creation of the delegate helper class instance is done in the helper method instead, and therefore that happens only if the particular swtich case is hit.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.