Skip to content
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

Jump to bottom

Add additional filter capabilities to dotnet-pgo tool. #89853

Merged
merged 6 commits into from
Aug 10, 2023
Merged

Add additional filter capabilities to dotnet-pgo tool. #89853

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
72df5ed
Add additional filter capabilities to dotnet-pgo tool.
lateralusX Aug 2, 2023
817e82a
Review feedback.
lateralusX Aug 4, 2023
930a574
Address feedback
mdh1418 Aug 8, 2023
8b7693f
Align spacing for R2RLoad and jitStart event processing
mdh1418 Aug 8, 2023
f95dc3e
Address more feedback
mdh1418 Aug 8, 2023
c6affef
Select largest range to bound events
mdh1418 Aug 9, 2023
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
12 changes: 12 additions & 0 deletions src/coreclr/tools/dotnet-pgo/PgoRootCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ internal sealed class PgoRootCommand : CliRootCommand
new("--exclude-events-before") { DefaultValueFactory = _ => Double.MinValue, Description = "Exclude data from events before specified time. Time is specified as milliseconds from the start of the trace" };
public CliOption<double> ExcludeEventsAfter { get; } =
new("--exclude-events-after") { DefaultValueFactory = _ => Double.MaxValue, Description = "Exclude data from events after specified time. Time is specified as milliseconds from the start of the trace" };
public CliOption<string> ExcludeEventsBeforeJittingMethod { get; } =
new("--exclude-events-before-jitting-method") { DefaultValueFactory = _ => string.Empty, Description = "Exclude data from events before observing a specific method getting jitted. Method is matched using a regular expression" };
public CliOption<string> ExcludeEventsAfterJittingMethod { get; } =
new("--exclude-events-after-jitting-method") { DefaultValueFactory = _ => string.Empty, Description = "Exclude data from events after observing a specific method getting jitted. Method is matched using a regular expression" };
public CliOption<string> IncludeMethods { get; } =
new("--include-methods") { DefaultValueFactory = _ => string.Empty, Description = "Include methods matching regular expression" };
public CliOption<string> ExcludeMethods { get; } =
new("--exclude-methods") { DefaultValueFactory = _ => string.Empty, Description = "Exclude methods matching regular expression" };
public CliOption<bool> Compressed { get; } =
new("--compressed") { DefaultValueFactory = _ => true, Description = "Generate compressed mibc" };
public CliOption<int> DumpWorstOverlapGraphs { get; } =
Expand Down Expand Up @@ -99,6 +107,10 @@ public PgoRootCommand(string[] args) : base(".NET PGO Tool")
ClrInstanceId,
ExcludeEventsBefore,
ExcludeEventsAfter,
ExcludeEventsBeforeJittingMethod,
ExcludeEventsAfterJittingMethod,
IncludeMethods,
ExcludeMethods,
AutomaticReferences,
_verbosity,
Compressed,
Expand Down
106 changes: 102 additions & 4 deletions src/coreclr/tools/dotnet-pgo/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
using System.Text;
using System.Text.Json;
using System.Text.Encodings.Web;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

using Microsoft.Diagnostics.Tools.Pgo;
Expand Down Expand Up @@ -1343,6 +1344,12 @@ private int InnerProcessTraceFileMain()

double excludeEventsBefore = Get(_command.ExcludeEventsBefore);
double excludeEventsAfter = Get(_command.ExcludeEventsAfter);
Regex excludeEventsBeforeJittingMethod = !string.IsNullOrEmpty(Get(_command.ExcludeEventsBeforeJittingMethod)) ? new Regex(Get(_command.ExcludeEventsBeforeJittingMethod)) : null;
Regex excludeEventsAfterJittingMethod = !string.IsNullOrEmpty(Get(_command.ExcludeEventsAfterJittingMethod)) ? new Regex(Get(_command.ExcludeEventsAfterJittingMethod)) : null;
Regex includeMethods = !string.IsNullOrEmpty(Get(_command.IncludeMethods)) ? new Regex(Get(_command.IncludeMethods)) : null;
Regex excludeMethods = !string.IsNullOrEmpty(Get(_command.ExcludeMethods)) ? new Regex(Get(_command.ExcludeMethods)) : null;

