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.

Sign up for GitHub

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

Introduce string builder factory template. #4611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+178 −81
Open

Introduce string builder factory template. #4611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/NSwag.CodeGeneration.CSharp/Templates/Client.Class.StringBuilder.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
private static global::System.Text.StringBuilder GetStringBuilder() => new global::System.Text.StringBuilder(8000);
Copy link

Choose a reason for hiding this comment

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

How common is this scenario? I think if you find a GC allocated StringBuilder to effect your performance, you might not want to use a generated REST client in the first place...
If this is approved though, I think the avarage url length is a lot shorter than 8000 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you run this code:

var chunk = typeof(StringBuilder).GetField("m_ChunkPrevious", BindingFlags.NonPublic | BindingFlags.Instance);
var sb = new StringBuilder();
var previousCapacity = sb.Capacity;
for (var i = 0; i < 10_000; i++)
{
    sb.Append('*');
    if (sb.Capacity != previousCapacity)
    {
        previousCapacity = sb.Capacity;
        var j = 0;
        var s = sb;
        while (s is not null)
        {
            if (j > 0)
            {
                Console.Write(new string(' ', j * 4));
            }
            Console.WriteLine($"Capacity: {s.Capacity}");
            s = (StringBuilder)chunk.GetValue(s);
            j++;
        }
    }
}

you'll see that the StringBuilder, by default, is a linked list of StringBuilder instances with 16 characters.

This is especially painful if you need to traverse the chain. It is especially painful if you are inserting, removing or replacing characters.

PR #4585 removed replace operations and generates append only operations.

By moving this to its own template, it allows users to use any source compatible version they want. Including special implementations (.NET is full of private ValueStringBuilder implementations) and pooling.

The 8000-character initial size is something I anticipate no one reaches and is easily collected by the GC.

Copy link

Choose a reason for hiding this comment

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

I know how StringBuilder works internally, I'm just saying I highly doubt theres a need out there to want to pool the StringBuilder.

I'm not against using a default capacity to the StringBuilder object though, but I still think 8000 is too large.
The reason is because the IIS maximum url length defaults to 4096, and at this point the HttpClient likely already has a BaseAddress attached to it, so it won't be part of the appending here. So even though 8000 is too low for LOH its still an unnecessarily big allocation.

By the way, how are you supposed to override the implementation of GetStringBuilder() from this class? And private partial static void ReturnStringBuilder is an illegal method declaration, you need to make it static partial void ReturnStringBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, how are you supposed to override the implementation of GetStringBuilder() from this class? And private partial static void ReturnStringBuilder is an illegal method declaration, you need to make it static partial void ReturnStringBuilder.

Replacing the template for generation.


private partial static void ReturnStringBuilder(global::System.Text.StringBuilder sb);
56 changes: 33 additions & 23 deletions src/NSwag.CodeGeneration.CSharp/Templates/Client.Class.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,11 @@
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("{{ operation.Produces }}"));
{% endif -%}

var urlBuilder_ = new System.Text.StringBuilder();
{% if UseBaseUrl %}if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);{% endif %}
// Operation Path: "{{ operation.Path }}"
string url_;
var urlBuilder_ = GetStringBuilder();
try
{
// Operation Path: "{{ operation.Path }}"
{% if operation.Path contains "{" -%}
{% assign pathParts = operation.Path | split: "{" -%}
{% for pathPart in pathParts -%}
Expand All @@ -276,47 +278,47 @@
{% if parameter.Name == pathParameterParts[0] -%}
{% if parameter.IsOptional -%}
{% assign pathParameterFound = false -%}
if ({{ parameter.VariableName }} != null)
{
{% template Client.Class.PathParameter %}
}
else
if (urlBuilder_.Length > 0) urlBuilder_.Length--;
if ({{ parameter.VariableName }} != null)
{
{% template Client.Class.PathParameter %}
}
else
if (urlBuilder_.Length > 0) urlBuilder_.Length--;
{% else -%}
{% template Client.Class.PathParameter %}
{% template Client.Class.PathParameter %}
{% endif -%}
{% endif -%}
{% endfor -%}
{% comment -%} >>> just in case {% endcomment -%}
{% unless pathParameterFound -%}
urlBuilder_.Append("{{ '{' }}{{ pathParameterParts[0] }}{{ '}' }}");
urlBuilder_.Append("{{ '{' }}{{ pathParameterParts[0] }}{{ '}' }}");
{% endunless -%}
{% comment -%} <<< just in case {% endcomment -%}
{% assign nonParameterPartLengh = pathParameterParts[1] | size -%}
{% if nonParameterPartLengh == 1 -%}
urlBuilder_.Append('{{ pathParameterParts[1] }}');
urlBuilder_.Append('{{ pathParameterParts[1] }}');
{% elsif nonParameterPartLengh > 0 -%}
urlBuilder_.Append("{{ pathParameterParts[1] }}");
urlBuilder_.Append("{{ pathParameterParts[1] }}");
{% endif -%}
{% else -%}
{% unless pathPart == "" -%}
urlBuilder_.Append("{{ pathPart }}");
urlBuilder_.Append("{{ pathPart }}");
{% endunless -%}
{% endif -%}
{% endfor -%}
{% else -%}
{% unless operation.Path == "" -%}
urlBuilder_.Append("{{ operation.Path }}");
urlBuilder_.Append("{{ operation.Path }}");
{% endunless -%}
{% endif -%}
{% if operation.HasQueryParameters -%}
urlBuilder_.Append('?');
urlBuilder_.Append('?');
{% for parameter in operation.QueryParameters -%}
{% if parameter.IsOptional -%}
if ({{ parameter.VariableName }} != null)
{
{% template Client.Class.QueryParameter %}
}
if ({{ parameter.VariableName }} != null)
{
{% template Client.Class.QueryParameter %}
}
{% else -%}
{% template Client.Class.QueryParameter %}
{% endif -%}
Expand All @@ -325,12 +327,18 @@
{% endif -%}

{% if GeneratePrepareRequestAndProcessResponseAsAsyncMethods %}
await PrepareRequestAsync(client_, request_, urlBuilder_, cancellationToken).ConfigureAwait(false);
await PrepareRequestAsync(client_, request_, urlBuilder_, cancellationToken).ConfigureAwait(false);
{% else -%}
PrepareRequest(client_, request_, urlBuilder_);
PrepareRequest(client_, request_, urlBuilder_);
{% endif -%}

var url_ = urlBuilder_.ToString();
url_ = urlBuilder_.ToString();
}
finally
{
ReturnStringBuilder(urlBuilder_);
}

request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);

{% if GeneratePrepareRequestAndProcessResponseAsAsyncMethods -%}
Expand Down Expand Up @@ -448,4 +456,6 @@

{% template Client.Class.ConvertToString %}
{% template Client.Class.Body %}

{% template Client.Class.StringBuilder %}
}
100 changes: 71 additions & 29 deletions ...jectName=NSwag.Sample.NET70Minimal_targetFramework=net7.0_generatesCode=True.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,21 @@ namespace MyNamespace
request_.Method = new System.Net.Http.HttpMethod("GET");
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json"));

var urlBuilder_ = new System.Text.StringBuilder();
if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);
// Operation Path: ""
string url_;
var urlBuilder_ = GetStringBuilder();
try
{
// Operation Path: ""

PrepareRequest(client_, request_, urlBuilder_);
PrepareRequest(client_, request_, urlBuilder_);

url_ = urlBuilder_.ToString();
}
finally
{
ReturnStringBuilder(urlBuilder_);
}

var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);

PrepareRequest(client_, request_, url_);
Expand Down Expand Up @@ -158,17 +166,25 @@ namespace MyNamespace
request_.Method = new System.Net.Http.HttpMethod("GET");
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json"));

