-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
performance patch of JsonConverterHelper.cs #26448
Conversation
sdk/mgmtcommon/ClientRuntime/ClientRuntime/Serialization/JsonConverterHelper.cs
Show resolved
Hide resolved
sdk/mgmtcommon/ClientRuntime/ClientRuntime/Serialization/JsonConverterHelper.cs
Show resolved
Hide resolved
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.
LGTM, and remove the Regex should be better
@PiDiBi thanks for your PR! Do you happen to have the benchmarks for allocations? Did they change? |
should I delete this PR? nobody interested in performance improvements? |
hallo, anyone here interested in performance improvement? |
@PiDiBi LGTM. Merging. Thanks for your contribution!
@AlexGhiondea I re-run the benchmark test with
|
as far as we use this in azure web apps and it makes us problems I'm kindly suggesting this performance update
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Xeon W-2133 CPU 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.101
[Host] : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT [AttachedDebugger]
DefaultJob : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT
On Dec 11, 2021, at 10:09 AM, Feng Yuan [email protected] wrote:
Found a super expensive regular expression stack in NetworkWatcherTrafficAnalytics-Prod:
GetPropertyName is costing 4.1 million CPU samples, 14.84% of total CPU samples in the stream I downloaded.
testing code