// Find all the R2RLoad events.
if (_command.ProcessR2REvents)
{
foreach (var e in p.EventsInProcess.ByEventType<R2RGetEntryPointTraceData>())
Expand All @@ -1351,6 +1358,7 @@ private int InnerProcessTraceFileMain()
string retArg = e.MethodSignature.Substring(0, parenIndex);
string paramsArgs = e.MethodSignature.Substring(parenIndex);
string methodNameFromEventDirectly = retArg + e.MethodNamespace + "." + e.MethodName + paramsArgs;

if (e.ClrInstanceID != clrInstanceId)
{
if (!_command.Warnings)
Expand Down Expand Up @@ -1382,8 +1390,75 @@ private int InnerProcessTraceFileMain()
continue;
}

if ((e.TimeStampRelativeMSec >= excludeEventsBefore) && (e.TimeStampRelativeMSec <= excludeEventsAfter))
methodsToAttemptToPrepare.Add((int)e.EventIndex, new ProcessedMethodData(e.TimeStampRelativeMSec, method, "R2RLoad"));
if (e.TimeStampRelativeMSec < excludeEventsBefore)
{
continue;
}

if (e.TimeStampRelativeMSec > excludeEventsAfter)
{
break;
}

if (includeMethods != null || excludeMethods != null)
{
string methodAsString = method.ToString();
Copy link
Member

@jakobbotsch jakobbotsch Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This string is generally meant for debugging, it goes through DebugNameFormatter which will produce method names that users may not be familiar with (e.g. I believe types will be module prefixed in the format "[module name]type name"). Would it be better to introduce a custom formatter, reuse the TypeString formatter, or to reuse the methodNameFromEventDirectly that we are already using to print the warnings above?

Copy link
Member

Choose a reason for hiding this comment

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

Is there expectation for users to be more specific than to use a regular expression for specifying which method to exclude events after? I was trying to get a local test loop going on Android to see the structural differences between a MethodDesc.ToString() and the methodNameFromEventDirectly but its taking me some time to get it working locally.

I can switch it to methodNameFromEventDirectly as that should still be able to match a regex provided by the user. Is it usually easier to match the methodNameFromEventDirectly form than to match the MethodDesc.ToString() form even though it is passed through the DebugNameFormatter?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that the way DebugNameFormatter formats names is an internal detail of the managed type system, so I do not think user-facing tools should be using this for any purpose but diagnostics. We do not want situations where we change this detail in the future and break something that took a dependency on the exact way that namespaces or class names are formatted by this formatter.

I looked again and realize methodNameFromEventDirectly is not actually a field that comes directly from the event, but a concatenation of various fields. I think a simpler concatenation like e.MethodNamespace + "." + e.MethodName would be better to do the matching against. I think MethodNamespace includes the class name, but you may want to double check.

Copy link
Member

@mdh1418 mdh1418 Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this also suggests that e.MethodNamespace will also contain the class name https://github.com/microsoft/perfview/blob/766a08ca8373d273d55da8b07147e7068e4d5f58/src/TraceEvent/Samples/21_ObserveJitEvents.cs#L126-L132. (haven't found the source code for such fields/classes yet)

Maybe methodNameFromEventDirectly would allow for more flexibility as it will allow users to also specify between different overloaded forms of the same method? It would also more closely align with a GetMethodName in Perfview https://github.com/microsoft/perfview/blob/766a08ca8373d273d55da8b07147e7068e4d5f58/src/TraceEvent/Computers/TraceManagedProcess.cs#L4170

Copy link
Member

Choose a reason for hiding this comment

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

I like that idea, but then we should match the PerfView name exactly, methodNameFromEventDirectly does not match that today. We should also update documentation of the CLI option to refer to the fact that matching works against names that are formatted the same as PerfView (I would also add a note that this includes parameters, so that users can realize they cannot use e.g. *MyClass* to match all methods defined on that class). Do you know where exactly this name is displayed in the PerfView UI?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too familiar with the user scenarios, so I dont know if it is likely that one would want to only include methods with a certain set of parameters and not include the others. Maybe we can start out with having the most customizable option first (allowing users to include/exclude particular overloaded methods) and later if we know users will never want to do that we can pursue the simpler format?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also noticing that the excludeEventsBeforeJittingMethod and excludeEventsAfterJittingMethod are based off of method.ToString(). It would probably be better to maintain parity for those as well. And on more deliberation and offline discussion, maybe keeping the simpler format is better for now. Also, do you happen to know if MethodDesc.ToString() will contain the parameter information or not @jakobbotsch ?

Copy link
Member

@mdh1418 mdh1418 Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, it looks like it does

runtime/src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.ToString.cs

Lines 70 to 79 in f5889ec

sb.Append('(');
try
{
sb.Append(Signature.ToString(includeReturnType: false));
}
catch
{
sb.Append("Unknown");
}
sb.Append(')');

Maybe it would be better to stick with @lateralusX's original intent of the regex matching.

I'm not very familiar with these types, would e.MethodNamespace + "." + e.MethodName + paramsArgs be the same values that are held by the resulting MethodDesc discovered by idParser.ResolveMethoID(e.MethodID...)? If not, then it might not be the right "substitution"

Copy link
Member

Choose a reason for hiding this comment

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

I'm also noticing that the excludeEventsBeforeJittingMethod and excludeEventsAfterJittingMethod are based off of method.ToString(). It would probably be better to maintain parity for those as well.

Yes, the code above should be changed in the same way.

We should not use MethodDesc.ToString() for anything except diagnostics when it is user facing like this. It should have a dedicated formatter then if we do not want to use the name from the event. I think you can base it on TypeString, except without including instantiations.

I'm not very familiar with these types, would e.MethodNamespace + "." + e.MethodName + paramsArgs be the same values that are held by the resulting MethodDesc discovered by idParser.ResolveMethoID(e.MethodID...)? If not, then it might not be the right "substitution"

Yes, they represent the same thing. dotnet-pgo has a managed implementation of the type system that replicates the native implementation of the type system that produced the strings in the event. Classes and methods are loaded into the type system based on events in the trace.

Copy link
Member

@mdh1418 mdh1418 Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to use e.MethodNamespace + "." + e.MethodName + paramsArgs as the method's name for regex matching, formatted after the method name in PerfView.

Also updated the documentation in the options' descriptions

if (includeMethods != null && !includeMethods.IsMatch(methodAsString))
{
continue;
}

if (excludeMethods != null && excludeMethods.IsMatch(methodAsString))
{
continue;
}
}

methodsToAttemptToPrepare.Add((int)e.EventIndex, new ProcessedMethodData(e.TimeStampRelativeMSec, method, "R2RLoad"));
}
}