var urlBuilder_ = new System.Text.StringBuilder();
if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);
// Operation Path: "sum/{a}/{b}"
urlBuilder_.Append("sum/");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(a, System.Globalization.CultureInfo.InvariantCulture)));
urlBuilder_.Append('/');
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(b, System.Globalization.CultureInfo.InvariantCulture)));
string url_;
var urlBuilder_ = GetStringBuilder();
try
{
// Operation Path: "sum/{a}/{b}"
urlBuilder_.Append("sum/");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(a, System.Globalization.CultureInfo.InvariantCulture)));
urlBuilder_.Append('/');
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(b, System.Globalization.CultureInfo.InvariantCulture)));

PrepareRequest(client_, request_, urlBuilder_);

PrepareRequest(client_, request_, urlBuilder_);
url_ = urlBuilder_.ToString();
}
finally
{
ReturnStringBuilder(urlBuilder_);
}

var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);

PrepareRequest(client_, request_, url_);
Expand Down Expand Up @@ -240,16 +256,24 @@ namespace MyNamespace
request_.Method = new System.Net.Http.HttpMethod("GET");
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json"));

var urlBuilder_ = new System.Text.StringBuilder();
if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);
// Operation Path: "abs({a})"
urlBuilder_.Append("abs(");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(a, System.Globalization.CultureInfo.InvariantCulture)));
urlBuilder_.Append(')');
string url_;
var urlBuilder_ = GetStringBuilder();
try
{
// Operation Path: "abs({a})"
urlBuilder_.Append("abs(");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(a, System.Globalization.CultureInfo.InvariantCulture)));
urlBuilder_.Append(')');

PrepareRequest(client_, request_, urlBuilder_);
PrepareRequest(client_, request_, urlBuilder_);

url_ = urlBuilder_.ToString();
}
finally
{
ReturnStringBuilder(urlBuilder_);
}

var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);

PrepareRequest(client_, request_, url_);
Expand All @@ -258,7 +282,9 @@ namespace MyNamespace
var disposeResponse_ = true;
try
{
var headers_ = System.Linq.Enumerable.ToDictionary(response_.Headers, h_ => h_.Key, h_ => h_.Value);
var headers_ = new System.Collections.Generic.Dictionary<string, System.Collections.Generic.IEnumerable<string>>();
foreach (var item_ in response_.Headers)
headers_[item_.Key] = item_.Value;
if (response_.Content != null && response_.Content.Headers != null)
{
foreach (var item_ in response_.Content.Headers)
Expand Down Expand Up @@ -407,6 +433,10 @@ namespace MyNamespace
var result = System.Convert.ToString(value, cultureInfo);
return result == null ? "" : result;
}

private static global::System.Text.StringBuilder GetStringBuilder() => new global::System.Text.StringBuilder(8000);

private partial static void ReturnStringBuilder(global::System.Text.StringBuilder sb);
}

[System.CodeDom.Compiler.GeneratedCode("NSwag", "14.0.0.0 (NJsonSchema v11.0.0.0 (Newtonsoft.Json v13.0.0.0))")]
Expand Down Expand Up @@ -467,14 +497,22 @@ namespace MyNamespace
request_.Method = new System.Net.Http.HttpMethod("GET");
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/octet-stream"));

var urlBuilder_ = new System.Text.StringBuilder();
if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);
// Operation Path: "examples"
urlBuilder_.Append("examples");
string url_;
var urlBuilder_ = GetStringBuilder();
try
{
// Operation Path: "examples"
urlBuilder_.Append("examples");

PrepareRequest(client_, request_, urlBuilder_);

PrepareRequest(client_, request_, urlBuilder_);
url_ = urlBuilder_.ToString();
}
finally
{
ReturnStringBuilder(urlBuilder_);
}

var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);

PrepareRequest(client_, request_, url_);
Expand Down Expand Up @@ -632,6 +670,10 @@ namespace MyNamespace
var result = System.Convert.ToString(value, cultureInfo);
return result == null ? "" : result;
}

private static global::System.Text.StringBuilder GetStringBuilder() => new global::System.Text.StringBuilder(8000);

private partial static void ReturnStringBuilder(global::System.Text.StringBuilder sb);
}


Expand Down
Loading
Loading