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

Optimize HttpUtility.JavaScriptStringEncode by using SearchValues #102917

Conversation

TrayanZapryanov
Copy link
Contributor

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

Benchmark

[MemoryDiagnoser]
public class HttpUtilityBenchmarks
{
	[Params(true, false)]
	public bool AddQuotes { get; set; }

	[Benchmark(Baseline = true)]
	public string JavaScriptStringEncode_NoEscape() => HttpUtility.JavaScriptStringEncode("No escaping needed.", AddQuotes);

	[Benchmark]
	public string JavaScriptStringEncode_NoEscape_PR() => MyHttpUtility.JavaScriptStringEncode("No escaping needed.", AddQuotes);

	[Benchmark]
	public string JavaScriptStringEncode_Escape() => HttpUtility.JavaScriptStringEncode("The \t and \n will need to be escaped.", AddQuotes);

	[Benchmark]
	public string JavaScriptStringEncode_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

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 31, 2024
@TrayanZapryanov
Copy link
Contributor Author

@EgorBot -arm64 -amd

using System.Web;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<HttpUtilityBenchmarks>(args: args);

public class HttpUtilityBenchmarks
{
    [Benchmark(Baseline = true)]
    public string JavaScriptStringEncode() => 
        HttpUtility.JavaScriptStringEncode("No escaping needed.");

    [Benchmark]
    public string JavaScriptStringEncode_Encode() => 
        HttpUtility.JavaScriptStringEncode("The \t and \n will need to be escaped.");
}

@EgorBo
Copy link
Member

EgorBo commented May 31, 2024

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

@MihaZupan
Copy link
Member

I see we had similar ideas @gfoidl :)

@MihaZupan MihaZupan added this to the 9.0.0 milestone May 31, 2024
@TrayanZapryanov
Copy link
Contributor Author

@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.

@MihaZupan
Copy link
Member

MihaZupan commented May 31, 2024

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

Do you see it go away if you move the fallback (everything after return 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)

@TrayanZapryanov
Copy link
Contributor Author

@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

I don't think this is so performance critical code, so both ways shows performance increase and it is up to you to decide which one do you prefer.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Thanks.

Looks like this one hit a merge conflict after #102805

@TrayanZapryanov TrayanZapryanov force-pushed the optimize_HttpUtility_JavaScriptStringEncode branch from 9ae9832 to 5d498a4 Compare June 4, 2024 11:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants