NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

Optimize HttpUtility.JavaScriptStringEncode by using SearchValues#102917

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

MihaZupan merged 6 commits intodotnet:mainfromTrayanZapryanov:optimize_HttpUtility_JavaScriptStringEncode

Jun 6, 2024

Merged

Optimize HttpUtility.JavaScriptStringEncode by using SearchValues#102917

MihaZupan merged 6 commits intodotnet:mainfromTrayanZapryanov:optimize_HttpUtility_JavaScriptStringEncode

Jun 6, 2024

Conversation

Copy link

Contributor

TrayanZapryanov commentedMay 31, 2024

Optimize HttpUtility.JavaScriptStringEncode by using SearchValues for invalid JavaScript characters.

Benchmark

[MemoryDiagnoser]publicclassHttpUtilityBenchmarks{[Params(true,false)]publicboolAddQuotes{get;set;}[Benchmark(Baseline=true)]publicstringJavaScriptStringEncode_NoEscape()=>HttpUtility.JavaScriptStringEncode("No escaping needed.",AddQuotes);[Benchmark]publicstringJavaScriptStringEncode_NoEscape_PR()=>MyHttpUtility.JavaScriptStringEncode("No escaping needed.",AddQuotes);[Benchmark]publicstringJavaScriptStringEncode_Escape()=>HttpUtility.JavaScriptStringEncode("The\t and\n will need to be escaped.",AddQuotes);[Benchmark]publicstringJavaScriptStringEncode_Escape_PR()=>MyHttpUtility.JavaScriptStringEncode("The\t and\n will need to be escaped.",AddQuotes);}

Results:

Method	AddQuotes	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
JavaScriptStringEncode_NoEscape	False	27.485 ns	0.2172 ns	0.2032 ns	1.00	0.00	-	-	NA
JavaScriptStringEncode_NoEscape_PR	False	2.558 ns	0.0873 ns	0.0857 ns	0.09	0.00	-	-	NA
JavaScriptStringEncode_Escape	False	75.264 ns	0.7011 ns	0.5854 ns	2.74	0.02	0.0315	264 B	NA
JavaScriptStringEncode_Escape_PR	False	36.093 ns	0.5054 ns	0.3945 ns	1.31	0.02	0.0315	264 B	NA

JavaScriptStringEncode_NoEscape	True	35.001 ns	0.4262 ns	0.3778 ns	1.00	0.00	0.0076	64 B	1.00
JavaScriptStringEncode_NoEscape_PR	True	8.809 ns	0.2115 ns	0.3100 ns	0.26	0.01	0.0076	64 B	1.00
JavaScriptStringEncode_Escape	True	86.737 ns	1.7250 ns	1.6136 ns	2.48	0.04	0.0440	368 B	5.75
JavaScriptStringEncode_Escape_PR	True	37.486 ns	0.7770 ns	1.4971 ns	1.10	0.03	0.0315	264 B	4.12

ghost added the area-System.Net label

May 31, 2024

dotnet-policy-servicebot added the community-contributionIndicates that the PR has been added by a community member label

May 31, 2024

Copy link

ContributorAuthor

TrayanZapryanov commentedMay 31, 2024

@EgorBot -arm64 -amd

usingSystem.Web;usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;BenchmarkRunner.Run<HttpUtilityBenchmarks>(args:args);publicclassHttpUtilityBenchmarks{[Benchmark(Baseline=true)]publicstringJavaScriptStringEncode()=>HttpUtility.JavaScriptStringEncode("No escaping needed.");[Benchmark]publicstringJavaScriptStringEncode_Encode()=>HttpUtility.JavaScriptStringEncode("The\t and\n will need to be escaped.");}

Copy link

Member

EgorBo commentedMay 31, 2024

@TrayanZapryanov please avoid using-arm64 if PR is not arch specific 🙂 I host this bot on my personal subscription

This was referencedMay 31, 2024

tracing/eventpipe/providervalidation/providervalidation/providervalidation.cmd test failure#101759

Closed

Crash in Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.WorkItemExecution#90019

Open

gfoidl reviewed

May 31, 2024

View reviewed changes

src/libraries/System.Web.HttpUtility/src/System/Web/Util/HttpEncoder.cs OutdatedShow resolvedHide resolved

MihaZupan reviewed

May 31, 2024

View reviewed changes

src/libraries/System.Web.HttpUtility/src/System/Web/Util/HttpEncoder.cs OutdatedShow resolvedHide resolved

Copy link

Member

MihaZupan commentedMay 31, 2024

I see we had similar ideas@gfoidl :)

MihaZupan added this to the9.0.0 milestone

May 31, 2024

Copy link

ContributorAuthor

TrayanZapryanov commentedMay 31, 2024

@gfoidl,@MihaZupan thanks for good ideas.
I've applied all of them and here are results

Method	AddQuotes	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
JavaScriptStringEncode_NoEscape	False	24.483 ns	0.4342 ns	0.3625 ns	1.00	0.00	-	-	NA
JavaScriptStringEncode_NoEscape_PR	False	4.922 ns	0.0432 ns	0.0383 ns	0.20	0.00	-	-	NA
JavaScriptStringEncode_Escape	False	78.210 ns	1.2223 ns	0.9543 ns	3.20	0.07	0.0315	264 B	NA
JavaScriptStringEncode_Escape_PR	False	42.045 ns	0.5002 ns	0.4679 ns	1.72	0.03	0.0124	104 B	NA

JavaScriptStringEncode_NoEscape	True	36.507 ns	0.3863 ns	0.3614 ns	1.00	0.00	0.0076	64 B	1.00
JavaScriptStringEncode_NoEscape_PR	True	16.740 ns	0.2302 ns	0.1922 ns	0.46	0.01	0.0076	64 B	1.00
JavaScriptStringEncode_Escape	True	86.635 ns	1.6189 ns	1.3518 ns	2.37	0.04	0.0440	368 B	5.75
JavaScriptStringEncode_Escape_PR	True	43.646 ns	0.8023 ns	0.7504 ns	1.20	0.03	0.0124	104 B	1.62

Allocations are much better, although there is a small regression(most probably due to ValueStringBuilder). In General PR gives good perf wins even now.

Copy link

Member

MihaZupan commentedMay 31, 2024•
edited
Loading

although there is a small regression(most probably due to ValueStringBuilder)

Do you see it go away if you move the fallback (everything afterreturn addDoubleQuotes ? $"\"{value}\"" : value;) to a separate helper (to move the stackalloc out of the fast path)?

E.g.string EncodeCore(ReadOnlySpan<char> value, int i)

MihaZupan reviewed

May 31, 2024

View reviewed changes

src/libraries/System.Web.HttpUtility/src/System/Web/Util/HttpEncoder.cs OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

TrayanZapryanov commentedJun 3, 2024

@MihaZupan requested changes are applied.
I've measured your suggestion for removing if(string.IsnullorEmpty(XXX)) and looks like without this check results are slightly worst. Here are results:
Without check for null(like in the PR code at the moment):

Method	AddQuotes	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
JavaScriptStringEncode_NoEscape	False	23.716 ns	0.1315 ns	0.1098 ns	23.710 ns	1.00	0.00	-	-	NA
JavaScriptStringEncode_NoEscape_PR	False	3.035 ns	0.0135 ns	0.0120 ns	3.033 ns	0.13	0.00	-	-	NA
JavaScriptStringEncode_Escape	False	85.928 ns	1.8087 ns	5.3047 ns	84.196 ns	3.79	0.21	0.0315	264 B	NA
JavaScriptStringEncode_Escape_PR	False	46.633 ns	0.7838 ns	0.7331 ns	46.815 ns	1.97	0.03	0.0124	104 B	NA

JavaScriptStringEncode_NoEscape	True	37.540 ns	0.6752 ns	0.5639 ns	37.431 ns	1.00	0.00	0.0076	64 B	1.00
JavaScriptStringEncode_NoEscape_PR	True	15.981 ns	0.3474 ns	0.4000 ns	15.940 ns	0.42	0.01	0.0076	64 B	1.00
JavaScriptStringEncode_Escape	True	92.901 ns	1.8431 ns	5.2286 ns	91.220 ns	2.54	0.13	0.0440	368 B	5.75
JavaScriptStringEncode_Escape_PR	True	46.662 ns	0.9257 ns	0.8659 ns	46.474 ns	1.24	0.03	0.0124	104 B	1.62

string.IsNullOrEmpty:

Method	AddQuotes	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
JavaScriptStringEncode_NoEscape	False	26.880 ns	0.3227 ns	0.2695 ns	26.949 ns	1.00	0.00	-	-	NA
JavaScriptStringEncode_NoEscape_PR	False	2.495 ns	0.0395 ns	0.0370 ns	2.501 ns	0.09	0.00	-	-	NA
JavaScriptStringEncode_Escape	False	81.711 ns	1.1379 ns	0.9502 ns	81.718 ns	3.04	0.06	0.0315	264 B	NA
JavaScriptStringEncode_Escape_PR	False	47.066 ns	0.6737 ns	0.5972 ns	46.978 ns	1.75	0.02	0.0124	104 B	NA

JavaScriptStringEncode_NoEscape	True	37.651 ns	0.7910 ns	0.7399 ns	37.339 ns	1.00	0.00	0.0076	64 B	1.00
JavaScriptStringEncode_NoEscape_PR	True	10.082 ns	0.3532 ns	1.0304 ns	9.684 ns	0.30	0.03	0.0076	64 B	1.00
JavaScriptStringEncode_Escape	True	90.945 ns	1.7183 ns	3.9480 ns	90.059 ns	2.49	0.13	0.0440	368 B	5.75
JavaScriptStringEncode_Escape_PR	True	46.321 ns	0.9600 ns	2.6441 ns	46.369 ns	1.19	0.06	0.0124	104 B	1.62