-
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
Conversation
ryzngard
commented
Mar 26, 2022
•
edited by Pilchie
Loading
edited by Pilchie
- 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.
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 >
etc
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.
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
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.
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.
// 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 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?
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.
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.
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?
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.
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
…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 ...