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.

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

Merged
merged 2 commits into from
Mar 31, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
<InternalsVisibleTo Include="AnalyzerRunner" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.EditorFeatures2.UnitTests" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.Workspaces.Test.Utilities" />
<InternalsVisibleTo Include="IdeBenchmarks" />
<!-- BEGIN MONODEVELOP
These MonoDevelop dependencies don't ship with Visual Studio, so can't break our
binary insertions and are exempted from the ExternalAccess adapter assembly policies.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars;

namespace Microsoft.CodeAnalysis.StackTraceExplorer
{
internal sealed class DefaultStackParser : IStackFrameParser
{
public bool TryParseLine(string line, [NotNullWhen(true)] out ParsedFrame? parsedFrame)
public bool TryParseLine(VirtualCharSequence line, [NotNullWhen(true)] out ParsedFrame? parsedFrame)
{
// For now we just keep all text so the user can still see lines they pasted and they
// don't disappear. In the future we might want to restrict what we show.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.EmbeddedLanguages.StackFrame;
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars;

namespace Microsoft.CodeAnalysis.StackTraceExplorer
{
Expand All @@ -12,7 +14,7 @@ internal sealed class DotnetStackFrameParser : IStackFrameParser
/// <summary>
/// Uses <see cref="StackFrameParser"/> to parse a line if possible
/// </summary>
public bool TryParseLine(string line, [NotNullWhen(true)] out ParsedFrame? parsedFrame)
public bool TryParseLine(VirtualCharSequence line, [NotNullWhen(true)] out ParsedFrame? parsedFrame)
{
parsedFrame = null;
var tree = StackFrameParser.TryParse(line);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars;

namespace Microsoft.CodeAnalysis.StackTraceExplorer
{
internal interface IStackFrameParser
{
bool TryParseLine(string line, [NotNullWhen(returnValue: true)] out ParsedFrame? parsedFrame);
bool TryParseLine(VirtualCharSequence line, [NotNullWhen(returnValue: true)] out ParsedFrame? parsedFrame);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,23 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars;

namespace Microsoft.CodeAnalysis.StackTraceExplorer
{
internal sealed class IgnoredFrame : ParsedFrame
{
private readonly string _originalText;
private readonly VirtualCharSequence _originalText;

public IgnoredFrame(string originalText)
public IgnoredFrame(VirtualCharSequence originalText)
{
_originalText = originalText;
}

public override string ToString()
{
return _originalText;
return _originalText.CreateString();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@ namespace Microsoft.CodeAnalysis.StackTraceExplorer
internal readonly struct StackTraceAnalysisResult
{
public StackTraceAnalysisResult(
string originalString,
ImmutableArray<ParsedFrame> parsedLines)
{
OriginalString = originalString;
ParsedFrames = parsedLines;
}

public string OriginalString { get; }
public ImmutableArray<ParsedFrame> ParsedFrames { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.EmbeddedLanguages.StackFrame;
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.StackTraceExplorer
{
Expand All @@ -29,16 +31,32 @@ 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()));
return Task.FromResult(new StackTraceAnalysisResult(callstack, parsedFrames.ToImmutableArray()));
}

private static IEnumerable<ParsedFrame> Parse(string callstack, CancellationToken cancellationToken)
{
foreach (var line in SplitLines(callstack))
// if the callstack comes from ActivityLog.xml it has been
// encoding to be passed over HTTP. This should only decode
// specific characters like "&gt;" and "&lt;" to their "normal"
// equivalents ">" and "<" so we can parse correctly
callstack = WebUtility.HtmlDecode(callstack);
Copy link
Contributor

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 &gt; etc

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without HtmlDecode

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
BenchmarkStackTraceParsing 20.78 ms 0.104 ms 0.087 ms 93.7500 - - 2 MB

With HtmlDecode

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
BenchmarkStackTraceParsing 20.88 ms 0.075 ms 0.062 ms 93.7500 31.2500 - 2 MB

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
{
Expand All @@ -51,17 +69,48 @@ private static IEnumerable<ParsedFrame> Parse(string callstack, CancellationToke
}
}

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')
Copy link
Member

Choose a reason for hiding this comment

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

any need to support \r\n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as of now the \r gets stripped in the Trim call, which is the same as current behavior. If we want behavior change we should do in a separate PR imo

{
yield return callstack.GetSubSequence(TextSpan.FromBounds(position, i));

private static IEnumerable<string> SplitLines(string callstack)
// +1 to skip over the \n character
position = i + 1;
}
}

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 "&gt;" and "&lt;" 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));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.EmbeddedLanguages.StackFrame;
using Microsoft.CodeAnalysis.EmbeddedLanguages.VirtualChars;

namespace Microsoft.CodeAnalysis.StackTraceExplorer
{
internal sealed class VSDebugCallstackParser : IStackFrameParser
{
public bool TryParseLine(string line, [NotNullWhen(true)] out ParsedFrame? parsedFrame)
public bool TryParseLine(VirtualCharSequence line, [NotNullWhen(true)] out ParsedFrame? parsedFrame)
{
// Example line:
// ConsoleApp4.dll!ConsoleApp4.MyClass.ThrowAtOne() Line 19 C#
Expand All @@ -21,14 +22,22 @@ public bool TryParseLine(string line, [NotNullWhen(true)] out ParsedFrame? parse
parsedFrame = null;

// +1 here because we always want to skip the '!' character
var startPoint = line.IndexOf('!') + 1;
var startPoint = -1;
for (var i = 0; i < line.Length; i++)
{
if (line[i].Value == '!')
{
startPoint = 0;
break;
}
}

if (startPoint == 0 || startPoint == line.Length)
if (startPoint <= 0 || startPoint == line.Length)
{
return false;
}

var textToParse = line[startPoint..];
var textToParse = line.GetSubSequence(TextSpan.FromBounds(startPoint, line.Length));
var tree = StackFrameParser.TryParse(textToParse);

if (tree is null)
Expand Down
Loading