Skip to content

Avoid creating instances of the generated delegate helper class every time ReplaceOperatorImpl is called #14128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iSazonov merged 1 commit into from
Nov 21, 2020
Merged

Avoid creating instances of the generated delegate helper class every time ReplaceOperatorImpl is called #14128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions src/System.Management.Automation/engine/lang/parserutils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,18 +1011,7 @@ private static object ReplaceOperatorImpl(ExecutionContext context, string input
return regex.Replace(input, replacementString);

case ScriptBlock sb:
MatchEvaluator me = match =>
{
var result = sb.DoInvokeReturnAsIs(
useLocalScope: false, /* Use current scope to be consistent with 'ForEach/Where-Object {}' and 'collection.ForEach{}/Where{}' */
errorHandlingBehavior: ScriptBlock.ErrorHandlingBehavior.WriteToCurrentErrorPipe,
dollarUnder: match,
input: AutomationNull.Value,
scriptThis: AutomationNull.Value,
args: Array.Empty<object>());

return PSObject.ToStringParser(context, result);
};
MatchEvaluator me = GetMatchEvaluator(context, sb);
return regex.Replace(input, me);

case object val when LanguagePrimitives.TryConvertTo(val, out MatchEvaluator matchEvaluator):
Expand All @@ -1032,6 +1021,24 @@ private static object ReplaceOperatorImpl(ExecutionContext context, string input
string replacement = PSObject.ToStringParser(context, substitute);
return regex.Replace(input, replacement);
}

// 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)
Copy link
Collaborator

@iSazonov iSazonov Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@xtqqczze xtqqczze Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the compiler not cache the delegate?

Copy link
Member Author

@daxian-dbw daxian-dbw Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

@daxian-dbw daxian-dbw Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

@iSazonov iSazonov Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iSazonov iSazonov Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@daxian-dbw daxian-dbw Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iSazonov iSazonov Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@daxian-dbw daxian-dbw Nov 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. the script uses many different patterns
  2. the substitute strings are known, not requiring calculation based on $match. (be noted on the $AScriptBlock part)

The repro script uses -replace in a way that doesn't allocate delegate at all.

Copy link
Collaborator

@iSazonov iSazonov Nov 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am definitely in another dimension :-)

I implemented my thoughts in #14221.

{
return match =>
{
var result = sb.DoInvokeReturnAsIs(
useLocalScope: false, /* Use current scope to be consistent with 'ForEach/Where-Object {}' and 'collection.ForEach{}/Where{}' */
errorHandlingBehavior: ScriptBlock.ErrorHandlingBehavior.WriteToCurrentErrorPipe,
dollarUnder: match,
input: AutomationNull.Value,
scriptThis: AutomationNull.Value,
args: Array.Empty<object>());

return PSObject.ToStringParser(context, result);
};
}
}

/// <summary>
Expand Down