// In case requesting events before/after jitting a method, discover the
// corresponding excludeEventsBefore/excludeEventsAfter in event stream based
// on filter criterias.
if (_command.ProcessJitEvents && excludeEventsBeforeJittingMethod != null || excludeEventsAfterJittingMethod != null)
mdh1418 marked this conversation as resolved.
Show resolved Hide resolved
{
foreach (var e in p.EventsInProcess.ByEventType<MethodJittingStartedTraceData>())
{
if (e.ClrInstanceID != clrInstanceId)
{
continue;
}

MethodDesc method = null;
bool failedDueToNonloadableModule = false;
try
{
method = idParser.ResolveMethodID(e.MethodID, out failedDueToNonloadableModule, false);
}
catch { }

if (method == null)
{
continue;
}

string methodAsString = method.ToString();
if (e.TimeStampRelativeMSec > excludeEventsBefore && excludeEventsBeforeJittingMethod != null && excludeEventsBeforeJittingMethod.IsMatch(methodAsString))
{
excludeEventsBefore = e.TimeStampRelativeMSec;
}

if (e.TimeStampRelativeMSec < excludeEventsAfter && excludeEventsAfterJittingMethod != null && excludeEventsAfterJittingMethod.IsMatch(methodAsString))
{
excludeEventsAfter = e.TimeStampRelativeMSec;
}
}

if (excludeEventsBefore > excludeEventsAfter)
{
PrintError($"Exclude events before timestamp: \"{excludeEventsBefore}\" can't be later than exclude events after timestamp: \"{excludeEventsAfter}\"");
return -1;
}
}

Expand Down Expand Up @@ -1428,8 +1503,31 @@ private int InnerProcessTraceFileMain()
continue;
}

if ((e.TimeStampRelativeMSec >= excludeEventsBefore) && (e.TimeStampRelativeMSec <= excludeEventsAfter))
methodsToAttemptToPrepare.Add((int)e.EventIndex, new ProcessedMethodData(e.TimeStampRelativeMSec, method, "JitStart"));
if (e.TimeStampRelativeMSec < excludeEventsBefore)
{
continue;
}

if (e.TimeStampRelativeMSec > excludeEventsAfter)
{
break;
}

if (includeMethods != null || excludeMethods != null)
{
string methodAsString = method.ToString();
if (includeMethods != null && !includeMethods.IsMatch(methodAsString))
{
continue;
}

if (excludeMethods != null && excludeMethods.IsMatch(methodAsString))
{
continue;
}
}

methodsToAttemptToPrepare.Add((int)e.EventIndex, new ProcessedMethodData(e.TimeStampRelativeMSec, method, "JitStart"));
mdh1418 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Loading