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

Introduce string builder factory template. #4611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulomorgado
Copy link
Contributor

With this PR, the creation of the StringBuilder for building operation URLs is moved to the Client.Class.StringBuilder.liquid template.

This allows the user to replace it with a template that uses, for example, a StringBuilder pool.

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

@@ -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>>();
Copy link

Choose a reason for hiding this comment

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

I think this part could just be written as new System.Collections.Generic.Dictionary<string, System.Collections.Generic.IEnumerable<string>>(response_.Headers);

Copy link
Contributor Author

@paulomorgado paulomorgado Nov 30, 2023

Choose a reason for hiding this comment

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

This was supposed to have been addressed in #4586 and improved in #4602.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants