-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Move StackTraceAnalyzer over to VirtualCharSequence #60404
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,9 @@ | |
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.EmbeddedLanguages.StackFrame; | ||
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars; | ||
using Microsoft.CodeAnalysis.PooledObjects; | ||
using Microsoft.CodeAnalysis.Text; | ||
|
||
namespace Microsoft.CodeAnalysis.StackTraceExplorer | ||
{ | ||
|
@@ -28,40 +31,91 @@ internal static class StackTraceAnalyzer | |
|
||
public static Task<StackTraceAnalysisResult> AnalyzeAsync(string callstack, CancellationToken cancellationToken) | ||
{ | ||
var parsedFrames = Parse(callstack, cancellationToken); | ||
return Task.FromResult(new StackTraceAnalysisResult(parsedFrames.ToImmutableArray())); | ||
var result = new StackTraceAnalysisResult(callstack, Parse(callstack, cancellationToken)); | ||
return Task.FromResult(result); | ||
} | ||
|
||
private static IEnumerable<ParsedFrame> Parse(string callstack, CancellationToken cancellationToken) | ||
private static ImmutableArray<ParsedFrame> Parse(string callstack, CancellationToken cancellationToken) | ||
{ | ||
foreach (var line in SplitLines(callstack)) | ||
using var _ = ArrayBuilder<ParsedFrame>.GetInstance(out var builder); | ||
|
||
// if the callstack comes from ActivityLog.xml it has been | ||
// encoding to be passed over HTTP. This should only decode | ||
// specific characters like ">" and "<" to their "normal" | ||
// equivalents ">" and "<" so we can parse correctly | ||
callstack = WebUtility.HtmlDecode(callstack); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this throw exceptions? do we need try/catch/bail-gracefully with this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I know of. It accepts null and empty values, and doesn't document any cases where an exception is expected to be thrown. https://docs.microsoft.com/en-us/dotnet/api/system.web.httputility.htmldecode?view=net-6.0 |
||
|
||
var sequence = VirtualCharSequence.Create(0, callstack); | ||
|
||
foreach (var line in SplitLines(sequence)) | ||
{ | ||
cancellationToken.ThrowIfCancellationRequested(); | ||
|
||
var trimmedLine = line.Trim(); | ||
// For now do the work to removing leading and trailing whitespace. | ||
// This keeps behavior we've had, but may not actually be the desired behavior in the long run. | ||
// Specifically if we ever want to add a copy feature to copy back contents from a frame | ||
var trimmedLine = Trim(line); | ||
|
||
if (trimmedLine.IsEmpty) | ||
{ | ||
continue; | ||
} | ||
|
||
foreach (var parser in s_parsers) | ||
{ | ||
if (parser.TryParseLine(trimmedLine, out var parsedFrame)) | ||
{ | ||
yield return parsedFrame; | ||
builder.Add(parsedFrame); | ||
break; | ||
} | ||
} | ||
} | ||
|
||
return builder.ToImmutable(); | ||
} | ||
|
||
private static readonly char[] s_lineSplit = new[] { '\n' }; | ||
private static IEnumerable<VirtualCharSequence> SplitLines(VirtualCharSequence callstack) | ||
{ | ||
var position = 0; | ||
|
||
for (var i = 0; i < callstack.Length; i++) | ||
{ | ||
if (callstack[i].Value == '\n') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any need to support \r\n? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as of now the \r gets stripped in the |
||
{ | ||
yield return callstack.GetSubSequence(TextSpan.FromBounds(position, i)); | ||
|
||
// +1 to skip over the \n character | ||
position = i + 1; | ||
} | ||
} | ||
|
||
private static IEnumerable<string> SplitLines(string callstack) | ||
if (position < callstack.Length) | ||
{ | ||
yield return callstack.GetSubSequence(TextSpan.FromBounds(position, callstack.Length)); | ||
} | ||
} | ||
|
||
private static VirtualCharSequence Trim(VirtualCharSequence virtualChars) | ||
{ | ||
// if the callstack comes from ActivityLog.xml it has been | ||
// encoding to be passed over HTTP. This should only decode | ||
// specific characters like ">" and "<" to their "normal" | ||
// equivalents ">" and "<" so we can parse correctly | ||
callstack = WebUtility.HtmlDecode(callstack); | ||
if (virtualChars.Length == 0) | ||
{ | ||
return virtualChars; | ||
} | ||
|
||
var start = 0; | ||
var end = virtualChars.Length - 1; | ||
|
||
while (virtualChars[start].IsWhiteSpace && start < end) | ||
{ | ||
start++; | ||
} | ||
|
||
while (virtualChars[end].IsWhiteSpace && end > start) | ||
{ | ||
end--; | ||
} | ||
|
||
return callstack.Split(s_lineSplit, StringSplitOptions.RemoveEmptyEntries); | ||
return virtualChars.GetSubSequence(TextSpan.FromBounds(start, end + 1)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this low hanging fruit?
Would be interesting to see what difference the benchmark shows if you comment out this line (since it doesn't look like it's needed for them). If it's a big enough one then might be better to make the parsers smarter and allow for
>
etcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really depends on what all we want to support. The
<
and>
are examples, but it also does newlines,$
and probably more. If we need to reduce this copy further then we might find a way to handle this. The good news is that with this implementation it also displays to the user in a more friendly way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know its not trivial, just wondering what impact this has since its called for every stack trace, and presumably does lots of string processing. If it doesn't make a big difference then clearly its not worth the complexity of the alternative, but we should get the data at least.
To be clear, I mean literally comment out this line, and run the benchmark on your machine, and see what the results are, thats all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without HtmlDecode
With HtmlDecode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slightly slower, and more Gen 1 allocations, but not a significant change on the benchmark. I could add a larger benchmark to highlight the difference but I think for now it's fine to leave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not worth worrying about. I'm pleasantly surprised.