Move StackTraceAnalyzer over to VirtualCharSequence #60404
Conversation
ryzngard
commented
Mar 26, 2022
•
ryzngard
commented
- Convert to using VirtualChars to reduce string allocations from the stack trace analyzer
- Add benchmarks
- Fixes AB#1504223
stack trace analyzer Add benchmarks
|
Allocations in the benchmark dropped from 5MB to 2MB |
| // 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.
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 > etc
There was a problem hiding this comment.
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.
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.
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 |
There was a problem hiding this comment.
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.
Yeah, not worth worrying about. I'm pleasantly surprised.
| // 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.
can this throw exceptions? do we need try/catch/bail-gracefully with this?
There was a problem hiding this comment.
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
|
|
||
| for (var i = 0; i < callstack.Length; i++) | ||
| { | ||
| if (callstack[i].Value == '\n') |
There was a problem hiding this comment.
any need to support \r\n?
There was a problem hiding this comment.
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
ghost
added this to the 
…ures/semi-auto-props * upstream/main: (110 commits) Add Rebuild badge to README (#60298) Update PublishData.json for 17.3 P1 (#60559) Note auto-default merged in feature status doc (#60564) Update Roslyn.Diagnostics.Analyzers and remove RS0005 suppressions Cleanup unused resources in Features layer Remove unnecessary `<Compile Remove` Remove overrides in Features Remove overrides in Analyzers Remove the single unused read of CodeFixCategory Remove unused abstract property Update Language Feature Status.md (#60482) Document ROSLYN_TEST_USEDASSEMBLIES (#60478) Add BoundArrayInitialization.IsInferred (#60391) Add an aggregate logger for inheritance margin (#60493) Move StackTraceAnalyzer over to VirtualCharSequence (#60404) PR feedback Fix test and review feedback Update Spanish queue to Windows.10.Amd64.Server2022.ES.Open Enable NRT in AbstractSyncNamespaceCodeRefactoringProvider test ...
