-
Notifications
You must be signed in to change notification settings - Fork 862
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
Avoid StringBuilder.AppendFormat #3412
Comments
Hi @paulomorgado, Thanks for submitting the feature request. I will further review the optimization technique you proposed with the .NET SDK team. Regards, |
@paulomorgado Thanks for the proposed solution. would you mind adding a benchmark that shows the before and after of making this change? Would be good to see if this is something we want to do. |
@peterrsongg, here is a benchmark: Invoking Using a [MemoryDiagnoser]
public class StringBuilderBenchmarks
{
private static readonly ObjectPool<StringBuilder> stringBuilderPool = (new DefaultObjectPoolProvider()).CreateStringBuilderPool(initialCapacity: 256, maximumRetainedCapacity: 1024);
[Params("aws-sdk-dotnet-coreclr/3.7.300.52 ua/2.0 os/windows#10.0.19045.0 md/ARCH#X64 lang/.NET_Core#8.0.7 md/aws-sdk-dotnet-core#3.7.302.12 api/KMS#3.7.300.52")]
public string UserAgent { get; set; } = null!;
[Params(null, "UserAgentAddition", "{[(UserAgentAddition)]}")]
public string? UserAgentAddition { get; set; }
[Params(null, "ClientAppId")]
public string? ClientAppId { get; set; }
[Params(RequestRetryMode.Standard, RequestRetryMode.Adaptive)]
public RequestRetryMode RetryMode { get; set; }
[Params(false, true)]
public bool IsAsync { get; set; }
[Params(false, true)]
public bool InitializeCollections { get; set; }
[Benchmark(Baseline = true)]
public string Existing_SetUserAgentHeader()
{
var sb = new StringBuilder(UserAgent);
var clientAppId = ClientAppId;
if (!string.IsNullOrEmpty(clientAppId))
sb.AppendFormat(" app/{0}", clientAppId);
var retryMode = RetryMode.ToString().ToLower();
sb.AppendFormat(" cfg/retry-mode#{0}", retryMode);
sb.AppendFormat(" md/{0}", IsAsync ? "ClientAsync" : "ClientSync");
sb.AppendFormat(" cfg/init-coll#{0}", InitializeCollections ? "1" : "0");
var userAgentAddition = UserAgentAddition;
if (!string.IsNullOrEmpty(userAgentAddition))
{
sb.AppendFormat(" {0}", userAgentAddition);
}
var userAgent = sb.ToString();
userAgent = InternalSDKUtils.ReplaceInvalidUserAgentCharacters(userAgent);
return userAgent;
}
[Benchmark]
public string NoAppendFormat_SetUserAgentHeader()
{
var sb = new StringBuilder(256);
return Optimized(sb);
}
[Benchmark]
public string NoAppendFormat_PooledStringBuilder_SetUserAgentHeader()
{
var sb = stringBuilderPool.Get();
try
{
return Optimized(sb);
}
finally
{
stringBuilderPool.Return(sb);
}
}
private string Optimized(StringBuilder sb)
{
sb.Append(InternalSDKUtils.ReplaceInvalidUserAgentCharacters(UserAgent));
//sb.Append(UserAgent);
var clientAppId = ClientAppId;
if (!string.IsNullOrEmpty(clientAppId))
{
sb.Append(" app/").Append(InternalSDKUtils.ReplaceInvalidUserAgentCharacters(clientAppId));
//sb.Append(" app/").Append(clientAppId);
}
sb.Append(" cfg/retry-mode#}");
sb.Append(RetryMode.ToUserAgentHeaderString());
sb.Append(" md/").Append(IsAsync ? "ClientAsync" : "ClientSync");
sb.Append(" cfg/init-coll#").Append(InitializeCollections ? '1' : '0');
var userAgentAddition = UserAgentAddition;
if (!string.IsNullOrEmpty(userAgentAddition))
{
sb.Append(' ').Append(InternalSDKUtils.ReplaceInvalidUserAgentCharacters(userAgentAddition));
//sb.Append(' ').Append(userAgentAddition);
}
var userAgent = sb.ToString();
//userAgent = InternalSDKUtils.ReplaceInvalidUserAgentCharacters(userAgent);
return userAgent;
}
}
internal static partial class InternalSDKUtils
{
private const string DisallowedCharactersRegexPattern = @"[^ /!#$%&'*+-.^_`|~\w\d]";
[GeneratedRegex(DisallowedCharactersRegexPattern)]
private static partial Regex DisallowedCharactersRegex();
internal static string ReplaceInvalidUserAgentCharacters(string userAgent)
{
// Use the regular expression to replace disallowed characters by a hyphen
var validUserAgent = DisallowedCharactersRegex().Replace(userAgent, "-");
//var validUserAgent = userAgent;
return validUserAgent;
}
public static string ToUserAgentHeaderString(this RequestRetryMode requestRetryMode)
{
if (requestRetryMode == RequestRetryMode.Adaptive)
return "adaptive";
else
return "standard";
}
}
public enum RequestRetryMode
{
Standard,
Adaptive
}
|
Your PR has been merged and will be included in the next preview release of the SDK. Thanks again for the contribution! |
Comments on closed issues are hard for our team to see. |
Describe the feature
Amazon.Runtime.Internal.Marshaller.SetUserAgentHeader
has this code:There's no practical use of `StringBuilder.AppendFormat here.
Use Case
Creating objects that do not need to be created, not only uses memory and CPU to create them, but also causes GC work.
Interpreting a format and formatting is unnecessary work when appending to a
StringBuilder
.Proposed Solution
Consider using:
Other Information
But, how much can change in the user-agent string that needs to be computed for every request?
Acknowledgements
AWS .NET SDK and/or Package version used
AWSSDK.KeyManagementService 3.7.300.52
Targeted .NET Platform
.NET 8
Operating System and version
Windows and Linux
The text was updated successfully, but these errors were encountered: